Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
53 changes: 36 additions & 17 deletions packages/button/src/ButtonBase.ts
Original file line number Diff line number Diff line change
Expand Up @@ -220,18 +220,45 @@ export class ButtonBase extends ObserveSlotText(LikeAnchor(Focusable), '', [
}
}

/**
* @label property is empty within changed propertyValues on slot change event.
* Need to check actualy @label property value instead.
*/
protected updateAriaLabel(): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

Only update aria-label if we have a label property and we're not in a pending state. You need to account for these scenarios too. A minor update to your code would be.

- `<slot @slotchange=${this.manageTextObservedSlot}></slot>` 

+ `<slot @slotchange=${this.handleSlotChange}></slot>` 

private handleSlotChange(): void {
    // Call the original manageTextObservedSlot method
    this.manageTextObservedSlot();
    // Only update aria-label if we have a label property and we're not in a pending state
    if (this.label && !('pending' in this && (this as { pending: boolean }).pending === true)) {
        this.updateAriaLabel();
    }
 }
    

Copy link
Author

Choose a reason for hiding this comment

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

Done.

Copy link
Author

Choose a reason for hiding this comment

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

This seem to cause the issue. I reverted my original solution.

if (
'pending' in this &&
(this as { pending: boolean }).pending === true
) {
return;
}
Comment on lines +228 to +233
Copy link
Author

Choose a reason for hiding this comment

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

Checking for pending state


// check if label exists and is not empty
if (this.label && this.label !== '') {
// prevent update if the label is the same as the aria-label
if (this.label !== this.getAttribute('aria-label')) {
this.setAttribute('aria-label', this.label);
}
} else {
// if dev set aria-label instead of label, don't remove it, and throw warning
if (
!this.hasAttribute('label') &&
this.hasAttribute('aria-label')
) {
console.warn(
this,
'DO NOT set aria-label attribute on sp-button, use label prop instead.'
);
return;
}
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 +270,8 @@ export class ButtonBase extends ObserveSlotText(LikeAnchor(Focusable), '', [
this.manageAnchor();
}

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.

Suggested change
this.updateAriaLabel();
if (changed.has('label')) {
this.updateAriaLabel();
}

Copy link
Author

Choose a reason for hiding this comment

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

Done.


if (this.anchorElement) {
// Ensure the anchor element is not focusable directly via tab
this.anchorElement.tabIndex = -1;
Expand All @@ -256,14 +285,4 @@ export class ButtonBase extends ObserveSlotText(LikeAnchor(Focusable), '', [
this.anchorElement.addEventListener('focus', this.proxyFocus);
}
}
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');
}
}
Comment on lines -261 to -267
Copy link
Contributor

Choose a reason for hiding this comment

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

In update it is still useful to track label changes.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

}
}
32 changes: 26 additions & 6 deletions packages/button/test/button.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,29 @@ describe('Button', () => {
<sp-button>Button</sp-button>
`)
);
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');
});

describe('dev mode', () => {
let consoleWarnStub!: ReturnType<typeof stub>;
before(() => {
Expand Down Expand Up @@ -395,15 +418,12 @@ describe('Button', () => {

it('manages aria-label from pending state', async () => {
const el = await fixture<Button>(html`
<sp-button
href="test_url"
target="_blank"
label="clickable"
pending
>
<sp-button href="test_url" target="_blank" label="clickable">
Click me
</sp-button>
`);

el.setAttribute('pending', '');
await elementUpdated(el);
expect(el.getAttribute('aria-label')).to.equal('Pending');

Expand Down
Loading