Skip to content

fix(eslint-plugin-template): any valid DOM element with role button is interactive #2488

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 31, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -547,6 +547,9 @@ interface Options {
<a href="#" (click)="onClick()"></a>
<a [attr.href]="href" class="anchor" (click)="onClick()"></a>
<a [routerLink]="'route'" (click)="onClick()"></a>
<div role="button" (click)="doSomething()"></div>
<span role="button" (click)="doSomething()"></span>
<p role="button" (click)="doSomething()"></p>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI @sandikbarr I am rushing this one a little bit to get the fix out in 19.x, I wasn't 100% if existing utils allowed me to figure this out so I put in a hardcoded check for role of button

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JamesHenry those elements with role button won’t behave like a button that would have built in keyboard support.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For sure, but for this rule, I think it makes sense not to report? It will be caught by other rules for not having a tabindex etc

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To fix this lint issue you would have to add those event handlers and then you would process the event multiple times

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As reported here: #1996 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s a fair point. Interactive-supports-focus will catch it for not being focusable, but interactive-supports-focus won’t enforce keyboard support.

```

<br>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@ import { getTemplateParserServices } from '@angular-eslint/utils';
import { createESLintRule } from '../utils/create-eslint-rule';
import { getDomElements } from '../utils/get-dom-elements';
import { isHiddenFromScreenReader } from '../utils/is-hidden-from-screen-reader';
import { isInteractiveElement } from '../utils/is-interactive-element';
import { isInherentlyInteractiveElement } from '../utils/is-interactive-element';
import { isPresentationRole } from '../utils/is-presentation-role';
import { getAttributeValue } from '../utils/get-attribute-value';
import { ARIARole } from 'aria-query';

export type Options = [
{
Expand Down Expand Up @@ -58,11 +60,18 @@ export default createESLintRule<Options, MessageIds>({
isIgnored(ignoreWithDirectives, node) ||
isPresentationRole(node) ||
isHiddenFromScreenReader(node) ||
isInteractiveElement(node)
isInherentlyInteractiveElement(node)
) {
return;
}

// The final case that should be ignored is element which is not inherently interactive, but which has an interactive role.
// TODO: extend utils with this check (and make it include all interactive roles)
const role = getAttributeValue(node, 'role') as ARIARole;
if (role === 'button') {
return;
}

let hasClick = false,
hasKeyEvent = false;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { createESLintRule } from '../utils/create-eslint-rule';
import { getDomElements } from '../utils/get-dom-elements';
import { isHiddenFromScreenReader } from '../utils/is-hidden-from-screen-reader';
import {
isInteractiveElement,
isInherentlyInteractiveElement,
isNonInteractiveRole,
} from '../utils/is-interactive-element';
import { isContentEditable } from '../utils/is-content-editable';
Expand Down Expand Up @@ -84,7 +84,7 @@ export default createESLintRule<Options, MessageIds>({
if (
interactiveOutput &&
!tabIndex &&
!isInteractiveElement(node) &&
!isInherentlyInteractiveElement(node) &&
!isNonInteractiveRole(node) &&
!isContentEditable(node)
) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ function checkIsNonInteractiveRole(node: TmplAstElement): boolean {
* has a dynamic handler on it and we need to discern whether or not
* it's intention is to be interacted with on the DOM.
*/
export function isInteractiveElement(node: TmplAstElement): boolean {
export function isInherentlyInteractiveElement(node: TmplAstElement): boolean {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed for clarity, I didn't understand it at first, and I added my role=button check in here which caused a bunch of regressions in other tests

return getDomElements().has(node.name) && checkIsInteractiveElement(node);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@ export const valid: readonly (string | ValidTestCase<Options>)[] = [
<a href="#" (click)="onClick()"></a>
<a [attr.href]="href" class="anchor" (click)="onClick()"></a>
<a [routerLink]="'route'" (click)="onClick()"></a>
<div role="button" (click)="doSomething()"></div>
<span role="button" (click)="doSomething()"></span>
<p role="button" (click)="doSomething()"></p>
`,
},
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ export const invalid: readonly InvalidTestCase<MessageIds, Options>[] = [
messageId,
}),

// interactive role, non interactive element
// interactive role on inherently non-interactive element without tabindex
convertAnnotatedSourceToFailureCase({
description:
'should fail interactive role but element does not support focus',
Expand Down
Loading