refactor: extract landmark-roles util (preserving deliberate per-rule region exclusion)#38
Closed
johanrd wants to merge 1 commit into
Closed
refactor: extract landmark-roles util (preserving deliberate per-rule region exclusion)#38johanrd wants to merge 1 commit into
johanrd wants to merge 1 commit into
Conversation
🏎️ Benchmark Comparison
Full mitata output |
johanrd
force-pushed
the
fix/landmark-roles-util
branch
from
a22e465 to
3084cc8
Compare
johanrd
changed the title
There was a problem hiding this comment.
Pull request overview
Refactors landmark ARIA role handling across multiple template a11y rules by extracting a shared landmark-roles utility that preserves the intentional region exclusion for rules that can’t statically verify accessible names.
Changes:
- Add
lib/utils/landmark-roles.jsexportingALL_LANDMARK_ROLES(full landmark set from aria-query, excludingdoc-*) andLANDMARK_ROLES(subset excludingregion). - Update
template-no-{nested,duplicate}-landmark*andtemplate-no-redundant-roleto consume the shared role sets (keeping per-rule intent). - Adjust tests/comments to reflect the intended
regionhandling and add coverage for the new util.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/lib/utils/landmark-roles-test.js | Adds tests for the new landmark role sets. |
| tests/lib/rules/template-no-redundant-role.js | Updates valid cases to reflect region handling. |
| tests/lib/rules/template-no-nested-landmark.js | Updates spec link/comment around region exclusion rationale. |
| lib/utils/landmark-roles.js | Introduces shared landmark role set derivation from aria-query. |
| lib/rules/template-no-redundant-role.js | Switches to shared LANDMARK_ROLES and removes region→section mapping. |
| lib/rules/template-no-nested-landmark.js | Switches to shared LANDMARK_ROLES. |
| lib/rules/template-no-duplicate-landmark-elements.js | Switches to shared full landmark set (including region) with clarifying comment. |
💡 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 7 out of 7 changed files in this pull request and generated no new 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 7 out of 7 changed files in this pull request and generated 2 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 8 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
johanrd
force-pushed
the
fix/landmark-roles-util
branch
from
c0413e4 to
2e0d215
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 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
johanrd
force-pushed
the
fix/landmark-roles-util
branch
from
35094e6 to
c893ddb
Compare
… region exclusion)
johanrd
force-pushed
the
fix/landmark-roles-util
branch
from
c893ddb to
e888a65
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 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+11
to
+27
| const ALL_LANDMARK_ROLES = buildLandmarkRoleSet(); | ||
|
|
||
| // The subset that's safe for static-linting rules to treat as landmarks | ||
| // without further verification. `region` is EXCLUDED because a static linter | ||
| // cannot determine at lint time whether the element has an accessible name | ||
| // (via aria-label / aria-labelledby / title), which is required for the | ||
| // `region` role to actually apply to a `<section>` per HTML-AAM. | ||
| // | ||
| // See PR #2694 for the full rationale and spec citation | ||
| // (https://www.w3.org/TR/html-aam-1.0/#el-section): | ||
| // `<section>` without an accessible name has role `generic`, not `region`. | ||
| // | ||
| // Most a11y rules that enumerate landmarks should use this subset. | ||
| // Rules that DO verify accessible names (e.g. template-no-duplicate- | ||
| // landmark-elements, which inspects aria-label / aria-labelledby before | ||
| // classifying a node as a landmark) should import ALL_LANDMARK_ROLES. | ||
| const LANDMARK_ROLES = new Set([...ALL_LANDMARK_ROLES].filter((role) => role !== 'region')); |
There was a problem hiding this comment.
Both exports are mutable Set instances shared by all importers; any accidental .add()/.delete() in one consumer would affect all rules/tests in the same process. To make this safer, consider exporting accessor functions that return a fresh Set copy (or exporting arrays and letting each rule construct its own Set), or at minimum documenting that these sets must be treated as immutable.
| return result; | ||
| } | ||
|
|
||
| module.exports = { LANDMARK_ROLES, ALL_LANDMARK_ROLES }; |
There was a problem hiding this comment.
Both exports are mutable Set instances shared by all importers; any accidental .add()/.delete() in one consumer would affect all rules/tests in the same process. To make this safer, consider exporting accessor functions that return a fresh Set copy (or exporting arrays and letting each rule construct its own Set), or at minimum documenting that these sets must be treated as immutable.
Suggested change
| module.exports = { LANDMARK_ROLES, ALL_LANDMARK_ROLES }; | |
| module.exports = { | |
| get LANDMARK_ROLES() { | |
| return new Set(LANDMARK_ROLES); | |
| }, | |
| get ALL_LANDMARK_ROLES() { | |
| return new Set(ALL_LANDMARK_ROLES); | |
| }, | |
| }; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.Premise
Three a11y rules share a concept of "landmark ARIA role." Today they each maintain their own hand-list. The counts look like drift:
template-no-redundant-roleregiontemplate-no-nested-landmarkregiontemplate-no-duplicate-landmark-elementsBut the two 7-role lists aren't drift — the
regionexclusion is deliberate, documented, and was landed last week in #2694 after @NullVoxPopuli's "what's MDN say?" review. The reasoning:Source: WAI-ARIA APG HTML5 landmark examples.
The third rule (
template-no-duplicate-landmark-elements) DOES includeregionbecause that rule inspects aria-label / aria-labelledby before classifying a node as a landmark — it can safely handle the full 8-role set.So the three rules have different landmark sets for different, justifiable reasons. Not drift.
What this PR does
Adds
lib/utils/landmark-roles.jsthat makes the distinction explicit by exporting two sets:ALL_LANDMARK_ROLES— the 8 WAI-ARIA 1.2 §5.3.4 landmark roles, derived from aria-query (non-abstract roles whose superClass chain includes 'landmark', minus DPub-ARIAdoc-*).LANDMARK_ROLES— the 7-role subset excludingregion. The safe default for static-linting rules that can't verify accessible names.Both sets are derived from aria-query's role metadata so future WAI-ARIA additions flow through on aria-query upgrade.
Rule migrations:
template-no-duplicate-landmark-elements→ALL_LANDMARK_ROLES(verifies accessible names).template-no-nested-landmark→LANDMARK_ROLES(preserves Post-merge-review: Fix template-no-nested-landmark: drop port-only section/region ember-cli/eslint-plugin-ember#2694 behavior).template-no-redundant-role→LANDMARK_ROLES(preserves existing landmark-only behavior).Also removes the
region: ['section']entry fromtemplate-no-redundant-role'sROLE_TO_ELEMENTSmapping, since ember-cli#2694's reasoning applies to that rule too:<section role=\"region\">shouldn't be flagged as redundant when we can't verify the section has an accessible name.Behavior vs. master
template-no-duplicate-landmark-elements: no change — same 8 roles.template-no-nested-landmark: no change — same 7 roles.template-no-redundant-role:<section role=\"region\">is no longer flagged as redundant (default mode). This aligns the rule with Post-merge-review: Fix template-no-nested-landmark: drop port-only section/region ember-cli/eslint-plugin-ember#2694's reasoning.Earlier (rejected) direction
An earlier revision of this PR treated the region exclusion as "drift" and re-added
regionto both rules via a single shared set. That would have silently reverted ember-cli#2694's fix last week. This revision is the correction — preserving the intent, codifying the reasoning in the util.Spec references
Prior art — how peers handle the landmark set
All four peer plugins draw landmark role definitions from
aria-query(the same source as this util). None expose a shared landmark-roles helper; each rule that needs landmarks includes the roles inline, typically the full 8-role spec list.no-nested-landmarkorno-duplicate-landmark-elementsruleSo the nested-landmark / duplicate-landmark / redundant-role checks are Ember-template-lint conventions; this util consolidates the landmark set across them. The region exclusion for static-linting is this plugin's specific convention (grounded in the WAI-ARIA APG's HTML5 landmark example via ember-cli#2694) — it is not a peer-plugin convention because peers don't ship these rules.