From 69d2da1932862a5acd06cb1c28a2ac9d6fc6b6f1 Mon Sep 17 00:00:00 2001 From: Elizabeth Mitchell Date: Fri, 10 May 2024 14:58:43 -0700 Subject: [PATCH] fix!: aria-labels announcing twice with "group" on components MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BREAKING CHANGE: `querySelector` for `[role]` and `[aria-*]` attributes may no longer work. See `@material/web/migrations/v2/README.md` and `@material/web/migrations/v2/query-selector-aria.ts`. Browser/SR test results (go/mwc-double-aria-test-results) - ✅ VoiceOver on Chrome - ✅ VoiceOver on iOS Safari - ✅ TalkBack on Chrome - ✅ ChromeVox on Chrome - ✅ NVDA on Chrome - ✅ NVDA on Firefox - ✅ JAWS on Chrome - ✅ JAWS on Firefox (Opt) - ❓ VoiceOver on Safari - ❓ VoiceOver on Firefox PiperOrigin-RevId: 632611852 --- button/internal/button.ts | 5 +- checkbox/internal/checkbox.ts | 12 +- chips/internal/chip.ts | 11 +- dialog/internal/dialog.ts | 11 +- fab/internal/shared.ts | 11 +- iconbutton/internal/icon-button.ts | 7 +- internal/aria/aria.ts | 2 +- internal/aria/aria_test.ts | 10 +- internal/aria/delegate.ts | 202 +++++++-- internal/aria/delegate_test.ts | 413 +++++++++++++++--- labs/navigationbar/internal/navigation-bar.ts | 14 +- .../internal/navigation-drawer-modal.ts | 11 +- .../internal/navigation-drawer.ts | 11 +- labs/navigationtab/internal/navigation-tab.ts | 14 +- .../internal/segmented-button.ts | 11 +- .../internal/segmented-button-set.ts | 11 +- list/internal/listitem/list-item.ts | 11 +- menu/internal/menuitem/menu-item.ts | 11 +- migrations/v2/README.md | 41 ++ migrations/v2/query-selector-aria.ts | 35 ++ migrations/v2/query-selector-aria_test.ts | 87 ++++ progress/internal/progress.ts | 11 +- select/internal/select.ts | 14 +- select/internal/selectoption/select-option.ts | 14 +- slider/internal/slider.ts | 10 +- switch/internal/switch.ts | 12 +- textfield/internal/text-field.ts | 14 +- tsconfig.json | 2 +- 28 files changed, 815 insertions(+), 203 deletions(-) create mode 100644 migrations/v2/README.md create mode 100644 migrations/v2/query-selector-aria.ts create mode 100644 migrations/v2/query-selector-aria_test.ts diff --git a/button/internal/button.ts b/button/internal/button.ts index 99679e4b99..7037f80b92 100644 --- a/button/internal/button.ts +++ b/button/internal/button.ts @@ -11,7 +11,7 @@ import {html, isServer, LitElement, nothing} from 'lit'; import {property, query, queryAssignedElements} from 'lit/decorators.js'; import {ARIAMixinStrict} from '../../internal/aria/aria.js'; -import {requestUpdateOnAriaChange} from '../../internal/aria/delegate.js'; +import {mixinDelegatesAria} from '../../internal/aria/delegate.js'; import { FormSubmitter, setupFormSubmitter, @@ -27,14 +27,13 @@ import { } from '../../labs/behaviors/element-internals.js'; // Separate variable needed for closure. -const buttonBaseClass = mixinElementInternals(LitElement); +const buttonBaseClass = mixinDelegatesAria(mixinElementInternals(LitElement)); /** * A button component. */ export abstract class Button extends buttonBaseClass implements FormSubmitter { static { - requestUpdateOnAriaChange(Button); setupFormSubmitter(Button); } diff --git a/checkbox/internal/checkbox.ts b/checkbox/internal/checkbox.ts index ce4942af06..e78248db6b 100644 --- a/checkbox/internal/checkbox.ts +++ b/checkbox/internal/checkbox.ts @@ -12,7 +12,7 @@ import {property, query, state} from 'lit/decorators.js'; import {classMap} from 'lit/directives/class-map.js'; import {ARIAMixinStrict} from '../../internal/aria/aria.js'; -import {requestUpdateOnAriaChange} from '../../internal/aria/delegate.js'; +import {mixinDelegatesAria} from '../../internal/aria/delegate.js'; import { dispatchActivationClick, isActivationClick, @@ -32,8 +32,10 @@ import { import {CheckboxValidator} from '../../labs/behaviors/validators/checkbox-validator.js'; // Separate variable needed for closure. -const checkboxBaseClass = mixinConstraintValidation( - mixinFormAssociated(mixinElementInternals(LitElement)), +const checkboxBaseClass = mixinDelegatesAria( + mixinConstraintValidation( + mixinFormAssociated(mixinElementInternals(LitElement)), + ), ); /** @@ -48,10 +50,6 @@ const checkboxBaseClass = mixinConstraintValidation( * --bubbles --composed */ export class Checkbox extends checkboxBaseClass { - static { - requestUpdateOnAriaChange(Checkbox); - } - /** @nocollapse */ static override shadowRootOptions = { ...LitElement.shadowRootOptions, diff --git a/chips/internal/chip.ts b/chips/internal/chip.ts index ae71a92ada..a1b4595254 100644 --- a/chips/internal/chip.ts +++ b/chips/internal/chip.ts @@ -11,18 +11,17 @@ import {html, LitElement, PropertyValues, TemplateResult} from 'lit'; import {property} from 'lit/decorators.js'; import {ClassInfo, classMap} from 'lit/directives/class-map.js'; -import {requestUpdateOnAriaChange} from '../../internal/aria/delegate.js'; +import {mixinDelegatesAria} from '../../internal/aria/delegate.js'; + +// Separate variable needed for closure. +const chipBaseClass = mixinDelegatesAria(LitElement); /** * A chip component. * * @fires update-focus {Event} Dispatched when `disabled` is toggled. --bubbles */ -export abstract class Chip extends LitElement { - static { - requestUpdateOnAriaChange(Chip); - } - +export abstract class Chip extends chipBaseClass { /** @nocollapse */ static override shadowRootOptions = { ...LitElement.shadowRootOptions, diff --git a/dialog/internal/dialog.ts b/dialog/internal/dialog.ts index ee626cbc38..e66c42d7f0 100644 --- a/dialog/internal/dialog.ts +++ b/dialog/internal/dialog.ts @@ -11,7 +11,7 @@ import {property, query, state} from 'lit/decorators.js'; import {classMap} from 'lit/directives/class-map.js'; import {ARIAMixinStrict} from '../../internal/aria/aria.js'; -import {requestUpdateOnAriaChange} from '../../internal/aria/delegate.js'; +import {mixinDelegatesAria} from '../../internal/aria/delegate.js'; import {redispatchEvent} from '../../internal/events/redispatch-event.js'; import { @@ -21,6 +21,9 @@ import { DialogAnimationArgs, } from './animations.js'; +// Separate variable needed for closure. +const dialogBaseClass = mixinDelegatesAria(LitElement); + /** * A dialog component. * @@ -31,11 +34,7 @@ import { * @fires cancel {Event} Dispatched when the dialog has been canceled by clicking * on the scrim or pressing Escape. */ -export class Dialog extends LitElement { - static { - requestUpdateOnAriaChange(Dialog); - } - +export class Dialog extends dialogBaseClass { // We do not use `delegatesFocus: true` due to a Chromium bug with // selecting text. // See https://bugs.chromium.org/p/chromium/issues/detail?id=950357 diff --git a/fab/internal/shared.ts b/fab/internal/shared.ts index c235c5a7ac..1aa52ad2f8 100644 --- a/fab/internal/shared.ts +++ b/fab/internal/shared.ts @@ -13,19 +13,18 @@ import {property} from 'lit/decorators.js'; import {classMap} from 'lit/directives/class-map.js'; import {ARIAMixinStrict} from '../../internal/aria/aria.js'; -import {requestUpdateOnAriaChange} from '../../internal/aria/delegate.js'; +import {mixinDelegatesAria} from '../../internal/aria/delegate.js'; /** * Sizes variants available to non-extended FABs. */ export type FabSize = 'medium' | 'small' | 'large'; -// tslint:disable-next-line:enforce-comments-on-exported-symbols -export abstract class SharedFab extends LitElement { - static { - requestUpdateOnAriaChange(SharedFab); - } +// Separate variable needed for closure. +const fabBaseClass = mixinDelegatesAria(LitElement); +// tslint:disable-next-line:enforce-comments-on-exported-symbols +export abstract class SharedFab extends fabBaseClass { /** @nocollapse */ static override shadowRootOptions: ShadowRootInit = { mode: 'open' as const, diff --git a/iconbutton/internal/icon-button.ts b/iconbutton/internal/icon-button.ts index f3bba31b6e..9a23674922 100644 --- a/iconbutton/internal/icon-button.ts +++ b/iconbutton/internal/icon-button.ts @@ -13,7 +13,7 @@ import {classMap} from 'lit/directives/class-map.js'; import {literal, html as staticHtml} from 'lit/static-html.js'; import {ARIAMixinStrict} from '../../internal/aria/aria.js'; -import {requestUpdateOnAriaChange} from '../../internal/aria/delegate.js'; +import {mixinDelegatesAria} from '../../internal/aria/delegate.js'; import { FormSubmitter, setupFormSubmitter, @@ -28,7 +28,9 @@ import { type LinkTarget = '_blank' | '_parent' | '_self' | '_top'; // Separate variable needed for closure. -const iconButtonBaseClass = mixinElementInternals(LitElement); +const iconButtonBaseClass = mixinDelegatesAria( + mixinElementInternals(LitElement), +); /** * A button for rendering icons. @@ -39,7 +41,6 @@ const iconButtonBaseClass = mixinElementInternals(LitElement); */ export class IconButton extends iconButtonBaseClass implements FormSubmitter { static { - requestUpdateOnAriaChange(IconButton); setupFormSubmitter(IconButton); } diff --git a/internal/aria/aria.ts b/internal/aria/aria.ts index 25d289875f..514227440b 100644 --- a/internal/aria/aria.ts +++ b/internal/aria/aria.ts @@ -73,7 +73,7 @@ export const ARIA_ATTRIBUTES = ARIA_PROPERTIES.map(ariaPropertyToAttribute); * @return True if the attribute is an aria attribute, or false if not. */ export function isAriaAttribute(attribute: string): attribute is ARIAAttribute { - return attribute.startsWith('aria-') || attribute === 'role'; + return ARIA_ATTRIBUTES.includes(attribute as ARIAAttribute); } /** diff --git a/internal/aria/aria_test.ts b/internal/aria/aria_test.ts index 671668991b..76ff24244f 100644 --- a/internal/aria/aria_test.ts +++ b/internal/aria/aria_test.ts @@ -16,10 +16,10 @@ describe('aria', () => { .toBeTrue(); }); - it('should return true for aria idref attributes', () => { + it('should return false for aria idref attributes', () => { expect(isAriaAttribute('aria-labelledby')) .withContext('aria-labelledby input') - .toBeTrue(); + .toBeFalse(); }); it('should return true for role', () => { @@ -29,6 +29,12 @@ describe('aria', () => { it('should return false for non-aria attributes', () => { expect(isAriaAttribute('label')).withContext('label input').toBeFalse(); }); + + it('should return false for custom aria-* attributes', () => { + expect(isAriaAttribute('aria-label-custom')) + .withContext('aria-label-custom input') + .toBeFalse(); + }); }); describe('ariaPropertyToAttribute()', () => { diff --git a/internal/aria/delegate.ts b/internal/aria/delegate.ts index 577dc5a78f..144b103aa1 100644 --- a/internal/aria/delegate.ts +++ b/internal/aria/delegate.ts @@ -4,30 +4,36 @@ * SPDX-License-Identifier: Apache-2.0 */ -import {ReactiveElement} from 'lit'; +import {LitElement, ReactiveElement, isServer} from 'lit'; -import {ARIA_PROPERTIES, ariaPropertyToAttribute} from './aria.js'; +import {MixinBase, MixinReturn} from '../../labs/behaviors/mixin.js'; +import { + ARIA_PROPERTIES, + ariaPropertyToAttribute, + isAriaAttribute, +} from './aria.js'; + +// Private symbols +const privateIgnoreAttributeChangesFor = Symbol( + 'privateIgnoreAttributeChangesFor', +); /** - * Sets up a `ReactiveElement` constructor to enable updates when delegating - * aria attributes. Elements may bind `this.aria*` properties to `aria-*` - * attributes in their render functions. + * Mixes in aria delegation for elements that delegate focus and aria to inner + * shadow root elements. * - * This function will: - * - Call `requestUpdate()` when an aria attribute changes. - * - Add `role="presentation"` to the host. + * This mixin fixes invalid aria announcements with shadow roots, caused by + * duplicate aria attributes on both the host and the inner shadow root element. * - * NOTE: The following features are not currently supported: - * - Delegating IDREF attributes (ex: `aria-labelledby`, `aria-controls`) - * - Delegating the `role` attribute + * Note: this mixin **does not yet support** ID reference attributes, such as + * `aria-labelledby` or `aria-controls`. * * @example - * class XButton extends LitElement { - * static { - * requestUpdateOnAriaChange(XButton); - * } + * ```ts + * class MyButton extends mixinDelegatesAria(LitElement) { + * static shadowRootOptions = {mode: 'open', delegatesFocus: true}; * - * protected override render() { + * render() { * return html` * + * `; + * ``` + * + * In the future, updates to the Accessibility Object Model (AOM) will provide + * built-in aria delegation features that will replace this mixin. + * + * @param base The class to mix functionality into. + * @return The provided class with aria delegation mixed in. + */ +export function mixinDelegatesAria>( + base: T, +): MixinReturn { + if (isServer) { + // Don't shift attributes when running with lit-ssr. The SSR renderer + // implements a subset of DOM APIs, including the methods this mixin + // overrides, causing errors. We don't need to shift on the server anyway + // since elements will shift attributes immediately once they hydrate. + return base; + } + + abstract class WithDelegatesAriaElement extends base { + [privateIgnoreAttributeChangesFor] = new Set(); + + override attributeChangedCallback( + name: string, + oldValue: string | null, + newValue: string | null, + ) { + if (!isAriaAttribute(name)) { + super.attributeChangedCallback(name, oldValue, newValue); + return; + } + + if (this[privateIgnoreAttributeChangesFor].has(name)) { + return; + } + + // Don't trigger another `attributeChangedCallback` once we remove the + // aria attribute from the host. We check the explicit name of the + // attribute to ignore since `attributeChangedCallback` can be called + // multiple times out of an expected order when hydrating an element with + // multiple attributes. + this[privateIgnoreAttributeChangesFor].add(name); + this.removeAttribute(name); + this[privateIgnoreAttributeChangesFor].delete(name); + const dataProperty = ariaAttributeToDataProperty(name); + if (newValue === null) { + delete this.dataset[dataProperty]; + } else { + this.dataset[dataProperty] = newValue; + } + + this.requestUpdate(ariaAttributeToDataProperty(name), oldValue); + } + + override getAttribute(name: string) { + if (isAriaAttribute(name)) { + return super.getAttribute(ariaAttributeToDataAttribute(name)); + } + + return super.getAttribute(name); + } + + override removeAttribute(name: string) { + super.removeAttribute(name); + if (isAriaAttribute(name)) { + super.removeAttribute(ariaAttributeToDataAttribute(name)); + // Since `aria-*` attributes are already removed`, we need to request + // an update because `attributeChangedCallback` will not be called. + this.requestUpdate(); + } + } + } + + setupDelegatesAriaProperties( + WithDelegatesAriaElement as unknown as typeof ReactiveElement, + ); + + return WithDelegatesAriaElement; +} + +/** + * Overrides the constructor's native `ARIAMixin` properties to ensure that + * aria properties reflect the values that were shifted to a data attribute. * * @param ctor The `ReactiveElement` constructor to patch. */ -export function requestUpdateOnAriaChange(ctor: typeof ReactiveElement) { +function setupDelegatesAriaProperties(ctor: typeof ReactiveElement) { for (const ariaProperty of ARIA_PROPERTIES) { + // The casing between ariaProperty and the dataProperty may be different. + // ex: aria-haspopup -> ariaHasPopup + const ariaAttribute = ariaPropertyToAttribute(ariaProperty); + // ex: aria-haspopup -> data-aria-haspopup + const dataAttribute = ariaAttributeToDataAttribute(ariaAttribute); + // ex: aria-haspopup -> dataset.ariaHaspopup + const dataProperty = ariaAttributeToDataProperty(ariaAttribute); + + // Call `ReactiveElement.createProperty()` so that the `aria-*` and `data-*` + // attributes are added to the `static observedAttributes` array. This + // triggers `attributeChangedCallback` for the delegates aria mixin to + // handle. ctor.createProperty(ariaProperty, { - attribute: ariaPropertyToAttribute(ariaProperty), - reflect: true, + attribute: ariaAttribute, + noAccessor: true, + }); + ctor.createProperty(Symbol(dataAttribute), { + attribute: dataAttribute, + noAccessor: true, }); - } - ctor.addInitializer((element) => { - const controller = { - hostConnected() { - element.setAttribute('role', 'presentation'); + // Re-define the `ARIAMixin` properties to handle data attribute shifting. + // It is safe to use `Object.defineProperty` here because the properties + // are native and not renamed. + // tslint:disable-next-line:ban-unsafe-reflection + Object.defineProperty(ctor.prototype, ariaProperty, { + configurable: true, + enumerable: true, + get(this: ReactiveElement): string | null { + return this.dataset[dataProperty] ?? null; }, - }; + set(this: ReactiveElement, value: string | null): void { + const prevValue = this.dataset[dataProperty] ?? null; + if (value === prevValue) { + return; + } + + if (value === null) { + delete this.dataset[dataProperty]; + } else { + this.dataset[dataProperty] = value; + } + + this.requestUpdate(ariaProperty, prevValue); + }, + }); + } +} + +function ariaAttributeToDataAttribute(ariaAttribute: string) { + // aria-haspopup -> data-aria-haspopup + return `data-${ariaAttribute}`; +} - element.addController(controller); - }); +function ariaAttributeToDataProperty(ariaAttribute: string) { + // aria-haspopup -> dataset.ariaHaspopup + return ariaAttribute.replace(/-\w/, (dashLetter) => + dashLetter[1].toUpperCase(), + ); } diff --git a/internal/aria/delegate_test.ts b/internal/aria/delegate_test.ts index 6da0685623..355a116d25 100644 --- a/internal/aria/delegate_test.ts +++ b/internal/aria/delegate_test.ts @@ -6,100 +6,409 @@ // import 'jasmine'; (google3-only) -import {html, LitElement, nothing} from 'lit'; -import {customElement, queryAsync} from 'lit/decorators.js'; +import {html, LitElement, nothing, TemplateResult} from 'lit'; +import {customElement, property, queryAsync} from 'lit/decorators.js'; import {Environment} from '../../testing/environment.js'; -import {requestUpdateOnAriaChange} from './delegate.js'; +import {ARIAMixinStrict} from './aria.js'; +import {mixinDelegatesAria} from './delegate.js'; declare global { interface HTMLElementTagNameMap { - 'test-aria-delegate': AriaDelegateElement; + 'test-delegates-aria': DelegatesAriaElement; } } -@customElement('test-aria-delegate') -class AriaDelegateElement extends LitElement { - static { - requestUpdateOnAriaChange(AriaDelegateElement); - } +// Separate variable needed for closure. +const delegatesAriaElementBaseClass = mixinDelegatesAria(LitElement); - @queryAsync('button') readonly button!: Promise; +@customElement('test-delegates-aria') +class DelegatesAriaElement extends delegatesAriaElementBaseClass { + @queryAsync('button') readonly delegate!: Promise; + @property({attribute: 'lit-attribute'}) litAttribute = ''; protected override render() { - return html``; + return html``; } } -describe('aria', () => { +describe('mixinDelegatesAria()', () => { const env = new Environment(); - async function setupTest({ariaLabel}: {ariaLabel?: string} = {}) { - const root = env.render(html` - - `); + // `mixinDelegatesAria()` patches `element.getAttribute()`, which makes it + // unreliable when testing what the screen reader sees. This function returns + // the "real" attribute value as read from the element's `outerHTML`, + // bypassing any patched methods or properties. + function getOuterHTMLAttribute( + element: Element, + attribute: string, + ): string | null { + const match = element.outerHTML.match( + new RegExp(`\\s${attribute}="([^"]*)"`), + ); + return match ? match[1] : null; + } - const host = root.querySelector('test-aria-delegate'); + async function setupTest(templateWithTestAriaDelegate: TemplateResult) { + const root = env.render(templateWithTestAriaDelegate); + const host = root.querySelector('test-delegates-aria'); if (!host) { - throw new Error('Could not query rendered '); + throw new Error('Could not query rendered .'); } await host.updateComplete; - const child = await host.button; - if (!child) { - throw new Error('Could not query rendered