fix(core): link Banner expand toggle to its content via aria-controls#3719
Open
bhamodi wants to merge 1 commit into
Open
fix(core): link Banner expand toggle to its content via aria-controls#3719bhamodi wants to merge 1 commit into
bhamodi wants to merge 1 commit into
Conversation
|
@bhamodi is attempting to deploy a commit to the Meta Open Source Team on Vercel. A member of the Team first needs to authorize it. |
9a06a5f to
08bc772
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Banner's expand/collapse toggle exposedaria-expandedbut had noaria-controls, and its collapsible content region had noid-- so the disclosure relationship (toggle button ↔ the region it shows/hides) was implicit only. Screen-reader users heard the expanded/collapsed state with no way to associate it with the region being controlled.This adds a
useId()-generatedidon the content region and wiresaria-controlsto it on the toggle button. Because the region is conditionally rendered (mounted only while expanded),aria-controlsis set only while the region is mounted, so it never points at a non-existent id. This matches thearia-controls={... ? ... : undefined}pattern already used by the Date inputs (DateInput,DateRangeInput,DateTimeInput) for their conditionally-mounted popovers. Purely additive; no behavior/visual change. Mirrors the sibling fix onCollapsible(#3707).Changes
Banner/Banner.tsx:useId()→idon the content region +aria-controlson the toggle button (set only while the region is mounted).Test plan
node_modules/.bin/vitest run --root . packages/core/src/Banner-- 33 pass, including two new tests (both confirmed failing before the fix):links the expand toggle to its content region via aria-controls-- asserts the toggle'saria-controlsresolves viadocument.getElementByIdto the region containing the banner children.sets aria-controls only while the content region is mounted-- asserts there is noaria-controlswhile collapsed (region unmounted, no dangling reference), and that it resolves to the mounted region after expanding.node_modules/.bin/tsc --project packages/core/tsconfig.json --noEmit-- clean.node_modules/.bin/eslinton changed files -- clean.Notes
Found during a broader a11y audit of core components. Scoped to one fix per PR.
Buttonforwards arbitraryaria-*props to the underlying<button>(the same spread the existingaria-expandedrelies on), soaria-controlsreaches the DOM. Skippedrole="region"/aria-labelledbyto keep the change minimal and consistent with #3707.