Add trigger ARIA state for detached AnchoredOverlay/SelectPanel anchors#8097
Add trigger ARIA state for detached AnchoredOverlay/SelectPanel anchors#8097joshfarrant wants to merge 5 commits into
Conversation
In detached-anchor mode (renderAnchor={null}) the consumer owns the trigger
element, so AnchoredOverlay never rendered the aria-haspopup/aria-expanded it
applies in the renderAnchor path. Screen reader users had no indication the
trigger opened a popup, nor its expanded state. (github/primer#6776)
- AnchoredOverlay: imperatively reflect aria-haspopup/aria-expanded onto the
detached anchor node, mirroring the existing anchor-name effect. The effect
reads anchorRef.current (available after commit) so the collapsed trigger is
labelled on mount, guards against clobbering a consumer-supplied
aria-haspopup, and cleans up on close/unmount.
- Add an anchorHasPopup prop ('true' | 'dialog' | 'menu' | 'listbox' | 'tree'
| 'grid', default 'true') so the popup role can be described accurately;
applied in both the renderAnchor and detached paths.
- SelectPanel passes anchorHasPopup="dialog" to match its role="dialog" popup.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🦋 Changeset detectedLatest commit: 04c6a90 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
Bot
added
the
integration-tests: recommended
|
There was a problem hiding this comment.
Pull request overview
Improves accessibility for AnchoredOverlay and consumers (notably SelectPanel) by ensuring trigger elements in detached-anchor mode receive appropriate popup semantics (aria-haspopup) and state (aria-expanded), matching the behavior of the renderAnchor path.
Changes:
- Added
anchorHasPopupprop toAnchoredOverlayand applied it to the rendered-anchor path and detached-anchor path. - Added an effect to imperatively reflect
aria-haspopup/aria-expandedonto a detached anchor element. - Updated
SelectPanelto useanchorHasPopup="dialog"and added/updated tests to cover both rendered and detached anchors.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/SelectPanel/SelectPanel.tsx | Passes anchorHasPopup="dialog" to accurately describe SelectPanel’s dialog overlay. |
| packages/react/src/SelectPanel/SelectPanel.test.tsx | Updates and adds coverage for aria-haspopup="dialog" and aria-expanded on default + detached anchors. |
| packages/react/src/AnchoredOverlay/AnchoredOverlay.tsx | Introduces anchorHasPopup and adds detached-anchor ARIA reflection via an effect. |
| packages/react/src/AnchoredOverlay/AnchoredOverlay.test.tsx | Adds tests for detached-anchor ARIA behavior and the new anchorHasPopup prop. |
| packages/react/src/AnchoredOverlay/AnchoredOverlay.docs.json | Documents the new anchorHasPopup prop. |
| .changeset/anchored-overlay-detached-anchor-aria.md | Adds a patch changeset for the accessibility fix + SelectPanel update. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Low
- Derive AnchorHasPopup from React.AriaAttributes['aria-haspopup'] (excluding boolean/'false') instead of hand-maintaining a parallel union, and export it so consumers and tests stay in sync. - Split the detached-anchor ARIA into two effects: aria-haspopup (stable, no longer keyed on `open`, so it isn't removed/re-added on every toggle) and aria-expanded (reflects `open`). - Reference the exported AnchorHasPopup type in tests instead of duplicating the union. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressed both review comments in 491679e:
|
Add anchorRef to the dependency arrays of the detached aria-haspopup and aria-expanded effects and drop the exhaustive-deps suppressions. anchorRef is stable in the common case (so no extra re-runs), but including it keeps the effects honest and ensures they re-run if a consumer swaps the anchorRef prop to a new RefObject. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressed in 98484d9: added |
…rs, revert SelectPanel dialog The github-ui integration test surfaced two regressions: 1. axe aria-allowed-attr violations: consumers using detached mode with the anchorRef pointing at a non-interactive element (e.g. a wrapper <div>) had aria-haspopup/aria-expanded imperatively written onto them, which is invalid ARIA. Guard the writes to only apply when the anchor element's role supports these attributes (native interactive elements or an appropriate explicit role). 2. SelectPanel changed aria-haspopup from 'true' to 'dialog' for ALL anchors, breaking downstream expectations. Revert to the existing 'true' default to keep the fix scoped to the detached-anchor gap #6776 describes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The integration test's remaining failure was DatePicker's 'Single Input Anchor', which uses a plain <input> (role textbox) as the detached anchor. role=textbox doesn't support aria-expanded, so writing it is still an axe aria-allowed-attr violation. Restrict the imperative write to genuinely button-like triggers: native <button>/<a href>/<summary>, or an explicit role that supports both aria-haspopup and aria-expanded (button, combobox, menuitem, link, tab, etc.). Plain inputs and divs are skipped; a consumer wanting the semantics on an input should give it role=combobox. Added tests for the input (skipped) and role=combobox (written) cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Integration test results from github/github-ui PR:
All checks passed! |
There was a problem hiding this comment.
This looks good to me implementation-wise, and satisfies https://github.com/github/primer/issues/6776. I kind of feel like if the consumer is choosing to use a custom anchor then they should also be responsible/empowered for handling the accessibility and attributes of it however they see fit 🤔
Added some more thoughts here https://github.com/github/primer/issues/6776#issuecomment-4861619362
| "required": false, | ||
| "description": "Indicates the type of popup opened by the anchor. Defaults to `'true'`, which is equivalent to `'menu'` in ARIA.", | ||
| "defaultValue": "'true'" | ||
| }, |
There was a problem hiding this comment.
I have thoughts on the API, but it's a a good to have for the bug fix we are doing here and I don't want to block the bug fix.
Can you split this PR into 2:
- the first PR is the bug fix that copies current renderAnchor behavior (value: true)
- second PR lets you customize the value of aria-haspopup, so that we can discuss it there.
Thanks!
Summary
In detached-anchor mode (
renderAnchor={null}, where the consumer owns and renders the trigger element),AnchoredOverlaynever applied thearia-haspopup/aria-expandedattributes it normally sets in therenderAnchorpath. Screen reader users had no indication that the trigger opens a popup, nor whether it's currently expanded.Fixes the accessibility gap described in github/primer#6776.
What changed
AnchoredOverlay— imperatively reflectsaria-haspopupandaria-expandedonto the detached anchor node, mirroring the existinganchor-nameeffect that already writes to that consumer-owned element. The effect:anchorRef.current(attached by commit time) so the collapsed trigger is labelled on mount — not just after the overlay is first opened;aria-haspopup;aria-expandedfromopen, and cleans up on close/unmount.anchorHasPopupprop —'true' | 'dialog' | 'menu' | 'listbox' | 'tree' | 'grid', default'true'(non-breaking;'true'is spec-equivalent to'menu'). Applied in both therenderAnchorand detached paths so the popup role can be described accurately.SelectPanelpassesanchorHasPopup="dialog"to match itsrole="dialog"popup (previously it hardcoded the spec-equivalent-of-menu default, which mis-described the dialog).Before / after
aria-haspopup, noaria-expandedaria-haspopup="…"aria-expanded="false"aria-expanded="true"aria-haspopup="dialog"aria-expanded="true"Testing
Added coverage on
AnchoredOverlay(collapsed state, open/close toggle, consumer-override,renderAnchorpath, cleanup) andSelectPanel(aria-haspopup="dialog"on both default and detached anchors)