-
Notifications
You must be signed in to change notification settings - Fork 207
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
LG-12616: Add axe accessibility scan #467
Conversation
Co-authored-by: Andrew Duthie <[email protected]>
Co-authored-by: Andrew Duthie <[email protected]>
Co-authored-by: Andrew Duthie <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
const page = await browser.newPage(); | ||
await page.goto(`http://localhost:${port}${path}`); | ||
const results = await new AxePuppeteer(page) | ||
.withTags(['wcag2a', 'wcag2aa']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depending how strict you want the checks, there's a number of "best practices" rules that aren't strictly tied to WCAG that you could opt-in to. I suspect it'd catch a handful more issues, but issues still worth taking a look at. Up to y'all 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think having stricter checks would be good! we should fix the best practice issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for flagging this. I will include it in the follow up ticket so we can add them once we've knocked out some of the higher urgency items.
- run: | ||
name: Switch Node.js version | ||
command: | | ||
wget -qO- https://raw.githubusercontent.com/nvm-sh/nvm/v0.39.0/install.sh | bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out of curiosity, why 0.39.0
and not 0.39.7
? Is there a better way to install ^0.39
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, this version is from 10/2021
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good call-out. This was copied from identity-site
, where it was the latest version at the time of introduction. It was pinned to the specific version to avoid unexpected breakage from changing versions and for security, though in retrospect there's not much security assurances relying on git tags, since they can be deleted and recreated. SHA might be a better bet.
I'd think this could be bumped to the current latest version, assuming it works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just grabbed the same version/command that the identity-design-system is using for the sake of consistency -- https://github.com/18F/identity-design-system/blob/948b3326379f182fd4d5dbabcc10daca510763d7/.circleci/config.yml#L13-L20
- run: | ||
name: Switch Node.js version | ||
command: | | ||
wget -qO- https://raw.githubusercontent.com/nvm-sh/nvm/v0.39.0/install.sh | bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, this version is from 10/2021
LG-12616
Setting up Axe / Puppeteer to run tests - thank you to @aduth borrowing from similar implementation done last week on identity-design-system.
After running the tests there are failures with missingtitle
element andlang
attribute on practically all the pages so I have commented out the line in the Makefile until those failures are addressed.There are quite a few errors on a lot of different pages that need to be addresses ranging from critical to mild.
See full log output in Google Doc here.
The plan is to address the failures in this ticket and then we can update the
disableRules
method on line 50 appropriately as we address the failures.