fix(label-content-name-mismatch): compare visible label to name by word - #5302
fix(label-content-name-mismatch): compare visible label to name by word#5302chutchins25 wants to merge 5 commits into
Conversation
Follow ACT rule 2ee8b8's "label in name" algorithm: treat non-text characters as word separators (replacing them with a space instead of removing them) and require the visible label's words to appear as a contiguous run within the accessible name, rather than matching as a raw substring. Hyphenated labels like "non-standard" no longer match "nonstandard". Add a replaceWith option to removeUnicode to preserve word boundaries. Closes issue #4311
There was a problem hiding this comment.
Pull request overview
This PR updates the label-content-name-mismatch check to follow ACT rule 2ee8b8’s “label in name” algorithm more closely by changing how visible label text is normalized and compared to the accessible name.
Changes:
- Update comparison logic from raw substring matching to word-token matching requiring a contiguous word run.
- Extend
removeUnicodewith a backward-compatiblereplaceWithoption to preserve word boundaries when removing non-text characters. - Update/expand unit, integration, and ACT conformance tests (including removing the ACT skip list and adding a new integration failure case).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/integration/rules/label-content-name-mismatch/label-content-name-mismatch.json | Adds #fail8 to integration fail cases to cover hyphenation behavior. |
| test/integration/rules/label-content-name-mismatch/label-content-name-mismatch.html | Adds a new failing fixture for non-standard vs nonstandard. |
| test/commons/text/unicode.js | Adds coverage for removeUnicode’s new replaceWith option. |
| test/checks/label/label-content-name-mismatch.js | Adds unit tests for hyphenation and contiguous-word matching behavior. |
| test/act-rules/visible-label-in-accessible-name-2ee8b8.spec.js | Removes the skip list (now runs full ACT suite). |
| lib/commons/text/remove-unicode.js | Adds replaceWith option so removed characters can be replaced (e.g. with spaces). |
| lib/checks/label/label-content-name-mismatch-evaluate.js | Implements ACT-aligned tokenization and contiguous word-run matching. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
sanitize() only collapses runs of two or more whitespace characters, so a lone newline or tab could keep two words in one token. Split on any whitespace so the word-level comparison stays correct.
scottmries
left a comment
There was a problem hiding this comment.
The word-level contiguous-subsequence change is the right algorithm — it matches the spec's own wording, and against latest ACT it fixes two testcases the PR body doesn't claim (Failed Example 3 Discover It/Discover Italy, Failed Example 15 1/1a). tsc, prettier, eslint, the check unit suite, the removeUnicode tests and the rule's integration suite are all green locally, and ACT 2ee8b8 against the pinned dep is 15/15 as claimed.
The problem is what happens against wcag-act-rules#main, which is what the nightly act job installs. I ran the 2ee8b8 suite against main twice — once with develop's lib code, once with this branch's — both with the empty skip list:
| Testcase | develop | this PR |
|---|---|---|
Passed 11 — <span> whitespace |
fail | fail |
Passed 14 — Search by date (YYYY-MM-DD) |
fail | fail |
Failed 3 — Discover It / Discover Italy |
fail | pass ✅ |
Failed 15 — 1 / 1a |
fail | pass ✅ |
Inapplicable 5 — University Ave. / University Avenue |
pass | fail ⛔ |
Inapplicable 6 — nonstandard / non-standard |
pass | fail ⛔ |
Both end at 34 passing / 4 failing. Today the nightly is green only because all four failures sit behind skips, so emptying the list turns it red. And the two new failures are there because ACT has since moved hyphenation and abbreviation differences out of the rule's scope — the opposite of what this PR implements. Details in the two inline comments on those lines; three smaller notes below them.
…applicable Per ACT rule 2ee8b8, a difference that is only hyphenation is inapplicable rather than a failure. Return undefined (needs review) when the label is contained in the name once hyphens are removed, so "non-standard" and "nonstandard" no longer report a violation. Also compare labels by whole words (contiguous run) rather than raw substring, rename isStringContained to isLabelContainedInName, and add a replaceWith option to removeUnicode. Closes issue #4311
|
@scottmries — thanks for the thorough review, especially catching the Reworked accordingly (per-point details in the inline replies):
Verified against |
Addressed all points in the latest push (inline replies + summary comment): implemented the hyphenation applicability narrowing, restored #5207's skips untouched, and corrected the skip list. 2ee8b8 against wcag-act-rules#main is now 33 passing / 5 pending / 0 failing. Re-requesting review.
scottmries
left a comment
There was a problem hiding this comment.
Re-reviewed against main — the three items from last round are resolved, and I reproduce your numbers. Locally on node 24: tsc, prettier, eslint clean; the checks, commons and integration unit suites green; 2ee8b8 is 15/15 against the pinned dep and 33 passing / 5 pending / 0 failing against wcag-act-rules#main.
Worth recording somewhere: undefined is the only value that satisfies both dependencies at once — pinned e9bbdbec is Failed Example 4 (expects failed, and act-runner accepts incomplete for that), main is Inapplicable Example 6 (expects zero violations). true would break the pinned run.
One blocker. replaceWith: ' ' also lands on getCategoryFormatRegExp() — Unicode category Cf, which is zero-width. Those characters can't create a visible word boundary, but they now split one visible word into two tokens. I built both 71c46831 and this branch and ran the same fixtures:
| visible text | accessible name | develop | this PR |
|---|---|---|---|
non­standard (U+00AD) |
nonstandard |
pass | violation ⛔ |
time\u200Bline (ZWSP) |
timeline |
pass | violation ⛔ |
mi\u200Cshavad (ZWNJ) |
mishavad |
pass | violation ⛔ |
sea\u200Erch (LRM) |
search |
pass | violation ⛔ |
sub\uFEFFmit (BOM) |
submit |
pass | violation ⛔ |
ZWNJ is grammatically required in Persian and Indic scripts, ­ is the standard long-word hyphenation hint, and bidi marks are pervasive in RTL content — so these are new false positives on a serious rule. Fix inline; three smaller notes below it.
…andling Strip zero-width format characters before tokenizing so they can't create a word boundary, widen the dash family removeHyphens covers to match the tokenizer, and add a templated "differ only in hyphenation" incomplete message so the needs-review result explains itself.
Addressed round 2 (inline replies): strip zero-width format chars before tokenizing, widen removeHyphens to the full dash family, add a templated 'differ only in hyphenation' incomplete message, and note #5203 at the branch. ACT vs main still 33 pass / 5 pending / 0 fail. Re-requesting review.
scottmries
left a comment
There was a problem hiding this comment.
0c949242 addresses all four items. I re-ran the fixtures rather than reading the diff:
- Zero-width (the blocker): soft hyphen, ZWSP, ZWNJ, LRM and BOM all return
trueagain, matching develop. - Dash family: all six visible dash variants now return
undefinedconsistently, and soft hyphen returnstrue. The three-way split is gone.
tsc, prettier, eslint clean; checks, commons and integration suites exit 0; test-locales 20/20 with the new key; 2ee8b8 15/15 against the pinned dep. The messageKey plumbing is right — process-message.js reads data.messageKey, MockCheckContext is wired with an afterEach reset, and _template.json is regenerated and committed.
Two things worth a line in the PR body, since both are user-visible and neither is mentioned: the doc/rule-descriptions.md flip to "failure, needs review" is a fix (the rule already returned undefined on the isHumanInterpretable path, so the table was wrong), and the new default message replaces the generic "axe couldn't tell the reason" fallback for the 17 pre-existing incomplete cases.
Since #5291 is in flight against the same three files: the two conflict textually. I merged them locally — conflicts are adjacent additions, and #pass10, #fail8 and #incomplete18 all stay green under the combined code. Rebase, not a rethink.
One suggestion inline.
| // The whole dash family, so it stays consistent with the dashes that | ||
| // `getPunctuationRegExp` treats as word separators during tokenizing. | ||
| return str.replace(/[\u002D\u2010-\u2015\u2212]/g, ''); |
There was a problem hiding this comment.
Suggestion — the widened class is untested: every hyphenation test uses ASCII -, so nothing pins \u2010-\u2015\u2212. I confirmed all six dash variants return undefined, but a future narrowing would pass CI silently. One case with aria-label="email" / e–mail would cover it.
Also, U+2212 isn't matched by getPunctuationRegExp — it's in \u2200-\u22FF, i.e. getUnicodeNonBmpRegExp. Both feed the tokenizer so the reasoning holds; "the dashes the tokenizer treats as word separators" would be accurate.
Garbee
left a comment
There was a problem hiding this comment.
The move from substring matching to word-token matching is the right direction for 2ee8b8, and the hyphenation incomplete branch is a nice touch. Two blockers before this can go in, plus three smaller items — all inline.
Blockers
-
Localized bundles will throw on the new
hyphenationmessage key.build/configure.mjsreplacescheck.metadata.messageswholesale with the locale entry, and none of the 18 non-English locales defineincompletefor this check, sopublish-metadata.js's unguardedmessages.incomplete[checkData.messageKey]becomesundefined['hyphenation']. This branch is newly reachable — previously the check never set amessageKey, so incompletes always took the guarded default path. (The runtimeaxe.configure({ locale })path is unaffected;mergeCheckLocalepreserves the baseincompleteobject.) -
Non-hyphen punctuation silently regresses to hard violations. Punctuation used to be deleted before comparison and is now a word separator, but only hyphens get an escape hatch.
it'svsits book,don't savevsdont save now, andu.s.avsusaall flip from pass to fail. Apostrophes are everyday UI copy; this needs to be either generalized to all punctuation-only differences or deliberately pinned with tests.
Also worth addressing
replaceWithis aString.prototype.replacepattern, not a literal —$&,$`,$',$$are live on a newly publicaxe.commons.textoption. Cheap to close now, breaking to close after release.removeHyphens's dash range is untested beyond ASCII-; narrowing it to/-/gleaves the suite green.- The
removeHyphenscomment attributes U+2212 togetPunctuationRegExp, which doesn't match it (it comes fromgetUnicodeNonBmpRegExp). - No open Shadow DOM case — pre-existing, but this change makes tokenization sensitive to boundary text concatenation.
For transparency: I reviewed this statically and by executing the tokenizer and the locale/ACT lookups in isolation — I did not run the karma or ACT suites, so the test-related points above are reasoned from the code rather than observed failures.
…d harden messages Treat any punctuation-only difference (not just hyphenation) as incomplete rather than a violation, so apostrophes, periods and dashes no longer flip labels like "it's"/"its book" to false positives. Fall back to a default incomplete message when a localized bundle omits one, and make removeUnicode's replaceWith a literal insertion.
Addressed all six points (inline replies): generalized the punctuation carve-out to incomplete (removeHyphens removed, messageKey 'punctuation'), guarded the localized-bundle incomplete-message path + regression test, made removeUnicode's replaceWith literal, and added an open-shadow-DOM boundary case. ACT vs main still 33 pass / 5 pending / 0 fail. Re-requesting review.
|
@Garbee — thorough review, thank you. Both blockers and the three smaller items are addressed in the latest push (per-point details in the inline replies). You flagged that you ran the tokenizer and the locale/ACT lookups in isolation but held off on the full karma/ACT suites, so for completeness I ran those against the changes — check unit (incl. the new apostrophe/period/en-dash One mechanism correction worth surfacing (also on the |
scottmries
left a comment
There was a problem hiding this comment.
Reopening — my approval was for 0c949242 (+150/-29); this is +257/-35 and changes behavior I'd signed off on. ACT is still clean (15/15 pinned, 33 passing / 5 pending / 0 failing vs main), and tsc/prettier/eslint plus the checks, commons, core and integration suites all pass. Two concerns with the new commit, both inline.
To be clear about what I actually asked for last round: a test pinning the \u2010-\u2015\u2212 range. This commit deleted the range instead and generalized the carve-out — that's a bigger change than the note warranted.
| // Localized bundles replace `messages` with a locale entry that may omit | ||
| // `incomplete`, so fall back to the default rather than throwing. | ||
| return ( | ||
| messages.incomplete?.[checkData.messageKey] ?? getDefaultMsg(messages) |
There was a problem hiding this comment.
Important — this is core message resolution for every check, in a PR scoped to one rule (plus the removeUnicode replacer change). CLAUDE.md §3 is "one change per PR."
The commit message justifies it as preventing a throw. That doesn't reproduce — I tested both routes against 0c949242:
axe.configure({locale})with a bundle lackingincomplete(the shape shipped inde/fr/ja) merges cleanly: no throw, correct message.axe.configure({checks})replacing the metadata wholesale: no throw either, justmessage: undefined.
So the real symptom is an undefined message, not a crash, and the new test reaches it via a synthetic axe._load rather than a public path. Worth fixing — in its own PR, with the justification corrected.
| // TODO(#5203): the incomplete result here is load-bearing only while the | ||
| // pinned wcag-act-rules dep is stale. When the dep bump tracks `main`, | ||
| // revisit whether these should stay incomplete or stop matching the rule. | ||
| if (isLabelContainedInName(visibleText, accText, { joinNonText: true })) { |
There was a problem hiding this comment.
Important — ACT 2ee8b8's applicability carves out abbreviations and hyphenation, not punctuation generally. joinNonText: true strips all punctuation, emoji and symbols, so genuine violations become undefined. Verified 0c949242 → this commit:
| visible / name | before | after |
|---|---|---|
it's / its book |
violation | incomplete |
us / u.s. army |
violation | incomplete |
1000 / 1,000 items |
violation | incomplete |
The abbreviation case is arguably right. 1,000 items is a plain punctuation difference ACT doesn't exempt — a speech user saying "1000" wouldn't match "1,000 items", which is exactly what 2.5.3 is about.
ACT stays green either way, so conformance isn't what's driving this. Suggest narrowing back to the hyphenation-scoped carve-out (plus abbreviations if you want to pick up #4821), or making the case in the PR body for why punctuation-wide is the right reading.
|
Surfacing a scope question where two reviews point different directions, so we settle it before I recode. cc @scottmries @Garbee @straker. The carve-out currently generalizes (this commit): a difference that is only non-text characters — hyphenation, apostrophes, periods, commas — returns @Garbee asked for exactly this: apostrophe/period cases like @scottmries (reopening) reads it as too broad: 2ee8b8 carves out abbreviation + hyphenation, not punctuation generally, so genuine violations now return The crux is the apostrophe/comma cases:
So the deciding question: should the carve-out be hyphenation-only (+ abbreviation via #4821), matching 2ee8b8's exemptions, or punctuation-wide to suppress the apostrophe/period false-positives @Garbee flagged? It's your rule @straker — what's the intended scope? I'll implement whichever you land on. Separately, @scottmries — agreed on the |
Summary
Fixes
label-content-name-mismatchfor hyphenated labels, per ACT rule 2ee8b8. Two parts:Discover ItvsDiscover Italythat substring matching missed.undefined(needs review) instead of a violation — sonon-standardvsnonstandardno longer reports a violation.Supporting changes: a backward-compatible
replaceWithoption onremoveUnicode(explicit unicode ranges — no\p{…}escape — to keep Safari 7+/IE11 support), and splitting label tokens on all whitespace.ACT 2ee8b8
The
actnightly installswcag-act-rules#main(38 2ee8b8 testcases), not the pinned dep (15). Verified againstmain: 33 passing / 5 pending / 0 failing.Inapplicable Example 6(non-standard) now passes via the hyphenation handling.Two categories remain skipped, tracked separately:
Passed Example 11) and parenthetical-content removal (Passed Example 14). Its skip entries are left untouched here.University Ave./University Avenue), which ACT also treats as inapplicable but cannot be reliably auto-detected. This PR's word-level matching surfaces one such case, so it is skipped.Related
This advances #5203 (consistency with ACT 2ee8b8) but does not close it — the
wcag-act-rulesdependency bump and the remaining skipped cases (#5207, #4821) are tracked there and in their own issues.Closes #4311