Skip to content

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

Open
Zac-Smucker-Bryan wants to merge 7 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 7 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. The mutation observer also has a mechanism for when removeAttribute is called. This lets the helper notify components when an inherited ARIA attribute is explicitly removed, since once the helper has stripped the host attribute, a later removeAttribute() is a DOM no-op and produces no MutationRecord.
    • One which watches only for aria attribute changes, which calls the mutation observer.
    • Added Export to ariaAttributes so it could be imported for use in tests
  • 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
    • 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
    • Remove previous watch block since now handled by helper
    • In connectedCallback()
      • Move this.inheritedAttributes to connectedCallback()
      • Add ariaWatcher and watchForAriaAttributeChanges, replicating functionality of the watch block, but for all aria attributes
      • Add specific exclusion of aria-disabled
  • In item.tsx,
    • Move this.inheritedAttributes to connectedCallback()
    • Add ariaWatcher and watchForAriaAttributeChanges, replicating functionality of the watch block, only applying to aria-label currently
  • 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,
    • Move this.inheritedAttributes to connectedCallback()
    • Add ariaWatcher and watchForAriaAttributeChanges, replicating functionality of the watch block, only applying to aria-label currently
  • 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

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.
@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, Comment Jul 13, 2026 1:33pm

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.

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

@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
Comment thread core/src/utils/helpers.ts
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
…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
@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

this.inheritedAttributes = inheritAriaAttributes(this.el);
}

connectedCallback() {

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.

* happen before removeAttribute is patched below — otherwise this
* strip would itself be treated as an external removal.
*/
this.inheritedAttributes = inheritAriaAttributes(this.el, ['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.

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?

await expect(nativeButton).not.toHaveAttribute('aria-disabled', 'true');
});

test('aria sync survives detach and reattach', 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.

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.

this.inheritedAriaAttributes = inheritAttributes(this.el, ['aria-label']);
}

connectedCallback() {

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.

Comment thread core/src/utils/helpers.ts

// Intercept removeAttribute so we can notify consumers when an
// already-synced attribute is explicitly cleared.
el.removeAttribute = (name: string) => {

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 the patch is avoidable, and dropping it would take a lot of the surrounding complexity with it. The only reason a removal is invisible is that inheritAttributes strips the attribute off the host to begin with. If we strip once at load and then leave later host values alone, a plain observer with attributeOldValue: true sees set, change and remove natively.

That gets rid of the originalRemoveAttribute threading, the recursion comment above the observer, the install-order invariant three components now have to document, and the restore issue below. The cost is that after the first external write the value sits on both host and native, which is inert here since the host isn't the element AT reads. role could stay on the strip path.

There are also holes the patch can't cover: toggleAttribute, attributes.removeNamedItem and removeAttributeNode all go around it.


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

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.

Comment on lines +168 to +169
componentWillLoad() {}

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.

});
}

test('does not sync aria-disabled, since button.tsx manages it internally', 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.

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.

Comment thread core/src/utils/helpers.ts
* https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes
*/
const ariaAttributes = [
export const ariaAttributes = [

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.

Exporting this so the spec can loop it generates exactly 300 test executions on the branch (50 attributes, 2 modes, 3 projects), all driving the same attributeFilter membership check. It also can't fail for the right reason, since the expectations come from the same constant the implementation reads.

Three or four representative attributes in the spec would give the same signal and let this go back to being module private. Up to you though.

// 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.

@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.

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