-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
tests: assert what axe checks matches our a11y audits #12699
Conversation
@@ -143,3 +156,4 @@ class Accessibility extends FRGatherer { | |||
} | |||
|
|||
module.exports = Accessibility; | |||
module.exports._runA11yChecksInTestMode = () => runA11yChecks(true); |
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.
future esmodule snipe: easier later if you just make this a method on the gatherer.
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 love this!
|
||
describe('axe-run', () => { | ||
/** | ||
* @jest-environment jsdom |
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.
is this for eslint? does it actually mutate jest's environment for the entire file? just this describe
?
for (const type of ['violations', 'incomplete', 'inapplicable', 'passes']) { | ||
axeResults[type] && axeResults[type].forEach(result => axeRuleIds.add(result.id)); | ||
} | ||
// The color-contrast rule is manually disabled as jsdom doesn't support the APIs needed. https://github.com/dequelabs/axe-core/blob/581c441c/doc/examples/jsdom/test/a11y.js#L40 |
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.
with the amount of complexity here from jsdom and distance from reality, I'm pretty close to sayin' we should just do this in puppeteer instead. WDYT?
let browser;
beforeAll(async () => {
browser = await require('puppeteer').launch();
})
afterAll(async () => {
await browser.close()
})
it('should work', () => {
const page = await browser.newPage()
await page.evaluate(`${axe} ; ${getNodeDetails} ; ${daFn}`) // or intercept the string by running the gatherer itself with a `mockCommand`?
})
Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>
Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>
@@ -38,8 +37,9 @@ async function runA11yChecks() { | |||
'wcag2aa', | |||
], | |||
}, | |||
resultTypes: ['violations', 'inapplicable'], | |||
resultTypes, |
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.
it looks like resultTypes
is a bit weird. We definitely have incomplete
results in our artifacts and I was very confused just noticing for the first time that 'incomplete'
is not in this array :)
The docs say
The resultTypes option can be used to limit the number of nodes for a rule to a maximum of one. This can be useful for improving performance on very large or complicated pages when you are only interested in certain types of results.
After axe has processed all rules normally, it generates a unique selector for all nodes in all rules. This process can be time consuming, especially for pages with lots of nodes. By limiting the nodes to a maximum of one for result types you are not interested in, you can greatly speed up the tail end performance of axe.
so I think resultTypes
can remain the same since we just need the id
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.
over in #12536 (comment) we also talked about the value of having passes
information in the artifact as well. Could start with
passes: axeResults.passes.map(result => ({id: result.id})),
like notApplicable
currently does to get started on that for minimal artifact cost, apparently no additional axe time (since resultTypes
wasn't limiting that anyways), and it's forward compatible if we ever need to do the full createAxeRuleResultArtifact
on passes
since it'll still be extending {id: string}
.
That would mean only opts.colorContrastEnabled
would have to be custom between the two things (and maybe wouldn't need a separate method unless folks really want it).
Bringing this one back from the dead. I moved it to pptr and took brendan's suggestion to actually know what resultTypes are before we eff with them. :) Both things simplified the implementation a lot. |
@@ -168,5 +175,7 @@ class Accessibility extends FRGatherer { | |||
}); | |||
} | |||
} | |||
|
|||
Accessibility.runA11yChecks = runA11yChecks; | |||
Accessibility.createAxeRuleResultArtifact = createAxeRuleResultArtifact; |
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.
just make these static functions on Accessibility class?
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 tried out making them static fns.. but an interesting problem is that when they .toString()
they don't have the function
keyword and our .evaluate()
basically wants to treat them like fn expressions and so there's a syntax error.
instead i just exposed them via a static property.
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.
still looks great!
'input-button-name': {enabled: false}, | ||
'role-img-alt': {enabled: false}, | ||
'select-name': {enabled: false}, |
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.
has this list changed since this pr?
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.
funnily enough, nope!
Whenever we bump axe (eg #11661, #9798 ) we manually look at what audits are created/removed and adjust our config. But sometimes we miss cases and that means we are running axe checks we never surface the results of. (eg #10453
relevant shoutout: #7127
This adds a test to ensure our audit lineup matches what axe gives us results on.
This comes with updates to the axe config. I added a link to 🔒 http://go/prcpg which is an internal evaluation of the rules.
aria-roledescription
andscrollable-region-focusable
are disabled as they're considered not very important. I have a followup that adds the 3 last audits that are disabled in this PR. (they're good, but seemed better to land them separately)