Merge pull request #2758 from johanrd/fix/landmark-roles-util

refactor: extract landmark-roles util (preserving deliberate per-rule region exclusion)

Author: NullVoxPopuli

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

@@ -1,3 +1,9 @@
+// This rule inspects aria-label / aria-labelledby before classifying a node
+// as a landmark (see getLabel + the dynamic-label skip in GlimmerElementNode),
+// so it can safely include `region` — it won't flag an unnamed <section> as
+// a landmark duplicate. Use the full spec-listed 8-role set.
+const { ALL_LANDMARK_ROLES: LANDMARK_ROLES } = require('../utils/landmark-roles');
+
 /** @type {import('eslint').Rule.RuleModule} */
 module.exports = {
   meta: {
@@ -33,18 +39,6 @@ module.exports = {
       form: 'form',
     };
 
-    // Landmark ARIA roles
-    const LANDMARK_ROLES = new Set([
-      'banner',
-      'complementary',
-      'contentinfo',
-      'form',
-      'main',
-      'navigation',
-      'region',
-      'search',
-    ]);
-
     // Sectioning elements that strip banner/contentinfo roles from header/footer
     const SECTIONING_ELEMENTS = new Set(['article', 'aside', 'main', 'nav', 'section']);
     const elementStack = [];

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

@@ -1,12 +1,4 @@
-const LANDMARK_ROLES = new Set([
-  'banner',
-  'complementary',
-  'contentinfo',
-  'form',
-  'main',
-  'navigation',
-  'search',
-]);
+const { LANDMARK_ROLES } = require('../utils/landmark-roles');
 
 const LANDMARK_ELEMENTS = new Set(['header', 'aside', 'footer', 'form', 'main', 'nav']);
 

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

@@ -1,4 +1,5 @@
 const { roles } = require('aria-query');
+const { LANDMARK_ROLES } = require('../utils/landmark-roles');
 
 const DEFAULT_CONFIG = {
   checkAllHTMLElements: true,
@@ -19,17 +20,6 @@ function createErrorMessageAnyElement(element, role) {
   return `Use of redundant or invalid role: ${role} on <${element}> detected.`;
 }
 
-// https://www.w3.org/TR/html-aria/#docconformance
-const LANDMARK_ROLES = new Set([
-  'banner',
-  'main',
-  'complementary',
-  'search',
-  'form',
-  'navigation',
-  'contentinfo',
-]);
-
 const ALLOWED_ELEMENT_ROLES = [
   { name: 'nav', role: 'navigation' },
   { name: 'form', role: 'search' },
@@ -114,7 +104,12 @@ const ROLE_TO_ELEMENTS = {
   navigation: ['nav'],
   option: ['option'],
   radio: ['input'],
-  region: ['section'],
+  // `region` is intentionally NOT mapped to `<section>` here. `<section>`
+  // only gets the `region` landmark role when it has an accessible name
+  // (aria-label / aria-labelledby / title); without one it has role
+  // `generic`. A static linter cannot verify accessible-name presence.
+  // Spec: https://www.w3.org/WAI/ARIA/apg/patterns/landmarks/examples/HTML5.html
+  // See #2694 where the same reasoning was applied to template-no-nested-landmark.
   row: ['tr'],
   rowgroup: ['tbody', 'tfoot', 'thead'],
   rowheader: ['th'],

lib/utils/landmark-roles.js

@@ -0,0 +1,46 @@
+'use strict';
+
+const { roles } = require('aria-query');
+
+// Non-abstract landmark roles derived from aria-query's role taxonomy: any
+// role whose superClass chain includes 'landmark', minus DPub-ARIA `doc-*`
+// (which share that superClass but are outside this plugin's rules' scope —
+// downstream callers can extend if needed). The exact size is intentionally
+// not hard-coded: the derivation is what matters, so if aria-query adds a
+// new non-abstract landmark upstream it will be picked up automatically.
+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'));
+
+function buildLandmarkRoleSet() {
+  const result = new Set();
+  for (const [role, def] of roles) {
+    if (def.abstract) {
+      continue;
+    }
+    if (role.startsWith('doc-')) {
+      continue;
+    }
+    const descendsFromLandmark = (def.superClass || []).some((chain) => chain.includes('landmark'));
+    if (descendsFromLandmark) {
+      result.add(role);
+    }
+  }
+  return result;
+}
+
+module.exports = { LANDMARK_ROLES, ALL_LANDMARK_ROLES };

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

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

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

@@ -66,6 +66,17 @@ ruleTester.run('template-no-redundant-role', rule, {
     // <button> resolves to `tab` (non-redundant — button's implicit is
     // `button`, not `tab`). WAI-ARIA §4.1 fallback-list semantics.
     '<template><button role="tab button"></button></template>',
+    // `region` is a landmark role, but <div> has no implicit role — role="region"
+    // on a <div> is not redundant.
+    '<template><div role="region"></div></template>',
+    {
+      code: '<template><div role="region"></div></template>',
+      options: [{ checkAllHTMLElements: false }],
+    },
+    // Per #2694: <section> has implicit role `generic` when unnamed and
+    // `region` when it has an accessible name. role="region" on <section> is
+    // therefore not unconditionally redundant — #38 aligns with this.
+    '<template><section role="region" aria-label="Quick facts">...</section></template>',
   ],
   invalid: [
     {
@@ -223,6 +234,10 @@ hbsRuleTester.run('template-no-redundant-role', rule, {
     // NOT redundant.
     '<select role="listbox"></select>',
     '<select role="listbox" size="1"></select>',
+    // Per #2694: <section> has implicit role `generic` when unnamed and
+    // `region` when it has an accessible name. role="region" on <section> is
+    // therefore not unconditionally redundant — #38 aligns with this.
+    '<section role="region" aria-label="Quick facts">...</section>',
   ],
   invalid: [
     {

tests/lib/utils/landmark-roles-test.js

@@ -0,0 +1,61 @@
+'use strict';
+
+const { LANDMARK_ROLES, ALL_LANDMARK_ROLES } = require('../../../lib/utils/landmark-roles');
+
+// The WAI-ARIA 1.2 §5.3.4 landmark roles known at authoring time. Asserting
+// set-inclusion (not an exact-equality / size match) keeps the test robust
+// to future aria-query updates that add non-abstract landmark roles — the
+// derivation (non-abstract + superClass contains 'landmark' + not doc-*)
+// is what we actually care about.
+const KNOWN_LANDMARK_ROLES = [
+  'banner',
+  'complementary',
+  'contentinfo',
+  'form',
+  'main',
+  'navigation',
+  'region',
+  'search',
+];
+
+describe('ALL_LANDMARK_ROLES', () => {
+  it('includes every WAI-ARIA 1.2 §5.3.4 landmark role', () => {
+    for (const role of KNOWN_LANDMARK_ROLES) {
+      expect(ALL_LANDMARK_ROLES.has(role)).toBe(true);
+    }
+    // Floor, not ceiling — lets the set grow if aria-query adds new
+    // non-abstract landmark roles upstream.
+    expect(ALL_LANDMARK_ROLES.size).toBeGreaterThanOrEqual(KNOWN_LANDMARK_ROLES.length);
+  });
+
+  it('excludes DPub-ARIA doc-* roles', () => {
+    for (const role of ALL_LANDMARK_ROLES) {
+      expect(role.startsWith('doc-')).toBe(false);
+    }
+  });
+});
+
+describe('LANDMARK_ROLES (the statically-verifiable subset)', () => {
+  it('includes every known landmark role except region', () => {
+    for (const role of KNOWN_LANDMARK_ROLES) {
+      if (role === 'region') {
+        continue;
+      }
+      expect(LANDMARK_ROLES.has(role)).toBe(true);
+    }
+    // Floor, not ceiling — mirrors the ALL_LANDMARK_ROLES rationale, minus
+    // the deliberate region exclusion.
+    expect(LANDMARK_ROLES.size).toBeGreaterThanOrEqual(KNOWN_LANDMARK_ROLES.length - 1);
+  });
+
+  it('excludes region (cannot verify accessible-name presence statically)', () => {
+    expect(LANDMARK_ROLES.has('region')).toBe(false);
+    expect(ALL_LANDMARK_ROLES.has('region')).toBe(true);
+  });
+
+  it('excludes non-landmark roles (button, link, article)', () => {
+    expect(LANDMARK_ROLES.has('button')).toBe(false);
+    expect(LANDMARK_ROLES.has('link')).toBe(false);
+    expect(LANDMARK_ROLES.has('article')).toBe(false);
+  });
+});