test(label-content-name-mismatch): cover visually-hidden text exclusion - #5291
test(label-content-name-mismatch): cover visually-hidden text exclusion#5291chutchins25 wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR extends the label-content-name-mismatch rule fixtures and tests to cover cases where visually-hidden descendant text should be excluded from the element’s visible text when comparing against the accessible name.
Changes:
- Added a new passing integration fixture case (
#pass10) involving visually-hidden text inside a link. - Added unit tests verifying behavior with visually-hidden descendants (including a shadow DOM case).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/integration/rules/label-content-name-mismatch/label-content-name-mismatch.json | Adds #pass10 to the list of passing selectors. |
| test/integration/rules/label-content-name-mismatch/label-content-name-mismatch.html | Adds a new <a id="pass10"> fixture with visually-hidden nested text. |
| test/checks/label/label-content-name-mismatch.js | Adds new test cases (including shadow DOM) for visually-hidden descendant handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The case placed the whole element inside one shadow root, so it never crossed a shadow boundary and only duplicated the light-DOM coverage.
Addressed in 3bf48f1 — removed the in-boundary shadow DOM test per your comment and resolved the thread. Re-requesting review.
scottmries
left a comment
There was a problem hiding this comment.
Built the branch and ran the check unit suite (26/26) and the generated integration suite (#pass10 green), plus prettier --check and eslint — all clean. I also reverted label-content-name-mismatch-evaluate.js to 3a012a14~1 to confirm which of the new tests actually guard the #5096 fix; the inline comments reference that.
Two things to address, both small additions rather than rework:
- Shadow DOM coverage was dropped rather than corrected — the code path this PR exists to protect (
visibleVirtualrecursing the flattened tree) is now untested. - #5096 flipped a sibling of the reported pattern from pass to violation, and this PR is what closes #4678 without pinning or noting it.
Plus two suggestions on test naming and fixture readability. Details inline.
Restore an open-shadow-DOM case that crosses a boundary (the visibleVirtual flattened-tree path), rewrite the mismatch test so it distinguishes visible-only from subtree comparison, pin the mid-hidden-text pattern as a failing fixture, and move the visually-hidden style into a shared class.
Addressed all four points (inline replies): restored a cross-boundary open-shadow-DOM test, rewrote the mismatch test to distinguish visible-only from subtree, pinned the mid-hidden pattern as #fail8, and moved the style to a shared .visually-hidden class. Re-requesting review.
scottmries
left a comment
There was a problem hiding this comment.
All items from my last round are addressed, and I confirmed the tests do what they claim rather than taking it on trust. Reverted label-content-name-mismatch-evaluate.js to 3a012a14~1 (pre-#5096), rebuilt, and re-ran: all three new unit tests fail there and pass here — so each is a real guard, including the rewritten mid-hidden case, which previously passed under both implementations. The shadow DOM case crosses a boundary now (queryShadowFixture attaches the root to the <span id="shadow"> child; #target stays in light DOM), so visibleVirtual's flattened-tree recursion is what's exercised. Check unit and generated integration suites both green.
On your open question: #fail8 as a fail is right — don't switch it to incomplete. Visible label deque are great isn't a contiguous run within deque labs are great, which is a genuine 2.5.3 concern and what ACT 2ee8b8 expects. It also stays a fail under #5302's word-level matching, so the two PRs agree.
Since these are in flight together: #5291 and #5302 both touch all three of these files and conflict textually (the const block and the tail of the check test file). I merged them locally — the conflicts are adjacent additions, and #pass10, #fail8 and #incomplete18 all stay green under the combined code, along with the full 30-test check suite. Rebase, not a rethink.
One suggestion: the description is now behind the branch. It doesn't mention #fail8 at all — which is the most consequential thing here, since it pins a pass→violation flip from #5096 that nothing else records — still describes the third unit test as "the mismatch counterpart", omits the .visually-hidden refactor, and cites axe 4.12.1 when develop is 4.13.0. Worth refreshing, since this PR is the durable record of what closing #4678 covered.
Garbee
left a comment
There was a problem hiding this comment.
The tests here are good and genuinely discriminating — each of the three new unit cases flips on pre-#5096 code, and the integration pair is correctly classified. Four things to resolve before merge: one about what this PR closes, three about what the tests actually pin.
Don't close #4678 yet
Issue #4678 reports two defects. The first — hidden text wrongly included — is what this PR covers. The second is the reporter's <a href="#">deque <span class="visually-hidden">labs</span> are great</a> with no aria-label, which still isn't reported: lib/rules/label-content-name-mismatch-matches.js:33-38 returns false when both aria-label and aria-labelledby are empty, so the node never reaches the check.
The body defers that to #5203, but #5203 is scoped to ACT rule 2ee8b8 "failed example 4" and general ACT drift — it never names the aria-label/aria-labelledby applicability gate. So as written, merging auto-closes a report whose second half has no home.
Either change the footer to Refs #4678 and leave the issue open, or add a comment to #5203 quoting the reporter's second repro verbatim and naming the matches-level gate as the cause — then Closes is honest. Second option is cleaner if @straker agrees #5203 is the right home; worth a quick check with them since it's their rule.
The shadow DOM test doesn't pin shadow traversal
test/checks/label/label-content-name-mismatch.js:243 passes under two mutually exclusive conditions: visibleVirtual descends into the shadow root and the span is excluded (intended), or it never visits the shadow subtree at all (broken). Either way visibleText is "deque are great" and the check returns true. Break flattened-tree recursion in lib/commons/text/visible-virtual.js and this test stays green, and there's no shadow case in test/commons/text/visible-virtual.js to backstop it.
It does still guard the original #5096 regression, so it isn't dead weight — but it doesn't deliver the cross-boundary guard @scottmries asked for. The fix is one negative counterpart: same markup, but a visible <span>labs</span> in the shadow root against aria-label="deque are great". Traversal working gives visibleText = "deque labs are great", which isn't contained in the name → isFalse. Traversal broken gives true and the test fails loudly. Adding that turns the existing isTrue case into a real guard, since the pair can only both pass if the boundary is genuinely crossed.
#fail8 pins behavior without recording whether it's intended
isStringContained does a contiguous includes(), so hidden text interrupting the visible label mid-string can never match. Whether SC 2.5.3 intends a fail here, or this is a known contiguous-matching limitation, isn't recorded anywhere retrievable — and you offered upthread to switch it to #incomplete, which tells me it's genuinely undecided.
Whichever way it lands, the decision needs to survive outside this diff, because #5203's ACT-alignment work will hit this exact case and see a bare #fail8 under a plain <!-- Fail --> heading. Get a ruling from @straker / @scottmries, then encode it: if it's intended, rename the unit test to name the reason (something like returns false when the accessible name includes text present only in a visually-hidden child) and add a line to the PR body cross-referencing #5203; if it's a limitation, #incomplete plus the same naming. Please don't settle this with a code comment — repo convention is to keep it in the name and the changelog/PR record.
The fixtures don't exercise clip — they're hidden by overflow
Both the unit constant and the .visually-hidden class are excluded by overflowHidden, not clipHidden. hiddenMethods in lib/commons/dom/is-visible-on-screen.js:13-19 runs overflowHidden before clipHidden and short-circuits; getOverflowHiddenAncestors includes the node itself, the position: absolute escape hatch needs the overflow ancestor to be static (here the ancestor is the absolute span), and nodeRect.width < 2 fires on the 1px width. clipHidden is never reached.
I confirmed this in Chrome rather than by reading: running the PR's exact fixture with and without the clip declaration produces an identical result, and a fixture with only position:absolute;width:1px;height:1px;overflow:hidden and no clip at all is still excluded. So the clip value in both files is inert, and the clip branch of the visibility stack is unpinned for this rule despite the fixtures being named for it.
Not a correctness bug — the coverage claim just reads broader than it is. Cheapest fix is one more variant that isolates the branch: position:absolute; clip:rect(0px,0px,0px,0px) with normal dimensions and no overflow:hidden. I verified that one is excluded on its own, so it does pin clipHidden. Worth considering a clip-path: inset(50%) sibling in the same pass, since that's what Bootstrap 5 and Tailwind sr-only ship today and it's a separate branch in the same function — but that one's optional.
…bility Add a cross-boundary negative case that only passes when shadow traversal genuinely crosses the boundary, and clip/clip-path fixtures that isolate clipHidden from overflowHidden.
|
Thanks @Garbee — all four addressed, one pending a ruling. 1. Don't close #4678 yet — agreed. Changed the footer to 2. Shadow test doesn't pin traversal — added the negative counterpart you described (commit 55794f0): a visible 4. Fixtures don't exercise 3. |
Summary
The false positive reported in #4678 — the rule "wrongly includes invisible text" — was already fixed in #5096, which switched the check from
subtreeText(all text) tovisibleVirtual(visible-on-screen text). That fix shipped without regression coverage. This PR adds it.Verified
Built current
developand ran the reporter's exact repro (Bootstrap.visually-hiddenCSS):<a aria-label="deque are great">deque <span class="visually-hidden">labs</span> are great</a>→ passes (no violation). Thelabstext is correctly excluded from the visible-text comparison.Tests added
test/checks/label/label-content-name-mismatch.js): a visually-hidden child excluded from the visible text (true); the same across an open shadow DOM boundary, paired with a negative case (visible shadow text) so the pair only passes when traversal genuinely crosses the boundary; clip and clip-path fixtures that isolateclipHiddenfromoverflowHidden; and the mid-hidden mismatch (false)..visually-hiddenpattern as#pass10, and the mid-hidden pattern as#fail8.Out of scope / follow-ups
aria-label/aria-labelledbyare gated out atlabel-content-name-mismatch-matches.js:33-38and never evaluated. This PR only covers the first (FP), so itRefslabel-content-name-mismatch wrongly includes invisible text #4678 rather than closing it. That gate is quoted verbatim and named on Get label-content-name-mismatch consistent with ACT's equivalent #5203.#fail8(mid-hidden text) pins current behavior as a violation; whether SC 2.5.3 intends a fail here or it's a contiguous-matching limitation (→incomplete) is an open question for @straker / @scottmries (see the PR comment).Refs #4678