Skip to content

Commit 3a1d640

Browse files
committed
refactor: extract landmark-roles util (preserving deliberate per-rule region exclusion)
Adds `lib/utils/landmark-roles.js` deriving landmark roles from aria-query (non-abstract roles with 'landmark' in their superClass chain, filtering out DPub-ARIA doc-* roles). Exports two sets: - `ALL_LANDMARK_ROLES` — the 8 WAI-ARIA 1.2 §5.3.4 landmark roles (banner, complementary, contentinfo, form, main, navigation, region, search). - `LANDMARK_ROLES` — the 7-role subset excluding `region`, safe for static-linting rules that cannot verify accessible names at lint time. `<section>` only gets the `region` landmark role when it has an accessible name (aria-label / aria-labelledby / title). Without one, `<section>` has role `generic` per the WAI-ARIA APG landmark examples: https://www.w3.org/WAI/ARIA/apg/patterns/landmarks/examples/HTML5.html A static linter cannot verify that accessible names resolve to non-empty values at runtime, so rules that enumerate landmarks should use the 7-role subset (`LANDMARK_ROLES`) to avoid false-positive-flagging unnamed-section nesting/redundancy. Rules that DO verify accessible names can use `ALL_LANDMARK_ROLES`. This was the reasoning behind ember-cli#2694 (post-merge-review that dropped section/region from `template-no-nested-landmark`). An earlier draft of this PR reversed that fix by treating the exclusion as "drift"; this revision preserves the exclusion and codifies the reasoning in the shared util so future rule authors inherit the safe default. - `template-no-duplicate-landmark-elements` — imports `ALL_LANDMARK_ROLES` as `LANDMARK_ROLES`. This rule inspects aria-label / aria-labelledby before classifying a node as a landmark (see `getLabel` + dynamic-label skip in `GlimmerElementNode`), so it can safely include `region`. - `template-no-nested-landmark` — imports `LANDMARK_ROLES` (the safe subset). Preserves the ember-cli#2694 behavior. - `template-no-redundant-role` — imports `LANDMARK_ROLES` (the safe subset). Also removes the `region: ['section']` entry from `ROLE_TO_ELEMENTS` for the same reason — ember-cli#2694's reasoning applies here too: `<section role="region">` should not be flagged as redundant because we can't verify the section has a name. - New util tests cover both sets, the DPub exclusion, and non-landmark role rejection. - `template-no-nested-landmark` tests preserve the original valid test for nested `role="region"` without accessible names. - `template-no-redundant-role` tests preserve the original `<div role="region">` as valid (div has no implicit role); the previously-regressed test for `<section role="region">` flagged-in-landmark-only is no longer present because the rule no longer flags that pattern (consistent with ember-cli#2694).
1 parent 414d6d5 commit 3a1d640

7 files changed

Lines changed: 119 additions & 34 deletions

lib/rules/template-no-duplicate-landmark-elements.js

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,9 @@
1+
// This rule inspects aria-label / aria-labelledby before classifying a node
2+
// as a landmark (see getLabel + the dynamic-label skip in GlimmerElementNode),
3+
// so it can safely include `region` — it won't flag an unnamed <section> as
4+
// a landmark duplicate. Use the full spec-listed 8-role set.
5+
const { ALL_LANDMARK_ROLES: LANDMARK_ROLES } = require('../utils/landmark-roles');
6+
17
/** @type {import('eslint').Rule.RuleModule} */
28
module.exports = {
39
meta: {
@@ -33,18 +39,6 @@ module.exports = {
3339
form: 'form',
3440
};
3541

36-
// Landmark ARIA roles
37-
const LANDMARK_ROLES = new Set([
38-
'banner',
39-
'complementary',
40-
'contentinfo',
41-
'form',
42-
'main',
43-
'navigation',
44-
'region',
45-
'search',
46-
]);
47-
4842
// Sectioning elements that strip banner/contentinfo roles from header/footer
4943
const SECTIONING_ELEMENTS = new Set(['article', 'aside', 'main', 'nav', 'section']);
5044
const elementStack = [];

lib/rules/template-no-nested-landmark.js

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,4 @@
1-
const LANDMARK_ROLES = new Set([
2-
'banner',
3-
'complementary',
4-
'contentinfo',
5-
'form',
6-
'main',
7-
'navigation',
8-
'search',
9-
]);
1+
const { LANDMARK_ROLES } = require('../utils/landmark-roles');
102

113
const LANDMARK_ELEMENTS = new Set(['header', 'aside', 'footer', 'form', 'main', 'nav']);
124

lib/rules/template-no-redundant-role.js

Lines changed: 7 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
const { roles } = require('aria-query');
2+
const { LANDMARK_ROLES } = require('../utils/landmark-roles');
23

34
const DEFAULT_CONFIG = {
45
checkAllHTMLElements: true,
@@ -19,17 +20,6 @@ function createErrorMessageAnyElement(element, role) {
1920
return `Use of redundant or invalid role: ${role} on <${element}> detected.`;
2021
}
2122

22-
// https://www.w3.org/TR/html-aria/#docconformance
23-
const LANDMARK_ROLES = new Set([
24-
'banner',
25-
'main',
26-
'complementary',
27-
'search',
28-
'form',
29-
'navigation',
30-
'contentinfo',
31-
]);
32-
3323
const ALLOWED_ELEMENT_ROLES = [
3424
{ name: 'nav', role: 'navigation' },
3525
{ name: 'form', role: 'search' },
@@ -114,7 +104,12 @@ const ROLE_TO_ELEMENTS = {
114104
navigation: ['nav'],
115105
option: ['option'],
116106
radio: ['input'],
117-
region: ['section'],
107+
// `region` is intentionally NOT mapped to `<section>` here. `<section>`
108+
// only gets the `region` landmark role when it has an accessible name
109+
// (aria-label / aria-labelledby / title); without one it has role
110+
// `generic`. A static linter cannot verify accessible-name presence.
111+
// Spec: https://www.w3.org/WAI/ARIA/apg/patterns/landmarks/examples/HTML5.html
112+
// See #2694 where the same reasoning was applied to template-no-nested-landmark.
118113
row: ['tr'],
119114
rowgroup: ['tbody', 'tfoot', 'thead'],
120115
rowheader: ['th'],

lib/utils/landmark-roles.js

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
'use strict';
2+
3+
const { roles } = require('aria-query');
4+
5+
// The 8 WAI-ARIA 1.2 §5.3.4 Landmark Roles, derived from aria-query:
6+
// non-abstract roles whose superClass chain includes 'landmark'. DPub-ARIA
7+
// `doc-*` landmark roles share that superClass but are excluded because this
8+
// codebase's rules target WAI-ARIA; downstream callers can extend if needed.
9+
//
10+
// The 8 roles: banner, complementary, contentinfo, form, main, navigation,
11+
// region, search.
12+
const ALL_LANDMARK_ROLES = buildLandmarkRoleSet();
13+
14+
// The subset that's safe for static-linting rules to treat as landmarks
15+
// without further verification. `region` is EXCLUDED because a static linter
16+
// cannot determine at lint time whether the element has an accessible name
17+
// (via aria-label / aria-labelledby / title), which is required for the
18+
// `region` role to actually apply to a `<section>` per HTML-AAM.
19+
//
20+
// See PR #2694 for the full rationale and spec citation
21+
// (https://www.w3.org/WAI/ARIA/apg/patterns/landmarks/examples/HTML5.html):
22+
// `<section>` without an accessible name has role `generic`, not `region`.
23+
//
24+
// Most a11y rules that enumerate landmarks should use this subset.
25+
// Rules that DO verify accessible names (e.g. template-no-duplicate-
26+
// landmark-elements, which inspects aria-label / aria-labelledby before
27+
// classifying a node as a landmark) should import ALL_LANDMARK_ROLES.
28+
const LANDMARK_ROLES = new Set([...ALL_LANDMARK_ROLES].filter((role) => role !== 'region'));
29+
30+
function buildLandmarkRoleSet() {
31+
const result = new Set();
32+
for (const [role, def] of roles) {
33+
if (def.abstract) {
34+
continue;
35+
}
36+
if (role.startsWith('doc-')) {
37+
continue;
38+
}
39+
const descendsFromLandmark = (def.superClass || []).some((chain) => chain.includes('landmark'));
40+
if (descendsFromLandmark) {
41+
result.add(role);
42+
}
43+
}
44+
return result;
45+
}
46+
47+
module.exports = { LANDMARK_ROLES, ALL_LANDMARK_ROLES };

tests/lib/rules/template-no-nested-landmark.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ ruleTester.run('template-no-nested-landmark', rule, {
4545

4646
// `<section>` only gets the `region` landmark role when it has an accessible name
4747
// (aria-label/aria-labelledby/title). Without one it has the generic role — see
48-
// https://www.w3.org/TR/html-aam-1.0/#el-section
48+
// https://www.w3.org/WAI/ARIA/apg/patterns/landmarks/examples/HTML5.html
4949
// This rule does not inspect accessible names, so unnamed sections are excluded.
5050
'<template><section><section>Content</section></section></template>',
5151
// `role="region"` is the landmark role a named `<section>` gets. Nesting it is

tests/lib/rules/template-no-redundant-role.js

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ ruleTester.run('template-no-redundant-role', rule, {
4848
options: [{ checkAllHTMLElements: false }],
4949
},
5050
'<template><input role="combobox"></template>',
51+
<<<<<<< HEAD
5152
// <select multiple> has implicit role listbox, so combobox is not redundant.
5253
'<template><select role="combobox" multiple></select></template>',
5354
// <select size="5"> (size > 1) has implicit role listbox.
@@ -66,6 +67,13 @@ ruleTester.run('template-no-redundant-role', rule, {
6667
// <button> resolves to `tab` (non-redundant — button's implicit is
6768
// `button`, not `tab`). WAI-ARIA §4.1 fallback-list semantics.
6869
'<template><button role="tab button"></button></template>',
70+
// `region` is a landmark role, but <div> has no implicit role — role="region"
71+
// on a <div> is not redundant.
72+
'<template><div role="region"></div></template>',
73+
{
74+
code: '<template><div role="region"></div></template>',
75+
options: [{ checkAllHTMLElements: false }],
76+
},
6977
],
7078
invalid: [
7179
{
Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
'use strict';
2+
3+
const { LANDMARK_ROLES, ALL_LANDMARK_ROLES } = require('../../../lib/utils/landmark-roles');
4+
5+
describe('ALL_LANDMARK_ROLES', () => {
6+
it('contains exactly the 8 WAI-ARIA 1.2 §5.3.4 landmark roles', () => {
7+
expect([...ALL_LANDMARK_ROLES].sort()).toEqual([
8+
'banner',
9+
'complementary',
10+
'contentinfo',
11+
'form',
12+
'main',
13+
'navigation',
14+
'region',
15+
'search',
16+
]);
17+
});
18+
19+
it('excludes DPub-ARIA doc-* roles', () => {
20+
for (const role of ALL_LANDMARK_ROLES) {
21+
expect(role.startsWith('doc-')).toBe(false);
22+
}
23+
});
24+
});
25+
26+
describe('LANDMARK_ROLES (the statically-verifiable subset)', () => {
27+
it('is the 7-role subset of ALL_LANDMARK_ROLES excluding region', () => {
28+
expect([...LANDMARK_ROLES].sort()).toEqual([
29+
'banner',
30+
'complementary',
31+
'contentinfo',
32+
'form',
33+
'main',
34+
'navigation',
35+
'search',
36+
]);
37+
});
38+
39+
it('excludes region (cannot verify accessible-name presence statically)', () => {
40+
expect(LANDMARK_ROLES.has('region')).toBe(false);
41+
expect(ALL_LANDMARK_ROLES.has('region')).toBe(true);
42+
});
43+
44+
it('excludes non-landmark roles (button, link, article)', () => {
45+
expect(LANDMARK_ROLES.has('button')).toBe(false);
46+
expect(LANDMARK_ROLES.has('link')).toBe(false);
47+
expect(LANDMARK_ROLES.has('article')).toBe(false);
48+
});
49+
});

0 commit comments

Comments
 (0)