feat: add template-no-role-presentation-on-focusable#22
feat: add template-no-role-presentation-on-focusable#22
Conversation
🏎️ Benchmark Comparison
Full mitata output |
There was a problem hiding this comment.
Pull request overview
Adds a new Ember template accessibility lint rule to flag role="presentation" / role="none" when applied to focusable/interactive native elements, plus supporting utilities and tests.
Changes:
- Added
ember/template-no-role-presentation-on-focusablerule with focusability/interactive detection and special-casing for<area href>. - Introduced new utils for component-invocation detection and HTML interactive-content classification.
- Added rule docs, README entry, and test coverage (including a peer-parity audit fixture).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/lib/utils/is-component-invocation-test.js | Adds unit tests for the new component-invocation detection helper. |
| tests/lib/utils/html-interactive-content-test.js | Adds unit tests for the new HTML interactive-content classification helper. |
| tests/lib/rules/template-no-role-presentation-on-focusable.js | Adds RuleTester coverage for the new rule in both GJS/GTS and HBS modes. |
| tests/audit/no-role-presentation-on-focusable/peer-parity.js | Adds an audit/peer-parity test fixture for behavioral comparison with vue-a11y. |
| lib/utils/is-component-invocation.js | Implements isComponentInvocation helper to skip opaque component nodes. |
| lib/utils/html-interactive-content.js | Implements isHtmlInteractiveContent helper based on HTML spec interactive content. |
| lib/rules/template-no-role-presentation-on-focusable.js | Implements the new rule logic and reporting. |
| docs/rules/template-no-role-presentation-on-focusable.md | Documents the new rule, examples, and intended scope. |
| README.md | Adds the new rule to the rules list table. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…view) Per HTML §6.6.3 'Sequential focus navigation', none of <details>, <option>, or <datalist> are focusable by default: - <details>: the focusable control is its <summary> child - <option>: not in default tab order; <select> is focused and arrow keys navigate options within it - <datalist>: no user-facing UI; the paired <input list> is focused Including them in UNCONDITIONAL_FOCUSABLE_TAGS was over-aggressive and caused false positives on this rule. Tests updated to pin the now-allowed cases.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
johanrd
force-pushed
the
feat/template-no-role-presentation-on-focusable
branch
from
19cd641 to
23f6ef7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
johanrd
force-pushed
the
feat/template-no-role-presentation-on-focusable
branch
from
a368d5c to
7857a1f
Compare
johanrd
force-pushed
the
feat/template-no-role-presentation-on-focusable
branch
from
7857a1f to
749af04
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (tag === 'img') { | ||
| return Boolean(findAttr(node, 'usemap')); | ||
| } |
There was a problem hiding this comment.
isKeyboardFocusable currently treats details, option, and datalist as unconditionally focusable, and treats img[usemap] as focusable. These elements are not generally keyboard-focusable themselves (e.g., <option>/<datalist> are not focusable form controls on their own; image maps transfer focus/interaction to <area href>, not the <img>). This can produce false positives for the rule. Recommendation: remove details/option/datalist from UNCONDITIONAL_FOCUSABLE_TAGS, and drop (or rework) the img[usemap] focusability branch (leave focusability to tabindex/other explicit focus vectors; handle <area href> separately as you already do). Add targeted tests proving the corrected behavior for these tags.
| // tabindex="-1" on an inherently-focusable element still accepts programmatic | ||
| // focus (focus() works; the element stays in the a11y tree). Rule flags it. | ||
| // vuejs-accessibility treats tabindex="-1" as "removed from tab order = not focusable" — we disagree. | ||
| { | ||
| code: '<template><button tabindex="-1" role="presentation">Press</button></template>', | ||
| output: null, | ||
| errors: [{ messageId: 'invalidPresentation' }], | ||
| }, |
There was a problem hiding this comment.
The PR description’s “Allows” section states that tabindex="-1" + role="presentation" is allowed (citing jsx-a11y/vue-a11y), but the implemented behavior + tests explicitly flag this case as invalid. Please align the PR description with the actual rule behavior (or, if the description is authoritative, adjust the rule/tests/docs to match).
| function isHtmlInteractiveContent(node, getTextAttrValue, options = {}) { | ||
| const rawTag = node && node.tag; | ||
| if (typeof rawTag !== 'string' || rawTag.length === 0) { | ||
| return false; | ||
| } | ||
| const tag = rawTag.toLowerCase(); | ||
|
|
||
| if (UNCONDITIONAL_INTERACTIVE_TAGS.has(tag)) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The new options.ignoreUsemap branch is user-visible behavior but isn’t covered by the new unit tests. Add a test asserting that isHtmlInteractiveContent(makeNode('img', { usemap: '#m' }), getTextAttrValue, { ignoreUsemap: true }) returns false (and ideally a companion test for the default behavior returning true).
| // img — interactive only when usemap is present (image map) | ||
| if (tag === 'img') { | ||
| if (options.ignoreUsemap) { | ||
| return false; | ||
| } | ||
| return hasAttribute(node, 'usemap'); | ||
| } |
There was a problem hiding this comment.
The new options.ignoreUsemap branch is user-visible behavior but isn’t covered by the new unit tests. Add a test asserting that isHtmlInteractiveContent(makeNode('img', { usemap: '#m' }), getTextAttrValue, { ignoreUsemap: true }) returns false (and ideally a companion test for the default behavior returning true).
Note
This is part of a series where Claude has audited
eslint-plugin-emberagainst jsx-a11y, vuejs-accessibility, angular-eslint, lit-a11y and html-validate,ember-template-lint, and the HTML and WCAG specs.presentationrole and expose the element with its implicit role, in order to ensure that the element is operable." That is a UA-directed conflict-resolution procedure, not an author-directed prohibition — but the practical consequence is the same:role="presentation"on a focusable element is silently ignored by the UA, producing a mixed signal (the author said "decorative," the UA exposes the element anyway). This rule flags the author-side anti-pattern.Fix: add
template-no-role-presentation-on-focusable. Shares focusable-detection withtemplate-no-aria-hidden-on-focusable(#19) — both target the same anti-pattern shape via different attributes.role="none"is handled identically. WAI-ARIA 1.2 explicitly definesnoneas a synonym forpresentation: "See synonympresentation."Flags
Allows
Prior art
Verified each peer in source:
no-role-presentation-on-focusablerole === "presentation"only (not "none"). Our rule extends to "none" per spec synonymy.