Skip to content
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.

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

fix(action-button): action-button with href can be perceived by screen reader #4927

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
36fb0b3
fix(action-button): fixed link activation with VoiceOver
nikkimk Nov 8, 2024
bd3d04b
test(button): added test for link click
nikkimk Nov 8, 2024
268b96c
test(action-button): added test for link click
nikkimk Nov 8, 2024
8d02778
docs(action-button): added story for action button with href
nikkimk Nov 8, 2024
5e5af16
test(action-button): fixed href click test
nikkimk Nov 8, 2024
dccf966
test(button): fixed href click test
nikkimk Nov 8, 2024
877b163
Merge branch 'main' into nikkimk/4576-bug-button-href
Rajdeepc Nov 11, 2024
4515ced
Merge branch 'main' into nikkimk/4576-bug-button-href
Rajdeepc Nov 11, 2024
fa15a3e
fix(button): refactored ro remove listener that only works with progr…
nikkimk Nov 11, 2024
38d9a05
test(button): updates test for mouse click event (which also works fo…
nikkimk Nov 11, 2024
4269b98
test(action-button): updates test for mouse click event (which also w…
nikkimk Nov 11, 2024
96605bc
test(action-button): added comments to test
nikkimk Nov 11, 2024
3ca3044
test(button): added comments to test
nikkimk Nov 11, 2024
9c04961
Merge branch 'main' into nikkimk/4576-bug-button-href
nikkimk Nov 12, 2024
2cee1f9
docs(button): updated docs
nikkimk Nov 13, 2024
f37bf95
docs(action-button): updated docs
nikkimk Nov 13, 2024
c8c3f2b
test(button): removed redundant attribute setting
nikkimk Nov 13, 2024
1e6f419
test(action-button): removed redundant attribute setting
nikkimk Nov 13, 2024
ea38806
fix(action-button): updated hash for VRTs
nikkimk Nov 14, 2024
b282aba
test(button): added missing href to link test
nikkimk Nov 14, 2024
781e501
ci: updated golden image cache
Nov 15, 2024
bd8befd
Merge branch 'main' into nikkimk/4576-bug-button-href
nikkimk Nov 15, 2024
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
9 changes: 9 additions & 0 deletions packages/action-button/stories/action-button.stories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,3 +44,12 @@ toggles.args = {
<sp-icon-edit hidden slot="icon"></sp-icon-edit>
`,
};

export const href = (args: Properties): TemplateResult =>
renderButtonsSelected(args);
href.args = {
href: 'https://github.com/adobe/spectrum-web-components',
icon: html`
<sp-icon-edit hidden slot="icon"></sp-icon-edit>
`,
};
2 changes: 2 additions & 0 deletions packages/action-button/stories/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,13 @@ export interface Properties {
icon?: TemplateResult;
label?: string;
[prop: string]: unknown;
href: undefined;
}

export function renderButton(properties: Properties): TemplateResult {
return html`
<sp-action-button
href=${ifDefined(properties.href)}
?quiet="${!!properties.quiet}"
?emphasized="${!!properties.emphasized}"
static-color="${ifDefined(properties.staticColor)}"
Expand Down
35 changes: 35 additions & 0 deletions packages/action-button/test/action-button.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import { sendKeys } from '@web/test-runner-commands';
import { spy } from 'sinon';
import { testForLitDevWarnings } from '../../../test/testing-helpers.js';
import { m as BlackActionButton } from '../stories/action-button-black.stories.js';
import { sendMouse } from '../../../test/plugins/browser.js';

describe('ActionButton', () => {
testForLitDevWarnings(
Expand Down Expand Up @@ -295,4 +296,38 @@ describe('ActionButton', () => {
expect(el.staticColor).to.be.null;
expect(el.hasAttribute('static-color')).to.be.false;
});
it('allows link click', async () => {
let clicked = false;
const el = await fixture<ActionButton>(html`
<sp-action-button href="#top" target="_blank">
With Target
</sp-action-button>
`);

// ensures that proxy listener isn't removed when button is updated to link
await elementUpdated(el);
el.href = '#top';
nikkimk marked this conversation as resolved.
Show resolved Hide resolved
await elementUpdated(el);

el.shadowRoot
?.querySelector('.anchor')
?.addEventListener('click', (event: Event) => {
event.preventDefault();
clicked = true;
});
const rect = el.getBoundingClientRect();
await sendMouse({
steps: [
{
position: [
rect.left + rect.width / 2,
rect.top + rect.height / 2,
],
type: 'click',
},
],
});
await elementUpdated(el);
expect(clicked).to.be.true;
});
});
40 changes: 25 additions & 15 deletions packages/button/src/ButtonBase.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,23 @@ export class ButtonBase extends ObserveSlotText(LikeAnchor(Focusable), '', [
return [buttonStyles];
}

// TODO we need to document this property for consumers,
// as it's not a 1:1 equivalent to active
@property({ type: Boolean, reflect: true })
public active = false;

/**
* The default behavior of the button.
* Possible values are: `button` (default), `submit`, and `reset`.
*/
@property({ type: String })
public type: 'button' | 'submit' | 'reset' = 'button';

/**
* HTML anchor element that component clicks by proxy
*/
@query('.anchor')
private anchorElement!: HTMLButtonElement;
private anchorElement!: HTMLAnchorElement;

public override get focusElement(): HTMLElement {
return this;
Expand Down Expand Up @@ -76,25 +85,17 @@ export class ButtonBase extends ObserveSlotText(LikeAnchor(Focusable), '', [
});
}

public override click(): void {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This event is only firing when with a programatic click()--like in parent components or in the majority of our tests. It is not firing with a mouse click (or, by extension, a VoiceOver click (i.e. CTRL+Option+Space. I removed it and made sure this functionality was in handleClickCapture() function which is called by the click event listener in the constructor(), which is fired by programatic and mouse clicks.

if (this.disabled) {
return;
}

if (this.shouldProxyClick()) {
return;
}

super.click();
}

private handleClickCapture(event: Event): void | boolean {
if (this.disabled) {
event.preventDefault();
event.stopImmediatePropagation();
event.stopPropagation();
return false;
}

if (this.shouldProxyClick()) {
return;
}
}

private proxyFocus(): void {
Expand All @@ -104,9 +105,12 @@ export class ButtonBase extends ObserveSlotText(LikeAnchor(Focusable), '', [
private shouldProxyClick(): boolean {
let handled = false;
if (this.anchorElement) {
// click HTML anchor element by proxy
this.anchorElement.click();
handled = true;
// if the button type is `submit` or `reset`
} else if (this.type !== 'button') {
// create an HTML Button Element by proxy, click it, and remove it
const proxy = document.createElement('button');
proxy.type = this.type;
this.insertAdjacentElement('afterend', proxy);
Expand Down Expand Up @@ -145,6 +149,7 @@ export class ButtonBase extends ObserveSlotText(LikeAnchor(Focusable), '', [
switch (code) {
case 'Space':
event.preventDefault();
// allows button to activate when `Space` is pressed
if (typeof this.href === 'undefined') {
this.addEventListener('keyup', this.handleKeyup);
this.active = true;
Expand All @@ -160,6 +165,7 @@ export class ButtonBase extends ObserveSlotText(LikeAnchor(Focusable), '', [
switch (code) {
case 'Enter':
case 'NumpadEnter':
// allows button or link to be activated with `Enter` and `NumpadEnter`
this.click();
break;
default:
Expand All @@ -181,22 +187,26 @@ export class ButtonBase extends ObserveSlotText(LikeAnchor(Focusable), '', [
}

private manageAnchor(): void {
// for a link
nikkimk marked this conversation as resolved.
Show resolved Hide resolved
if (this.href && this.href.length > 0) {
// if the role is set to button
if (
!this.hasAttribute('role') ||
this.getAttribute('role') === 'button'
) {
// change role to link
this.setAttribute('role', 'link');
}
this.removeEventListener('click', this.shouldProxyClick);
// else for a button
} else {
// if the role is set to link
if (
!this.hasAttribute('role') ||
this.getAttribute('role') === 'link'
) {
// change role to button
this.setAttribute('role', 'button');
}
this.addEventListener('click', this.shouldProxyClick);
}
}

Expand Down
33 changes: 33 additions & 0 deletions packages/button/test/button.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
findAccessibilityNode,
sendKeys,
} from '@web/test-runner-commands';
import { sendMouse } from '../../../test/plugins/browser.js';
import { spy, stub } from 'sinon';

type TestableButtonType = {
Expand Down Expand Up @@ -219,6 +220,38 @@ describe('Button', () => {
expect(el).to.not.be.undefined;
expect(el.textContent).to.include('With Target');
});
it('allows link click', async () => {
nikkimk marked this conversation as resolved.
Show resolved Hide resolved
let clicked = false;
const el = await fixture<Button>(html`
<sp-button>Button as link</sp-button>
`);

// ensures that proxy listener isn't removed when button is updated to link
await elementUpdated(el);
el.href = '#top';
await elementUpdated(el);

el.shadowRoot
?.querySelector('.anchor')
?.addEventListener('click', (event: Event) => {
event.preventDefault();
clicked = true;
});
const rect = el.getBoundingClientRect();
await sendMouse({
steps: [
{
position: [
rect.left + rect.width / 2,
rect.top + rect.height / 2,
],
type: 'click',
},
],
});
await elementUpdated(el);
expect(clicked).to.be.true;
});
it('accepts shift+tab interactions', async () => {
let focusedCount = 0;
const el = await fixture<Button>(html`
Expand Down
Loading