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

feat: upgrade accessibility of tab/tab-list family of elements #296

Merged
merged 2 commits into from
Dec 17, 2019
Merged

feat: upgrade accessibility of tab/tab-list family of elements #296

merged 2 commits into from
Dec 17, 2019

Conversation

Westbrook
Copy link
Contributor

@Westbrook Westbrook commented Nov 18, 2019 •
edited
Loading

Description

Manage role and tabindex values in <sp-tab> and <sp-tab-list> elements. Take multiple <sp-tab> elements out of the tab order and make them accessible via arrow keys. Support similar coverage for the [direction="vertical"] variant.

Follow up

Looking deeper into this pattern, I wonder if we want to support a <sp-tab-panels> style component in the future that takes <sp-tab-panel> elements and uses the attributes/properties thereof to build an <sp-tab-list> element dynamically. 🤔

Related Issue

refs #292

Motivation and Context

Things should be accessible.

How Has This Been Tested?

  • Unit tests

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

jnurthen
jnurthen requested changes Nov 18, 2019
View reviewed changes
Copy link
Member

@jnurthen jnurthen left a comment

Choose a reason for hiding this comment

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

Looks good except for the orientation issues.
Found an unrelated issue where focus gets lost when selecting tabs on iOS w/ Voiceover but not related to this PR so will file a new issue.

packages/tab-list/src/tab-list.ts Outdated

public handleKeydown(event: KeyboardEvent): void {
const { code } = event;
if (code !== 'ArrowLeft' && code !== 'ArrowRight') {
Copy link
Member

Choose a reason for hiding this comment

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

Vertical Tabs need to handle ArrowUp and ArrowDown

packages/tab-list/src/tab-list.ts
@@ -72,6 +75,34 @@ export class TabList extends LitElement {
`;
}

protected firstUpdated(): void {
this.setAttribute('role', 'tablist');
Copy link
Member

Choose a reason for hiding this comment

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

Need to set aria-orientation="vertical" for a vertical tab list (no need to set for horizontal)

@Westbrook
Copy link
Contributor Author

Now with [direction="vertical"] support.

@Westbrook Westbrook force-pushed the westbrook/292/tab branch 2 times, most recently from c3bd018 to 6503fb2 Compare November 25, 2019 22:46
@Westbrook Westbrook mentioned this pull request Nov 27, 2019
Closed
@Westbrook Westbrook force-pushed the westbrook/292/tab branch 3 times, most recently from 817cf44 to 5710b5b Compare December 12, 2019 20:42
@andrewhatran
Copy link
Collaborator

andrewhatran commented Dec 12, 2019 •
edited
Loading

Looks great functionally, just a minor styling thing.

In spectrum-tab.css, this following css manages the focus style of the selectionIndicator border, which is intended to be blue when the tab is focused.

:host(:focus-visible:before) {
    border-color: var(
        --spectrum-tabs-focus-ring-color,
        var(--spectrum-alias-border-color-focus)
    );
}

Our implementation of the selection indicator deviates from spectrum-css, where it's represented by the selectionIndicator element in the shadow down of sp-tab-list instead of a pseudo element

Maybe not in scope for this PR, would be cool to get the right focus styles for the indicator sometime down the line

@Westbrook Westbrook force-pushed the westbrook/292/tab branch 3 times, most recently from 5545c7b to 4a66ed0 Compare December 14, 2019 22:47
Westbrook
Westbrook commented Dec 17, 2019
View reviewed changes
Copy link
Contributor Author

@Westbrook Westbrook left a comment

Choose a reason for hiding this comment

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

@andrewhatran This is actually a great catch here! Thanks for being vigilant. Take a look at my other notes to see what went down, but I think things are in a much better place for this PR now!

scripts/process-spectrum-postcss-plugin.js
node.value === '::before' ||
node.value === '::after'
node.value.match(/before$/) !== null ||
node.value.match(/after$/) !== null
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spectrum CSS changed from ::before to :before and broke the processing here. End with before should make us resilient to similar variances in the future and this is a build task, so it shouldn't effect performance to use regex.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahh that looks annoying, thanks for making this more robust 👍

packages/shared/src/focusable.ts
@@ -82,7 +82,8 @@ export class Focusable extends FocusVisiblePolyfillMixin(LitElement) {
}
}

protected firstUpdated(): void {
protected firstUpdated(changes: PropertyValues): 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.

Wasn't extendable without this.

packages/tab-list/src/tab-list.ts
this.querySelector('sp-tab')) as Tab;
}

protected manageAutoFocus(): 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.

Wait till all of the child items are ready before trying to focus on one of them.

andrewhatran
andrewhatran approved these changes Dec 17, 2019
View reviewed changes
Copy link
Collaborator

@andrewhatran andrewhatran left a comment

Choose a reason for hiding this comment

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

The focus styles look great now, thanks for making that change!

@Westbrook Westbrook merged commit 83dddb0 into master Dec 17, 2019
@Westbrook Westbrook deleted the westbrook/292/tab branch December 17, 2019 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jnurthen jnurthen jnurthen requested changes

@andrewhatran andrewhatran andrewhatran approved these changes

@msdewey msdewey Awaiting requested review from msdewey

@clef-impromptu clef-impromptu Awaiting requested review from clef-impromptu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Westbrook @andrewhatran @jnurthen