fix(button): update aria-label when label changes #5691
Conversation
🦋 Changeset detectedLatest commit: 605828b The changes in this PR will be included in the next version bump. This PR includes changesets to release 84 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📚 Branch Preview🔍 Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
Tachometer resultsChromebutton permalinktest-basic
Firefoxbutton permalinktest-basic
|
|
|
||
| private updateAriaLabel(): void { | ||
| if (this.label) { | ||
| this.setAttribute('aria-label', this.label); |
There was a problem hiding this comment.
If a user sets aria-label directly after the component has set it via the label property, the component might override the user's intent here. I would be checking if aria-label is set by the user or not before we trigger component logic.
private _userSetAriaLabel = false;
connectedCallback() {
super.connectedCallback();
// Detect if aria-label was set by the user before component logic runs
this._userSetAriaLabel = this.hasAttribute('aria-label');
}
attributeChangedCallback(name: string, oldValue: string | null, newValue: string | null) {
if (name === 'aria-label' && oldValue !== newValue) {
this._userSetAriaLabel = true;
}
super.attributeChangedCallback(name, oldValue, newValue);
}
private updateAriaLabel(): void {
if (this._userSetAriaLabel) return; // Don't override user intent
if (this.label) {
this.setAttribute('aria-label', this.label);
} else {
this.removeAttribute('aria-label');
}
}There was a problem hiding this comment.
So we want to allow both label and aria-label on the component at the same time? Isn't label used just for aria-label purposes, as per docs and code?
If the consumer doesn't set a label but sets an aria-label, the component will have the desired aria-label.
There was a problem hiding this comment.
Theoretically we generally don’t want both to apply at the same time. The implementation should respect a consumer’s explicit aria-label and only use label as a fallback for accessibility. This will avoid race conditions and user confusion.
I think we should document that aria-label always takes priority over label.
- If both are set, use
aria-label. - If only label is set, set
aria-labelto label. - If neither is set, no
aria-labelis present.
There was a problem hiding this comment.
I agree that this might be a good improvement, even though there is no need for a consumer to provide both label and aria-label. Currently, on main, if label and aria-label are set, the label will override the aria-label. That being said, I feel like adding more is out of scope and we're looking forward to see this fix merged. Can we keep this in mind and act on it in another PR if it truly is something we want?
There was a problem hiding this comment.
I think keeping that change as out of scope makes sense. I'd like to see us create a follow-up ticket to cover the suggestion though. @Rajdeepc would you be able to take care of that and include your code suggestions?
There was a problem hiding this comment.
Yes I agree with both of you! Let's create a follow up ticket and unblock this PR for now.
|
|
||
| // Don't override PendingStateController's aria-label changes | ||
| if (changed.has('label') && !this.isPendingState()) { | ||
| this.updateAriaLabel(); |
There was a problem hiding this comment.
Both update and updated might call updateAriaLabel for the same change. Can you call updateAriaLabel in only one lifecycle or just make sure it runs once per change.
Do you have any specific reason to call this in update method?
There was a problem hiding this comment.
This surfaces multiple conflicts since the Button component has label and the PendingStateController that modify the aria-label. Maybe a long term major change would be to get rid of the label prop altogether 🤔 or if we keep it we need a library level way to determine if it's only for aria-label purposes or not, in order to update the PendingStateController accordingly.
For now I gated the updates according to the pending state to happen either in update or updated.
There was a problem hiding this comment.
Cool thats something we would like to do in future. Do you mind validating this in screen readers?
There was a problem hiding this comment.
Sure, I added a screen recording, validating the manual test cases with a screen reader. Let me know if I should add anything else.
There was a problem hiding this comment.
IMO we should not add support for consumers using aria-label directly on SWC.
aria-label is an aria global so it forces the component itself to be exposed as an accessible node. If in the future we wanted to change the implementation to instead use a button within the shadow DOM we wouldn't be able to do so.
This PR does not "add support for aria-label". It only fixes an inconsistency bug in that area. |
| } | ||
|
|
||
| private updateAriaLabel(): void { | ||
| if (this.label) { |
There was a problem hiding this comment.
Should we explicitly check for undefined instead because empty quotes would be a valid label (label="")? cc @nikkimk is that allowed on buttons or am I just thinking of images that use the empty label string sometimes?
There was a problem hiding this comment.
Good question, I haven't thought of that since this check was also used previously. 🤔
My take is that we don't want to allow consumers to use an empty string as an aria-label, they can just skip using it altogether. I know something like this is used with the alt attribute of images though.
|
|
||
| private updateAriaLabel(): void { | ||
| if (this.label) { | ||
| this.setAttribute('aria-label', this.label); |
There was a problem hiding this comment.
I think keeping that change as out of scope makes sense. I'd like to see us create a follow-up ticket to cover the suggestion though. @Rajdeepc would you be able to take care of that and include your code suggestions?
Description
This PR modifies the aria-label update logic of the
ButtonBasein order to correctly update when thelabelproperty changes during its lifetime, gating the buttons that use thePendingStateControllerto not override their aria-label manipulation.Unit tests were added to validate these usecases in the future. All existing tests pass.
Implementation based on #5674
Motivation and context
Currently when the
labelproperty changes, thearia-labelof the button components do not reflect this change.This issue was introduced in 0.48.0 with the new PendingStateController, hence this PR fixes a regression.
Related issue(s)
Screenshots (if appropriate)
labelSWCdemo.mp4
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
Validate
aria-labelupdates whenlabelupdatesValidate
aria-labelvalues when element's content changesDevice review