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

[SPIKE] Check all pages' accessibility #2957

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

domoscargin
Copy link
Contributor

@domoscargin domoscargin commented Jul 19, 2023

Popped this together to check how long it'd take to run accessibility tests on ALL our pages. Just a hacky function to get all directories with an index.html file, then running an axe validation test on each of those.

Takes around 80 seconds on my machine, but open for ways to speed it up.

There's a few failures flagged, mostly to do with our heading order.

If taken forward, the code would need to be cleaned up and optimised.

Edit: looks like the tests took about 2m 45s.

@netlify
Copy link

netlify bot commented Jul 19, 2023

You can preview this change here:

Name Link
🔨 Latest commit 969033f
🔍 Latest deploy log https://app.netlify.com/sites/govuk-design-system-preview/deploys/6694e8f49d6c4100082882a3
😎 Deploy Preview https://deploy-preview-2957--govuk-design-system-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@domoscargin domoscargin force-pushed the bk-extend-accessibility-tests branch 2 times, most recently from 5861a47 to d519a77 Compare July 20, 2023 08:43
@domoscargin
Copy link
Contributor Author

domoscargin commented Jul 20, 2023

Had a brief look at it.concurrent.each, but at first glance this doesn't play nicely with Puppeteer. Maybe something with promise.all?

@colinrotherham
Copy link
Contributor

Hey @domoscargin I've rebased this one, seems a shame to leave it

I've brought over the Axe rules from alphagov/govuk-frontend@ebb9e4e added in:

Locally I'm seeing Puppeteer (with Axe) disconnect after a while but let's hope it's resolved by:

@colinrotherham
Copy link
Contributor

@domoscargin Just rebased this to see if the @axe-core/puppeteer update fixed the disconnect issue

Looking good locally

@domoscargin
Copy link
Contributor Author

Darn, failed here.

@colinrotherham
Copy link
Contributor

Darn, failed here.

How frustrating 😭

Worth trying a separate await browser.newPage() per test (just pushed)

Otherwise, let's check again when more next major updates come out 🤦‍♂️

@colinrotherham
Copy link
Contributor

Another Puppeteer update so rebasing this again

@colinrotherham
Copy link
Contributor

Had some more Axe and Puppeteer updates in #3468 so giving this one a rebase

@colinrotherham
Copy link
Contributor

@domoscargin Looks like it's all working 🙌

Status check failures are now accessibility report issues, as intended

@domoscargin
Copy link
Contributor Author

domoscargin commented Feb 14, 2024

I've reduced failures on this PR down to zero BUT only by disabling rules across the board.

Just wanted to get this down to zero and allow us to think about how to address each of the special cases (which are all documented in __tests__/accessibility-audit.test.js).

The rules that cause failures unless disabled:

[
  'region',
  'color-contrast-enhanced',
  'aria-allowed-attr',
  'target-size',
  'aria-allowed-role'
]

@domoscargin domoscargin force-pushed the bk-extend-accessibility-tests branch from 071a6bd to 9199c96 Compare March 4, 2024 11:14
domoscargin and others added 8 commits July 15, 2024 09:46
It might be an axe bug, but exclude doesn't seem to work properly with an array argument. Splitting them out reduces failures from 300+ to 139.

Additionally, I've made some changes to which files are included in the glob, with a view to splitting these out and only running certain tests in certain environments to avoid jammingthings up for too long.
@domoscargin domoscargin force-pushed the bk-extend-accessibility-tests branch from 9199c96 to 4d4d44f Compare July 15, 2024 08:48
@domoscargin
Copy link
Contributor Author

domoscargin commented Jul 15, 2024

Failures notes

aria-allowed-role

This flags our use of role="region" on the <video> element, which doesn't allow that role.

This was done on purpose, but unclear if it's still relevant.

target-size

Bit of a weird one, this. It's flagging the "Open this example in a new tab" links, but only on certain examples:

#table-second-example in /components/table/index.html
#select-second-example in /components/select/default/index.html
#panel-second-example in /components/panel/default/index.html
#details-second-example in /components/details/default/index.html

There are plenty more examples with the second-example suffix, but maybe there's something about these that's confusing Axe? When I check with the web version, I don't get the same error.

It's also flagging:

button[value="yes"] (Accept analytics cookies) in /styles/page-template/custom/code/index.html
.govuk-header__service-name (Service name) in /styles/page-template/custom/code/index.html
.govuk-header__navigation-item--active > .govuk-header__link[href="#"] (Navigation item 1, 2 & 3) in /styles/page-template/custom/code/index.html
.govuk-footer__inline-list-item:nth-child(1) > .govuk-footer__link[href="#"] (Footer links) in /styles/page-template/custom/code/index.html

Again, I can't recreate this on the web.

aria-allowed-attr

This is flagging a bunch of conditional radios on the Equality Information examples. It doesn't like them having aria-expanded="false". Again, I can't recreate this on the web, and there are other examples using conditionals that don't get flagged.

color-contrast-enhanced

This flags 180+ issues, but is a AAA rule and I think we can potentially ignore it for now in our automated testing and deal with the important cases manually.

region

"All page content should be contained by landmarks" - this flags over 100 issues, but we already ignore a few instances separately.

@selfthinker If you're ever bored, feel free to have a gander at these and have a reckon (this isn't currently prioritised work).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants