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: normalize node information in gathering #11405

Merged
merged 22 commits into from
Sep 16, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
ef4db92
added consistency within artifacts that use nodes - for better node i…
adrianaixba Sep 8, 2020
9b97ab9
cleaning up getNodeInfo
adrianaixba Sep 9, 2020
c299b95
fixed declarationa and oopif smoke rect error
adrianaixba Sep 9, 2020
279d86b
fixed some smoke tests, and added implementation of getNodeInfo on li…
adrianaixba Sep 10, 2020
82d4e72
lint things
adrianaixba Sep 10, 2020
3bec7f1
removed boundingRect from script-elements
adrianaixba Sep 10, 2020
f0b3938
iframe-elements clientRect for backwards capabilities
adrianaixba Sep 10, 2020
55a5f00
cool object.assign() implementation for cleaning up getNodeInfo spread
adrianaixba Sep 10, 2020
76e7d72
Merge branch 'master' into devtools-node-path
adrianaixba Sep 11, 2020
9fe609f
anchor-elements outerHTML property not needed, same as node snippet
adrianaixba Sep 11, 2020
dc516c9
Merge branch 'devtools-node-path' of https://github.com/GoogleChrome/…
adrianaixba Sep 11, 2020
c3346c3
cleanups
adrianaixba Sep 11, 2020
af20d48
cleanups
adrianaixba Sep 11, 2020
df39ee1
fixed passwords artifact, deleted truncate method
adrianaixba Sep 14, 2020
f18bb19
cleanup
adrianaixba Sep 14, 2020
cda6169
changing getNodeInfo to getNodeDetails
adrianaixba Sep 14, 2020
bb4ab94
cleanups
adrianaixba Sep 14, 2020
1a7d1d1
cleanups
adrianaixba Sep 14, 2020
9d57d5f
cleanups
adrianaixba Sep 14, 2020
ae2782a
added NodeDetails interface
adrianaixba Sep 15, 2020
f9afa97
cleanups
adrianaixba Sep 15, 2020
3da2b5e
cleanups
adrianaixba Sep 15, 2020
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
Original file line number Diff line number Diff line change
Expand Up @@ -357,15 +357,15 @@ const expectations = [
'tapTarget': {
'type': 'node',
/* eslint-disable max-len */
'snippet': '<a data-gathered-target="zero-width-tap-target-with-overflowing-child-content" style="display: block; width: 0; white-space: nowrap">\n <!-- TODO: having the span should not be necessary to cause a failure here, but\n right now we don\'t try to get the client rects of children that …',
'snippet': '<a data-gathered-target="zero-width-tap-target-with-overflowing-child-content" style="display: block; width: 0; white-space: nowrap">',
'path': '2,HTML,1,BODY,14,DIV,0,A',
'selector': 'body > div > a',
'nodeLabel': 'zero width target',
},
'overlappingTarget': {
'type': 'node',
/* eslint-disable max-len */
'snippet': '<a data-gathered-target="passing-tap-target-next-to-zero-width-target" style="display: block; width: 110px; height: 100px;background: #aaa;">\n passing target\n </a>',
'snippet': '<a data-gathered-target="passing-tap-target-next-to-zero-width-target" style="display: block; width: 110px; height: 100px;background: #aaa;">',
'path': '2,HTML,1,BODY,14,DIV,1,A',
'selector': 'body > div > a',
'nodeLabel': 'passing target',
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/accessibility/axe-audit.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ class AxeAudit extends Audit {
node: /** @type {LH.Audit.Details.NodeValue} */ ({
type: 'node',
selector: Array.isArray(node.target) ? node.target.join(' ') : '',
path: node.path,
path: node.devtoolsNodePath,
Copy link
Member

@paulirish paulirish Sep 12, 2020

Choose a reason for hiding this comment

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

in connor's braindump he noted our artifacts use both path and devtoolsNodePath, and suggested we normalize to dNP.

however our public audit details type of NodeValue uses path:

path?: string;

this is the only property that gets renamed from artifacts to audit details... snippet, selector and even nodeLabel are consistent on both sides.

renaming these as path in the artifacts would be nice for consistency.. it does lose a bit of signal, but perhaps its worth it? wdyt @connorjclark @patrickhulce

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd agree if path weren't so painfully ambiguous (never in a million years as a standard consumer of an LHR would I expect path to be this series of random integers nor would I know how to use it) 😬

For that reason I think it's worth the cost to normalize on devtoolsNodePath and upgrade the renderer to support both in a future PR (which eventual removal of .path in some future breaking version). The fact that it also is only relevant to DevTools integration makes it easier of a change in the renderer/LHR too since it's unlikely our other big report renderer implementations (LHCI, Calibre, Treo, etc) are relying on this.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW when I was helping with adding devtoolsNodePath in #11061, I found it confusing when trying to remember how NodeDetails worked and the extra context from the name helped on the artifacts side. It would be unfortunate to lose that.

Copy link
Member

Choose a reason for hiding this comment

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

yah sg. renaming the audit details prop to devtoolsNodePath seems like a win, even if its a bit more painful to do.

snippet: node.snippet,
boundingRect: node.boundingRect,
explanation: node.failureSummary,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ class ExternalAnchorsUseRelNoopenerAudit extends Audit {
try {
return new URL(anchor.href).host !== pageHost;
} catch (err) {
warnings.push(str_(UIStrings.warning, {anchorHTML: anchor.outerHTML}));
warnings.push(str_(UIStrings.warning, {anchorHTML: anchor.snippet}));
return true;
}
})
Expand All @@ -74,12 +74,12 @@ class ExternalAnchorsUseRelNoopenerAudit extends Audit {
path: anchor.devtoolsNodePath || '',
selector: anchor.selector || '',
nodeLabel: anchor.nodeLabel || '',
snippet: anchor.outerHTML || '',
snippet: anchor.snippet || '',
}),
href: anchor.href || 'Unknown',
target: anchor.target || '',
rel: anchor.rel || '',
outerHTML: anchor.outerHTML || '',
outerHTML: anchor.snippet || '',
};
});

Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/seo/crawlable-anchors.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ class CrawlableAnchors extends Audit {
path: anchor.devtoolsNodePath || '',
selector: anchor.selector || '',
nodeLabel: anchor.nodeLabel || '',
snippet: anchor.outerHTML || '',
snippet: anchor.snippet || '',
},
};
});
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/seo/tap-targets.js
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ function targetToTableNode(target) {
return {
type: 'node',
snippet: target.snippet,
path: target.path,
path: target.devtoolsNodePath,
selector: target.selector,
boundingRect,
nodeLabel: target.nodeLabel,
Expand Down
22 changes: 5 additions & 17 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, getOuterHTMLSnippet, getBoundingClientRect, getNodePath, getNodeLabel */
/* global window, document, getNodeDetails */

const Gatherer = require('./gatherer.js');
const fs = require('fs');
Expand Down Expand Up @@ -59,19 +59,10 @@ function runA11yChecks() {
// @ts-expect-error
const augmentAxeNodes = result => {
// @ts-expect-error
result.nodes.forEach(node => {
// @ts-expect-error - getNodePath put into scope via stringification
node.path = getNodePath(node.element);
// @ts-expect-error - getOuterHTMLSnippet put into scope via stringification
node.snippet = getOuterHTMLSnippet(node.element);
// @ts-expect-error - getBoundingClientRect put into scope via stringification
const rect = getBoundingClientRect(node.element);
if (rect.width > 0 && rect.height > 0) {
adrianaixba marked this conversation as resolved.
Show resolved Hide resolved
node.boundingRect = rect;
}

// @ts-expect-error - getNodeLabel put into scope via stringification
node.nodeLabel = getNodeLabel(node.element);
result.nodes.forEach(node => {
// @ts-expect-error - getNodeDetails put into scope via stringification
Object.assign(node, getNodeDetails(node.element));
// avoid circular JSON concerns
node.element = node.any = node.all = node.none = undefined;
});
Expand Down Expand Up @@ -110,10 +101,7 @@ class Accessibility extends Gatherer {
afterPass(passContext) {
const driver = passContext.driver;
const expression = `(function () {
${pageFunctions.getOuterHTMLSnippetString};
${pageFunctions.getBoundingClientRectString};
${pageFunctions.getNodePathString};
${pageFunctions.getNodeLabelString};
${pageFunctions.getNodeDetailsString};
${axeLibSource};
return (${runA11yChecks.toString()}());
})()`;
Expand Down
28 changes: 7 additions & 21 deletions lighthouse-core/gather/gatherers/anchor-elements.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
*/
'use strict';

/* global getNodeDetails */

const Gatherer = require('./gatherer.js');
const pageFunctions = require('../../lib/page-functions.js');

Expand Down Expand Up @@ -41,15 +43,8 @@ function collectAnchorElements() {
const anchorElements = getElementsInDocument('a'); // eslint-disable-line no-undef

return anchorElements.map(node => {
// @ts-expect-error - put into scope via stringification
const outerHTML = getOuterHTMLSnippet(node); // eslint-disable-line no-undef
// @ts-expect-error - put into scope via stringification
const nodePath = getNodePath(node); // eslint-disable-line no-undef
// @ts-expect-error - getNodeSelector put into scope via stringification
const selector = getNodeSelector(node); // eslint-disable-line no-undef
// @ts-expect-error - getNodeLabel put into scope via stringification
const nodeLabel = getNodeLabel(node); // eslint-disable-line no-undef

// @ts-expect-error - getNodeDetails put into scope via stringification
const nodeInfo = getNodeDetails(node);
if (node instanceof HTMLAnchorElement) {
return {
href: node.href,
Expand All @@ -60,10 +55,7 @@ function collectAnchorElements() {
text: node.innerText, // we don't want to return hidden text, so use innerText
rel: node.rel,
target: node.target,
devtoolsNodePath: nodePath,
selector,
nodeLabel,
outerHTML,
...nodeInfo,
};
}

Expand All @@ -75,10 +67,7 @@ function collectAnchorElements() {
text: node.textContent || '',
rel: '',
target: node.target.baseVal || '',
devtoolsNodePath: nodePath,
selector,
nodeLabel,
outerHTML,
...nodeInfo,
};
});
}
Expand Down Expand Up @@ -107,11 +96,8 @@ class AnchorElements extends Gatherer {
async afterPass(passContext) {
const driver = passContext.driver;
const expression = `(() => {
${pageFunctions.getOuterHTMLSnippetString};
${pageFunctions.getElementsInDocumentString};
${pageFunctions.getNodePathString};
${pageFunctions.getNodeSelectorString};
${pageFunctions.getNodeLabelString};
${pageFunctions.getNodeDetailsString};

return (${collectAnchorElements})();
})()`;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/
'use strict';

/* global document ClipboardEvent getOuterHTMLSnippet */
/* global document ClipboardEvent getNodeDetails */

const Gatherer = require('../gatherer.js');
const pageFunctions = require('../../../lib/page-functions.js');
Expand All @@ -22,10 +22,10 @@ function findPasswordInputsWithPreventedPaste() {
new ClipboardEvent('paste', {cancelable: true})
)
)
.map(passwordInput => ({
// @ts-expect-error - getOuterHTMLSnippet put into scope via stringification
snippet: getOuterHTMLSnippet(passwordInput),
}));
.map(passwordInput => (
// @ts-expect-error - getNodeDetails put into scope via stringification
getNodeDetails(passwordInput)
));
}

class PasswordInputsWithPreventedPaste extends Gatherer {
Expand All @@ -34,10 +34,12 @@ class PasswordInputsWithPreventedPaste extends Gatherer {
* @return {Promise<LH.Artifacts['PasswordInputsWithPreventedPaste']>}
*/
afterPass(passContext) {
return passContext.driver.evaluateAsync(`(() => {
${pageFunctions.getOuterHTMLSnippetString};
const expression = `(() => {
${pageFunctions.getNodeDetailsString};
return (${findPasswordInputsWithPreventedPaste.toString()}());
})()`);
})()`;

return passContext.driver.evaluateAsync(expression);
}
}

Expand Down
23 changes: 9 additions & 14 deletions lighthouse-core/gather/gatherers/form-elements.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
*/
'use strict';

/* global getNodeDetails */

const Gatherer = require('./gatherer.js');
const pageFunctions = require('../../lib/page-functions.js');

Expand Down Expand Up @@ -37,10 +39,8 @@ function collectFormElements() {
id: parentFormElement.id,
name: parentFormElement.name,
autocomplete: parentFormElement.autocomplete,
// @ts-expect-error - put into scope via stringification
nodeLabel: getNodeLabel(parentFormElement), // eslint-disable-line no-undef,
// @ts-expect-error - put into scope via stringification
snippet: getOuterHTMLSnippet(parentFormElement), // eslint-disable-line no-undef
// @ts-expect-error - getNodeDetails put into scope via stringification
...getNodeDetails(parentFormElement),
},
inputs: [],
labels: [],
Expand All @@ -60,19 +60,15 @@ function collectFormElements() {
attribute: child.getAttribute('autocomplete'),
prediction: child.getAttribute('autofill-prediction'),
},
// @ts-expect-error - put into scope via stringification
nodeLabel: getNodeLabel(child), // eslint-disable-line no-undef,
// @ts-expect-error - put into scope via stringification
snippet: getOuterHTMLSnippet(child), // eslint-disable-line no-undef
// @ts-expect-error - getNodeDetails put into scope via stringification
...getNodeDetails(child),
});
}
if (child instanceof HTMLLabelElement) {
formObj.labels.push({
for: child.htmlFor,
// @ts-expect-error - put into scope via stringification
nodeLabel: getNodeLabel(child), // eslint-disable-line no-undef,
// @ts-expect-error - put into scope via stringification
snippet: getOuterHTMLSnippet(child), // eslint-disable-line no-undef
// @ts-expect-error - getNodeDetails put into scope via stringification
...getNodeDetails(child),
});
}
}
Expand All @@ -96,8 +92,7 @@ class FormElements extends Gatherer {

const expression = `(() => {
${pageFunctions.getElementsInDocumentString};
${pageFunctions.getOuterHTMLSnippetString};
${pageFunctions.getNodeLabelString};
${pageFunctions.getNodeDetailsString};
return (${collectFormElements})();
})()`;

Expand Down
6 changes: 5 additions & 1 deletion lighthouse-core/gather/gatherers/iframe-elements.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
*/
'use strict';

/* global getNodeDetails */

const Gatherer = require('./gatherer.js');
const pageFunctions = require('../../lib/page-functions.js');

Expand All @@ -26,6 +28,8 @@ function collectIFrameElements() {
clientRect: {top, bottom, left, right, width, height},
// @ts-expect-error - put into scope via stringification
isPositionFixed: isPositionFixed(node), // eslint-disable-line no-undef
// @ts-expect-error - getNodeDetails put into scope via stringification
...getNodeDetails(node),
};
});
}
Expand All @@ -40,9 +44,9 @@ class IFrameElements extends Gatherer {
const driver = passContext.driver;

const expression = `(() => {
${pageFunctions.getOuterHTMLSnippetString};
${pageFunctions.getElementsInDocumentString};
${pageFunctions.isPositionFixedString};
${pageFunctions.getNodeDetailsString};
return (${collectIFrameElements})();
})()`;

Expand Down
28 changes: 7 additions & 21 deletions lighthouse-core/gather/gatherers/image-elements.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ const pageFunctions = require('../../lib/page-functions.js');
const Driver = require('../driver.js'); // eslint-disable-line no-unused-vars
const FontSize = require('./seo/font-size.js');

/* global window, getElementsInDocument, Image, getNodePath, getNodeSelector, getNodeLabel, getOuterHTMLSnippet, ShadowRoot */
/* global window, getElementsInDocument, Image, getNodeDetails, ShadowRoot */


/** @param {Element} element */
Expand Down Expand Up @@ -86,14 +86,8 @@ function getHTMLImages(allElements) {
isInShadowDOM: element.getRootNode() instanceof ShadowRoot,
// https://html.spec.whatwg.org/multipage/images.html#pixel-density-descriptor
usesSrcSetDensityDescriptor: / \d+(\.\d+)?x/.test(element.srcset),
// @ts-ignore - getNodePath put into scope via stringification
devtoolsNodePath: getNodePath(element),
// @ts-ignore - put into scope via stringification
selector: getNodeSelector(element),
// @ts-ignore - put into scope via stringification
nodeLabel: getNodeLabel(element),
// @ts-ignore - put into scope via stringification
snippet: getOuterHTMLSnippet(element),
// @ts-expect-error - getNodeDetails put into scope via stringification
...getNodeDetails(element),
};
});
}
Expand Down Expand Up @@ -143,14 +137,8 @@ function getCSSImages(allElements) {
),
usesSrcSetDensityDescriptor: false,
resourceSize: 0, // this will get overwritten below
// @ts-ignore - getNodePath put into scope via stringification
devtoolsNodePath: getNodePath(element),
// @ts-ignore - put into scope via stringification
selector: getNodeSelector(element),
// @ts-ignore - put into scope via stringification
nodeLabel: getNodeLabel(element),
// @ts-ignore - put into scope via stringification
snippet: getOuterHTMLSnippet(element),
// @ts-expect-error - getNodeDetails put into scope via stringification
...getNodeDetails(element),
});
}

Expand Down Expand Up @@ -312,10 +300,8 @@ class ImageElements extends Gatherer {

const expression = `(function() {
${pageFunctions.getElementsInDocumentString}; // define function on page
${pageFunctions.getNodePathString};
${pageFunctions.getNodeSelectorString};
${pageFunctions.getNodeLabelString};
${pageFunctions.getOuterHTMLSnippetString};
${pageFunctions.getBoundingClientRectString};
${pageFunctions.getNodeDetailsString};
${getClientRect.toString()};
${getPosition.toString()};
${getHTMLImages.toString()};
Expand Down
Loading