-
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
misc: reorganize accessibility gatherer #12076
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,7 @@ const pageFunctions = require('../../lib/page-functions.js'); | |
/* c8 ignore start */ | ||
async function runA11yChecks() { | ||
/** @type {import('axe-core/axe')} */ | ||
// @ts-expect-error axe defined by axeLibSource | ||
// @ts-expect-error - axe defined by axeLibSource | ||
const axe = window.axe; | ||
const application = `lighthouse-${Math.random()}`; | ||
axe.configure({ | ||
|
@@ -65,47 +65,52 @@ async function runA11yChecks() { | |
// are relative to the top of the page | ||
document.documentElement.scrollTop = 0; | ||
|
||
/** @param {import('axe-core/axe').Result} result */ | ||
const augmentAxeNodes = result => { | ||
result.helpUrl = result.helpUrl.replace(application, 'lighthouse'); | ||
if (axeResults.inapplicable.includes(result)) return; | ||
return { | ||
violations: axeResults.violations.map(createAxeRuleResultArtifact), | ||
incomplete: axeResults.incomplete.map(createAxeRuleResultArtifact), | ||
notApplicable: axeResults.inapplicable.map(result => ({id: result.id})), | ||
version: axeResults.testEngine.version, | ||
}; | ||
} | ||
|
||
result.nodes.forEach(node => { | ||
// @ts-expect-error - getNodeDetails put into scope via stringification | ||
node.node = getNodeDetails(node.element); | ||
// @ts-expect-error - avoid circular JSON concerns | ||
node.element = node.any = node.all = node.none = node.html = undefined; | ||
}); | ||
/** | ||
* @param {import('axe-core/axe').Result} result | ||
* @return {LH.Artifacts.AxeRuleResult} | ||
*/ | ||
function createAxeRuleResultArtifact(result) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice idea to pull this out to its own function |
||
// Simplify `nodes` and collect nodeDetails for each. | ||
const nodes = result.nodes.map(node => { | ||
const {target, failureSummary, element} = node; | ||
// TODO: with `elementRef: true`, `element` _should_ always be defined, but need to verify. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. very debatable TODO. The type is
So...worth marking it TODO or just a comment? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally we could generate a warning if this invariant fails (and when it does, just skip this node and not show it in the results / or keep and fake a nodeDetails / use the body node). would require a not-ideal restructure of these two functions, so maybe not worth it generating a warning. am hesitant to have it fail the entire run, but it is an option to just throw an error if element does not exist.. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
what do you think of beaconing to Sentry? I assume we'd get some kind of |
||
// @ts-expect-error - getNodeDetails put into scope via stringification | ||
const nodeDetails = getNodeDetails(/** @type {HTMLElement} */ (element)); | ||
|
||
// Ensure errors can be serialized over the protocol | ||
/** @type {(Error & {message: string, errorNode: any}) | undefined} */ | ||
// @ts-expect-error - when rules error axe sets these properties | ||
// see https://github.com/dequelabs/axe-core/blob/eeff122c2de11dd690fbad0e50ba2fdb244b50e8/lib/core/base/audit.js#L684-L693 | ||
const error = result.error; | ||
if (error instanceof Error) { | ||
// @ts-expect-error | ||
result.error = { | ||
name: error.name, | ||
message: error.message, | ||
stack: error.stack, | ||
errorNode: error.errorNode, | ||
}; | ||
} | ||
}; | ||
return { | ||
target, | ||
failureSummary, | ||
node: nodeDetails, | ||
}; | ||
}); | ||
|
||
// Augment the node objects with outerHTML snippet & custom path string | ||
axeResults.violations.forEach(augmentAxeNodes); | ||
axeResults.incomplete.forEach(augmentAxeNodes); | ||
axeResults.inapplicable.forEach(augmentAxeNodes); | ||
// Ensure errors can be serialized over the protocol. | ||
/** @type {Error | undefined} */ | ||
// @ts-expect-error - when rules throw an error, axe saves it here. | ||
// see https://github.com/dequelabs/axe-core/blob/eeff122c2de11dd690fbad0e50ba2fdb244b50e8/lib/core/base/audit.js#L684-L693 | ||
const resultError = result.error; | ||
let error; | ||
if (resultError instanceof Error) { | ||
error = { | ||
name: resultError.name, | ||
message: resultError.message, | ||
}; | ||
} | ||
|
||
// We only need violations, and circular references are possible outside of violations | ||
return { | ||
// @ts-expect-error value is augmented above. | ||
violations: axeResults.violations, | ||
notApplicable: axeResults.inapplicable, | ||
// @ts-expect-error value is augmented above. | ||
incomplete: axeResults.incomplete, | ||
version: axeResults.testEngine.version, | ||
id: result.id, | ||
impact: result.impact || undefined, | ||
tags: result.tags, | ||
nodes, | ||
error, | ||
}; | ||
} | ||
/* c8 ignore stop */ | ||
|
@@ -129,15 +134,8 @@ class Accessibility extends FRGatherer { | |
deps: [ | ||
axeLibSource, | ||
pageFunctions.getNodeDetailsString, | ||
createAxeRuleResultArtifact, | ||
], | ||
}).then(returnedValue => { | ||
if (!returnedValue) { | ||
throw new Error('No axe-core results returned'); | ||
} | ||
if (!Array.isArray(returnedValue.violations)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we trust our There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ya |
||
throw new Error('Unable to parse axe results' + returnedValue); | ||
} | ||
return returnedValue; | ||
}); | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -102,10 +102,9 @@ describe('Accessibility: axe-audit', () => { | |
} | ||
const artifacts = { | ||
Accessibility: { | ||
passes: [{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not a thing :) |
||
id: 'fake-axe-pass', | ||
help: 'http://example.com/', | ||
}], | ||
violations: [], | ||
notApplicable: [], | ||
incomplete: [], | ||
}, | ||
}; | ||
|
||
|
@@ -134,15 +133,7 @@ describe('Accessibility: axe-audit', () => { | |
}], | ||
help: 'http://example.com/', | ||
}], | ||
// TODO: remove: axe-core v3.3.0 backwards-compatibility test | ||
violations: [{ | ||
id: 'fake-axe-failure-case', | ||
nodes: [{ | ||
html: '<input id="multi-label-form-element" />', | ||
node: {}, | ||
}], | ||
help: 'http://example.com/', | ||
}], | ||
violations: [], | ||
}, | ||
}; | ||
|
||
|
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.
threw this one in for #12075 (comment) and because technically I did this, when I was updating old
rawValue: true
s toscore: 1
s all over the place :)