Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 32 additions & 23 deletions core/src/components/button/button.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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

Copy link
Copy Markdown
Member

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-disabled note right below it though. Up to you!

* 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(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 ion-button and get reactions before going wider. On the scope question: the main thing from my earlier review is covering the other button-like components too. The same one-time-copy bug is in ion-back-button, ion-menu-button, ion-fab-button, and ion-breadcrumb, plus ion-item and ion-card which @ReneZeidler called out on the issue. Could we pull at least ion-item and ion-card into this PR? One thing to watch out for: item/card only inherit aria-label today, so we can't copy the button wiring as-is, we'd want to pick the attribute set / ignoreList per component so we don't suddenly start stripping all ~50 aria attributes off the host.

this.el,
(changed) => {
this.inheritedAttributes = { ...this.inheritedAttributes, ...changed };
forceUpdate(this);
},
['aria-disabled']
);
}

// Prevents
disconnectedCallback() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 componentWillLoad, which only runs once, but it's disconnected and cleared here on every disconnectedCallback. There's no connectedCallback to recreate it, so the first time an ion-button is detached and reattached (keyed list reorder, ion-nav, virtual scroll) the observer is gone for good and aria reactivity silently stops. The old @Watch decorators survived reconnection on their own. Every other MutationObserver in core is created in connectedCallback and torn down in disconnectedCallback (input.tsx, select.tsx, checkbox.tsx), so moving creation there would match those and fix the reconnect case. Worth an e2e test that detaches and reattaches, then checks a later aria change still lands.

Also the // Prevents comment just above looks cut off, might want to finish it or drop it.

this.ariaWatcher?.disconnect();
this.ariaWatcher = undefined;
}

private get hasIconOnly() {
Expand Down
34 changes: 34 additions & 0 deletions core/src/components/button/test/a11y/button.e2e.ts
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 }) => {
Expand Down Expand Up @@ -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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 disabled prop, so it shouldn't be host-synced. Up to you!

// 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 }) => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 aria-disabled exclusion test.

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');
});
});
});
72 changes: 71 additions & 1 deletion core/src/utils/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -190,6 +190,76 @@ export const inheritAriaAttributes = (el: HTMLElement, ignoreList?: string[]) =>
return inheritAttributes(el, attributesToInherit);
};

export interface AttributeWatcher {
disconnect: () => void;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: our other wrapper-object controllers use destroy() (SlotMutationController, NotchController), and disconnect() is what we tend to call on a raw MutationObserver. Since this returns a wrapper, destroy would match what we do elsewhere. No worries if you'd rather leave it.

}

/**
* 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;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One gap here: because we skip on value === null and strip the attribute off the host after the first copy, there's no way to reactively remove an aria attribute from the native button once it's been set. A later removeAttribute on the host fires no mutation since it's already gone, so the native element keeps the stale value. Toggling between two non-null values is fine, it's only clearing that's affected. Might be worth recording removals in changed so the native element can drop them too, with a test to cover it. I'm not sure how often clearing an aria attr comes up in practice, so I'll leave the priority up to you.

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);
};
Expand Down
Loading