Skip to content

fix(button): update aria-label when label changes #5691

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
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

fix(button): update aria-label when label changes #5691

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3543797
test(button): added aria-label tests for label and slot changes
mizgaionutalexandru Aug 12, 2025
47725c8
fix(button): aria-label update when label updates
mizgaionutalexandru Aug 12, 2025
421653f
fix(button): narrow pending state gate to after controller update
mizgaionutalexandru Aug 12, 2025
52489e7
chore: changeset
mizgaionutalexandru Aug 12, 2025
7ff7359
fix: update pendingState type check
mizgaionutalexandru Aug 13, 2025
e090f5b
chore: changeset adjustments
mizgaionutalexandru Sep 3, 2025
0509970
test: changed title to sentence case
mizgaionutalexandru Sep 3, 2025
605828b
Merge branch 'main' into imizga/button-aria-label-fixes
mizgaionutalexandru Sep 10, 2025
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
5 changes: 5 additions & 0 deletions .changeset/hot-clouds-burn.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@spectrum-web-components/button': patch
---

**Fixed** aria-label updates when `label` property changes.
33 changes: 21 additions & 12 deletions packages/button/src/ButtonBase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,18 +220,26 @@ export class ButtonBase extends ObserveSlotText(LikeAnchor(Focusable), '', [
}
}

private isPendingState(): boolean {
return (
'pending' in this && (this as { pending: boolean }).pending === true
);
}

private updateAriaLabel(): void {
if (this.label) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

this.setAttribute('aria-label', this.label);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@Rajdeepc Rajdeepc Aug 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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-label to label.
  • If neither is set, no aria-label is present.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I agree with both of you! Let's create a follow up ticket and unblock this PR for now.

} else {
this.removeAttribute('aria-label');
}
}

protected override firstUpdated(changed: PropertyValues): void {
super.firstUpdated(changed);
if (!this.hasAttribute('tabindex')) {
this.setAttribute('tabindex', '0');
}
if (changed.has('label')) {
if (this.label) {
this.setAttribute('aria-label', this.label);
} else {
this.removeAttribute('aria-label');
}
}

this.manageAnchor();
this.addEventListener('keydown', this.handleKeydown);
this.addEventListener('keypress', this.handleKeypress);
Expand All @@ -243,6 +251,11 @@ export class ButtonBase extends ObserveSlotText(LikeAnchor(Focusable), '', [
this.manageAnchor();
}

// Don't override PendingStateController's aria-label changes
if (changed.has('label') && !this.isPendingState()) {
this.updateAriaLabel();
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@mizgaionutalexandru mizgaionutalexandru Aug 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool thats something we would like to do in future. Do you mind validating this in screen readers?

Copy link
Contributor Author

@mizgaionutalexandru mizgaionutalexandru Aug 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Sure, I added a screen recording, validating the manual test cases with a screen reader. Let me know if I should add anything else.

}

if (this.anchorElement) {
// Ensure the anchor element is not focusable directly via tab
this.anchorElement.tabIndex = -1;
Expand All @@ -259,11 +272,7 @@ export class ButtonBase extends ObserveSlotText(LikeAnchor(Focusable), '', [
protected override update(changes: PropertyValues): void {
super.update(changes);
if (changes.has('label')) {
if (this.label) {
this.setAttribute('aria-label', this.label);
} else {
this.removeAttribute('aria-label');
}
this.updateAriaLabel();
}
}
}
39 changes: 39 additions & 0 deletions packages/button/test/button.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,45 @@ describe('Button', () => {
expect(el.getAttribute('aria-label')).to.equal('clickable');
});

it('updates aria-label when label changes', async () => {
const el = await fixture<Button>(html`
<sp-button label="Initial Label">Button</sp-button>
`);

await elementUpdated(el);
expect(el.getAttribute('aria-label')).to.equal('Initial Label');

// Change the label
el.label = 'New Label';
await elementUpdated(el);

// The aria-label should also update
expect(el.getAttribute('aria-label')).to.equal('New Label');
});

it('preserves aria-label when slot content changes', async () => {
const el = await fixture<Button>(html`
<sp-button label="Test Label">Initial Content</sp-button>
`);

await elementUpdated(el);
expect(el.getAttribute('aria-label')).to.equal('Test Label');

// Change the slot content
el.textContent = 'Updated Content';
await elementUpdated(el);

// The aria-label should still be preserved
expect(el.getAttribute('aria-label')).to.equal('Test Label');

// Change slot content again
el.innerHTML = '<span>New Content</span>';
await elementUpdated(el);

// The aria-label should still be preserved
expect(el.getAttribute('aria-label')).to.equal('Test Label');
});

it('manages aria-label set from outside', async () => {
const el = await fixture<Button>(html`
<sp-button
Expand Down
Loading