refactor+fix: migrate utils, narrow focusability check, normalize aria-hidden (Copilot review)

Author: johanrd

lib/rules/template-no-aria-hidden-on-focusable.js

@@ -1,7 +1,6 @@
 'use strict';
 
-const { isComponentInvocation } = require('../utils/is-component-invocation');
-const { isHtmlInteractiveContent } = require('../utils/html-interactive-content');
+const { isNativeElement } = require('../utils/is-native-element');
 
 function findAttr(node, name) {
   return node.attributes?.find((a) => a.name === name);
@@ -26,51 +25,112 @@ function isAriaHiddenTrue(node) {
     return false;
   }
   if (value.type === 'GlimmerTextNode') {
-    return value.chars.toLowerCase() === 'true';
+    return value.chars.trim().toLowerCase() === 'true';
   }
   if (value.type === 'GlimmerMustacheStatement' && value.path) {
     if (value.path.type === 'GlimmerBooleanLiteral') {
       return value.path.value === true;
     }
     if (value.path.type === 'GlimmerStringLiteral') {
-      return value.path.value.toLowerCase() === 'true';
+      return value.path.value.trim().toLowerCase() === 'true';
     }
   }
   return false;
 }
 
-function isFocusable(node) {
-  const tag = node.tag?.toLowerCase();
-  if (!tag) {
+// Tags with an unconditional default focusable UI (sequentially focusable per
+// HTML §6.6.3 "focusable area" + widget roles per HTML-AAM).
+// NOTE: <label> is HTML-interactive-content (§3.2.5.2.7) but NOT keyboard-
+// focusable by default — clicks on a label forward to its associated control,
+// but the label itself isn't in the tab order. So it's excluded here even
+// though `isHtmlInteractiveContent` would return true for it.
+const UNCONDITIONAL_FOCUSABLE_TAGS = new Set([
+  'button',
+  'select',
+  'textarea',
+  'iframe',
+  'embed',
+  'summary',
+  'details',
+  'option',
+  'datalist',
+]);
+
+// Form-control tags whose `disabled` attribute removes them from the tab order
+// (HTML §4.10.18.5 "disabled" + HTML §6.6.3 "focusable area").
+const DISABLEABLE_TAGS = new Set(['button', 'input', 'select', 'textarea', 'fieldset']);
+
+function isDisabledFormControl(node, tag) {
+  if (!DISABLEABLE_TAGS.has(tag)) {
+    return false;
+  }
+  return Boolean(findAttr(node, 'disabled'));
+}
+
+// Narrow rule-local "keyboard-focusable" check. Intentionally distinct from
+// `isHtmlInteractiveContent` (HTML content-model) — we want the sequential-
+// focus + programmatic-focus axis only. See WAI-ARIA "focusable" definition
+// and HTML §6.6.3.
+function isKeyboardFocusable(node, getTextAttrValueFn) {
+  const rawTag = node?.tag;
+  if (typeof rawTag !== 'string' || rawTag.length === 0) {
+    return false;
+  }
+  const tag = rawTag.toLowerCase();
+
+  // Disabled form controls are not focusable.
+  if (isDisabledFormControl(node, tag)) {
     return false;
   }
 
-  // Opt-out via tabindex="-1" makes the element programmatically focusable
-  // (still reachable via .focus()) but removes it from the tab order.
-  // `aria-hidden` on such an element is still problematic — if it can receive
-  // focus, assistive tech should be able to see it. Match jsx-a11y: flag any
-  // tabindex that's not "undefined" (i.e. any tabindex attribute at all).
-  const tabindex = findAttr(node, 'tabindex');
-  if (tabindex) {
+  // Any tabindex (including "-1") makes the element at least programmatically
+  // focusable — still a keyboard-trap risk under aria-hidden.
+  if (findAttr(node, 'tabindex')) {
     return true;
   }
 
-  // Delegate interactive-content classification to the shared util (HTML
-  // §3.2.5.2.7 + summary): button/details/embed/iframe/label/select/summary/
-  // textarea, input (non-hidden), a[href], img[usemap], and
-  // audio[controls]/video[controls].
-  return isHtmlInteractiveContent(node, getTextAttrValue);
+  // contenteditable (truthy) makes the element focusable.
+  const contentEditable = getTextAttrValueFn(node, 'contenteditable');
+  if (contentEditable !== undefined && contentEditable !== null) {
+    const normalized = contentEditable.trim().toLowerCase();
+    // per HTML spec, "", "true", and "plaintext-only" all enable editing.
+    if (normalized === '' || normalized === 'true' || normalized === 'plaintext-only') {
+      return true;
+    }
+  }
+
+  if (UNCONDITIONAL_FOCUSABLE_TAGS.has(tag)) {
+    return true;
+  }
+
+  if (tag === 'input') {
+    const type = getTextAttrValueFn(node, 'type');
+    return type === undefined || type === null || type.trim().toLowerCase() !== 'hidden';
+  }
+
+  if (tag === 'a' || tag === 'area') {
+    return Boolean(findAttr(node, 'href'));
+  }
+
+  if (tag === 'img') {
+    return Boolean(findAttr(node, 'usemap'));
+  }
+
+  if (tag === 'audio' || tag === 'video') {
+    return Boolean(findAttr(node, 'controls'));
+  }
+
+  return false;
 }
 
 // A focusable descendant of an aria-hidden="true" ancestor can still receive
 // focus (aria-hidden does not remove elements from the tab order), so the
 // ancestor hides AT-visible content that remains keyboard-reachable — a
 // keyboard trap. This rule targets the anti-pattern flagged by axe's
-// `aria-hidden-focus` check and by jsx-a11y's `no-aria-hidden-on-focusable`;
-// the WAI-ARIA 1.2 spec itself only says authors MAY "with caution" use
-// aria-hidden, so the rule rests on community a11y guidance, not a
-// normative WAI-ARIA MUST-NOT.
-function hasFocusableDescendant(node) {
+// `aria-hidden-focus` check and by jsx-a11y's `no-aria-hidden-on-focusable`.
+// WAI-ARIA 1.2 says authors SHOULD NOT put aria-hidden on focusable content
+// (the spec normatively warns against this in the aria-hidden authoring note).
+function hasFocusableDescendant(node, sourceCode) {
   const children = node.children;
   if (!children || children.length === 0) {
     return false;
@@ -81,14 +141,14 @@ function hasFocusableDescendant(node) {
       // expressions, and anything else whose rendered element we can't inspect.
       continue;
     }
-    if (isComponentInvocation(child)) {
-      // Component / dynamic tag — opaque. Don't recurse.
+    if (!isNativeElement(child, sourceCode)) {
+      // Component / dynamic / shadowed tag — opaque. Don't recurse.
       continue;
     }
-    if (isFocusable(child)) {
+    if (isKeyboardFocusable(child, getTextAttrValue)) {
       return true;
     }
-    if (hasFocusableDescendant(child)) {
+    if (hasFocusableDescendant(child, sourceCode)) {
       return true;
     }
   }
@@ -116,16 +176,20 @@ module.exports = {
   },
 
   create(context) {
+    const sourceCode = context.sourceCode ?? context.getSourceCode();
     return {
       GlimmerElementNode(node) {
         if (!isAriaHiddenTrue(node)) {
           return;
         }
-        if (isFocusable(node)) {
+        if (!isNativeElement(node, sourceCode)) {
+          return;
+        }
+        if (isKeyboardFocusable(node, getTextAttrValue)) {
           context.report({ node, messageId: 'noAriaHiddenOnFocusable' });
           return;
         }
-        if (hasFocusableDescendant(node)) {
+        if (hasFocusableDescendant(node, sourceCode)) {
           context.report({ node, messageId: 'noAriaHiddenOnAncestorOfFocusable' });
         }
       },

lib/utils/html-interactive-content.js

@@ -63,7 +63,7 @@ function isHtmlInteractiveContent(node, getTextAttrValue, options = {}) {
   // input — interactive unless type="hidden"
   if (tag === 'input') {
     const type = getTextAttrValue(node, 'type');
-    return type !== 'hidden';
+    return type === undefined || type === null || type.trim().toLowerCase() !== 'hidden';
   }
 
   // a — interactive only when href is present

lib/utils/is-component-invocation.js

@@ -0,0 +1,79 @@
+'use strict';
+
+const htmlTags = require('html-tags');
+const svgTags = require('svg-tags');
+const { mathmlTagNames } = require('mathml-tag-names');
+
+// Authoritative set of native element tag names. Mirrors the approach
+// established by #2689 (template-no-block-params-for-html-elements), which
+// the maintainer requires for component-vs-element discrimination in this
+// plugin. Heuristic approaches (PascalCase detection, etc.) were explicitly
+// rejected there because a lowercase tag CAN be a component in GJS/GTS when
+// the name is bound in scope (e.g. `const div = MyComponent; <div />`).
+const ELEMENT_TAGS = new Set([...htmlTags, ...svgTags, ...mathmlTagNames]);
+
+/**
+ * Returns true if the Glimmer element node is a native HTML / SVG / MathML
+ * element — i.e. the tag name is in the authoritative list AND is not
+ * shadowed by an in-scope binding.
+ *
+ * "Native" here means **spec-registered tag name** (in the HTML, SVG, or
+ * MathML spec registries, reached via the `html-tags` / `svg-tags` /
+ * `mathml-tag-names` packages). It is NOT the same as:
+ *
+ *  - "native accessibility" / "widget-ness" — see `interactive-roles.js`
+ *    (aria-query widget taxonomy; an ARIA-tree-semantics question)
+ *  - "native interactive content" / "focus behavior" — see
+ *    `html-interactive-content.js` (HTML §3.2.5.2.7; an HTML-content-model
+ *    question about which tags can be nested inside what)
+ *  - "natively focusable" / sequential-focus — see HTML §6.6.3
+ *
+ * This util answers only: "is this tag a first-class built-in element of one
+ * of the three markup-language standards, rather than a component invocation
+ * or a shadowed local binding?" Callers compose it with the other utils
+ * above when they need a more specific question (see e.g. `template-no-
+ * noninteractive-tabindex`, which consults both this and
+ * `html-interactive-content`).
+ *
+ * Returns false for:
+ *  - components (PascalCase, dotted, @-prefixed, this.-prefixed, ::-namespaced —
+ *    none of these tag names appear in the HTML/SVG/MathML lists)
+ *  - custom elements (`<my-widget>`) — accepted false negative; the web-
+ *    components namespace is open and can't be enumerated
+ *  - scope-bound identifiers (`<div>` when `div` is a local `let` / `const` /
+ *    import / block-param in the enclosing scope)
+ *
+ * @param {object} node - GlimmerElementNode
+ * @param {object} [sourceCode] - ESLint SourceCode, for scope lookup. When
+ *   omitted, the scope check is skipped (the result is then list-based only —
+ *   suitable for unit tests).
+ */
+function isNativeElement(node, sourceCode) {
+  if (!node || typeof node.tag !== 'string') {
+    return false;
+  }
+  if (!ELEMENT_TAGS.has(node.tag)) {
+    return false;
+  }
+  if (!sourceCode || !node.parent) {
+    return true;
+  }
+  const scope = sourceCode.getScope(node.parent);
+  const firstPart = node.parts && node.parts[0];
+  if (firstPart && scope.references.some((ref) => ref.identifier === firstPart)) {
+    return false;
+  }
+  return true;
+}
+
+/**
+ * Inverse of {@link isNativeElement}. Returns true when the node should NOT
+ * be treated as a native HTML element — either because it's a component
+ * invocation (PascalCase, dotted, @-prefixed, this.-prefixed, custom element)
+ * OR a tag name that's shadowed by a scope binding.
+ */
+function isComponentInvocation(node, sourceCode) {
+  return !isNativeElement(node, sourceCode);
+}
+
+module.exports = { isNativeElement, isComponentInvocation, ELEMENT_TAGS };

lib/utils/is-native-element.js

@@ -35,6 +35,16 @@ ruleTester.run('template-no-aria-hidden-on-focusable', rule, {
     // <a> without href isn't focusable by default.
     '<template><a aria-hidden="true">Not a link</a></template>',
 
+    // <label> is HTML interactive content but NOT keyboard-focusable by default
+    // (clicks forward to the associated control; the label itself isn't in the
+    // tab order). So aria-hidden on it is fine.
+    '<template><label aria-hidden="true">Name</label></template>',
+
+    // Disabled form controls are removed from the tab order (HTML §4.10.18.5),
+    // so they're not keyboard-focusable and aria-hidden on them isn't a trap.
+    '<template><button disabled aria-hidden="true">Click me</button></template>',
+    '<template><input disabled aria-hidden="true" /></template>',
+
     // Components — we don't know if they render a focusable element.
     '<template><CustomBtn aria-hidden="true" /></template>',
 
@@ -112,6 +122,13 @@ ruleTester.run('template-no-aria-hidden-on-focusable', rule, {
       output: null,
       errors: [{ messageId: 'noAriaHiddenOnFocusable' }],
     },
+    {
+      // Whitespace-padded "true" is still a truthy aria-hidden per enumerated-
+      // attribute normalization (trim + case-insensitive).
+      code: '<template><button aria-hidden=" true "></button></template>',
+      output: null,
+      errors: [{ messageId: 'noAriaHiddenOnFocusable' }],
+    },
 
     // Descendant-focusable check. Per WAI-ARIA 1.2 §aria-hidden
     // "may receive focus" — focusable descendants are keyboard-reachable

tests/lib/rules/template-no-aria-hidden-on-focusable.js

@@ -53,6 +53,18 @@ describe('isHtmlInteractiveContent', () => {
         isHtmlInteractiveContent(makeNode('input', { type: 'hidden' }), getTextAttrValue)
       ).toBe(false);
     });
+
+    it('is NOT interactive when type="HIDDEN" (case-insensitive)', () => {
+      expect(
+        isHtmlInteractiveContent(makeNode('input', { type: 'HIDDEN' }), getTextAttrValue)
+      ).toBe(false);
+    });
+
+    it('is NOT interactive when type=" hidden " (whitespace-trimmed)', () => {
+      expect(
+        isHtmlInteractiveContent(makeNode('input', { type: ' hidden ' }), getTextAttrValue)
+      ).toBe(false);
+    });
   });
 
   describe('<a>', () => {