fix(button): sync aria description between host and native button - #31264
fix(button): sync aria description between host and native button#31264Zac-Smucker-Bryan wants to merge 4 commits into
Conversation
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.
|
@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. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
| * 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( |
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| // Prevents | ||
| disconnectedCallback() { |
There was a problem hiding this comment.
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.
| const name = mutation.attributeName; | ||
| if (!attributes.includes(name)) continue; | ||
| const value = el.getAttribute(name); | ||
| if (value === null) continue; |
There was a problem hiding this comment.
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.
| const watchedAriaAttributes = ariaAttributes.filter((attr) => attr !== 'aria-disabled'); | ||
|
|
||
| for (const attr of watchedAriaAttributes) { | ||
| test(`native button updates ${attr} when host attribute changes`, async ({ page }) => { |
There was a problem hiding this comment.
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.
|
|
||
| /** | ||
| * Keeps inherited ARIA attributes in sync with the host element for the | ||
| * lifetime of the component, not just at initial load. This replaces the |
There was a problem hiding this comment.
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!
| }; | ||
|
|
||
| export interface AttributeWatcher { | ||
| disconnect: () => void; |
There was a problem hiding this comment.
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.
|
|
||
| configs({ directions: ['ltr'] }).forEach(({ title, config }) => { | ||
| test.describe(title('button: aria attribute sync'), () => { | ||
| // Mirrors the ignoreList passed to inheritAriaAttributes/watchForAriaAttributeChanges |
There was a problem hiding this comment.
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!
Issue number: resolves #30626
What is the current behavior?
The native
buttoninsideion-buttonis not being updated with the aria-attributes if it changes onion-buttonafter initial render, includingaria-description.What is the new behavior?
The native
buttoninsideion-buttonupdates with the aria attributes if it changes onion-button. Changes include:ariaAttributesso it could be imported for use in testsariaAttributesaria-disabledsince it is managed inrender()separatelyprivate ariaWatcher?: AttributeWatcher;to class ButtoncomponentWillLoad()ariaWatcherandwatchForAriaAttributeChanges, replicating functionality of the watch block, but for all aria attributesaria-disabledDoes this introduce a breaking change?