build: leverage eslint--plugin-lit-a11y #976

Westbrook · 2020-10-21T12:45:29Z

Description

Use eslint-plugin-lit-a11y. Make changes to support its findings. I'll comment a bunch inline so we can discuss whether there are additional settings/considerations we'd like to take into account.

Motivation and Context

The level of accessibility that our elements deliver should be both explainable and maintainable.

How Has This Been Tested?

It's lints, so it blocks the CI when things are missing.

Types of changes

Bug fix (non-breaking change which fixes an issue)

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.

Westbrook · 2020-10-21T12:46:21Z

.eslintrc.json

+                    "sp-radio",
+                    "sp-switch",
+                    "sp-menu-item",
+                    "sp-clear-button",


We know that all of the above element have interactive descendent that would associate key events with any click events for which we are listening, so allow.

Westbrook · 2020-10-21T12:48:36Z

.eslintrc.json

+                    "sp-switch",
+                    "sp-menu-item",
+                    "sp-clear-button",
+                    "sp-underlay"


This element doesn't explicitly have an interactive descendent, but the underlay of modal content (event when it "dismisses" the modal content) isn't focusable, so it would never receive key events anyways.

Westbrook · 2020-10-21T12:50:13Z

documentation/src/components/layout.ts

        const next = target.nextElementSibling as Dropdown;
        if (!next || next.open) return;
        next.click();
    }

+    private onKeypressLabel(event: KeyboardEvent & { target: HTMLElement }) {


The label isn't technically focusable, so this code path will likely never be met, but, inline with the comment in line 108, this is manual code while #475 is yet to be addressed.

Westbrook · 2020-10-21T13:39:33Z

documentation/src/components/side-nav.ts

            <aside>
                <div id="nav-header">
                    <div id="logo-container">
-                        <a href="#">
+                        <a href="#index">


We've gone back and forth on whether this rule is required. # is technically a legit destination, but not explicit. Any insight on the accessibility of # vs #index or #top will be submitted back to the linter!

Westbrook · 2020-10-21T13:40:57Z

packages/.eslintrc.json

+                            "sp-menu-item",
+                            "sp-clear-button",
+                            "sp-underlay",
+                            "sp-popover"


I know in this file this element has interactive children, but not in others. We have the ability to skip this rule in ALL custom elements, but seems like an over step...

Westbrook · 2020-10-21T13:41:40Z

packages/asset/stories/asset.stories.ts

@@ -23,7 +23,7 @@ export default {
 export const Default = (): TemplateResult => {
    return html`
        <sp-asset style="height: 128px">
-            <img src=${portrait} alt="Demo Image" />
+            <img src=${portrait} alt="Demo Graphic" />


Everywhere I switch Image to Graphic is in a demo or test, but it seems like good practice to get into.

Westbrook · 2020-10-21T13:42:07Z

packages/button/stories/button.stories.ts

@@ -196,7 +196,6 @@ export const withIcon = (): TemplateResult => {
                        viewBox="0 0 36 36"
                        focusable="false"
                        aria-hidden="true"
-                        role="img"


role="img" isn't really needed with aria-hidden="true"...

Westbrook · 2020-10-21T13:45:46Z

packages/menu/src/MenuGroup.ts

@@ -46,7 +46,7 @@ export class MenuGroup extends SpectrumElement {
            <span class="header" id=${labelledby} aria-hidden="true">
                <slot name="header"></slot>
            </span>
-            <div aria-labelledby=${labelledby} role="none">
+            <div aria-labelledby=${labelledby} role="presentation">


IIUC, presentation allows the pass through without expecting this <div> to have actual role content.

Westbrook · 2020-10-21T13:46:51Z

packages/overlay/src/OverlayTrigger.ts

@@ -72,6 +72,7 @@ export class OverlayTrigger extends LitElement {
            <div
                id="trigger"
                @click=${this.onTrigger}
+                @keypress=${this.onKeypress}


We can't guarantee that a user uses an interactive element below here, so I've added keypress with some gating, just in case. Could be over kill...

This does feel like overkill to me. I'd rather just see the documentation cover this.

In general I think we should document and have examples of best practices, and not try to "fix" bad practices.

This makes sense to me. I'll work on the right way to skip this rule here and add a sentence or two to the documentation for this.

adixon-adobe

One change request and one question. Overall looks good!

adixon-adobe · 2020-10-22T16:40:28Z

documentation/src/components/side-nav.css

+    display: none;
+    pointer-events: none;
+    opacity: 0;
+}


How do these css changes relate to the linting change?

Indeed. I should be able to take even more of this away as we're moving from a <div> element here that would require special rule skipping or keyboard event handling in exchange for an <sp-underlay> element that already has agreement with the lint rules. Will fix!

adixon-adobe · 2020-10-22T16:47:42Z

packages/overlay/src/OverlayTrigger.ts

@@ -72,6 +72,7 @@ export class OverlayTrigger extends LitElement {
            <div
                id="trigger"
                @click=${this.onTrigger}
+                @keypress=${this.onKeypress}


This does feel like overkill to me. I'd rather just see the documentation cover this.

In general I think we should document and have examples of best practices, and not try to "fix" bad practices.

adixon-adobe

Updates look great. Nice!

adixon-adobe · 2020-10-23T14:06:39Z

packages/overlay/README.md

+
+## Accessibility
+
+When using an `<overlay-trigger>` element, it is important to be sure the that content you project into `slot="trigger"` is "interactive". This means that an element within that branch of DOM will be able to receive focus and said element will appropriately convert keyboard interactions to `click` events similar to what you find with `<a href="#">Anchors</a>`, `<button>Buttons</button>`, etc. You can find further reading on the subject of accessible keyboard interactions at [https://www.w3.org/WAI/WCAG21/Understanding/keyboard](https://www.w3.org/WAI/WCAG21/Understanding/keyboard).


Westbrook requested review from jnurthen, yalagand and adixon-adobe October 21, 2020 12:45

Westbrook force-pushed the westbrook/lint branch 4 times, most recently from 103d840 to 209b03d Compare October 21, 2020 13:48

Westbrook commented Oct 21, 2020

View reviewed changes

Westbrook changed the base branch from main to next October 22, 2020 11:37

Westbrook force-pushed the westbrook/lint branch from 209b03d to b728e7f Compare October 22, 2020 14:10

adixon-adobe requested changes Oct 22, 2020

View reviewed changes

Westbrook force-pushed the westbrook/lint branch from b728e7f to 39283c3 Compare October 22, 2020 17:26

adixon-adobe approved these changes Oct 23, 2020

View reviewed changes

refactor: leverage eslint--plugin-lit-a11y

257a0fc

Westbrook force-pushed the westbrook/lint branch from 39283c3 to 257a0fc Compare October 23, 2020 15:30

Westbrook merged commit 80a18e4 into next Oct 23, 2020

Westbrook deleted the westbrook/lint branch October 23, 2020 15:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: leverage eslint--plugin-lit-a11y #976

build: leverage eslint--plugin-lit-a11y #976

Westbrook commented Oct 21, 2020

Westbrook Oct 21, 2020

Westbrook Oct 21, 2020

Westbrook Oct 21, 2020

Westbrook Oct 21, 2020

Westbrook Oct 21, 2020

Westbrook Oct 21, 2020

Westbrook Oct 21, 2020

Westbrook Oct 21, 2020

Westbrook Oct 21, 2020

adixon-adobe Oct 22, 2020

Westbrook Oct 22, 2020

adixon-adobe left a comment

adixon-adobe Oct 22, 2020

Westbrook Oct 22, 2020

adixon-adobe Oct 22, 2020

adixon-adobe left a comment

adixon-adobe Oct 23, 2020


		## Accessibility

		When using an `<overlay-trigger>` element, it is important to be sure the that content you project into `slot="trigger"` is "interactive". This means that an element within that branch of DOM will be able to receive focus and said element will appropriately convert keyboard interactions to `click` events similar to what you find with `<a href="#">Anchors</a>`, `<button>Buttons</button>`, etc. You can find further reading on the subject of accessible keyboard interactions at [https://www.w3.org/WAI/WCAG21/Understanding/keyboard](https://www.w3.org/WAI/WCAG21/Understanding/keyboard).

build: leverage eslint--plugin-lit-a11y #976

build: leverage eslint--plugin-lit-a11y #976

Conversation

Westbrook commented Oct 21, 2020

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adixon-adobe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adixon-adobe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment