Skip to content

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

Open
Zac-Smucker-Bryan wants to merge 12 commits into
ionic-team:mainfrom
Zac-Smucker-Bryan:button-aria-description
Open

fix(button): sync aria attributes between host and native button#31264
Zac-Smucker-Bryan wants to merge 12 commits into
ionic-team:mainfrom
Zac-Smucker-Bryan:button-aria-description

Conversation

@Zac-Smucker-Bryan

@Zac-Smucker-Bryan Zac-Smucker-Bryan commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Issue number: resolves #30626


What is the current behavior?

The native button inside ion-button is not being updated with the aria-attributes if it changes on ion-button after initial render, including aria-description. Additionally, the same dynamic is happening with other button like elements, including ion-card and ion-item.

What is the new behavior?

The native button inside ion-button updates with the aria attributes if it changes on ion-button. Changes include:

  • Add 2 helper functions to helpers.ts
    • A mutation observer watching for all attribute changes.
    • One which watches only for aria attribute changes, which calls the mutation observer.
  • Add 4 tests to button.e2e.ts in a11y:
    • One which loops through each aria attribute (imported from the helper) using ariaAttributes
    • One which specifically tests aria-disabled since it is managed in render() separately. Render forced with prop change.
    • One which looks at detaching and reattaching behavior
    • One which tests the helper strips, sets, accepts empty strings, and can process removeAttribute
  • Update button.tsx to:
    • Add import of new helpers watchForAriaAttributeChanges, type AttributeWatcher from '@utils/helpers'
    • Add private ariaWatcher?: AttributeWatcher; to class Button
    • Add private didLoad = false; for conditional use in connectedCallbackand use in added componentDidLoad for starting the watcher after initial component load
    • Remove previous watch block since now handled by helper
  • In item.tsx,
    • Add ariaWatcher and watchForAriaAttributeChanges, replicating functionality of the watch block, only applying to aria-label currently
    • Use existingprivate didLoad = false; for conditional use in connectedCallbackand use in existing componentDidLoad for starting the watcher after initial component load
  • In item.e2e.test, add tests to ensure:
    • native element updates aria-label when host attribute changes
    • aria-label sync survives detach and reattach
    • helper strips host attribute and syncs native element through set, empty, and remove
  • In card.tsx,
    • Add ariaWatcher and watchForAriaAttributeChanges, replicating functionality of the watch block, only applying to aria-label currently
    • Add private didLoad = false; for conditional use in connectedCallbackand use in added componentDidLoad for starting the watcher after initial component load
  • In card.e2e.test, add tests to ensure:
    • native element updates aria-label when host attribute changes
    • aria-label sync survives detach and reattach
    • helper strips host attribute and syncs native element through set, empty, and remove

Does this introduce a breaking change?

  • Yes
  • No

@vercel

vercel Bot commented Jul 10, 2026

Copy link
Copy Markdown

@Zac-Smucker-Bryan is attempting to deploy a commit to the Ionic Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions github-actions Bot added the package: core @ionic/core package label Jul 10, 2026
@Zac-Smucker-Bryan
Zac-Smucker-Bryan marked this pull request as ready for review July 11, 2026 18:09
@Zac-Smucker-Bryan
Zac-Smucker-Bryan requested a review from a team as a code owner July 11, 2026 18:09
@vercel

vercel Bot commented Jul 13, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
ionic-framework Ready Ready Preview Aug 31, 2026 9:05pm

Request Review

@ShaneK ShaneK left a comment

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.

Thanks for this PR!

I talked to the team about this PR and, while it does look solid and solve the issue's title problem, there's a bit more to it than this.

There's actually 2 remaining issues in the original issue that won't be resolved by this PR:

  • Many other aria attributes not being copied
  • Other components that can act like buttons not getting this fix (this was a comment on the issue, but should still be addressed)

The first one I've created an internal follow-up ticket to address because it's much more involved than this solution and something we'll need to review on how to do well. We can't just add 50 @Watchs and call it good.

The second one I'd prefer if we address in this PR

@codeCraft-Ritik codeCraft-Ritik left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Great fix! Synchronizing dynamic aria-description updates with the native button improves accessibility while keeping the component behavior consistent. The accompanying test provides good confidence in the change.

@ShaneK ShaneK left a comment

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.

Thanks for reworking this! The helper approach is the right direction and a lot better than adding a @watch per attribute. Left a handful of comments inline.

The two I'd want sorted before this goes in: the watcher gets torn down on disconnectedCallback but never recreated, so aria sync breaks the first time the button moves in the DOM, and covering the other button-like components (ion-item/ion-card at least) from my earlier review. The rest are nits.

Comment thread core/src/components/button/button.tsx
Comment thread core/src/components/button/button.tsx Outdated
Comment thread core/src/utils/helpers.ts Outdated
Comment thread core/src/components/button/test/a11y/button.e2e.ts
Comment thread core/src/components/button/button.tsx Outdated
Comment thread core/src/utils/helpers.ts Outdated
Comment thread core/src/components/button/test/a11y/button.e2e.ts Outdated
@Zac-Smucker-Bryan Zac-Smucker-Bryan changed the title fix(button): sync aria description between host and native button fix(button): sync aria attributes between host and native button Aug 5, 2026
@Zac-Smucker-Bryan

Copy link
Copy Markdown
Contributor Author

PR revised based on feedback and expanded scope

@ShaneK ShaneK left a comment

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.

Thank you so much for taking a shot at this massive undertaking! Let me know if you'd like me to help you out with this, I know it's a pretty daunting task

Comment thread core/src/components/button/button.tsx
Comment thread core/src/components/button/button.tsx Outdated
Comment thread core/src/components/button/test/a11y/button.e2e.ts Outdated
Comment thread core/src/components/card/card.tsx
Comment thread core/src/utils/helpers.ts Outdated
Comment thread core/src/components/card/test/a11y/card.e2e.ts Outdated
Comment thread core/src/components/item/item.tsx Outdated
Comment thread core/src/components/button/test/a11y/button.e2e.ts Outdated
Comment thread core/src/utils/helpers.ts Outdated
Comment thread core/src/components/item/item.tsx Outdated
// strip would itself be treated as an external removal.
this.inheritedAriaAttributes = inheritAttributes(this.el, ['aria-label']);

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If you want me to widen it for the item (same thing on card), let me know. I just didn't want to widen things any more than I have.

Likely those other components should be a follow up issue just to try to wrap up these 3 components.

@Zac-Smucker-Bryan

Copy link
Copy Markdown
Contributor Author

Thank you so much for taking a shot at this massive undertaking! Let me know if you'd like me to help you out with this, I know it's a pretty daunting task

@ShaneK Yes, this PR is pushing the limits of what I know. But I have learned a lot.

I would love your help, especially with the helper.ts to get it to a point where it would... actually be helpful. I suspect you can reduce the complexity better than I can.

I have done an initial review of your comments and think I can address everything else with more helpful tests and such, once we get the right helper structure in place. My only other question would be: in your comment #31264 (comment), you say "Moving the inherit into connectedCallback fixes the watcher lifetime, but I think it trades that for a worse problem." If the ultimate helper means things can go back to the way they were in terms of calls, all the better, I just didn't want to create other issues without thinking it through.

Let me know the best way to collaborate to get it done.

@Zac-Smucker-Bryan

Copy link
Copy Markdown
Contributor Author

As I am working through things, I think I can have a revised PR/commits/tests etc. next week @ShaneK, and think I even have a helper that is simpler based on your specific feedback.

@github-actions github-actions Bot added package: angular @ionic/angular package package: vue @ionic/vue package package: react @ionic/react package labels Aug 26, 2026
Adds @watch('aria-description') to button.tsx before onAriaChanged
Set aria description and both buttons should match. Update aria description on host button, and both buttons should still match.
Previously, ARIA attributes inherited from the host were only captured once at componentWillLoad. Attributes set or changed after initial load (e.g. by ion-input-password-toggle updating aria-label/aria-pressed as
visibility toggles) were not reflected onto the native button, causing screen readers to announce stale values unless a watch decorator was used for each attribute.

Adds watchAttributes/watchForAriaAttributeChanges to helpers.ts, which use a MutationObserver to keep inherited ARIA attributes in sync for the lifetime of the component. Replaces the previous per-attribute @watch decorators with this more general mechanism.

Update Button.tsx to reflect this and use these new helpers. Add tests to test syncing all attributes.

Fixes ionic-team#30626
…pt to helper

Change disconnect to destroy to match other ionic conventions
Update onChange to accept null values
Add support for removeAttribute, including if null values triggered
…ttributes to connectedCallback

Import helper to ion-item and ion-card
move inheritedAriaAttributes to connectedCallback in these components to preserve helper call order
Update tests for ion-button with annotations
Add tests for removeAttribute and attribute sync to ion-button, ion-card, and ion-item
Consistency based on element being tested
@github-actions github-actions Bot removed package: angular @ionic/angular package package: vue @ionic/vue package package: react @ionic/react package labels Aug 27, 2026
@utils/helpers was imported twice and passed npm run lint but failed build. Now, it is imported only once.

@ShaneK ShaneK left a comment

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.

This is really close now, and the helper itself is in good shape after the last round.

The one I'd want sorted is the ignore list on ion-card and ion-item. They inherit only aria-label but watch all ~49 ARIA attributes, so role and aria-hidden reach the native element after load when neither does at load. On ion-item that means the inner <button> can end up carrying role="listitem". Swapping those two calls to watchAttributes(this.el, ['aria-label'], ...) covers it.

The other is the detach/reattach and aria-disabled tests. I reverted the four source files to main, kept the specs, and both still pass, so neither covers the thing it's named for.

Everything else is nits. If you'd rather hand this off at this point just say the word and I can finish it up. Thank you for sticking with this one, it's a much better fix than where it started.

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

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.

This is the ignore list, so the watcher covers every ARIA attribute except aria-disabled, around 49 of them including role. The inherit above only takes aria-label.

The role case is the one that worries me. Item renders role={inList ? 'listitem' : null} on the Host, so moving an item into an ion-list after load has the watcher pick up Stencil's own host write, and the native element comes out as a <button> carrying role="listitem". An aria-hidden or aria-describedby set after load reaches the native element too, where neither does at load.

I think this wants watchAttributes(this.el, ['aria-label'], ...) instead. That one is exported already and takes an include list, so the watch set matches the inherit set and the role case goes with it.

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

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.

Same ignore-list issue as the item watcher, and the same fix. Card has no role expression on its Host so that one doesn't apply here, but aria-describedby and aria-hidden still reach the native element after load when neither does at load.

Two small things while you're here. Card never renders aria-disabled on its Host and never inherits it, so that ignore entry isn't guarding anything. And a card with no button and no href renders no native element at all, so on a plain ion-card the watcher runs with nowhere to put its output. Worth gating on isClickable().

});

// Assert the original value survived
await expect(nativeButton).toHaveAttribute('aria-label', 'label');

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 don't think this can fail. Reverting the four source files to main and keeping the specs, this one still passes, along with the card and item copies. The value comes from inheritedAttributes, which is instance state, and componentWillLoad doesn't run again on a move, so it survives whether or not there's a watcher.

That means the connectedCallback re-arm has no coverage: deleting startAriaWatcher() from connectedCallback in all three components fails nothing. Setting aria-label to a new value after the reattach and asserting the native element picks it up would cover what I was after in the earlier thread.

// Initial inheritance moves the developer-provided value to native.
// The host's aria-disabled is subsequently owned by the disabled prop.
await expect(host).not.toHaveAttribute('aria-disabled');
await expect(nativeButton).toHaveAttribute('aria-disabled', 'true');

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.

This one passes with the source reverted to main too. It only checks the initial inheritance, which main already did, and the aria-disabled ignore list only affects post-load mutations, so you could delete the ignore list entirely and nothing here would notice. Setting aria-disabled on the host after load and asserting the native value doesn't change would cover it.

The name reads a bit odd too, since the assertion is that the value does get inherited at load.

Comment thread core/src/utils/helpers.ts
* stays in sync for the lifetime of the component — not just at initial load.
*
* Call this in connectedCallback, alongside the initial inheritAriaAttributes
* call, and call destroy() on the returned watcher in 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.

Still the pre-rename doc, same as my earlier note. None of the three callers do what it says: they arm from componentDidLoad plus connectedCallback behind a didLoad guard, and the inherit stays in componentWillLoad. Following it would break things, since inheritAttributes calls removeAttribute and an observer armed in connectedCallback would see that strip as a removal and merge null over the value just captured.

Worth @param tags too, matching the two functions above. This ignoreList means "don't watch" while theirs means "don't reflect or strip", and that's the distinction that got lost in card and item.

Comment thread core/src/utils/helpers.ts

/**
* Watches an element for changes to a given set of attributes and calls
* onChange whenever one changes. Returns null when an attribute is removed.

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.

That last line reads as though the function can return null, but it always returns an AttributeWatcher. The null is a value in the map handed to onChange.

Worth saying in here that clearing only works for values written after load. An ion-button with aria-label in the initial HTML has it stripped off the host, so a later removeAttribute fires no mutation and the native element keeps the old value. I checked and it does stay stale. That's the tradeoff we settled on, I'd just rather have it written down than left for someone to rediscover.

Comment thread core/src/utils/helpers.ts
observer.observe(el, {
attributes: true,
attributeFilter: attributes,
attributeOldValue: true,

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 suggested attributeOldValue when we talked about dropping the patch, but nothing ends up reading mutation.oldValue, since the callback re-reads with getAttribute. Probably fine to drop it. Same for attributes: true, which attributeFilter implies, and attributeFilter also makes both continue guards above unreachable.

Nit on the guard above: the rest of core does win !== undefined && 'MutationObserver' in win off @utils/browser, the way createSlotMutationController does. Up to you though.

});

configs({ directions: ['ltr'] }).forEach(({ title, config }) => {
test.describe(title('item: aria attribute sync'), () => {

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
test.describe(title('item: aria attribute sync'), () => {
test.describe(title('card: aria attribute sync'), () => {

Looks like this one survived the copy/paste cleanup from the earlier pass. It collides exactly with the item spec's describe block, so both come through as item: in the report.

Same file, the detach/reattach test still names its evaluate arg itemEl and casts an ion-card to HTMLIonButtonElement, which only compiles because all three elements happen to have color. Item spec has the same cast.

this.ariaWatcher = undefined;
}

private startAriaWatcher() {

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 didLoad and ariaWatcher fields plus this method and the three lifecycle hooks are near-identical across all three components, about thirty lines each. Each one re-deriving the arming rule for itself is, I think, how the card and item watch sets ended up wrong. I'd like this folded into a controller in the style of createSlotMutationController, but that's a bigger change than the rest of this and I'm happy to take it on rather than push it back to you.

One thing worth keeping though: the JSDoc on the @Watch block this replaces was the only record we had that ion-button needs live ARIA because ion-input-password-toggle re-renders aria-label and aria-pressed on it. A line of that here would stop a future refactor quietly dropping the watcher. The toggle still works, for what it's worth, all six of its a11y tests pass on this branch.

if (this.didLoad) {
this.watchForIndicatorControls();
this.updateInteractivityOnSlotChange();
this.startAriaWatcher();

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.

To answer your question from last round: yeah, leave item and card at aria-label only and let the rest go to a follow-up. Sorry for the slow reply on that one.

The only thing I'd still want here is the follow-up written down. Could you open an issue for the components still on the load-once path and link it from the PR body? The list is ion-back-button, ion-menu-button, ion-fab-button, ion-breadcrumb, ion-select, ion-tab-button, ion-segment-button, ion-picker-column-option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

package: core @ionic/core package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: ion-button aria-description does not reactively change

3 participants