Skip to content
This repository has been archived by the owner on Jan 8, 2024. It is now read-only.

UI: Fix most a11y violations, modernize a11y testing setup #1499

Merged
merged 7 commits into from
May 19, 2021

Conversation

jgwhite
Copy link
Contributor

@jgwhite jgwhite commented May 18, 2021

We were facing a number of accessibility violations with no easy way to triage them. This patch updates us to the latest ember-a11y-testing and adopts the new setupGlobalA11yHooks approach wherein the audit is run automatically whenever we call visit, click etc.

The new approach also allows us a single point to pass exclude options to axe-core, giving us an easy way to mark certain DOM elements as pending (meaning: “we know these have issues, but we can’t fix them right now”). I’ve only added three violations to the exclude list. The rest of the violations were straightforward to fix so I’ve addressed them all directly.

The only noticeable changes for users are as follows:

Before After
a b
a b

How should I check this?

Check out the branch locally and run the ember test suite. You should see everything is green. The complete steps:

git checkout ui-a11y-updates
cd ui
yarn ember serve

Then visit http://localhost:4200/tests

Where have the dev-mode stripes gone?

Previously in development mode we’d see prominent diagonal stripes on any elements that violated axe-core’s a11y rules. It looked like this:

stripes

ember-a11y-testing dropped this feature in 4.0.0. The rationale can be found in ember-a11y/ember-a11y-testing#205 and the README, but in short the idea is to nudge users towards more-mainstream tools and focus on the ember testing workflow.

I still see violations when I run ember test — what gives?

There are still a11y failures reported when running the tests headless (ember test). These will be addressed in a subsequent PR.

@jgwhite jgwhite added pr/no-changelog No automatic changelog entry required for this pull request ui labels May 18, 2021
@jgwhite jgwhite marked this pull request as ready for review May 18, 2021 21:11
@jgwhite jgwhite requested review from gregone and sabrinako May 18, 2021 21:11
@briancain briancain requested a review from a team May 18, 2021 21:30
@jgwhite jgwhite requested review from dizzyup and removed request for gregone, sabrinako and dizzyup May 19, 2021 05:51
Copy link
Contributor

@gregone gregone left a comment

Choose a reason for hiding this comment

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

Great job. I had not thought about using the exclusion list, and I think it make perfect sense!

// Selectors of elements to exclude from a11y auditing. See the following docs
// for more:
// https://github.com/dequelabs/axe-core/blob/develop/doc/API.md#include-exclude-object
const exclude = [['.pds-logomark'], ['.pds-tabNav'], ['.card-header']];
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

This is great, I really like the idea of having an exclusion list for known issues (but that we can still keep track of)

--success-border: #{dehex(color.$green-100)};
--error: #{dehex(color.$red-050)};
--error-text: #{dehex(color.$red-600)};
--error-border: #{dehex(color.$red-100)};
--warning: #{dehex(color.$yellow-050)};
--warning-text: #{dehex(color.$yellow-600)};
--warning-text: #{dehex(color.$yellow-800)};
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be curious to see how this impacts the UI, and possibly share that with design so they're aware of good accessible defaults for the warning and success styles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I’d like design’s sign-off on this too. I think the before/after screenshots in the PR description are the extent of the changes but there may be more.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, these look good. Orange turning to brown is always the tough one! Marketing's new stateful color palette is something we'll look to leverage in product soon 🙂

@jgwhite jgwhite merged commit a7be62b into main May 19, 2021
@jgwhite jgwhite deleted the ui-a11y-updates branch May 19, 2021 10:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr/no-changelog No automatic changelog entry required for this pull request ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants