fix(keyboard): roving tabindex needs key handling; modifier key listeners count (0.15.4)

CHANGELOG.md

@@ -3,6 +3,32 @@
 All notable changes to `@ngbracket/a11y-devtools` are documented here.
 This project adheres to [Semantic Versioning](https://semver.org/).
 
+## 0.15.4
+
+### Fixed
+
+- **`ngbr/unreachable-control` checks that arrow keys are handled before
+  skipping a roving-tabindex item.** 0.15.2 skipped an item with
+  `tabindex="-1"` whenever Tab could get into its composite widget, so a half-built
+  tab list (the first tab at `tabindex="0"`, the rest at `-1`, no arrow-key
+  handling) passed silently. The item is now skipped only when something handles
+  keys: the widget, anything inside it, an element that controls it, or an
+  element around it up to and including the nearest component's host (so an app
+  shell's shortcut listener doesn't count). When nothing does, it's reported as
+  moderate, to verify by hand. Without Angular's dev-mode debug API (a
+  production build), listeners can't be read, so these items are still skipped.
+- **Key listeners bound with modifiers now count as key handling.** Angular
+  reports `(keydown.enter)` as `keydown.enter`, which wasn't recognised, so
+  `ngbr/click-without-key` flagged a control with `(click)`, `(keydown.enter)`
+  and `(keydown.space)`: the fix its own docs recommend. The same applies to
+  `(keydown.arrowRight)` and friends in the checks above.
+- **The mouse-shortcut skip no longer hides `ngbr/click-without-key`.** 0.15.3's
+  skip for click-only parts of a keyboard-operated widget ran before the
+  Tab-reachability check, so a click-only element Tab *does* reach, inside such a
+  widget, got no finding at all. The skip now applies only to elements Tab can't
+  reach.
+- `--from` with `--serve` no longer also names `--base` in its error message.
+
 ## 0.15.3
 
 ### Fixed

bin/ngbr-a11y-report.mjs

@@ -111,7 +111,6 @@ function parseArgs(argv) {
 }
 
 const opts = parseArgs(process.argv.slice(2));
-if (opts.serve && !opts.base) opts.base = 'http://localhost:4200';
 
 // Options that only mean something when scanning — refused with --from, rather
 // than silently ignored.
@@ -131,6 +130,9 @@ if (opts.from && !opts.help) {
   }
 }
 
+// After the --from check, so --serve alone isn't reported as --base too.
+if (opts.serve && !opts.base) opts.base = 'http://localhost:4200';
+
 if (opts.help || (!opts.from && (!opts.base || opts.routes.length === 0))) {
   process.stderr.write(USAGE);
   process.exit(opts.help ? 0 : 1);

e2e/angular-app/src/app.ts

@@ -43,6 +43,17 @@ export class FancyDirective {}
     <input class="fancy" appFancy />
     <div class="fake-button" role="button" (click)="noop()">Unreachable</div>
     <span class="focusable-no-key" tabindex="0" (click)="noop()">No key handler</span>
+    <!-- Key handling through event modifiers: (keydown.enter) is still keydown. -->
+    <span class="key-modifiers" tabindex="0" (click)="noop()" (keydown.enter)="noop()" (keydown.space)="noop()">Keys</span>
+    <div role="tablist" aria-label="Roving" (keydown.arrowRight)="noop()" (keydown.arrowLeft)="noop()">
+      <div role="tab" tabindex="0">One</div>
+      <div class="roving-tab" role="tab" tabindex="-1">Two</div>
+    </div>
+    <!-- Half-built: out of the tab order, but nothing handles the arrow keys. -->
+    <div role="tablist" aria-label="Half built">
+      <div role="tab" tabindex="0">One</div>
+      <div class="half-built-tab" role="tab" tabindex="-1">Two</div>
+    </div>
   `,
 })
 export class HomePageComponent {

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@ngbracket/a11y-devtools",
-  "version": "0.15.3",
+  "version": "0.15.4",
   "description": "Dev-only in-app accessibility auditing for Angular that maps each axe violation back to the component that rendered it — the attribution React overlay tools can't do.",
   "license": "MIT",
   "author": "Duncan Faulkner",

src/keyboard/keyboard-scan.ts

@@ -17,6 +17,7 @@ import {
   resolveComponentPath,
   resolveDirectiveNames,
   resolveListenerEvents,
+  ngDebug,
 } from '../attribution.js';
 import type { A11yFinding } from '../scan.js';
 import {
@@ -62,6 +63,15 @@ const INTERACTIVE_ROLES = new Set([
 
 const KEY_EVENTS = ['keydown', 'keyup', 'keypress'];
 
+/**
+ * True when `events` includes a key event. Angular reports a listener by the name
+ * in the template, key modifiers and all (`(keydown.enter)` → `keydown.enter`),
+ * so match on the event before the first dot.
+ */
+function hasKeyEvent(events: readonly string[]): boolean {
+  return events.some((e) => KEY_EVENTS.includes(e.split('.')[0]));
+}
+
 /**
  * Composite widgets: Tab reaches the widget once, and arrow keys move between
  * its items (WAI-ARIA APG "Keyboard navigation inside components").
@@ -71,17 +81,26 @@ const COMPOSITE_SELECTOR = ['tablist', 'toolbar', 'menu', 'menubar', 'radiogroup
   .join(',');
 
 /**
- * True when `element` is an item that its composite widget reaches with arrow
- * keys, so Tab skipping it is correct:
- * - **aria-activedescendant**: focus stays on the widget (or on a combobox that
- *   controls it) and points at the item; or
- * - **roving tabindex**: the item has a tabindex (usually -1), so script can
- *   focus it, and Tab can get into the widget: one of its items is tabbable,
- *   the widget itself is, or another element controls it (a menu or listbox
- *   popup, which gets focus when it opens).
- * An item with neither, or a widget Tab can't enter at all, is still reported.
+ * How a composite widget's item is reached, when Tab skipping it may be correct:
+ * - `'reached'`: the keyboard reaches it within the widget —
+ *   **aria-activedescendant** (focus stays on the widget, or on a combobox that
+ *   controls it, and points at the item), or **roving tabindex** (the item has a
+ *   tabindex, usually -1, so script can focus it; Tab can get into the widget —
+ *   one of its items is tabbable, the widget itself is, or another element
+ *   controls it, like a menu button — and something handles keys: see
+ *   {@link handlesKeys}).
+ * - `'no-keys'`: set up for roving tabindex, but no key handling was found, so
+ *   arrow keys may not move to it — the classic half-built tab list.
+ * - `false`: not a composite item, or one Tab can't get into / script can't focus.
+ *
+ * Key listeners are read through Angular's dev-mode debug API. Without it (a
+ * production build) nothing can be seen, so roving tabindex counts as reached.
  */
-function reachedWithinWidget(element: Element, isVisible?: (el: Element) => boolean): boolean {
+function reachedWithinWidget(
+  element: Element,
+  isVisible?: (el: Element) => boolean,
+  keyCache: Map<Element, boolean> = new Map(),
+): 'reached' | 'no-keys' | false {
   const widget = element.parentElement?.closest(COMPOSITE_SELECTOR);
   if (!widget) return false;
   const controllers = widget.id
@@ -91,11 +110,52 @@ function reachedWithinWidget(element: Element, isVisible?: (el: Element) => bool
           .includes(widget.id),
       )
     : [];
-  if ([widget, ...controllers].some((el) => el.hasAttribute('aria-activedescendant'))) return true;
+  if ([widget, ...controllers].some((el) => el.hasAttribute('aria-activedescendant'))) return 'reached';
 
   const tabindex = element.getAttribute('tabindex');
   if (tabindex === null || Number.isNaN(Number.parseInt(tabindex, 10))) return false;
-  return isTabbable(widget, isVisible) || controllers.length > 0 || hasTabbableDescendant(widget, isVisible);
+  const enterable =
+    isTabbable(widget, isVisible) || controllers.length > 0 || hasTabbableDescendant(widget, isVisible);
+  if (!enterable) return false;
+  if (!listenersVisible()) return 'reached';
+  // Same widget, same answer: work it out once per scan, not once per item.
+  let keys = keyCache.get(widget);
+  if (keys === undefined) keyCache.set(widget, (keys = handlesKeys(widget, controllers)));
+  return keys ? 'reached' : 'no-keys';
+}
+
+/** True when Angular's debug API can tell us which listeners an element has. */
+function listenersVisible(): boolean {
+  return typeof ngDebug()?.getListeners === 'function';
+}
+
+/**
+ * True when something that could move focus between `widget`'s items listens
+ * for keys: the widget, an element that controls it, an ancestor up to and
+ * including the nearest component host (Angular Material, for one, puts a tab
+ * list's keydown on a wrapper around the `role="tablist"`), or anything inside
+ * the widget. The walk stops at that host so an app shell's shortcut listener
+ * doesn't vouch for every widget below it.
+ */
+function handlesKeys(widget: Element, controllers: readonly Element[]): boolean {
+  const ng = ngDebug();
+  const body = widget.ownerDocument?.body;
+  const ancestors: Element[] = [];
+  for (let el = widget.parentElement; el && el !== body; el = el.parentElement) {
+    ancestors.push(el);
+    if (isComponentHost(el, ng)) break;
+  }
+  const keyed = (el: Element) => hasKeyEvent(resolveListenerEvents(el));
+  // Cheapest first; the widget's contents (every cell of a grid) last.
+  return [widget, ...controllers, ...ancestors].some(keyed) || [...widget.querySelectorAll('*')].some(keyed);
+}
+
+function isComponentHost(el: Element, ng: ReturnType<typeof ngDebug>): boolean {
+  try {
+    return ng?.getComponent(el) != null;
+  } catch {
+    return false;
+  }
 }
 
 const ITEM_SELECTOR = [...INTERACTIVE_ROLES].map((role) => `[role="${role}"]`).join(',');
@@ -110,14 +170,19 @@ const ITEM_SELECTOR = [...INTERACTIVE_ROLES].map((role) => `[role="${role}"]`).j
  *   keys. `<body>` is never tabbable, so a page-wide shortcut listener doesn't
  *   count.
  */
-function partOfKeyboardOperatedWidget(element: Element, isVisible?: (el: Element) => boolean): boolean {
+function partOfKeyboardOperatedWidget(
+  element: Element,
+  isVisible?: (el: Element) => boolean,
+  keyCache?: Map<Element, boolean>,
+): boolean {
   const item = element.parentElement?.closest(ITEM_SELECTOR);
   if (item?.parentElement?.closest(COMPOSITE_SELECTOR)) {
-    if (isTabbable(item, isVisible) || reachedWithinWidget(item, isVisible)) return true;
+    // 'no-keys' too: the item itself is reported, so its parts needn't be.
+    if (isTabbable(item, isVisible) || reachedWithinWidget(item, isVisible, keyCache)) return true;
   }
   for (let el = element.parentElement; el; el = el.parentElement) {
     if (!isTabbable(el, isVisible)) continue;
-    return resolveListenerEvents(el).some((e) => KEY_EVENTS.includes(e));
+    return hasKeyEvent(resolveListenerEvents(el));
   }
   return false;
 }
@@ -159,6 +224,7 @@ export function scanKeyboard(
   const prefixes = options.frameworkPrefixes ?? DEFAULT_FRAMEWORK_PREFIXES;
   const isVisible = options.isVisible;
   const findings: A11yFinding[] = [];
+  const keyCache = new Map<Element, boolean>();
 
   const make = (
     element: Element,
@@ -204,12 +270,30 @@ export function scanKeyboard(
     // Hidden (a closed <details>, [hidden], display:none): not reachable because
     // it isn't shown. Check it when it is.
     if (isHidden(element, isVisible)) continue;
-    if (!interactiveByRole && partOfKeyboardOperatedWidget(element, isVisible)) continue;
 
     const focusable = isTabbable(element, isVisible);
 
     if (!focusable) {
-      if (interactiveByRole && reachedWithinWidget(element, isVisible)) continue;
+      // A click-only part of something the keyboard operates is a mouse shortcut.
+      // Only for unreachable elements: one Tab reaches is still checked below.
+      if (!interactiveByRole && partOfKeyboardOperatedWidget(element, isVisible, keyCache)) continue;
+      const within = interactiveByRole ? reachedWithinWidget(element, isVisible, keyCache) : false;
+      if (within === 'reached') continue;
+      if (within === 'no-keys') {
+        findings.push(
+          make(
+            element,
+            'ngbr/unreachable-control',
+            'moderate',
+            `Keyboard users may not be able to reach this item: it has role="${role}" and is ` +
+              `out of the tab order, like a roving-tabindex item, but nothing in or around its ` +
+              `widget handles keys, so arrow keys may not move to it. Handle the arrow keys, or ` +
+              `use aria-activedescendant. Heuristic — verify manually.`,
+            `${RULE_DOCS}/unreachable-control`,
+          ),
+        );
+        continue;
+      }
       const reason = interactiveByRole ? `has role="${role}"` : 'has a click handler';
       findings.push(
         make(
@@ -227,7 +311,7 @@ export function scanKeyboard(
 
     // Focusable, but a bare (click) never fires on Enter/Space the way a native
     // button does — so a keyboard user can reach it and still not activate it.
-    const hasKey = events.some((e) => KEY_EVENTS.includes(e));
+    const hasKey = hasKeyEvent(events);
     if (hasClick && !hasKey) {
       findings.push(
         make(

src/testing/cli-from.spec.ts

@@ -101,6 +101,12 @@ describe.skipIf(!built)('CLI: --from re-renders a saved report', () => {
     expect(stderr).toContain("--route, --keyboard can't be used with it");
   });
 
+  it('names only --serve, not a defaulted --base, when --serve is used with it', async () => {
+    const { code, stderr } = await runCli(['--from', saved, '--serve', 'npx ng serve']);
+    expect(code).toBe(2);
+    expect(stderr).toContain("so --serve can't be used with it");
+  });
+
   it('refuses a file that is not a JSON report', async () => {
     const md = join(dir, 'report.md');
     writeFileSync(md, toMarkdown(report));

src/testing/e2e-angular.spec.ts

@@ -143,6 +143,18 @@ describe.skipIf(!ready)('attribution in a real Angular app (E2E)', () => {
       );
     });
 
+    it('counts listeners with key modifiers, like (keydown.enter), as key handling', () => {
+      const on = (marker: string) => page('/').findings.filter((f) => f.html.includes(marker)).map((f) => f.id);
+      expect(on('key-modifiers')).toEqual([]);
+      expect(on('roving-tab')).toEqual([]);
+    });
+
+    it('flags a roving-tabindex tab when nothing handles the arrow keys, as moderate', () => {
+      const tab = find(page('/').findings, 'ngbr/unreachable-control', 'half-built-tab');
+      expect(tab.impact).toBe('moderate');
+      expect(tab.component).toBe('HomePageComponent');
+    });
+
     it('attributes findings on a second route to that route’s page component', () => {
       expect(find(page('/settings').findings, 'color-contrast', 'faint').component).toBe('SettingsPageComponent');
     });