[ACCESSIBILITY] Global changes to pass AXE/Lighthouse Audits #66

austingreendev · 2018-07-24T22:51:30Z

TL;DR: We have improved from an aggregate lighthouse accessibility score of 45% to 98.5% across ALL packages (with the remaining failures being lighthouse miss-fires) 🎉

BREAKING CHANGES?

⚠️ There are several breaking changes in this PR. This is just a collection of all changes that I completed. This should (and will) be broken up into several smaller PRs that can address the changes in more depth. ⚠️

Tooling Used

Google Lighthouse Audits
- The google Lighthouse accessibility audit is a good first-pass for accessibility health
AXE Auditing Tool
- Chrome extension for more involved accessibility testing
Mac Voiceover
- Nothing beats actually testing with a screen reader

All three tools were used for the tests/assumptions performed during this process.

Here is a link to the lighthouse exports after the changes shown in this PR: https://drive.google.com/open?id=11bOPsxBof8gMa8Md0ZXhKzvMg0PHyVYq

These files can be viewed with https://googlechrome.github.io/lighthouse/viewer2x/

Description

Accessibility is important. All components within this repository are built to the W3C Design Patterns spec; so this means that everything is 100% accessible, right?

WRONG 😢 (this is react-buttons)

The majority of component implementations are accessible, but our examples are currently lacking some good best practices. Some of this is a combination of color-contrast issues and customizations on top of react-styleguidist, but the worst offenders currently are the handwritten examples and some invalid assumptions about menu accessibility.

Currently all examples are hovering around the 40-60% lighthouse score. This PR is a collection of several breaking API changes, styleguidist improvements, and example updates. These will be split into several individual PRs, but I felt a background PR would help.

Detail

I've split this PR into 3 commits:

Breaking (and non-breaking) API Changes
- Turns out sticking to the W3C spec doesn't provide everything. I'll go into depth in the individual PRs
- Examples:
  - Can't apply two IDs to aria-describedby
  - aria-controls and aria-owns can only to reference elements that are rendered already (duh)
  - .etc
Example updates
- Assorted aria-label and documentation changes
Styleguidist upgrades
- Tried to use Garden colors throughout
- Migrated propTypes table to use the Garden table component

Before:

After:

Learnings

Even thought we have static accessibility linting withjsx-a11y a large chunk of our codebase lives within the Markdown examples and isn't reachable with this tool. We should perform more frequent accessibility audits or find some additional ways of auditing this in an automated fashion.

Checklist

👌 design updates are Garden Designer approved (add the
designer as a reviewer)
💅 view component styling is based on a Garden CSS
component
🌐 Styleguidist demo is up-to-date (yarn start)
⬅️ renders as expected with reversed (RTL) direction
💂‍♂️ includes new unit and snapshot tests
📒 any new files are included in the packages src/index.js export
📝 tested in Chrome, Firefox, Safari, Edge, and IE11

allisonacs · 2018-07-25T01:45:11Z

Happy to see this happen @Austin94! Less "do as I say," more "do as I do."

ryanseddon

Nice, so these all came out of running the styleguide through the chrome dev tools audit panel?

ryanseddon · 2018-07-25T03:08:05Z

packages/selection/src/containers/FieldContainer.spec.js

+      expect(input).toHaveProp('aria-describedby', `${CONTAINER_ID}--hint`);
+    });
+
+    it('excludes aria-describedby if option is provided', () => {


'excludes aria-describedby if option is not provided'

Fixed. Was going to save this for the separate PR, but depending on what we view as the most common scenario, we might have this default to false.

Updated for now.

ryanseddon · 2018-07-25T03:08:30Z

packages/selection/src/containers/FieldContainer.spec.js

+        </FieldContainer>
      );
+
+      expect(findInput(wrapper)).toHaveProp('aria-describedby', null);


Would this work too?

expect(findInput(wrapper)).not.toHaveProp('aria-describedby')

Unfortunately no. The syntax above would expect the prop to have undefined as a value, but is null as provided.

If we were to provide undefined through the render-prop it would actually show as undefined in the DOM. With null React is removing it from the attribute list.

ryanseddon · 2018-07-25T03:17:39Z

packages/autocomplete/src/containers/AutocompleteContainer.js

      role,
      'aria-autocomplete': 'list',
      'aria-haspopup': 'true',
-      'aria-owns': this.getMenuId(),


Does this apply when the menu is open? I can't see that anywhere.

No it does not. Since we are moving focus to the menu it seems to not be required? I ran AXE across all of the states in the menu and the only thing it picked up was us appending it to the body in a "non-landmark region" which would be something a consumer would customize.

austingreendev · 2018-07-25T15:36:23Z

Nice, so these all came out of running the styleguide through the chrome dev tools audit panel

Unfortunately no. The overall scores did, but the majority of corrections were guided by the AXE plugin. It seems to be a lot more picky about things than the lighthouse audit and helped find some of the larger holes.

jzempel · 2018-07-25T16:45:24Z

packages/radios/src/views/Label.example.md

-    </Col>
-  </Row>
-</Grid>
+<div role="group" aria-label="State Usage Example">


Can these attributes apply to <Grid>? In other words, make role="group" automatic with the component and apply aria-label manually.

These are no added to the <Grid>

jzempel · 2018-07-25T16:48:32Z

packages/tables/src/examples/overflow-menu.md

      },
      offset: {
-        fn: (data) => {
+        fn: data => {


hopefully grabbed by rebase on #65

Fixed with a rebase

jzempel · 2018-07-25T16:56:16Z

packages/textfields/src/views/__snapshots__/Input.spec.js.snap


 exports[`Input renders RTL styling 1`] = `
 <input
+  aria-invalid={false}


Isn't {false} in these (and following) snaps unexpected?

We are defaulting this to false to match the "default" state (https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_aria-invalid_attribute).

Without any validation this would still be false. AXE didn't seem to find any issues with this.

But shouldn't it be aria-invalid="false" rather than {false}? I haven't seen JSX syntax in snapshots before.

nvmd. Bogus expectations on my part here. 😳

jzempel · 2018-07-25T16:58:45Z

utils/styleguide/ArgumentsRenderer/index.js

+import { zdFontSizeEpsilon, zdFontWeightSemibold } from '@zendeskgarden/css-variables';
+
+const ArgumentsContainer = styled.div`
+  margin-bottom: 8px;


Use xs spacing from css-variables.

jzempel · 2018-07-25T16:59:04Z

utils/styleguide/ArgumentsRenderer/index.js

+`;
+
+const Heading = styled.div`
+  margin-bottom: 4px;


Use xxs spacing from css-variables.

jzempel · 2018-07-25T17:01:49Z

utils/styleguide/github-hljs-theme.css

@@ -0,0 +1,90 @@
+.hljs {


If you can't access variables, it would be good to comment this file with selected color names from https://garden.zendesk.com/css-components/utilities/color/.

I've added the equivalent color values as comments. I also updated all of the colors to match an equivalent in the palette so it should be a little clearer now.

- @zendeskgarden/react-ranges@2.2.11

ryanseddon · 2018-07-27T00:53:59Z

AXE plugin

Oh weird I thought from this post that lighthouse actually internally uses AXE in the audit?

ryanseddon

So next steps is to introduce smaller PRs for these changes?

austingreendev · 2018-07-27T01:11:56Z

Oh weird I thought from this post that lighthouse actually internally uses AXE in the audit?

I think it does, it at least linked to the dequeue links that AXE provides, but it seems to be a sub-set of the audits that the standalone plugin uses.

So next steps is to introduce smaller PRs for these changes?

Yes. Probably 3-4

austingreendev · 2018-10-02T20:41:08Z

Closing this now that all areas have been merged separately.

austingreendev requested review from alanhogan, jzempel and ryanseddon July 24, 2018 22:51

ryanseddon suggested changes Jul 25, 2018

View reviewed changes

jzempel requested changes Jul 25, 2018

View reviewed changes

Austin Green added 6 commits July 26, 2018 12:58

chore(release): publish

74ef2ab

- @zendeskgarden/react-ranges@2.2.11

feat(accessibility): assorted accessibility upgrades

3da9842

fix(examples): update examples to pass axe audit

f5197df

fix(docs): fix all accessibility audit failures in react-styleguidist

bfb2fb4

Addressed Ryan feedback

dcb155d

Addressed JZ feedback

be32908

austingreendev force-pushed the agreen/accessibility branch from eebae58 to be32908 Compare July 26, 2018 20:44

Fix typo

ba8394e

jzempel approved these changes Jul 26, 2018

View reviewed changes

ryanseddon approved these changes Jul 27, 2018

View reviewed changes

This was referenced Aug 7, 2018

fix(docs): update styleguidist internal components to pass WCAG 2.0 compliance #96

Merged

feat(buttons|selection): allow customization of selected accessibility value #109

Merged

This was referenced Aug 20, 2018

fix(tooltips): fix rendering stutter in Safari #119

Merged

fix(pagination): remove invalid aria role #127

Merged

Merge branch 'master' into agreen/accessibility

45026d8

austingreendev mentioned this pull request Sep 18, 2018

feat(accessibility): add isDescribedBy flag to FormContainer and update global accessibility values #162

Merged

7 tasks

austingreendev closed this Oct 2, 2018

austingreendev deleted the agreen/accessibility branch October 2, 2018 20:41

[ACCESSIBILITY] Global changes to pass AXE/Lighthouse Audits #66

[ACCESSIBILITY] Global changes to pass AXE/Lighthouse Audits #66

Uh oh!

Conversation

austingreendev commented Jul 24, 2018 • edited by allisonacs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tooling Used

Description

Detail

Learnings

Checklist

Uh oh!

allisonacs commented Jul 25, 2018

Uh oh!

ryanseddon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

austingreendev commented Jul 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanseddon commented Jul 27, 2018

Uh oh!

ryanseddon left a comment

Choose a reason for hiding this comment

Uh oh!

austingreendev commented Jul 27, 2018

Uh oh!

austingreendev commented Oct 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

austingreendev commented Jul 24, 2018 •

edited by allisonacs

Loading