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

core: page function updates relating to upcoming tap targets audit #6702

Merged
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 3 additions & 32 deletions lighthouse-core/gather/gatherers/accessibility.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/
'use strict';

/* global window, document, Node, getOuterHTMLSnippet */
/* global window, document, getOuterHTMLSnippet, getNodePath */

const Gatherer = require('./gatherer');
const fs = require('fs');
Expand Down Expand Up @@ -45,6 +45,7 @@ function runA11yChecks() {
// Augment the node objects with outerHTML snippet & custom path string
// @ts-ignore
axeResult.violations.forEach(v => v.nodes.forEach(node => {
// @ts-ignore - getNodePath put into scope via stringification
node.path = getNodePath(node.element);
Copy link
Collaborator

Choose a reason for hiding this comment

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

you could keep this the same, if you also export getNodePath in page-functions.js.

Will need that module to export getNodePath for linking nodes in the report for the font-size audit (#6436) (and for other non-accessibility audits).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure what you mean @hoten, it's run in the context of the page and he's exported the stringified version which is what he'll need, no?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Didn't realize all of that was running in context of the page. Nevermind!

// @ts-ignore - getOuterHTMLSnippet put into scope via stringification
node.snippet = getOuterHTMLSnippet(node.element);
Expand All @@ -56,37 +57,6 @@ function runA11yChecks() {
axeResult = {violations: axeResult.violations, notApplicable: axeResult.inapplicable};
return axeResult;
});

/**
* Adapted from DevTools' SDK.DOMNode.prototype.path
* https://github.com/ChromeDevTools/devtools-frontend/blob/7a2e162ddefd/front_end/sdk/DOMModel.js#L530-L552
* TODO: Doesn't handle frames or shadow roots...
* @param {Node} node
*/
function getNodePath(node) {
/** @param {Node} node */
function getNodeIndex(node) {
let index = 0;
let prevNode;
while (prevNode = node.previousSibling) {
node = prevNode;
// skip empty text nodes
if (node.nodeType === Node.TEXT_NODE && node.textContent &&
node.textContent.trim().length === 0) continue;
index++;
}
return index;
}

const path = [];
while (node && node.parentNode) {
const index = getNodeIndex(node);
path.push([index, node.nodeName]);
node = node.parentNode;
}
path.reverse();
return path.join(',');
}
}

class Accessibility extends Gatherer {
Expand All @@ -98,6 +68,7 @@ class Accessibility extends Gatherer {
const driver = passContext.driver;
const expression = `(function () {
${pageFunctions.getOuterHTMLSnippetString};
${pageFunctions.getNodePathString};
${axeLibSource};
return (${runA11yChecks.toString()}());
})()`;
Expand Down
72 changes: 70 additions & 2 deletions lighthouse-core/lib/page-functions.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
// @ts-nocheck
'use strict';

/* global window document */
/* global window document Node */

/**
* Helper functions that are passed by `toString()` by Driver to be evaluated in target page.
Expand Down Expand Up @@ -119,7 +119,7 @@ function getOuterHTMLSnippet(element, ignoreAttrs = []) {
clone.removeAttribute(attribute);
});

const reOpeningTag = /^.*?>/;
const reOpeningTag = /^[\s\S]*?>/;
const match = clone.outerHTML.match(reOpeningTag);

return (match && match[0]) || '';
Expand Down Expand Up @@ -153,6 +153,71 @@ function ultradumbBenchmark() {
return Math.round(iterations / durationInSeconds);
}

/**
* Adapted from DevTools' SDK.DOMNode.prototype.path
* https://github.com/ChromeDevTools/devtools-frontend/blob/7a2e162ddefd/front_end/sdk/DOMModel.js#L530-L552
* TODO: Doesn't handle frames or shadow roots...
* @param {Node} node
*/
/* istanbul ignore next */
function getNodePath(node) {
/** @param {Node} node */
function getNodeIndex(node) {
let index = 0;
let prevNode;
while (prevNode = node.previousSibling) {
node = prevNode;
// skip empty text nodes
if (node.nodeType === Node.TEXT_NODE && node.textContent &&
node.textContent.trim().length === 0) continue;
index++;
}
return index;
}

const path = [];
while (node && node.parentNode) {
const index = getNodeIndex(node);
path.push([index, node.nodeName]);
node = node.parentNode;
}
path.reverse();
return path.join(',');
}

/**
* @param {Element} node
* @returns {string}
*/
/* istanbul ignore next */
function getNodeSelector(node) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

think we could get a quick test for this function? don't need crazy coverage just something to make sure we don't break it

/**
* @param {Element} node
*/
function getSelectorPart(node) {
let part = node.tagName.toLowerCase();
if (node.id) {
part += '#' + node.id;
} else if (node.classList.length > 0) {
part += '.' + node.classList[0];
}
return part;
}

const parts = [];
while (parts.length < 4) {
parts.unshift(getSelectorPart(node));
if (!node.parentElement) {
break;
}
node = node.parentElement;
if (node.tagName === 'HTML') {
break;
}
}
return parts.join(' > ');
}

module.exports = {
wrapRuntimeEvalErrorInBrowserString: wrapRuntimeEvalErrorInBrowser.toString(),
registerPerformanceObserverInPageString: registerPerformanceObserverInPage.toString(),
Expand All @@ -162,4 +227,7 @@ module.exports = {
getOuterHTMLSnippet: getOuterHTMLSnippet,
ultradumbBenchmark: ultradumbBenchmark,
ultradumbBenchmarkString: ultradumbBenchmark.toString(),
getNodePathString: getNodePath.toString(),
getNodeSelectorString: getNodeSelector.toString(),
getNodeSelector: getNodeSelector,
};
16 changes: 15 additions & 1 deletion lighthouse-core/test/lib/page-functions-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ const pageFunctions = require('../../lib/page-functions');

/* eslint-env jest */

describe('DetailsRenderer', () => {
describe('Page Functions', () => {
let dom;

beforeAll(() => {
Expand Down Expand Up @@ -44,5 +44,19 @@ describe('DetailsRenderer', () => {
['style-missing', 'aria-label-missing']
), '<div id="1" style="style" aria-label="label">');
});

it('works if attribute values contain line breaks', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

❤️

assert.equal(pageFunctions.getOuterHTMLSnippet(
dom.createElement('div', '', {style: 'style1\nstyle2'})), '<div style="style1\nstyle2">');
});
});

describe('getNodeSelector', () => {
it('Uses IDs where available and otherwise falls back to classes', () => {
const parentEl = dom.createElement('div', '', {id: 'wrapper', class: 'dont-use-this'});
const childEl = dom.createElement('div', '', {class: 'child'});
parentEl.appendChild(childEl);
assert.equal(pageFunctions.getNodeSelector(childEl), 'div#wrapper > div.child');
});
});
});