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

misc: reorganize accessibility gatherer #12076

Merged
merged 4 commits into from
Mar 2, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions lighthouse-core/audits/accessibility/axe-audit.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class AxeAudit extends Audit {
const isNotApplicable = notApplicables.find(result => result.id === this.meta.id);
if (isNotApplicable) {
return {
score: 1,
score: null,
Copy link
Member Author

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: trues to score: 1s all over the place :)

notApplicable: true,
};
}
Expand Down Expand Up @@ -66,7 +66,7 @@ class AxeAudit extends Audit {
// Since there is no score impact from informative rules, display the rule as not applicable
if (isInformative && !rule) {
return {
score: 1,
score: null,
notApplicable: true,
};
}
Expand Down
88 changes: 43 additions & 45 deletions lighthouse-core/gather/gatherers/accessibility.js
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down Expand Up @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.
Copy link
Member Author

Choose a reason for hiding this comment

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

very debatable TODO. The type is {element?: HTMLElement}, true, and we could be defensive, but AFAICT with elementRef: true element is always set.

  • On the one hand I haven't been able to verify that it's always set (there doesn't seem to be a single place in the code that sets it, but maybe I'm missing it).
  • On the other hand we've been assuming that it's always set since 2016.

So...worth marking it TODO or just a comment?

Copy link
Collaborator

Choose a reason for hiding this comment

The 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..

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

what do you think of beaconing to Sentry? I assume we'd get some kind of Cannot read property 'whatever' of undefined if we really did sometimes have no element, but I can't find any hint of that in Sentry for the accessibility gatherer or audits. But this way it would be explicit and we'd have as good a signal as any after a month that it really can't ever happen?

// @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 */
Expand All @@ -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)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

we trust our evaluate() by now and don't do these checks in other gatherers. No need to keep these

Copy link
Collaborator

Choose a reason for hiding this comment

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

ya

throw new Error('Unable to parse axe results' + returnedValue);
}
return returnedValue;
});
}
}
Expand Down
17 changes: 4 additions & 13 deletions lighthouse-core/test/audits/accessibility/axe-audit-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -102,10 +102,9 @@ describe('Accessibility: axe-audit', () => {
}
const artifacts = {
Accessibility: {
passes: [{
Copy link
Member Author

Choose a reason for hiding this comment

The 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: [],
},
};

Expand Down Expand Up @@ -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: [],
},
};

Expand Down
28 changes: 0 additions & 28 deletions lighthouse-core/test/gather/gatherers/accessibility-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,34 +17,6 @@ describe('Accessibility gatherer', () => {
accessibilityGather = new AccessibilityGather();
});

it('fails if nothing is returned', () => {
return accessibilityGather.afterPass({
driver: {
executionContext: {
async evaluate() {},
},
},
}).then(
_ => assert.ok(false),
_ => assert.ok(true));
});

it('fails if result has no violations array', () => {
return accessibilityGather.afterPass({
driver: {
executionContext: {
async evaluate() {
return {
url: 'https://example.com',
};
},
},
},
}).then(
_ => assert.ok(false),
_ => assert.ok(true));
});

it('propagates error retrieving the results', () => {
const error = 'There was an error.';
return accessibilityGather.afterPass({
Expand Down