-
Notifications
You must be signed in to change notification settings - Fork 13.3k
fix(button): sync aria description between host and native button #31264
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?
Changes from all commits
495ca01
a7941a1
35a82d7
4ed0c4f
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 |
|---|---|---|
|
|
@@ -2,7 +2,12 @@ import type { ComponentInterface, EventEmitter } from '@stencil/core'; | |
| import { Component, Element, Event, Host, Prop, Watch, State, forceUpdate, h } from '@stencil/core'; | ||
| import type { AnchorInterface, ButtonInterface } from '@utils/element-interface'; | ||
| import type { Attributes } from '@utils/helpers'; | ||
| import { inheritAriaAttributes, hasShadowDom } from '@utils/helpers'; | ||
| import { | ||
| inheritAriaAttributes, | ||
| hasShadowDom, | ||
| watchForAriaAttributeChanges, | ||
| type AttributeWatcher, | ||
| } from '@utils/helpers'; | ||
| import { printIonWarning } from '@utils/logging'; | ||
| import { createColorClasses, hostContext, openURL } from '@utils/theme'; | ||
|
|
||
|
|
@@ -35,6 +40,7 @@ export class Button implements ComponentInterface, AnchorInterface, ButtonInterf | |
| private formButtonEl: HTMLButtonElement | null = null; | ||
| private formEl: HTMLFormElement | null = null; | ||
| private inheritedAttributes: Attributes = {}; | ||
| private ariaWatcher?: AttributeWatcher; | ||
|
|
||
| @Element() el!: HTMLElement; | ||
|
|
||
|
|
@@ -158,27 +164,6 @@ export class Button implements ComponentInterface, AnchorInterface, ButtonInterf | |
| */ | ||
| @Event() ionBlur!: EventEmitter<void>; | ||
|
|
||
| /** | ||
| * This component is used within the `ion-input-password-toggle` component | ||
| * to toggle the visibility of the password input. | ||
| * These attributes need to update based on the state of the password input. | ||
| * Otherwise, the values will be stale. | ||
| * | ||
| * @param newValue | ||
| * @param _oldValue | ||
| * @param propName | ||
| */ | ||
| @Watch('aria-checked') | ||
| @Watch('aria-label') | ||
| @Watch('aria-pressed') | ||
| onAriaChanged(newValue: string, _oldValue: string, propName: string) { | ||
| this.inheritedAttributes = { | ||
| ...this.inheritedAttributes, | ||
| [propName]: newValue, | ||
| }; | ||
| forceUpdate(this); | ||
| } | ||
|
|
||
| /** | ||
| * This is responsible for rendering a hidden native | ||
| * button element inside the associated form. This allows | ||
|
|
@@ -220,7 +205,31 @@ export class Button implements ComponentInterface, AnchorInterface, ButtonInterf | |
| this.inToolbar = !!this.el.closest('ion-buttons'); | ||
| this.inListHeader = !!this.el.closest('ion-list-header'); | ||
| this.inItem = !!this.el.closest('ion-item') || !!this.el.closest('ion-item-divider'); | ||
| this.inheritedAttributes = inheritAriaAttributes(this.el); | ||
| this.inheritedAttributes = inheritAriaAttributes(this.el, ['aria-disabled']); | ||
|
|
||
| /** | ||
| * Keeps inherited ARIA attributes in sync with the host element for the | ||
| * lifetime of the component, not just at initial load. This replaces the | ||
| * previous approach of manually re-declaring @Watch for each aria attribute | ||
| * that could change post-load | ||
| * | ||
| * aria-disabled is excluded here (and from the initial inheritAriaAttributes | ||
| * call above) because button.tsx sets it itself on Host based on the `disabled` prop | ||
| */ | ||
| this.ariaWatcher = watchForAriaAttributeChanges( | ||
|
Member
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. Good call making this a reusable helper. Makes sense to keep the first pass to |
||
| this.el, | ||
| (changed) => { | ||
| this.inheritedAttributes = { ...this.inheritedAttributes, ...changed }; | ||
| forceUpdate(this); | ||
| }, | ||
| ['aria-disabled'] | ||
| ); | ||
| } | ||
|
|
||
| // Prevents | ||
| disconnectedCallback() { | ||
|
Member
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. I think this breaks after the button moves in the DOM. The watcher gets created in Also the |
||
| this.ariaWatcher?.disconnect(); | ||
| this.ariaWatcher = undefined; | ||
| } | ||
|
|
||
| private get hasIconOnly() { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| import AxeBuilder from '@axe-core/playwright'; | ||
| import { expect } from '@playwright/test'; | ||
| import { ariaAttributes } from '@utils/helpers'; | ||
| import { configs, test } from '@utils/test/playwright'; | ||
|
|
||
| configs({ directions: ['ltr'], palettes: ['light', 'dark'] }).forEach(({ title, config }) => { | ||
|
|
@@ -148,3 +149,36 @@ configs({ directions: ['ltr'] }).forEach(({ title, screenshot, config }) => { | |
| }); | ||
| }); | ||
| }); | ||
|
|
||
| configs({ directions: ['ltr'] }).forEach(({ title, config }) => { | ||
| test.describe(title('button: aria attribute sync'), () => { | ||
| // Mirrors the ignoreList passed to inheritAriaAttributes/watchForAriaAttributeChanges | ||
|
Member
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. Nit: comments in tests hold up better describing the behavior instead of pointing at the source file, since the file reference goes stale if things move. The reason is worth keeping though, something like: aria-disabled is excluded because the button owns it via the |
||
| // in button.tsx. aria-disabled is excluded because button.tsx manages it internally | ||
| // via the `disabled` prop. | ||
| const watchedAriaAttributes = ariaAttributes.filter((attr) => attr !== 'aria-disabled'); | ||
|
|
||
| for (const attr of watchedAriaAttributes) { | ||
| test(`native button updates ${attr} when host attribute changes`, async ({ page }) => { | ||
|
Member
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. These verify the fix for the linked issue, so could you add the issue annotation to keep the context linkable? test(`native button updates ${attr} when host attribute changes`, async ({ page }, testInfo) => {
testInfo.annotations.push({ type: 'issue', description: 'https://github.com/ionic-team/ionic-framework/issues/30626' });Same for the |
||
| await page.setContent(`<ion-button ${attr}="initial">Button</ion-button>`, config); | ||
|
|
||
| const host = page.locator('ion-button'); | ||
| const nativeButton = host.locator('button'); | ||
|
|
||
| await expect(nativeButton).toHaveAttribute(attr, 'initial'); | ||
|
|
||
| await host.evaluate((el, attr) => el.setAttribute(attr, 'updated'), attr); | ||
|
|
||
| await expect(nativeButton).toHaveAttribute(attr, 'updated'); | ||
| }); | ||
| } | ||
|
|
||
| test('does not sync aria-disabled, since button.tsx manages it internally', async ({ page }) => { | ||
| await page.setContent(`<ion-button aria-disabled="true">Button</ion-button>`, config); | ||
|
|
||
| const host = page.locator('ion-button'); | ||
| const nativeButton = host.locator('button'); | ||
|
|
||
| await expect(nativeButton).not.toHaveAttribute('aria-disabled', 'true'); | ||
| }); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -121,7 +121,7 @@ export const inheritAttributes = (el: HTMLElement, attributes: string[] = []) => | |
| * Removed deprecated attributes. | ||
| * https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes | ||
| */ | ||
| const ariaAttributes = [ | ||
| export const ariaAttributes = [ | ||
| 'role', | ||
| 'aria-activedescendant', | ||
| 'aria-atomic', | ||
|
|
@@ -190,6 +190,76 @@ export const inheritAriaAttributes = (el: HTMLElement, ignoreList?: string[]) => | |
| return inheritAttributes(el, attributesToInherit); | ||
| }; | ||
|
|
||
| export interface AttributeWatcher { | ||
| disconnect: () => void; | ||
|
Member
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. Nit: our other wrapper-object controllers use |
||
| } | ||
|
|
||
| /** | ||
| * Watches an element for changes to a given set of attributes and calls | ||
| * onChange whenever one of them is set. Because inheritAttributes() strips | ||
| * the attribute from the host as it reads it, any subsequent mutation is | ||
| * just checked that the new value isn't null. | ||
| */ | ||
| export const watchAttributes = ( | ||
| el: HTMLElement, | ||
| attributes: string[], | ||
| onChange: (changed: { [k: string]: string }) => void | ||
| ): AttributeWatcher => { | ||
| if (typeof MutationObserver === 'undefined') { | ||
| // Not available in Stencil's mock-doc test environment (used by | ||
| // `stencil test --spec`), and, as a defensive fallback, environments | ||
| // without native MutationObserver support. | ||
| return { disconnect: () => {} }; | ||
| } | ||
|
|
||
| // Set up mutation observer to observe attribute changes | ||
| const observer = new MutationObserver((mutations) => { | ||
| const changed: { [k: string]: string } = {}; | ||
| for (const mutation of mutations) { | ||
| if (mutation.type !== 'attributes' || !mutation.attributeName) continue; | ||
| const name = mutation.attributeName; | ||
| if (!attributes.includes(name)) continue; | ||
| const value = el.getAttribute(name); | ||
| if (value === null) continue; | ||
|
Member
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. One gap here: because we skip on |
||
| changed[name] = value; | ||
| } | ||
|
|
||
| // If attribute changes, re-strip so the value doesn't live on both host | ||
| // and native element. | ||
| if (Object.keys(changed).length > 0) { | ||
| Object.keys(changed).forEach((name) => el.removeAttribute(name)); | ||
| onChange(changed); | ||
| } | ||
| }); | ||
|
|
||
| // Watch for attribute changes on this element | ||
| observer.observe(el, { attributes: true, attributeFilter: attributes }); | ||
|
|
||
| // Stop watching, called by `disconnectedCallback` | ||
| return { disconnect: () => observer.disconnect() }; | ||
| }; | ||
|
|
||
| /** | ||
| * Watches an element for changes to ARIA attributes (and `role`) and invokes | ||
| * a callback whenever one is set externally, so that inherited ARIA state | ||
| * stays in sync for the lifetime of the component — not just at initial load. | ||
| * | ||
| * This should be called once in componentWillLoad, alongside the initial | ||
| * call to inheritAriaAttributes, and the returned AttributeWatcher must be | ||
| * disconnected in disconnectedCallback to avoid leaking the observer. | ||
| */ | ||
| export const watchForAriaAttributeChanges = ( | ||
| el: HTMLElement, | ||
| onChange: (changed: { [k: string]: string }) => void, | ||
| ignoreList?: string[] | ||
| ): AttributeWatcher => { | ||
| let attributesToWatch = ariaAttributes; | ||
| if (ignoreList && ignoreList.length > 0) { | ||
| attributesToWatch = attributesToWatch.filter((attr) => !ignoreList.includes(attr)); | ||
| } | ||
| return watchAttributes(el, attributesToWatch, onChange); | ||
| }; | ||
|
|
||
| export const addEventListener = (el: any, eventName: string, callback: any, opts?: any) => { | ||
| return el.addEventListener(eventName, callback, opts); | ||
| }; | ||
|
|
||
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.
Small nit: the "This replaces the previous approach of manually re-declaring @watch..." line is more PR context than something the code needs to carry, since a future reader won't have the old code to compare against. I'd keep the
aria-disablednote right below it though. Up to you!