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.

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

Jump to bottom

fix(dropdown): improve accessibility #905

Merged
merged 4 commits into from
Sep 29, 2020
Merged

fix(dropdown): improve accessibility #905

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
068b3b3
fix(dropdown): improve accessibility
Westbrook Sep 7, 2020
82fd8ea
ci: update golden images cache
Westbrook Sep 26, 2020
94b8180
test: update timeout configuration for test stability
Westbrook Sep 28, 2020
2f25390
refactor(button): structure base class render method for easier compo…
Westbrook Sep 29, 2020
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
2 changes: 1 addition & 1 deletion .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ commands:
- restore_cache:
name: Restore Golden Images Cache
keys:
- v2-golden-images-c1f45565c447e1d047188056a3d92051a5256a7d-<< parameters.regression_color >>-<< parameters.regression_scale >>-<< parameters.regression_dir >>
- v2-golden-images-d03d62ccb93494ae1bc7ecfc21fe66e1c5bc46a9-<< parameters.regression_color >>-<< parameters.regression_scale >>-<< parameters.regression_dir >>
- v2-golden-images-main-<< parameters.regression_color >>-<< parameters.regression_scale >>-<< parameters.regression_dir >>-
- run: yarn test:visual:ci --color=<< parameters.regression_color >> --scale=<< parameters.regression_scale >> --dir=<< parameters.regression_dir >>
- run:
Expand Down
8 changes: 4 additions & 4 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,10 +89,10 @@
"@commitlint/cli": "^9.1.2",
"@commitlint/config-conventional": "^11.0.0",
"@commitlint/config-lerna-scopes": "^9.1.2",
"@open-wc/building-webpack": "^2.13.42",
"@open-wc/demoing-storybook": "^2.4.1",
"@open-wc/building-webpack": "^2.13.43",
"@open-wc/demoing-storybook": "^2.4.2",
"@open-wc/polyfills-loader": "^0.3.3",
"@open-wc/testing": "^2.5.26",
"@open-wc/testing": "^2.5.27",
"@spectrum-css/table": "^3.0.0-beta.3",
"@types/chai": "^4.1.7",
"@types/command-line-args": "^5.0.0",
Expand Down Expand Up @@ -146,7 +146,7 @@
"husky": "^4.2.1",
"lerna": "^3.20.2",
"linebyline": "^1.3.0",
"lit-analyzer": "^1.2.0",
"lit-analyzer": "^1.2.1",
"lit-element": "^2.4.0",
"lit-html": "^1.0.0",
"lodash": "^4.17.15",
Expand Down
22 changes: 13 additions & 9 deletions packages/button/src/ButtonBase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,21 +72,25 @@ export class ButtonBase extends LikeAnchor(
return content;
}

protected renderButton(): TemplateResult {
return html`
<button
id="button"
class="button"
aria-label=${ifDefined(this.label)}
>
${this.buttonContent}
</button>
`;
}

protected render(): TemplateResult {
return this.href && this.href.length > 0
? this.renderAnchor({
id: 'button',
className: 'button',
anchorContent: this.buttonContent,
})
: html`
<button
id="button"
class="button"
aria-label=${ifDefined(this.label)}
>
${this.buttonContent}
</button>
`;
: this.renderButton();
}
}
2 changes: 2 additions & 0 deletions packages/dropdown/src/Dropdown.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,8 @@ export class DropdownBase extends Focusable {
return html`
<button
aria-haspopup="true"
aria-controls="popover"
aria-expanded=${this.open ? 'true' : 'false'}
aria-label=${ifDefined(this.label || undefined)}
id="button"
class="button"
Expand Down
4 changes: 1 addition & 3 deletions packages/dropdown/stories/dropdown.stories.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,8 @@ export const Default = (): TemplateResult => {
const dropdown = event.target as Dropdown;
action(`Change: ${dropdown.value}`)();
}}"
label="Select a Country with a very long label, too long in fact"
>
<span slot="label">
Select a Country with a very long label, too long in fact
</span>
<sp-menu>
<sp-menu-item>
Deselect
Expand Down
32 changes: 15 additions & 17 deletions packages/dropdown/test/dropdown.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,14 +329,16 @@ describe('Dropdown', () => {
const el = await dropdownFixture();

await elementUpdated(el);
const menu = el.querySelector('sp-menu') as Menu;
const firstItem = el.querySelector('sp-menu-item') as MenuItem;

el.open = true;
await elementUpdated(el);
await waitUntil(
() => document.activeElement === firstItem,
() => document.activeElement === menu,
'first item focused'
);
expect(firstItem.focused).to.be.true;

el.blur();
await elementUpdated(el);
Expand All @@ -345,39 +347,40 @@ describe('Dropdown', () => {
el.focus();
await elementUpdated(el);
await waitUntil(
() => document.activeElement === firstItem,
() => document.activeElement === menu,
'first item refocused'
);
expect(el.open).to.be.true;
expect(document.activeElement === firstItem).to.be.true;
expect(document.activeElement === menu).to.be.true;
expect(firstItem.focused).to.be.true;
});
it('allows tabing to close', async () => {
const el = await dropdownFixture();

await elementUpdated(el);
const firstItem = el.querySelector('sp-menu-item') as MenuItem;
const menu = el.querySelector('sp-menu') as Menu;

el.open = true;
await elementUpdated(el);

expect(el.open).to.be.true;
el.focus();
await elementUpdated(el);
await waitUntil(() => document.activeElement === firstItem);
await waitUntil(() => document.activeElement === menu);
await waitUntil(
() => document.activeElement === firstItem,
() => document.activeElement === menu,
'first item refocused'
);
expect(el.open).to.be.true;
expect(document.activeElement === firstItem).to.be.true;
expect(document.activeElement === menu).to.be.true;

firstItem.dispatchEvent(tabEvent);
menu.dispatchEvent(tabEvent);
await elementUpdated(el);
await waitUntil(() => !el.open);

expect(el.open, 'closes').to.be.false;
expect(document.activeElement === firstItem, 'focuses something else')
.to.be.false;
expect(document.activeElement === menu, 'focuses something else').to.be
.false;
});
it('displays selected item text by default', async () => {
const focusSelectedSpy = spy();
Expand Down Expand Up @@ -439,15 +442,10 @@ describe('Dropdown', () => {
button.click();

await elementUpdated(menu);
await waitUntil(
() => document.activeElement === secondItem,
'second item focused'
);
await waitUntil(() => document.activeElement === menu, 'menu focused');

expect(focusFirstSpy.called, 'do not focus first element').to.be.false;
expect(focusSelectedSpy.called, 'focused selected element').to.be.true;
expect(focusSelectedSpy.calledOnce, 'focused selected element once').to
.be.true;
expect(secondItem.focused, 'secondItem "focused"').to.be.true;
});
it('resets value when item not available', async () => {
const el = await fixture<Dropdown>(
Expand Down
21 changes: 15 additions & 6 deletions packages/menu/src/Menu.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,13 +63,11 @@ export class Menu extends SpectrumElement {
}

public focus(): void {
if (this.menuItems.length === 0) {
if (!this.menuItems.length) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you need this change -- is this just a code style choice, or is there something subtle I'm missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just style, I had removed this check at one point and then re-added it not knowing the style changed. There may be some slight amount of type security outside of the TS space in the small fraction of time this.menuItems === undefined, but I don't know enough V8/timing to confirm.

return;
}

const focusInItem = this.menuItems[this.focusInItemIndex] as MenuItem;
this.focusedItemIndex = this.focusInItemIndex;
focusInItem.focus();
this.focusMenuItemByOffset(0);
super.focus();
}

private onClick(event: Event): void {
Expand Down Expand Up @@ -105,6 +103,10 @@ export class Menu extends SpectrumElement {
this.prepareToCleanUp();
return;
}
if (code === 'Space' || code === 'Enter') {
this.menuItems[this.focusedItemIndex].click();
return;
}
if (code !== 'ArrowDown' && code !== 'ArrowUp') {
return;
}
Expand All @@ -115,6 +117,7 @@ export class Menu extends SpectrumElement {

public focusMenuItemByOffset(offset: number): void {
const focusedItem = this.menuItems[this.focusedItemIndex] as MenuItem;
focusedItem.focused = false;
this.focusedItemIndex =
(this.menuItems.length + this.focusedItemIndex + offset) %
this.menuItems.length;
Expand All @@ -125,7 +128,8 @@ export class Menu extends SpectrumElement {
this.menuItems.length;
itemToFocus = this.menuItems[this.focusedItemIndex] as MenuItem;
}
itemToFocus.focus();
itemToFocus.focused = true;
this.setAttribute('aria-activedescendant', itemToFocus.id);
focusedItem.tabIndex = -1;
}

Expand Down Expand Up @@ -163,6 +167,11 @@ export class Menu extends SpectrumElement {
item = this.menuItems[index] as MenuItem;
}
index = Math.max(index, 0);
this.menuItems.forEach((item, i) => {
if (i !== index) {
item.focused = false;
}
});
this.focusedItemIndex = index;
this.focusInItemIndex = index;
}
Expand Down
4 changes: 2 additions & 2 deletions packages/menu/src/MenuGroup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,13 @@ export class MenuGroup extends SpectrumElement {
<span class="header" id=${labelledby} aria-hidden="true">
<slot name="header"></slot>
</span>
<div aria-labelledby=${labelledby} role="group">
<div aria-labelledby=${labelledby} role="none">
<slot></slot>
</div>
`;
}

protected firstUpdated(): void {
this.setAttribute('role', 'group');
this.setAttribute('role', 'none');
}
}
32 changes: 32 additions & 0 deletions packages/menu/src/MenuItem.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import {
property,
CSSResultArray,
TemplateResult,
PropertyValues,
ifDefined,
} from '@spectrum-web-components/base';

import '@spectrum-web-components/icon/sp-icon.js';
Expand All @@ -37,8 +39,13 @@ export class MenuItem extends ActionButton {
return [menuItemStyles, checkmarkMediumStyles];
}

static instanceCount = 0;

private _value = '';

@property({ type: Boolean, reflect: true })
public focused = false;

@property({ type: String })
public get value(): string {
return this._value || this.itemText;
Expand Down Expand Up @@ -83,6 +90,31 @@ export class MenuItem extends ActionButton {
return content;
}

protected renderButton(): TemplateResult {
return html`
<div id="button" class="button" aria-label=${ifDefined(this.label)}>
${this.buttonContent}
</div>
`;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hadn't realized this extended from ActionButton -- neat. Whenever something like a core render method is overridden I try to think about maintainability. Since the only difference is the tag name, is that something you could parameterize?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could make the source render method more composable so that we only have to override part of it:

protected render(): TemplateResult {
    return this.isAnchor ?  this.renderAnchor : this.renderButton;
}
protected get renderAnchor() {}
protected get renderButton() {}

That's probably nicer system wide, I'll take swing at it.


protected firstUpdated(changes: PropertyValues): void {
super.firstUpdated(changes);
if (!this.hasAttribute('id')) {
this.id = `sp-menu-item-${MenuItem.instanceCount++}`;
}
}

protected updated(changes: PropertyValues): void {
super.updated(changes);
if (this.getAttribute('role') === 'option' && changes.has('selected')) {
this.setAttribute(
'aria-selected',
this.selected ? 'true' : 'false'
);
}
}

public connectedCallback(): void {
super.connectedCallback();
if (!this.hasAttribute('role')) {
Expand Down
15 changes: 2 additions & 13 deletions packages/menu/src/menu-item.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,19 +12,8 @@ governing permissions and limitations under the License.

@import './spectrum-menu-item.css';

#button {
width: 100%;
}

button {
border: 0;
background: 0;
padding: 0;
margin: 0;
display: inherit;
font: inherit;
color: inherit;
text-align: inherit;
:host {
display: block;
}

#selected {
Expand Down
4 changes: 4 additions & 0 deletions packages/menu/src/menu.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ governing permissions and limitations under the License.
display: inline-block;
}

:host(:focus) {
outline: none;
}

:host sp-menu {
/* .spectrum-Menu .spectrum-Menu */
display: block;
Expand Down
5 changes: 5 additions & 0 deletions packages/menu/src/spectrum-config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ const config = {
selector: '.is-selected',
name: 'selected',
},
{
type: 'boolean',
selector: '.is-focused',
name: 'focused',
},
],
ids: [
{
Expand Down
6 changes: 3 additions & 3 deletions packages/menu/src/spectrum-menu-item.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ slot[name='icon'] + #label,
transform: matrix(-1, 0, 0, 1, 0, 0);
}
:host([dir='ltr']) #button:focus-visible,
:host([dir='ltr']) #button.is-focused {
:host([dir='ltr'][focused]) #button {
/* [dir=ltr] .spectrum-Menu-item.focus-ring,
* [dir=ltr] .spectrum-Menu-item.is-focused */
border-left-color: var(
Expand All @@ -264,7 +264,7 @@ slot[name='icon'] + #label,
);
}
:host([dir='rtl']) #button:focus-visible,
:host([dir='rtl']) #button.is-focused {
:host([dir='rtl'][focused]) #button {
/* [dir=rtl] .spectrum-Menu-item.focus-ring,
* [dir=rtl] .spectrum-Menu-item.is-focused */
border-right-color: var(
Expand All @@ -273,7 +273,7 @@ slot[name='icon'] + #label,
);
}
#button:focus-visible,
#button.is-focused {
:host([focused]) #button {
/* .spectrum-Menu-item.focus-ring,
* .spectrum-Menu-item.is-focused */
background-color: var(
Expand Down
Loading