Skip to content

fix(button): sync aria attributes 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.

Sign up for GitHub

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

Open
Zac-Smucker-Bryan wants to merge 7 commits into
base: main
Choose a base branch
from
Open

fix(button): sync aria attributes between host and native button #31264

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
495ca01
fix(button): sync aria description between host and native button
Zac-Smucker-Bryan Jul 10, 2026
a7941a1
test(button): add e2e test for aria-description sync
Zac-Smucker-Bryan Jul 10, 2026
35a82d7
fix(button): sync aria attributes to native button reactively
Zac-Smucker-Bryan Jul 25, 2026
4ed0c4f
npm run lint.fix
Zac-Smucker-Bryan Jul 27, 2026
a906acb
fix(helper): update Mutation Observer and add removeAttribute interce…
Zac-Smucker-Bryan Aug 5, 2026
4f78686
fix(button): Add helper to ion-item and ion-card, move inheritedAriaA…
Zac-Smucker-Bryan Aug 5, 2026
b585cd5
test(button): Add e2e tests
Zac-Smucker-Bryan Aug 5, 2026
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
61 changes: 38 additions & 23 deletions core/src/components/button/button.tsx
View file
Open in desktop
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,37 @@ 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);
}

connectedCallback() {

@ShaneK ShaneK Aug 7, 2026

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.

Moving the inherit into connectedCallback fixes the watcher lifetime, but I think it trades that for a worse problem. This runs on every attach and it assigns rather than merges, so on the second attach the host has already been stripped, inheritAriaAttributes returns {}, and everything captured the first time is gone.

Comparing against a build of main: take <ion-button aria-label="Close" aria-describedby="hint">, move it with parent.removeChild(b); parent.appendChild(b), then change anything that re-renders like b.color = 'primary'. Both attributes come off the native button, and the host doesn't have them either, so the button ends up with no accessible name at all. On main both survive. The same is true for ion-item, and ion-card avoids it only because it left the inherit in componentWillLoad.

Any keyed list reorder or ion-reorder-group move drops the labels, so I'd need this fixed before I could approve. Either keep the inherit in componentWillLoad like card does, or merge instead of assign here.

/**
* Must run before watchForAriaAttributeChanges: it calls removeAttribute
* internally to strip the host's initial values, and that call must
* happen before removeAttribute is patched below — otherwise this
* strip would itself be treated as an external removal.
*/
this.inheritedAttributes = inheritAriaAttributes(this.el, ['aria-disabled']);

@ShaneK ShaneK Aug 7, 2026

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.

Adding aria-disabled to the ignore list here changes more than the issue asks for. Keeping it out of the watcher makes sense since the Host expression would fight it, but keeping it out of the inherit means a developer-set value lands nowhere.

On main, <ion-button aria-disabled="true"> puts the attribute on the shadow button. On this branch it stays on the host, which isn't the element AT reads. Setting disabled to true and back to false then deletes it off the host as well, since the Host expression writes null over it.

That breaks the "disabled but still focusable" pattern, and I don't think it's intended. Could we drop the ignore list from this call and keep it only on the watcher below?


/**
* Keeps inherited ARIA attributes in sync with the host element for the
* lifetime of the component, not just at initial 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(
Comment thread
Zac-Smucker-Bryan marked this conversation as resolved.
this.el,
(changed) => {
this.inheritedAttributes = { ...this.inheritedAttributes, ...changed };
forceUpdate(this);
},
['aria-disabled']
);
}

disconnectedCallback() {
Comment thread
Zac-Smucker-Bryan marked this conversation as resolved.
this.ariaWatcher?.destroy();
this.ariaWatcher = undefined;
}

private get hasIconOnly() {
Expand Down
106 changes: 106 additions & 0 deletions core/src/components/button/test/a11y/button.e2e.ts
View file
Open in desktop
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,108 @@ configs({ directions: ['ltr'] }).forEach(({ title, screenshot, config }) => {
});
});
});

configs({ directions: ['ltr'] }).forEach(({ title, config }) => {
test.describe(title('button: aria attribute sync'), () => {
// 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 }) => {
Comment thread
Zac-Smucker-Bryan marked this conversation as resolved.
test.info().annotations.push({
type: 'issue',
description: 'https://github.com/ionic-team/ionic-framework/issues/30626',
});

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

@ShaneK ShaneK Aug 7, 2026

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.

Not sure this one can fail. The assertion passes whether the attribute is on the host, on the native element with some other value, or missing entirely, so it doesn't pin down what actually happens to a developer-set aria-disabled. Given the behavior change I flagged above, I think it needs to assert where the value ends up.

The name points at button.tsx as well, which is the same thing I mentioned last time about references going stale. Something like "should not sync aria-disabled from the host" reads better.

test
.info()
.annotations.push({ type: 'issue', description: 'https://github.com/ionic-team/ionic-framework/issues/30626' });

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');
});

test('aria sync survives detach and reattach', async ({ page }) => {

@ShaneK ShaneK Aug 7, 2026

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.

All 14 of these pass on the branch with the reattach problem above still present. The setAttribute right before the assertion re-arms the watcher and repopulates the map, so the test can't fail. Nothing forces a render after the reattach either, so the shadow DOM would still be holding the old value even if you asserted the original.

await host.evaluate((el) => {
  const parent = el.parentElement!;
  parent.removeChild(el);
  parent.appendChild(el);
  (el as HTMLIonButtonElement).color = 'primary'; // force a render
});
await expect(nativeButton).toHaveAttribute('aria-label', 'label');

This one and the "helper strips host attribute" test below are also missing the issue annotation, same for the two in the item spec and the one in card.

await page.setContent(
`
<div id="container">
<ion-button aria-label="label">Button</ion-button>
</div>
`,
config
);

const host = page.locator('ion-button');
const nativeButton = host.locator('button');

await expect(nativeButton).toHaveAttribute('aria-label', 'label');

// Detach and reattach
await host.evaluate((buttonEl) => {
const parent = buttonEl.parentElement!;
parent.removeChild(buttonEl);
parent.appendChild(buttonEl);
});

await host.evaluate((el) => el.setAttribute('aria-label', 'updated'));
await expect(nativeButton).toHaveAttribute('aria-label', 'updated');
});

test('helper strips host attribute and syncs native element through set, empty, and remove', async ({ page }) => {
page.on('console', (msg) => {
console.log(`[browser] ${msg.type()}: ${msg.text()}`);
});
Comment on lines +218 to +220

@ShaneK ShaneK Aug 7, 2026

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.

Suggested change
page.on('console', (msg) => {
console.log(`[browser] ${msg.type()}: ${msg.text()}`);
});

Leftover debugging by the look of it. Same block is in the card and item specs.


await page.setContent(
`
<ion-button aria-label="initial">Button</ion-button>
`,
config
);

const host = page.locator('ion-button');
const nativeButton = host.locator('button');

// Initial load: inheritAriaAttributes should have stripped aria-label
// from the host and copied it onto the native button.
await expect(host).not.toHaveAttribute('aria-label');
await expect(nativeButton).toHaveAttribute('aria-label', 'initial');

// Setting a new value on the host: watcher should capture it, sync it
// to native, and re-strip it from the host.
await host.evaluate((el) => el.setAttribute('aria-label', 'second'));
await expect(host).not.toHaveAttribute('aria-label');
await expect(nativeButton).toHaveAttribute('aria-label', 'second');

// Setting to empty string: empty string is a valid, non-null value.
await host.evaluate((el) => el.setAttribute('aria-label', ''));
await expect(host).not.toHaveAttribute('aria-label');
await expect(nativeButton).toHaveAttribute('aria-label', '');

// Removing the attribute directly: the patched removeAttribute should
// fire onChange with null, which should remove aria-label from native
// and host.
await host.evaluate((el) => el.removeAttribute('aria-label'));
await expect(host).not.toHaveAttribute('aria-label');
await expect(nativeButton).not.toHaveAttribute('aria-label');
});
});
});
19 changes: 16 additions & 3 deletions core/src/components/card/card.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import type { ComponentInterface } from '@stencil/core';
import { Element, Component, Host, Prop, h } from '@stencil/core';
import { Element, Component, Host, Prop, h, forceUpdate } from '@stencil/core';
import type { AnchorInterface, ButtonInterface } from '@utils/element-interface';
import type { Attributes } from '@utils/helpers';
import { inheritAttributes } from '@utils/helpers';
import type { Attributes, AttributeWatcher } from '@utils/helpers';
import { inheritAttributes, watchAttributes } from '@utils/helpers';
import { createColorClasses, openURL } from '@utils/theme';

import { getIonMode } from '../../global/ionic-global';
Expand All @@ -24,6 +24,7 @@ import type { RouterDirection } from '../router/utils/interface';
})
export class Card implements ComponentInterface, AnchorInterface, ButtonInterface {
private inheritedAriaAttributes: Attributes = {};
private ariaWatcher?: AttributeWatcher;

@Element() el!: HTMLElement;
/**
Expand Down Expand Up @@ -91,6 +92,18 @@ export class Card implements ComponentInterface, AnchorInterface, ButtonInterfac
this.inheritedAriaAttributes = inheritAttributes(this.el, ['aria-label']);
}

connectedCallback() {

@ShaneK ShaneK Aug 7, 2026

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.

The comments in button.tsx and item.tsx say the inherit has to run before the watcher is installed, but card does the opposite. Stencil runs connectedCallback before componentWillLoad, so the patch is already in place when inheritAttributes strips the host. Logging the order on this branch gives:

connectedCallback:installWatcher
componentWillLoad:start
onChange fired: {"aria-label":null}
componentWillLoad:end value={"aria-label":"initial"}

It comes out right because the assignment overwrites the null immediately after, but that's luck rather than design, and it stops being true the moment card merges instead of assigns. Worth getting all three components to agree on where the inherit lives.

this.ariaWatcher = watchAttributes(this.el, ['aria-label'], (changed) => {
this.inheritedAriaAttributes = { ...this.inheritedAriaAttributes, ...changed };
forceUpdate(this);
});
}

disconnectedCallback() {
this.ariaWatcher?.destroy();
this.ariaWatcher = undefined;
}

private isClickable(): boolean {
return this.href !== undefined || this.button;
}
Expand Down
74 changes: 74 additions & 0 deletions core/src/components/card/test/a11y/card.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,77 @@ configs({ directions: ['ltr'] }).forEach(({ title, screenshot, config }) => {
});
});
});

configs({ directions: ['ltr'] }).forEach(({ title, config }) => {
test.describe(title('card: aria attribute sync'), () => {
test('aria sync survives detach and reattach', async ({ page }) => {
test.info().annotations.push({
type: 'issue',
description: 'https://github.com/ionic-team/ionic-framework/issues/30626',
});

await page.setContent(
`
<div id="container">
<ion-card button="true" aria-label="label">Card</ion-card>
</div>
`,
config
);

const host = page.locator('ion-card');
const nativeCard = host.locator('[part="native"]');

await expect(nativeCard).toHaveAttribute('aria-label', 'label');

// Detach and reattach
await host.evaluate((cardEl) => {
const parent = cardEl.parentElement!;
parent.removeChild(cardEl);
parent.appendChild(cardEl);
});

await host.evaluate((el) => el.setAttribute('aria-label', 'updated'));
await expect(nativeCard).toHaveAttribute('aria-label', 'updated');
});

test('helper strips host attribute and syncs native element through set, empty, and remove', async ({ page }) => {
page.on('console', (msg) => {
console.log(`[browser] ${msg.type()}: ${msg.text()}`);
});

await page.setContent(
`
<ion-card button="true" aria-label="initial">Button</ion-button>

@ShaneK ShaneK Aug 7, 2026

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.

Suggested change
<ion-card button="true" aria-label="initial">Button</ion-button>
<ion-card button="true" aria-label="initial">Card</ion-card>

Copy/paste from the button spec. The item one has the same mismatched closing tag. The locator below is still called nativeButton too, and the comments in this block say inheritAriaAttributes where card calls inheritAttributes.

`,
config
);

const host = page.locator('ion-card');
const nativeButton = host.locator('[part="native"]');

// Initial load: inheritAriaAttributes should have stripped aria-label
// from the host and copied it onto the native element.
await expect(host).not.toHaveAttribute('aria-label');
await expect(nativeButton).toHaveAttribute('aria-label', 'initial');

// Setting a new value on the host: watcher should capture it, sync it
// to native, and re-strip it from the host.
await host.evaluate((el) => el.setAttribute('aria-label', 'second'));
await expect(host).not.toHaveAttribute('aria-label');
await expect(nativeButton).toHaveAttribute('aria-label', 'second');

// Setting to empty string: empty string is a valid, non-null value.
await host.evaluate((el) => el.setAttribute('aria-label', ''));
await expect(host).not.toHaveAttribute('aria-label');
await expect(nativeButton).toHaveAttribute('aria-label', '');

// Removing the attribute directly: the patched removeAttribute should
// fire onChange with null, which should remove aria-label from native
// and host.
await host.evaluate((el) => el.removeAttribute('aria-label'));
await expect(host).not.toHaveAttribute('aria-label');
await expect(nativeButton).not.toHaveAttribute('aria-label');
});
});
});
23 changes: 19 additions & 4 deletions core/src/components/item/item.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import type { ComponentInterface } from '@stencil/core';
import { Component, Element, Host, Listen, Prop, State, Watch, forceUpdate, h } from '@stencil/core';
import type { AnchorInterface, ButtonInterface } from '@utils/element-interface';
import type { Attributes } from '@utils/helpers';
import { inheritAttributes, raf } from '@utils/helpers';
import type { Attributes, AttributeWatcher } from '@utils/helpers';
import { inheritAttributes, watchAttributes, raf } from '@utils/helpers';
import { createColorClasses, hostContext, openURL } from '@utils/theme';
import { chevronForward } from 'ionicons/icons';

Expand Down Expand Up @@ -34,6 +34,7 @@ export class Item implements ComponentInterface, AnchorInterface, ButtonInterfac
private labelColorStyles = {};
private itemStyles = new Map<string, CssClassMap>();
private inheritedAriaAttributes: Attributes = {};
private ariaWatcher?: AttributeWatcher;

@Element() el!: HTMLIonItemElement;

Expand Down Expand Up @@ -164,12 +165,26 @@ export class Item implements ComponentInterface, AnchorInterface, ButtonInterfac
}
}

componentWillLoad() {}

Comment on lines +168 to +169

@ShaneK ShaneK Aug 7, 2026

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.

Suggested change
componentWillLoad() {}

Looks like this got left behind when the body moved down to connectedCallback.

connectedCallback() {
this.hasStartEl();
}

componentWillLoad() {
// Must run before watchForAriaAttributeChanges: it calls removeAttribute
// internally to strip the host's initial values, and that call must
// happen before removeAttribute is patched below — otherwise this
// strip would itself be treated as an external removal.
this.inheritedAriaAttributes = inheritAttributes(this.el, ['aria-label']);

this.ariaWatcher = watchAttributes(this.el, ['aria-label'], (changed) => {

@ShaneK ShaneK Aug 7, 2026

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.

Worth flagging that the coverage is uneven: button watches everything in ariaAttributes while item and card only watch aria-label, so <ion-item button aria-describedby="hint"> still never reaches the native button. That matches what inheritAttributes already did here so it isn't a regression, but this seems like the natural moment to widen it.

The other button-like components from my earlier comment are still on the load-once path too, ion-back-button and ion-menu-button among them, and I think a handful of others like ion-select and ion-tab-button are as well. Happy for those to go to a follow-up card, I'd just want it written down rather than left implicit.

this.inheritedAriaAttributes = { ...this.inheritedAriaAttributes, ...changed };
forceUpdate(this);
});
}

disconnectedCallback() {
this.ariaWatcher?.destroy();
this.ariaWatcher = undefined;
}

componentDidLoad() {
Expand Down
Loading