UI: Add acceptance test accessibility auditing and fixes #8455
Conversation
Ember Asset Size actionAs of 4095504 Files that got Bigger 🚨:
Files that stayed the same size 🤷:
|
Ember Test Audit comparison
|
This lets us centralise rule exclusions instead of having them sprinkled around in test files.
I’m torn about whether this approach of making separate tests for the audit is preferable to embedding the audit in the existing tests, perhaps similarly to how assertions on the page title are embedded. 🤔
backspace
force-pushed
the
f-ui/accessibility
branch
from
f614b6c to
d06919b
Compare
Some of these (color-contrast and heading-order) will be harder to fix than others, but I will hopefully be able to address the majority in subsequent commits.
This rule is described here: https://dequeuniversity.com/rules/axe/3.5/svg-img-alt
This rule is described here: https://dequeuniversity.com/rules/axe/3.5/link-name
This rule is described here: https://dequeuniversity.com/rules/axe/3.5/landmark-unique
This rule is described here: https://dequeuniversity.com/rules/axe/3.5/label While adding a label for the token input field, I noticed that its placeholder produces a bad VoiceOver experience, which I will address separately. I removed the action to focus the stepper input when clicking the label as I believe connecting the label and input with an id is equivalent. This includes an override for ivy-codemirror’s codemirror dependency to take advantage of the screenReaderLabel configuration option in 5.53; thanks to @adityatoshniwal for that addition in codemirror/codemirror5#6197.
This rule is described here: https://dequeuniversity.com/rules/axe/3.5/button-name For the log streaming toggles, normally I would assert against conditionals like these, but I couldn’t find any assertions on the icons 😯
This turns off one rule for the job definition test, as the CodeMirror component is failing: https://dequeuniversity.com/rules/axe/3.5/scrollable-region-focusable
backspace
changed the title
backspace
changed the title
There was a problem hiding this comment.
So many great little fixes 🤩
As for integrating tests into existing files vs. one big new file, I too prefer integrating into existing ones.
As for the eslint rule, I think if it existed it would be helpful, but without knowing how much work goes into making it, it's hard to say if it would be worth it.
| class="stepper-input-label" | ||
| onClick={{action "focusInput"}}>{{yield}}</label> | ||
| for="stepper-input-{{this.elementId}}" | ||
| class="stepper-input-label">{{yield}}</label> |
There was a problem hiding this comment.
This is a much nicer implementation than the action route I took.
|
👍 thanks, I’ll change it around to have the audits in the existing tests. re ESLint, it seemed to me to not be too difficult, I think there are some rules I can take inspiration from, I’ll make a timeboxed attempt 😀 |
|
oh oops maybe we’re talking about slightly different things re my question about where the audits should happen. I followed the suggestion of the documentation in adding a separate test into each acceptance test file (almost), but I was asking about whether it would make more sense to add the |
|
Ah, I see what you mean now. I think it's good the way it is? If I understand correctly, when a page doesn't pass the audit, then If that's the case, then I think it's helpful to know that |
|
The
I pushed a commit with that and a deliberate accessibility violation and you can see that the test fails without needing the Unrelatedly, re ESLint, you saw that I got that PR merged, but I’ll make a separate PR to use it in Nomad 😀 |
|
My concern is around the presentation of errors given that the underlying failure condition is a promise error rather than an assertion error. Note the title differences in these two error cards.
But fewer lines of code is always compelling, and the error message body still says it's accessibility related, so maybe I'm just overthinking things. |
|
hmm yes, the test name does help emphasise… well I’ll merge for now, can always change later! |
This continues #8455 by adding accessibility audits to component integration tests and fixing associated errors. It adds audits to existing tests rather than adding separate ones to facilitate auditing the various permutations a component’s rendering can go through. It also adds linting to ensure audits happen in component tests. This necessitated consolidating test files that were scattered.
|
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. |
This is a first pass at incorporating axe/ember-a11y-testing and addressing some failures it catches. The commits can be read in order, they:
The wrapper for the
a11yAudithelper let me globally skip certain axe rules for now:color-contrast, which will be a significant undertaking to address but also self-contained and appropriate for a separate PRheading-order, which ensures thath1toh6are always in order and don’t skip levels, I need to look more into how people handle this in dynamic applicationsI chose to only add accessibility audits to acceptance tests for now, but will proceed with component tests in a subsequent pull request. Some other things to address/survey (in addition to the above):
scrollable-region-focusable, there’s a targeted exception for that rulescrollable-region-focusablefailure when I ensured the job definition was long enough that it would cause the container to be scrollableThe documentation shows adding a separate test for the audit, which is also the route Terraform Cloud is taking, but I’m torn about it… I wonder whether it might make more sense to include in an existing test, similarly to how there are assertions on
document.titlein most acceptance test files’s first tests. I’m leaning toward that direction but want to check in before doing it.One possibility I looked into was adding a custom ESLint rule that would check each acceptance test file to ensure it uses the helper at least once. Does this seem worth it? 🤔