-
Notifications
You must be signed in to change notification settings - Fork 242
fix(overlay): add ARIA attribute management to overlay-trigger for screen reader accessibility #6044
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix(overlay): add ARIA attribute management to overlay-trigger for screen reader accessibility #6044
Changes from all commits
109153f
b833bda
c2b7f23
47634b6
44b3a16
ffcdb67
7d8632e
322006b
70af855
4b9d2f1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| --- | ||
| '@spectrum-web-components/overlay': patch | ||
| '@spectrum-web-components/dialog': patch | ||
| --- | ||
|
|
||
| **Fixed**: Added automatic ARIA attribute management to `<overlay-trigger>` for screen reader accessibility (WCAG 1.3.2 Meaningful Sequence): | ||
|
|
||
| - `aria-expanded` is now automatically set on the trigger element, reflecting the overlay's open/closed state | ||
| - `aria-controls` is set on the trigger element, pointing to the overlay content's `id` (generated if not provided) | ||
| - `aria-haspopup` is set to `"dialog"` by default (respects consumer overrides; component tracks its own values to allow type changes) | ||
| - ARIA attributes are cleaned up from the trigger element when the overlay-trigger is disconnected, the trigger element changes, or content is removed | ||
| - Updated dialog README behaviors example to use `<overlay-trigger>` for automatic ARIA management | ||
| - Added comprehensive accessibility documentation to overlay-trigger covering ARIA attributes, focus management, keyboard navigation, and screen reader considerations |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,7 @@ import { | |
| query, | ||
| state, | ||
| } from '@spectrum-web-components/base/src/decorators.js'; | ||
| import { randomID } from '@spectrum-web-components/shared/src/random-id.js'; | ||
|
|
||
| /* eslint-disable import/no-extraneous-dependencies */ | ||
| import '@spectrum-web-components/overlay/sp-overlay.js'; | ||
|
|
@@ -133,6 +134,14 @@ export class OverlayTrigger extends SpectrumElement { | |
| @query('#hover-overlay', true) | ||
| hoverOverlayElement!: Overlay; | ||
|
|
||
| /** | ||
| * Tracks elements where this component has taken ownership | ||
| * of ARIA attributes, so consumer-set values are never removed. | ||
| */ | ||
| private ariaManagedElements = new WeakSet<HTMLElement>(); | ||
|
|
||
| private previousTriggerElement?: HTMLElement; | ||
|
|
||
| private getAssignedElementsFromSlot(slot: HTMLSlotElement): HTMLElement[] { | ||
| return slot.assignedElements({ flatten: true }) as HTMLElement[]; | ||
| } | ||
|
|
@@ -177,6 +186,101 @@ export class OverlayTrigger extends SpectrumElement { | |
| } | ||
| } | ||
|
|
||
| private static readonly VALID_HASPOPUP_ROLES = new Set([ | ||
| 'menu', | ||
| 'listbox', | ||
| 'tree', | ||
| 'grid', | ||
| 'dialog', | ||
| ]); | ||
|
|
||
| private resolveHaspopupValue(): string { | ||
| const content = this.clickContent[0] || this.longpressContent[0]; | ||
| if (!content) { | ||
| return 'dialog'; | ||
| } | ||
| const role = content.getAttribute('role'); | ||
| if (role && OverlayTrigger.VALID_HASPOPUP_ROLES.has(role)) { | ||
| return role; | ||
| } | ||
| const firstChild = content.querySelector('[role]') as HTMLElement | null; | ||
| if ( | ||
| firstChild && | ||
| OverlayTrigger.VALID_HASPOPUP_ROLES.has(firstChild.getAttribute('role')!) | ||
| ) { | ||
| return firstChild.getAttribute('role')!; | ||
| } | ||
| return 'dialog'; | ||
| } | ||
|
|
||
| private removeAriaFromTrigger(element: HTMLElement): void { | ||
| if (!this.ariaManagedElements.has(element)) { | ||
| return; | ||
| } | ||
| element.removeAttribute('aria-expanded'); | ||
| element.removeAttribute('aria-controls'); | ||
| element.removeAttribute('aria-haspopup'); | ||
| this.ariaManagedElements.delete(element); | ||
| } | ||
|
Comment on lines
216
to
224
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| private manageAriaOnTrigger(): void { | ||
| const triggerElement = this.targetContent[0]; | ||
|
|
||
| if ( | ||
| this.previousTriggerElement && | ||
| this.previousTriggerElement !== triggerElement | ||
| ) { | ||
| this.removeAriaFromTrigger(this.previousTriggerElement); | ||
| } | ||
| this.previousTriggerElement = triggerElement; | ||
|
|
||
| if (!triggerElement) { | ||
| return; | ||
| } | ||
|
|
||
| const hasClickContent = this.clickContent.length > 0; | ||
| const hasLongpressContent = this.longpressContent.length > 0; | ||
|
|
||
| if (!hasClickContent && !hasLongpressContent) { | ||
| this.removeAriaFromTrigger(triggerElement); | ||
| return; | ||
| } | ||
|
|
||
| const isExpanded = this.open === 'click' || this.open === 'longpress'; | ||
| triggerElement.setAttribute('aria-expanded', String(isExpanded)); | ||
|
|
||
| if ( | ||
| this.ariaManagedElements.has(triggerElement) || | ||
| !triggerElement.hasAttribute('aria-haspopup') | ||
| ) { | ||
| triggerElement.setAttribute('aria-haspopup', this.resolveHaspopupValue()); | ||
| } | ||
| this.ariaManagedElements.add(triggerElement); | ||
|
|
||
| const content = | ||
| this.open === 'longpress' | ||
| ? this.longpressContent[0] | ||
| : this.open === 'click' | ||
| ? this.clickContent[0] | ||
| : this.clickContent[0] || this.longpressContent[0]; | ||
| if (content) { | ||
| if (!content.id) { | ||
| content.id = `sp-overlay-content-${randomID()}`; | ||
| } | ||
| triggerElement.setAttribute('aria-controls', content.id); | ||
| } else { | ||
| triggerElement.removeAttribute('aria-controls'); | ||
| } | ||
| } | ||
|
|
||
| override disconnectedCallback(): void { | ||
| if (this.previousTriggerElement) { | ||
| this.removeAriaFromTrigger(this.previousTriggerElement); | ||
| this.previousTriggerElement = undefined; | ||
| } | ||
| super.disconnectedCallback(); | ||
| } | ||
|
|
||
| protected override update(changes: PropertyValues): void { | ||
| if (changes.has('clickContent')) { | ||
| this.clickPlacement = | ||
|
|
@@ -339,6 +443,16 @@ export class OverlayTrigger extends SpectrumElement { | |
| this.open = undefined; | ||
| return; | ||
| } | ||
|
|
||
| if ( | ||
| changedProperties.has('open') || | ||
| changedProperties.has('type') || | ||
| changedProperties.has('targetContent') || | ||
| changedProperties.has('clickContent') || | ||
| changedProperties.has('longpressContent') | ||
| ) { | ||
| this.manageAriaOnTrigger(); | ||
| } | ||
| } | ||
|
|
||
| protected override async getUpdateComplete(): Promise<boolean> { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we not doing like this so that hover-only does not remove consumer-provided aria-expanded/aria-controls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I would rather do an early return here if
ariaManageElementshas no element