dequelabs / dequelabs/axe-core
feat(matches): replace hasAccessibleName matcher with a re-entrancy-safe hasNameFromAuthor
- Dominant language
- JavaScript
- Stars
- 7.5k
- Forks
- 933
- Avg merge
- 2d 23h
- Merged PRs (30d)
- 17
Description
## Summary
The `hasAccessibleName` matcher runs a full accessible name computation, which re-enters `getElementSpec`. Replace it with a matcher that only reads the node's own naming attributes and resolved idrefs, consolidate the near-duplicate private helper in `implicit-html-roles.js` into the same function, and remove the special case in `get-element-spec.js` that currently breaks the cycle.
Motivated by the design discussion on #5262, where relational matchers (`withAncestor` and similar) would let definitions nest and bypass the existing guard.
## Background
There are two functions doing almost the same job with different semantics:
- `lib/commons/matches/has-accessible-name.js` — the matcher, calls `accessibleTextVirtual` (full accname).
- `lib/commons/standards/implicit-html-roles.js:57-70` — a private helper used by the `aside`, `form` and `section` implicit roles. Its comment already describes this hazard: _"can't go through the normal accessible name computation as it leads into an infinite loop of asking for the role of the element while the implicit role needs the name."_
Both reach `getElementSpec`, the first directly and the second via `arialabelledbyText`:
`getElementSpec` → variant `matches` → accname → `nativeTextAlternative` / `subtreeText` → `getElementSpec`
The cycle is currently broken by a shape check in `lib/commons/standards/get-element-spec.js:33-37`, gated on the `noMatchAccessibleName` option.
**This is not a live bug.** It holds because of four facts that aren't asserted anywhere:
1. `img.variant.nonEmptyAlt` is the only variant using `hasAccessibleName`.
2. Both `getElementSpec` calls inside the accname computation pass `noMatchAccessibleName: true` (`lib/commons/text/native-text-alternative.js:40`, `lib/commons/text/subtree-text.js:22`).
3. The one unguarded call reachable from inside accname (`lib/commons/aria/implicit-role.js:34`, the `chromium` branch) only fires for elements with no `implicitHtmlRoles` entry, and none of those have accname-dependent variants.
4. Definitions can't nest, so the top-level `hasOwnProperty` check sees everything.
Point 4 stops being true as soon as matchers can take nested definitions. Point 1 stops being true the moment anyone adds a second one.
## Additional problems with the current guard
- It does `return standard`, skipping the `variant.default` merge that the normal path performs, and leaking the `variant` key that the normal path strips.
- `img` has no top-level `contentTypes` — they live only in `variant.usemap` and `variant.default` (`lib/standards/html-elms.js:381-394`). `subtree-text.js:22` reads `contentTypes` through the guarded call, so for `` it comes back `undefined` and the `contentTypes?.includes('embedded')` early return never fires.
## Proposal
Add `lib/commons/aria/has-name-from-author.js`, taking `(vNode, { checkTitle = false })` and returning true when any of the following hold:
- `aria-label` has non-empty content after `sanitize` (via `arialabelText`, which uses `getAriaValue` and is node-local)
- `aria-labelledby` resolves to at least one existing element (via `getResolvedRefs`, **without** computing their accessible text)
- `checkTitle` and `title` is non-empty after `sanitize`
`getResolvedRefs` imports only `getRootNode`, `tokenList` / `nodeLookup` / `getNodeFromTree` and `standards`, so resolving the refs without computing their text is safe.
Then:
- Replace the private helper in `implicit-html-roles.js`. Keep the existing per-caller options: `aside` passes `checkTitle: true`, `form` and `section` don't.
- Add a `hasNameFromAuthor` matcher wrapping it with `checkTitle: true`.
- Reduce `matches.hasAccessibleName` to a thin alias of `hasNameFromAuthor`, marked `@deprecated`, so the full accname computation can no longer be reached from any matcher. Its only in-repo consumer is `img.variant.nonEmptyAlt`; no rule or check uses it as a definition key.
- Remove the `noMatchAccessibleName` option and the accname bail-out loop at `get-element-spec.js:33-37`, plus the option at its two call sites. This also fixes the `img.contentTypes` bug above. The defaults merge at `get-element-spec.js:49-56` is unaffected.
This is also what severs the module cycle. `commons/matches/semantic-role.js` currently reaches `accessible-text-virtual` via `get-role` → `implicit-role` → `implicit-html-roles` → `arialabelledby-text`. Aliasing the matcher alone would not break that; replacing `arialabelledbyText` inside `implicit-html-roles.js` does.
## Behaviour changes
**`aria-labelledby` resolving to an element with no accessible text.** `` with `
` currently has no accessible name and no role; afterwards it counts as named and gets `role=region`. Same for `form` and `aside`. `aria-labelledby="does-not-exist"` is unaffected, since the ref doesn't resolve. This is an authoring bug in practice, but it is a real change and needs an integration sweep, not just unit tests.**`hasAccessibleName` matcher semantics.** Since it becomes an alias, custom rules or `axe.configure` data using it will get the weaker predicate. This is a breaking change for that (narrow) API surface: needs a `BREAKING CHANGE` footer, an `@deprecated` tag pointing at the new name, and a CHANGELOG migration note.
## Tasks
- [ ] Add `has-name-from-author.js` with JSDoc stating explicitly that it does not compute referenced text, and why
- [ ] Unit tests: whitespace-only `aria-label`, unresolvable idref, idref resolving to empty content, element internals, reflected properties, `title` with and without `checkTitle`, `SerialVirtualNode`
- [ ] Replace the private helper in `implicit-html-roles.js`, preserving per-caller `checkTitle`
- [ ] Add the `hasNameFromAuthor` matcher; reduce `hasAccessibleName` to a deprecated alias
- [ ] Migrate `img.variant.nonEmptyAlt` in `lib/standards/html-elms.js`
- [ ] Remove the `noMatchAccessibleName` option and the bail-out loop at `get-element-spec.js:33-37`, plus the option at its two call sites
- [ ] Regression test that `` resolves `contentTypes` through `subtreeText`
- [ ] Regression and virtual-rule tests for the `section` / `form` / `aside` role changes
- [ ] Verify `commons/matches/**` no longer reaches `text/accessible-text-virtual` through any import path
- [ ] Update `doc/API.md` and `CHANGELOG.md`
Suggested sequencing: do the `implicit-html-roles.js` replacement and its test sweep as its own commit first, then the matcher work on top. If the sweep turns up more fallout than expected, there is still the option of keeping the bounded behaviour for those three roles and accepting that the module cycle survives.
## Out of scope
Build-time validation of which matchers are permitted in `htmlElms` variant data (including disallowing `condition` and function-valued matchers there). Related and worth filing separately — after this change there should be no grandfathered exceptions left, which is what makes such a check enforceable.
Contributor guide
Research direction
Start with lib/commons/standards/implicit-html-roles.js and lib/commons/standards/get-element-spec.js, then trace the named accessible-name call sites and matcher paths. Run the unit, integration, and virtual-rule tests for hasAccessibleName, section/form/aside roles, and the img contentTypes regression. Done means the listed migration, cycle removal, deprecation and documentation updates, and behavior tests are complete.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- javascript
- Domain
- accessibility, testing
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 38/100