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.

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

UI: Add component accessibility auditing and fixes #8679

Merged
merged 8 commits into from
Aug 25, 2020

Conversation

backspace
Copy link
Contributor

@backspace backspace commented Aug 17, 2020

This continues #8455 by adding accessibility audits to component integration tests and fixing associated errors. I chose to add audits to existing tests rather than make separate ones to facilitate auditing the various permutations a component’s rendering can go through.

The a11yAudit interface for auditing a component is slightly different as it requires the component’s element to be passed in, so I made a different helper that uses a shared rule ignore list for acceptance tests. The linting rule for component tests expects the helper to be called at least once per file. I committed a bulk move of component test files into tests/integration/components so the linting rule would be properly scoped, which seemed justified since ember g component-test … generates files in that directory.

The only significant change required was to MultiSelectDropdown, which is configured with a listbox role that needed option role children and a non-listbox for the empty case. I’m not surprised at how few changes were needed because acceptance test audits already uncovered a lot of automated accessibility audit failures.

I decided to add these to existing tests vs create new ones
because audits will need to happen for the various states that
components can be in and it seems wasteful to need to reproduce
all the iterations separately vs use what already exists. I also
decided to fold the assertion into the audit wrapper so it doesn’t
need to be duplicated everywhere.
@github-actions
Copy link

github-actions bot commented Aug 17, 2020

Ember Asset Size action

As of 227abdd

Files that got Bigger 🚨:

File raw gzip
nomad-ui.js +1.66 kB -34 B

Files that stayed the same size 🤷‍:

File raw gzip
vendor.js 0 B 0 B
nomad-ui.css 0 B 0 B
vendor.css 0 B 0 B

There are some failures to address!
@github-actions
Copy link

github-actions bot commented Aug 17, 2020

Ember Test Audit comparison

master 227abdd change
passes 1379 1381 +2
failures 0 0 0
flaky 0 0 0
duration 6m 39s 505ms 6m 49s 450ms +09s 945ms

This satisfies a button-name axe violation.
This doesn’t catch most of the component tests because of the
directory structure… sadly it seems like many files should be
moved. I could perhaps instead use an elaborate filter to
ensure integration/util is excluded but everythig else is
included but considering that `ember g component-test …`
generates the file in integration/components, it seems best
to move everything there.
This lets the linting rule find them properly.
This addresses several violations:

* aria-required-children/listitem
  * the children LIs have role `option`
  * the empty case isn’t a listbox and has an LI child
* aria-input-field-name: the listbox has a connected label
@backspace backspace marked this pull request as ready for review August 24, 2020 15:25
Copy link
Contributor

@DingoEatingFuzz DingoEatingFuzz 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! Just the one note about tucking away the compulsory assertion.

I am also a +1 to the bulk move of test files, as that is the convention. I noticed the other day that we had test files in both locations and I just pretended I didn't see anything hahaaaaa.

ui/tests/helpers/a11y-audit.js Show resolved Hide resolved
@backspace
Copy link
Contributor Author

I had also previously noticed and ignored the chaos of the integration directory hierarchy haha I’m glad this prompted actually dealing with it 🙃

@backspace backspace merged commit 5eddb14 into master Aug 25, 2020
@backspace backspace deleted the f-ui/component-accessibility branch August 25, 2020 15:56
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants