Problem/Motivation
In an issue in a different queue @dmundra figured out how to integrate Axe testing into our Nightwatch tests: #2857808: Automate Accessibility Checks for Core. In the time since that discovery, Nightwatch has added Axe support, so this can be done without any new dependencies.
Steps to reproduce
Proposed resolution
Create a profile for a11y testingCreate default and admin theme tests that visit a few common pages which are then scanned with AxeIdeally, some of these tests will include a few failing tests, so we can verify Axe is doing its job and establish the value of these tests.Update readme with info on running a11y tests
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Core Nightwatch tests now include Axe accessibility scans that check common pages and forms for accessibility bugs.
| Comment | File | Size | Author |
|---|
Issue fork drupal-3293469
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #3
dmundra@bnjmnm thanks for creating the issue and branch. I pushed up commits to install nightwatch-axe-core and two accessibility tests. To run the tests do I create a merge request?
Comment #5
bnjmnmComment #6
nod_Comment #9
finnsky commentedAdded possible usage in new MR:
* To run an ally test for olivero and claro.
`yarn test:nightwatch --tag a11y`
* To run an ally test only for claro.
`yarn test:nightwatch --tag adminA11y`
* To run an ally test for custom theme.
`yarn test:nightwatch --tag themeA11y --defaultTheme bartik`
* To run an ally test for custom admin theme.
`yarn test:nightwatch --tag adminA11y --adminTheme classy`
Works with composer installed themes like https://www.drupal.org/project/adminimal_theme
`yarn test:nightwatch --tag adminA11y --adminTheme adminimal_theme`
- Issues wih [['#toolbar-administration']] so it is excluded from axe.conf.
- Should be some command to fill CKEDITOR fields
- For testing default theme should be created new testing profile with some content.
Comment #10
andypostTo fix CI
Comment #11
dmundra@finnsky those are great additions. Thank you! Should we add it to the other MR?
Comment #12
finnsky commentedYes, this is just example of possible approach for contrib and other themes. I see few issues about it at the moment. Not sure which one is correct
Comment #13
dmundra@finnsky I ran eslint fixes to your code and then merged it into our MR. I might make some more changes but I think it is good to have your code in there.
Comment #14
dmundraIn the last updates we have a few accessibility errors, seen here https://dispatcher.drupalci.org/job/drupal_patches/139557/console. Brief breakdown:
20:39:00 TEST FAILURE: 3 assertions failed, 1976 passed (6m 9s)
20:39:00
20:39:00 ✖ Tests/adminA11yTest
20:39:00 – Accessibility Structure Block Page (3.298s)
20:39:00
20:39:00 ✖ Tests/themeA11yTest
20:39:00 – Accessibility Test Search (737ms)
20:39:00 – Accessibility Test Login (762ms)
Comment #16
finnsky commentedRe: #13
Closed MR
Comment #17
finnsky commentedAs temporary solution we can extend
axe.conf.js with `exclude` option and report in claro/olivero issues.
https://www.deque.com/axe/core-documentation/api-documentation/#include-...
same as excluded '#toolbar-administration' now.
and remove it when issues will fixed
Comment #18
andypostI think kinda test should help to catch duplicated IDs on page (we have some tests like it in core - use
git grep pageContainsNoDuplicateIdComment #19
andypostThere's existing issue for search page #1852090: Cached render elements can have duplicate HTML IDs
Comment #20
dmundra@finnsky if that is acceptable. That was my question when starting this.
Nice find @andypost that would probably help use TDD to fix.
Comment #21
finnsky commentedSo imo next steps should be:
1. Create all pages tests listed in https://www.drupal.org/project/drupalci/issues/2857808
2. Gather all core a11y issues.
3. Add them to `exclude` part of axe configuration.
4. Create/update tickets for fixes. With links to `excluded` lines.
Comment #22
dmundraThat makes sense @finnsky. A semantic question I had, should we add separate tests for those or try to incorporate an axe() check to existing tests for those pages? I would prefer the latter to encourage a11y tests being added to potentially any tests specially interactive ones. Maybe a11y only tests should go in a folder called 'a11y_only' or something like that?
Comment #23
bnjmnmWhoa! This is moving along nicely. This is in the shape of something I'd likely RTBC once some details are addressed. Here are some more general things we'd need done for me to do that:
axe-coreandnightwatch-axe-coreboth require a dependency evaluation. See #3201547: [Policy] Dependency evaluation critera for some thoughts on how to approach evaluating dev only dependencies (it can be a little more relaxed).Comment #24
dmundraThanks @bnjmnm. Here are my comments:
Comment #25
dmundraDependency evaluation for
axe-coreandnightwatch-axe-core:axe-core
nightwatch-axe-core
Comment #26
dmundraWith the default nightwatch profile (https://dispatcher.drupalci.org/job/drupal_patches/141154/console) we only see 1 a11y error:
09:27:22 - Running Accessibility Content Page:
09:27:22
09:27:22 -----1 aXe violations-----
09:27:23 #1: Contentinfo landmark should not be contained in another landmark (landmark-contentinfo-is-top-level)
09:27:23 Impact: moderate
09:27:23 Count: 1
09:27:23 See: https://dequeuniversity.com/rules/axe/4.4/landmark-contentinfo-is-top-level
09:27:23 #1.1: Fix any of the following:
09:27:23 The contentinfo landmark is contained in another landmark.
09:27:23
09:29:47 TEST FAILURE: 1 assertions failed, 1780 passed (5m 23s)
09:29:47
09:29:47 ✖ Tests/adminA11yTest
09:29:47 – Accessibility Content Page (1.094s)
Comment #27
finnsky commentedAdded nightwatch_a11y_testing minimal profile for current tests.
Comment #28
dmundraThanks @finnsky. With the 'nightwatch_a11y_testing' minimal profile here is the current accessibility errors:
On '/admin/structure/block' page:
On the home page:
On the search page:
On the login page:
My suggestion is to do the following:
Comment #30
bnjmnmGreen MR!
Because dependencies can make it difficult for an issue to land in core, and https://github.com/rikki-iki/nightwatch-axe-core doesn't have a regular release cycle, I checked with @rikki_iki to see if it was fine directly adding some version of that Nightwatch axe command directly to core. It can largely be the same, but with any needed code standard refactoring and removing any blocks that aren't needed for Drupal core testing.
The resource is small and effective enough that I don't think it'll add a maintenance burden to core - and if it does it's arguably easier than coordinating a new release.
So TLDR, lets move nightwatch-axe-core into core's Nightwatch code and this will be an easier sell.
Comment #31
dmundraThanks @bnjmnm. That makes sense. I will work on that.
Comment #32
dmundraComment #33
dmundraI moved the nightwatch-axe-core package into Nightwatch/Commands folder. I used eslint to adjust the code styling and I left the eslint-disable items. If those are show stoppers than I could use some help to convert the code to pass eslint requirements. In any case back to needs review.
Comment #35
bnjmnmHere's an intentionally failing patch to demonstrate the Axe checks are doing what we expect
Comment #36
bnjmnmApparently Nightwatch now includes Axe support. There's already an issue to update Nightwatch in core, so once it lands we'll have the Axe integration. The issue is #3306446: Update Nightwatch to 2.4.1.
Comment #37
dmundraNice that will definitely make it even easier to add a11y tests.
Comment #39
anchal_gupta commentedI have Fixed CS error. Please review it
Comment #43
bnjmnmThis is a version of the MR without the skipped tests to demonstrate the Axe checker is doing its job.
I also filed followup issues for the rules that are necessary to skip for tests to pass. They are all noted with a @todo in the test code.
Comment #44
dmundraThank you @bnjmnm. This is looking good!
Comment #45
nod_couple of questions in the MR
Comment #46
nod_for addressing the few comments and rerolling the MR
Comment #47
bnjmnmFeedback and reroll needs addressed.
Comment #48
nod_Pretty minor comments. In any case it works, a11y tests are run, I didn't see any obvious problems so RTBC.
Comment #49
quietone commentedI'm thinking this should have a CR to explain how to run these tests and a release note too.
Comment #50
bnjmnmAdded CR and release notes snippet
Comment #51
nod_Reviewed the changes to code and change notice, RTBC +1
Comment #52
mgiffordI think we'll need a new issue shortly on how to document this and include it as part of the Testing Core Gate.
It is also going to have an impact on the Accessibility Coding Standards document.
Comment #53
quietone commented+1. I was just coming here to leave a comment about the Core gate and see it is already done!
Comment #54
xjmBased on number #52 to it sounds like a follow-up needs to be filed?
Comment #55
xjmComment #56
xjmAlso could you move the dependency evaluation issue to the issue summary? Thanks!
Comment #57
bnjmnmI believe the first paragraph of the issue summary already does this? If there's there's something additional that should be there, specify and it can get taken care of.
Comment #58
lauriiiWhich dependency is #56 requesting to evaluate? There are no new dependencies being added in this issue. Automated accessibility testing is a feature of Nightwatch which was added in 2.3.0: https://nightwatchjs.org/guide/using-nightwatch/accessibility-testing.html.
I didn't review any of the config changes in the MR in detail but the approach seems fine. I don't didn't see anything controversial there. Looking forward to having these tests running automatically as part of the test suite! Thanks everyone who worked on this.
Comment #59
xjm#25 contains a dependency evaluation. I guess the issue changed scope since then?
Comment #60
xjmHiding confusing patches.
Comment #61
xjmI think this should probably be documented both on the accessibility and testing gates and the frontend developer tools?
Edit: Or basically what was already said in #52.
Comment #62
xjmAlso is the release note accurate if the Axe dependency is no longer being added?
Comment #63
xjmNW for docs updates and to clarify the CR and release note: Either this is making better use of something that is already a dependency, or the dependency was a prior approach that was abandoned.
Comment #65
bnjmnmYes, the issue changed scope and the issue summary was changed accordingly on Nov 1 2022 Since that change, the opening sentence of the issue summary has been (and continues to be)
In the time since that discovery, Nightwatch has added Axe support, so this can be done without any new dependencies.
The release note is accurate. Axe support is now included with Nightwatch. Added October 2022 #3306446: Update Nightwatch to 2.4.1
Neither. When work on this issue started, Nightwatch did not have built-in Axe support. Nightwatch introduced Axe support in version 2.3.6. The overall approach is effectively the same, but can now be done directly via the Nightwatch API.
The CR is accurate and was written after Nightwatch introduced built in Axe support. While I could add points that clarify what the change isn't, I think it's best and least confusing to report soley on what it is.
Comment #66
bnjmnmRemoving needs issue summary update as the tag was added requesting a dependency evaluation and no new dependency is being added.
Removing docs tag as the additions are below. These can get added to the documentation pages when this issue lands.
**Accessiblity Gates**
Will add to https://www.drupal.org/about/core/policies/core-change-policies/core-gat...
The change does not result in any Nighwatch + Axe test failures.
Drupal's Nightwatch tests include basic accessibliity scans with Axe as of Drupal 10.1. a11yTestAdmin.js scans a set of admin pages using Claro as the theme, and a11yTestDefault.js scans a set of default pages using Olivero as the theme. (These tests can be configured to run with other themes, but Claro/Olivero is what is used in core tests). Like any change to Drupal, all tests must pass for a change to be considered for core inclusion. If a change introdces a failure in an A11y test, it must either be addressed, or there needs to be sufficient evidence that the failure was a false positive, in which case that specific A11y assertion can be bypassed.
More information on the Nightwatch + Axe testing can be found in core/tests/README.md
** Testing Gates**
Will Add to https://www.drupal.org/about/core/policies/core-change-policies/core-gat...
If a new UI element or pattern is introduced, it should be included somewhere in the Nightwatch accessbility scans.
From Drupal 10.1 forward, Nightwatch runs Axe accessibility scans on a set of admin/default pages. The tests are run using the nightwatch_a11y_testing profile. If a change to Drupal introduces any kind of element/interaction/etc that is distinct enough from existing elements to have different accessiblity concerns, it should be added to one of the pages scanned in the automated accessiblity tests. If it can't be easily added to an existing page scanned in the tests, a new page should be added to the nightwatch_a11y_testing profile
The tests should be added to one (or both) of the two Nightwatch A11y tests: a11yTestAdmin.js, which scans a set of admin pages using Claro as the theme, and/or a11yTestDefault.js, which scans a set of default pages using Olivero as the theme
More information on the Nightwatch + Axe testing can be found in core/tests/README.md
** Frontend Developer Tools**
A line added to https://www.drupal.org/about/core/policies/core-change-policies/frontend...
Under the yarn command under the "Run Tests" h2 add "As of Drupal 10.1 Nightwatch tests include basic Axe accessiblity scans "
Comment #67
mgiffordVery cool. Thanks so much @bnjmnm
In #52 I was just thinking that the accessibility standards documentation that we proposed as part of the GAAD Pledge (but that haven't been approved by the Coding Standards team).
I made some slight changes to your draft, but I think much of it is going to be basically the same (spellcheck these before posting). I am not sure where we will be able to see the results of these tests on D.o. Also not sure how well this applies to Drupal Modules/Themes. Suspect the Webform module is the first one that might take advantage of this. For the most part, Nightwatch is still largely being used by Core.
Nightwatch-axe tests
From Drupal 10.1 forward, Nightwatch runs Axe accessibility scans on a set of Core admin/default pages. The tests are run using the nightwatch_a11y_testing profile. If a change to Drupal introduces any kind of element/interaction/etc. that is distinct enough from existing elements to have different accessibility concerns, it should be added to one of the pages scanned in the automated accessibility tests. If it can't be easily added to an existing page scanned in the tests, a new page should be added to the nightwatch_a11y_testing profile.
The tests should be added to one (or both) of the two Nightwatch A11y tests: a11yTestAdmin.js, which scans a set of admin pages using Claro as the theme, and/or a11yTestDefault.js, which scans a set of default pages using Olivero as the theme.
It is recommended that where possible, nightwatch-axe tests are added to test dynamic elements that are using Nightwatch. This will expand our accessibility coverage and reduce the need for manual testing.
More information on the Nightwatch + Axe testing can be found in core/tests/README.md
Comment #68
bnjmnm@mgifford confirmed the "needs followup" he added was addressed by the doc snippets I provided above and the tag can be removed. I'm setting it back to needs review as the docs are done and the CR and IS are current and accurate.
Comment #69
catchMoving this to a task since it's an implementation issue.
Comment #70
vladimirausHiding patches to concentrate on MR.
Comment #71
lauriiiLeaving few thoughts since @bnjmnm reached out for a review.
I'd be comfortable moving this to RTBC after the core gate changes are moved to a separate issue.
Comment #72
bnjmnm@lauriii makes a good point about moving the core gates portion to a separate issue, and I created #3339065: [PP-1] Consider updating core gates to include automated Nightwatch A11y tests as a followup. These tests benefit Drupal accessibility whether or not they're part of a defined gate, and their presence do not alter how existing defined gates work.
Comment #73
andy-blumAdding #3301173: Allow starterkit theme generator tool to clone Olivero as a related issue as we intend to use the testing methods introduced here.
Comment #74
smustgrave commentedSeems the MR 2918 has some failures.
Comment #75
bnjmnmAdded #3349028: Improper use of aria-label in "System Powered By" block, which is a newly-caught A11y error by the tests added here. The
aria-allowed-attrtests need to be bypassed for the default theme until that issue is resolved.Comment #76
smustgrave commentedSeems to be a valid failure. But using this to help test 3349028
Comment #77
bnjmnmComment #78
smustgrave commentedAll the tests now pass.
Undoing the change
'aria-allowed-attr': { enabled: false },that @bnjmnm added I can see the error when runningddev exec -d /var/www/html/web/core yarn test:nightwatch tests/Drupal/Nightwatch/Tests/a11yTestDefault.jsSo appears to be doing its job
Comment #79
nod_#3349028: Improper use of aria-label in "System Powered By" block has been committed, we can remove
'aria-allowed-attr': { enabled: false }Comment #80
bnjmnmRemoved the @todos and the rule skipping for the now-closed #3349028: Improper use of aria-label in "System Powered By" block.
@VladimirAus if an issue needs work could you not merge HEAD without if it isn't accompanied by additional changes working towards completing this? Whoever is working on the work-that-needs-to-be-done can get the MR current with HEAD pretty easily, it doesn't really need to be done as an isolated task.
Comment #83
lauriiiCommitted 9e84cb4 and pushed to 10.1.x. Thanks!
Comment #84
wim leersEPIC work here! 🤩
Comment #86
bnjmnm