fix(overlay): add ARIA attribute management to overlay-trigger for screen reader accessibility#6044
fix(overlay): add ARIA attribute management to overlay-trigger for screen reader accessibility#6044
Conversation
…reen reader accessibility The overlay-trigger component now automatically manages aria-expanded, aria-controls, and aria-haspopup on the trigger element for click and longpress interactions. This ensures screen readers can announce the overlay relationship and state, fixing WCAG 1.3.2 (Meaningful Sequence) compliance. Also fixes the dialog README behaviors example to use valid overlay types and proper ARIA attributes, and adds comprehensive accessibility documentation to overlay-trigger covering ARIA attributes, focus management, keyboard navigation, and screen reader considerations. Co-authored-by: Cursor <cursoragent@cursor.com>
🦋 Changeset detectedLatest commit: 4b9d2f1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 83 packages
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 |
Rajdeepc
changed the title
📚 Branch Preview Links🔍 First Generation Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
…ment - Add disconnectedCallback to clean up ARIA attributes from trigger element when overlay-trigger is removed from the DOM - Track previous trigger element and clean up stale ARIA attributes when the trigger slot content changes - Clean up aria-controls when content is removed from click/longpress slots (prevents stale ID references) - Use WeakSet to track component-managed aria-haspopup values so consumer overrides are respected while still allowing type changes to update the value - Default aria-haspopup to "dialog" for all overlay types (previously "true" for non-modal, which maps to "menu" per ARIA spec) - Consolidate manageAriaOnTrigger calls to updated() lifecycle only, removing redundant calls from slot change and beforetoggle handlers - Remove misleading static-attribute sp-overlay example from dialog README (aria-expanded would never toggle); use overlay-trigger pattern exclusively since it handles ARIA automatically - Add comprehensive test suite covering: aria-expanded toggling, aria-haspopup defaults and consumer overrides, aria-controls with auto-generated and pre-existing IDs, hover-only exclusion, cleanup on content removal, disconnect, and trigger swap Co-authored-by: Cursor <cursoragent@cursor.com>
Design decisions for team reviewThis PR adds automatic ARIA attribute management to 1. Automatic ARIA management vs. opt-inThe component now automatically sets Question: Are we comfortable with OverlayTrigger taking ownership of these attributes, or should this be opt-in? 2.
|
Rajdeepc
added
a11y
| private removeAriaFromTrigger(element: HTMLElement): void { | ||
| element.removeAttribute('aria-expanded'); | ||
| element.removeAttribute('aria-controls'); | ||
| if (this.ariaHaspopupManagedElements.has(element)) { | ||
| element.removeAttribute('aria-haspopup'); | ||
| this.ariaHaspopupManagedElements.delete(element); | ||
| } | ||
| } |
There was a problem hiding this comment.
removeAriaFromTrigger() always removes aria-expanded and aria-controls, and manageAriaOnTrigger() calls it whenever there is no click/longpress content. That means hover-only usage can strip consumer-authored ARIA state even though the docs say ARIA management is for click/longpress interactions.
| this.ariaHaspopupManagedElements.add(triggerElement); | ||
| } | ||
|
|
||
| const content = this.clickContent[0] || this.longpressContent[0]; |
There was a problem hiding this comment.
when both click + longpress exist, aria-controls should switch to longpress content id when open='longpress' and back for click.
| const content = this.clickContent[0] || this.longpressContent[0]; | |
| const content = | |
| this.open === 'longpress' | |
| ? this.longpressContent[0] | |
| : this.open === 'click' | |
| ? this.clickContent[0] | |
| : this.clickContent[0] || this.longpressContent[0]; | |
| element.removeAttribute('aria-expanded'); | ||
| element.removeAttribute('aria-controls'); |
There was a problem hiding this comment.
Why are we not doing like this so that hover-only does not remove consumer-provided aria-expanded/aria-controls
| element.removeAttribute('aria-expanded'); | |
| element.removeAttribute('aria-controls'); | |
| if (this.ariaExpandedManagedElements.has(element)) { | |
| element.removeAttribute('aria-expanded'); | |
| this.ariaExpandedManagedElements.delete(element); | |
| } | |
| if (this.ariaControlsManagedElements.has(element)) { | |
| element.removeAttribute('aria-controls'); | |
| this.ariaControlsManagedElements.delete(element); | |
| } |
There was a problem hiding this comment.
Good point. I would rather do an early return here if ariaManageElements has no element
if (!this.ariaManagedElements.has(element)) {
return;
}- Add curly braces to early return (eslint curly rule) - Remove unused waitUntil import from test file - Replace per-attribute WeakSet with single ariaManagedElements set so removeAriaFromTrigger is a no-op for elements the component never managed (prevents hover-only triggers from stripping consumer-authored aria-expanded/aria-controls) - Switch aria-controls to point at the active content element when both click and longpress content exist, based on the current open state - Add tests for hover-only consumer ARIA preservation and click/longpress aria-controls switching Co-authored-by: Cursor <cursoragent@cursor.com>
…ttps://github.com/adobe/spectrum-web-components into rajdeepchandra/fix-overlay-trigger-aria-attributes
What about listbox, [tree],(https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/tree_role), and [grid]? Pickers and comboboxes should be And what about tooltip which is not interactive and should not have |
nikkimk
left a comment
nikkimk
left a comment
There was a problem hiding this comment.
While I like the idea of setting aria-controls and aria-expanded via the component, we have overloaded overlay to so many use cases that I don't think it's a good idea to set a default aria-haspopup.
It would be better to have consumers be intentional about this attribute. We should make sure our documentation has examples of each of the possible values, and that we explicitly mention this as a requirement in the accessibility section of the docs.
| if ( | ||
| this.ariaManagedElements.has(triggerElement) || | ||
| !triggerElement.hasAttribute('aria-haspopup') | ||
| ) { | ||
| triggerElement.setAttribute('aria-haspopup', 'dialog'); |
There was a problem hiding this comment.
Addressed -- aria-haspopup now auto-resolves from the content element's role instead of defaulting to "dialog". See my reply to your issue-level comment for the full breakdown. Commit: 322006b.
…ulting to dialog Address nikkimk's review: aria-haspopup should reflect the actual content type, not always default to "dialog". Pickers/comboboxes need "listbox", action-menus need "menu", etc. - Add resolveHaspopupValue() that inspects the content element's role attribute (and falls back to first child with [role]) to determine the correct aria-haspopup value (menu, listbox, tree, grid, dialog) - Default remains "dialog" when no recognized role is found - Consumer overrides are still respected (unchanged behavior) - Tooltips (hover-only) remain unaffected (unchanged behavior) - Add 5 tests: role="dialog", role="menu", role="listbox", nested role detection through wrapper, and fallback to "dialog" for unknown roles - Update overlay-trigger.md docs to describe role-based detection Made-with: Cursor
|
@nikkimk Great feedback -- both points addressed in 322006b: 1. listbox, tree, grid, and menu support
So a picker/combobox using overlay-trigger with a Consumer overrides are still respected -- if someone sets 2. Tooltips Tooltips were already handled correctly -- the ARIA management only runs for click and longpress content. Hover-only overlays (tooltips) never get 5 new tests were added covering: |
…y-trigger-aria-attributes
| const changed = oneEvent(el, 'change'); | ||
| menuItem.click(); | ||
| await closed; | ||
| await changed; |
4 participants
Description
The
<overlay-trigger>component did not set any ARIA attributes on the trigger element for click/longpress overlays. This caused screen readers (e.g., NVDA on Chrome/Windows) to not announce the overlay relationship or read the overlay content when expanded, violating WCAG 1.3.2 (Meaningful Sequence, Level A).Motivation and context
A screen reader user navigating the dialog behaviors documentation could not hear the overlay content after opening it via the trigger button. The root cause was that the trigger element lacked
aria-expanded,aria-controls, andaria-haspopupattributes, so the screen reader had no programmatic way to discover or announce the controlled content.This fix follows the same pattern used by other component libraries (Shoelace, Radix, Headless UI) where the component owns the ARIA contract for its trigger-content relationship.
This PR adds automatic ARIA attribute management to
<overlay-trigger>:aria-expandedis set on the trigger element, toggling between"false"(closed) and"true"(open)aria-controlsis set on the trigger, pointing to the overlay content element'sid(auto-generated withsp-overlay-content-prefix if not provided)aria-haspopupis set to"dialog"by default (consumer-set values are never overwritten)aria-describedbyinsteadRelated issue(s)
Screenshots (if appropriate)
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
ARIA attribute verification
Screen reader testing (NVDA + Chrome on Windows)
Device review
Accessibility testing checklist
Required: Complete each applicable item and document your testing steps (replace the placeholders with your component-specific instructions).
Keyboard (required — document steps below) — What to test for: Focus order is logical; Tab reaches the component and all interactive descendants; Enter/Space activate where appropriate; arrow keys work for tabs, menus, sliders, etc.; no focus traps; Escape dismisses when applicable; focus indicator is visible.
Screen reader (required — document steps below) — What to test for: Role and name are announced correctly; state changes (e.g. expanded, selected) are announced; labels and relationships are clear; no unnecessary or duplicate announcements.