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

Switch from HTML_CodeSniffer to Axe Puppeteer #435

Merged
merged 3 commits into from
Mar 20, 2024

Conversation

aduth
Copy link
Member

@aduth aduth commented Mar 20, 2024

🛠 Summary of changes

Updates accessibility tests to use @axe-core/puppeteer instead of HTML_CodeSniffer, which has a much more thorough accessibility test suite and is more consistent with how we test accessibility in other Login.gov projects.

As part of this, updates were made to resolve preexisting accessibility issues:

📜 Testing Plan

  1. make build
  2. node --test test/accessibility.test.js

throw message;
}
}
const results = await new AxePuppeteer(page).withTags(['wcag2a', 'wcag2aa']).analyze();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is "bare minimum". We could / should choose to omit .withTags(['wcag2a', 'wcag2aa']) which identifies several more "best practice" failures, but we can improve that iteratively.

Suggested change
const results = await new AxePuppeteer(page).withTags(['wcag2a', 'wcag2aa']).analyze();
const results = await new AxePuppeteer(page).analyze();

@@ -0,0 +1,11 @@
module Kramdown
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to explicitly load this anywhere so it can take effect? I'd expect a _config.yml change listing this file. Or does it get magically loaded somehow?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anything in _plugins is magically loaded by Jekyll, and this will override the module method from Kramdown referenced indirectly through Jekyll compilation.

I adapted this approach from similar plugins we use on some other sites like with how we add design system classes to Markdown links in the brochure site.

@aduth
Copy link
Member Author

aduth commented Mar 20, 2024

Visual regression failure is catching updated markup for header component.

image

@aduth aduth merged commit 47fdeff into main Mar 20, 2024
3 of 4 checks passed
@aduth aduth deleted the aduth-axe-accessibility-tests branch March 20, 2024 19:43
@aduth
Copy link
Member Author

aduth commented Mar 21, 2024

This is "bare minimum". We could / should choose to omit .withTags(['wcag2a', 'wcag2aa']) which identifies several more "best practice" failures, but we can improve that iteratively.

Some notes for posterity / future self after looking at this a bit: It's complicated by the fact that many code examples assume an isolated environment from sibling code examples so issues like distinct aria-label for landmarks are flagged on pages where we show multiple variations of the same base markup. In practice we'd expect only one of those markups to be used on a site so that it wouldn't generate the issue.

Couple ideas for addressing this:

  • Easier is to exempt certain "expected" violations for documentation pages after acknowledging it's not actually an issue
  • Better would be to actually create that isolation with something like an iframe, similar to what was done in Add header documentation with mobile preview #418
    • Creating the frame page for every code example would be cumbersome to do manually.
    • As a solution, we could consider...
    • In either case, one additional complication of a dynamic frame is that unlike the static sizing of the mobile frame in Add header documentation with mobile preview #418, the frame would need to adjust to its own content, which would require a bit more work to talk (postMessage) between the iframe and the parent page to resize the frame after load.

The "Easier" option might be the best bet for a short-term solution, after timebox exploration.

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