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 creation of NodeValue #11877

Merged
merged 53 commits into from
Jan 12, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
53 commits
Select commit Hold shift + click to select a range
90018b2
core(driver): create page code using structured interface
connorjclark May 20, 2020
f7e38f5
rename type
connorjclark May 20, 2020
78f386a
pr feedback
connorjclark May 20, 2020
32856e7
no strings
connorjclark May 20, 2020
e2dd63d
test
connorjclark May 20, 2020
f38bf41
Merge remote-tracking branch 'origin/master' into driver-eval-enhanced
connorjclark Jun 4, 2020
ce9839b
test
connorjclark Jun 4, 2020
4c903de
restrucutre
connorjclark Jun 5, 2020
0413a4a
Merge remote-tracking branch 'origin/master' into driver-eval-enhanced
connorjclark Jun 12, 2020
d4bd2bf
Merge remote-tracking branch 'origin/master' into driver-eval-enhanced
connorjclark Jun 17, 2020
02dc215
remove obj
connorjclark Jun 17, 2020
ce2827a
Merge remote-tracking branch 'origin/master' into driver-eval-enhanced
connorjclark Jul 15, 2020
c81a636
update master
connorjclark Nov 5, 2020
70d8210
rm dead code
connorjclark Nov 5, 2020
df5e8dd
Merge remote-tracking branch 'origin/master' into driver-eval-enhanced
connorjclark Nov 10, 2020
4e59476
adam feedback
connorjclark Nov 10, 2020
147b98c
fix tests
connorjclark Nov 10, 2020
a4051a1
fix mangle issues
connorjclark Nov 10, 2020
9eea206
wip
connorjclark Nov 10, 2020
d79b23f
Merge remote-tracking branch 'origin/master' into driver-eval-enhanced-2
connorjclark Dec 2, 2020
b8c76ed
Merge remote-tracking branch 'origin/master' into driver-eval-enhanced-2
connorjclark Dec 2, 2020
2088109
fix
connorjclark Dec 2, 2020
7b01e8c
wip
connorjclark Dec 2, 2020
4cc1eea
fix
connorjclark Dec 2, 2020
aae0a9f
Merge remote-tracking branch 'origin/master' into driver-eval-enhanced-2
connorjclark Dec 2, 2020
77af88d
fix tests
connorjclark Dec 3, 2020
d35fd5e
fix
connorjclark Dec 3, 2020
eb897cf
tmp
connorjclark Dec 21, 2020
9c10cda
Merge remote-tracking branch 'origin/master' into driver-eval-enhanced-2
connorjclark Dec 21, 2020
d643baf
update
connorjclark Dec 21, 2020
9638b90
master
connorjclark Dec 21, 2020
c526772
fixtest
connorjclark Dec 21, 2020
b893bb1
fix
connorjclark Dec 22, 2020
606db62
oops
connorjclark Dec 22, 2020
8c98d05
pr
connorjclark Dec 22, 2020
ed13e97
Merge branch 'driver-eval-enhanced-2' into node-details-yay
connorjclark Dec 22, 2020
3195324
update
connorjclark Dec 22, 2020
af84552
Merge remote-tracking branch 'origin/master' into node-details-yay
connorjclark Dec 22, 2020
f799583
update
connorjclark Dec 22, 2020
f2b649b
Merge remote-tracking branch 'origin/master' into node-details-yay
connorjclark Dec 22, 2020
d850391
more
connorjclark Dec 22, 2020
7b7aeb3
deps
connorjclark Dec 23, 2020
2e37d0b
Merge remote-tracking branch 'origin/master' into node-details-yay
connorjclark Dec 23, 2020
685d356
fix tests
connorjclark Dec 23, 2020
a04e261
test
connorjclark Dec 23, 2020
098411b
rename
connorjclark Dec 23, 2020
cc8cdb3
no meta els
connorjclark Dec 23, 2020
f763a1f
tweak
connorjclark Dec 23, 2020
25210c5
revert
connorjclark Dec 23, 2020
ba30ab5
update
connorjclark Jan 11, 2021
0679ff5
Apply suggestions from code review
connorjclark Jan 11, 2021
68ee66f
kiss
connorjclark Jan 11, 2021
d7c562c
Merge branch 'node-details-yay' of github.com:GoogleChrome/lighthouse…
connorjclark Jan 11, 2021
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
8 changes: 1 addition & 7 deletions lighthouse-core/audits/accessibility/axe-audit.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,14 +76,8 @@ class AxeAudit extends Audit {
if (rule && rule.nodes) {
items = rule.nodes.map(axeNode => ({
node: {
type: /** @type {'node'} */ ('node'),
lhId: axeNode.node.lhId,
selector: axeNode.node.selector,
path: axeNode.node.devtoolsNodePath,
snippet: axeNode.node.snippet,
boundingRect: axeNode.node.boundingRect,
...Audit.makeNodeItem(axeNode.node),
explanation: axeNode.failureSummary,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why do we add explanation here?

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't look like this gets included in the report 🤔 . Is it supposed to?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only purpose I can see is in the smoke tests.

Copy link
Member

Choose a reason for hiding this comment

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

Why do we add explanation here?

#5402

nodeLabel: axeNode.node.nodeLabel,
},
}));
}
Expand Down
16 changes: 16 additions & 0 deletions lighthouse-core/audits/audit.js
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,22 @@ class Audit {
};
}

/**
* @param {LH.Artifacts.NodeDetails} node
* @return {LH.Audit.Details.NodeValue}
*/
static makeNodeItem(node) {
return {
type: 'node',
lhId: node.lhId,
path: node.devtoolsNodePath,
selector: node.selector,
boundingRect: node.boundingRect,
snippet: node.snippet,
nodeLabel: node.nodeLabel,
};
}

/**
* @param {number|null} score
* @param {LH.Audit.ScoreDisplayMode} scoreDisplayMode
Expand Down
6 changes: 1 addition & 5 deletions lighthouse-core/audits/autocomplete.js
Original file line number Diff line number Diff line change
Expand Up @@ -260,11 +260,7 @@ class AutocompleteAudit extends Audit {
continue;
}
failingFormsData.push({
node: {
type: /** @type {'node'} */ ('node'),
snippet: input.node.snippet,
nodeLabel: input.node.nodeLabel,
},
node: Audit.makeNodeItem(input.node),
suggestion: suggestion,
current: input.autocomplete.attribute,
});
Expand Down
16 changes: 2 additions & 14 deletions lighthouse-core/audits/dobetterweb/dom-size.js
Original file line number Diff line number Diff line change
Expand Up @@ -98,24 +98,12 @@ class DOMSize extends Audit {
value: stats.totalBodyElements,
},
{
node: {
type: /** @type {'node'} */ ('node'),
path: stats.depth.devtoolsNodePath,
snippet: stats.depth.snippet,
selector: stats.depth.selector,
nodeLabel: stats.depth.nodeLabel,
},
node: Audit.makeNodeItem(stats.depth),
statistic: str_(UIStrings.statisticDOMDepth),
value: stats.depth.max,
},
{
node: {
type: /** @type {'node'} */ ('node'),
path: stats.width.devtoolsNodePath,
snippet: stats.width.snippet,
selector: stats.width.selector,
nodeLabel: stats.width.nodeLabel,
},
node: Audit.makeNodeItem(stats.width),
statistic: str_(UIStrings.statisticDOMWidth),
value: stats.width.max,
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,13 +69,7 @@ class ExternalAnchorsUseRelNoopenerAudit extends Audit {
})
.map(anchor => {
return {
node: {
type: /** @type {'node'} */ ('node'),
path: anchor.node.devtoolsNodePath || '',
selector: anchor.node.selector || '',
nodeLabel: anchor.node.nodeLabel || '',
snippet: anchor.node.snippet || '',
},
node: Audit.makeNodeItem(anchor.node),
href: anchor.href || 'Unknown',
target: anchor.target || '',
rel: anchor.rel || '',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,10 @@ class PasswordInputsCanBePastedIntoAudit extends Audit {

/** @type {LH.Audit.Details.Table['items']} */
const items = [];

passwordInputsWithPreventedPaste.forEach(input => {
items.push({
node: {
type: /** @type {'node'} */ ('node'),
snippet: input.node.snippet,
path: input.node.devtoolsNodePath,
},
node: Audit.makeNodeItem(input.node),
});
});

Expand Down
10 changes: 1 addition & 9 deletions lighthouse-core/audits/largest-contentful-paint-element.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,15 +42,7 @@ class LargestContentfulPaintElement extends Audit {
const lcpElementDetails = [];
if (lcpElement) {
lcpElementDetails.push({
node: {
type: /** @type {'node'} */ ('node'),
lhId: lcpElement.node.lhId,
path: lcpElement.node.devtoolsNodePath,
selector: lcpElement.node.selector,
nodeLabel: lcpElement.node.nodeLabel,
snippet: lcpElement.node.snippet,
boundingRect: lcpElement.node.boundingRect,
},
node: Audit.makeNodeItem(lcpElement.node),
});
}

Expand Down
10 changes: 1 addition & 9 deletions lighthouse-core/audits/layout-shift-elements.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,15 +43,7 @@ class LayoutShiftElements extends Audit {

const clsElementData = clsElements.map(element => {
return {
node: {
type: /** @type {'node'} */ ('node'),
lhId: element.node.lhId,
path: element.node.devtoolsNodePath,
selector: element.node.selector,
nodeLabel: element.node.nodeLabel,
snippet: element.node.snippet,
boundingRect: element.node.boundingRect,
},
node: Audit.makeNodeItem(element.node),
score: element.score,
};
});
Expand Down
12 changes: 2 additions & 10 deletions lighthouse-core/audits/non-composited-animations.js
Original file line number Diff line number Diff line change
Expand Up @@ -129,19 +129,11 @@ class NonCompositedAnimations extends Audit {
};
}

/** @type LH.Audit.Details.TableItem[] */
/** @type {LH.Audit.Details.TableItem[]} */
const results = [];
let shouldAddAnimationNameColumn = false;
artifacts.TraceElements.forEach(element => {
if (element.traceEventType !== 'animation') return;
/** @type LH.Audit.Details.NodeValue */
const node = {
type: 'node',
path: element.node.devtoolsNodePath,
selector: element.node.selector,
nodeLabel: element.node.nodeLabel,
snippet: element.node.snippet,
};

const animations = element.animations || [];
const animationReasons = new Map();
Expand Down Expand Up @@ -171,7 +163,7 @@ class NonCompositedAnimations extends Audit {
}

results.push({
node,
node: Audit.makeNodeItem(element.node),
subItems: {
type: 'subitems',
items: allFailureReasons,
Expand Down
8 changes: 1 addition & 7 deletions lighthouse-core/audits/seo/crawlable-anchors.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,13 +81,7 @@ class CrawlableAnchors extends Audit {
/** @type {LH.Audit.Details.Table['items']} */
const itemsToDisplay = failingAnchors.map(anchor => {
return {
node: {
type: 'node',
path: anchor.node.devtoolsNodePath || '',
selector: anchor.node.selector || '',
nodeLabel: anchor.node.nodeLabel || '',
snippet: anchor.node.snippet || '',
},
node: Audit.makeNodeItem(anchor.node),
};
});

Expand Down
20 changes: 1 addition & 19 deletions lighthouse-core/audits/seo/plugins.js
Original file line number Diff line number Diff line change
Expand Up @@ -129,26 +129,8 @@ class Plugins extends Audit {
return failingParams.length > 0;
})
.map(plugin => {
const tagName = plugin.tagName.toLowerCase();
/** @type {Array<keyof LH.Artifacts.EmbeddedContentInfo>} */
const attributeKeys = ['src', 'data', 'code', 'type'];
const attributes = attributeKeys
.reduce((result, attr) => {
if (plugin[attr] !== null) {
result += ` ${attr}="${plugin[attr]}"`;
}
return result;
}, '');
const params = plugin.params
.filter(param => SOURCE_PARAMS.has(param.name.trim().toLowerCase()))
.map(param => `<param ${param.name}="${param.value}" />`)
.join('');

return {
source: {
type: /** @type {'node'} */ ('node'),
snippet: `<${tagName}${attributes}>${params}</${tagName}>`,
},
source: Audit.makeNodeItem(plugin.node),
};
});

Expand Down
7 changes: 1 addition & 6 deletions lighthouse-core/audits/seo/tap-targets.js
Original file line number Diff line number Diff line change
Expand Up @@ -248,13 +248,8 @@ function targetToTableNode(target) {
const boundingRect = getBoundingRect(target.clientRects);

return {
type: 'node',
lhId: target.node.lhId,
snippet: target.node.snippet,
path: target.node.devtoolsNodePath,
selector: target.node.selector,
...Audit.makeNodeItem(target.node),
boundingRect,
Copy link
Member

Choose a reason for hiding this comment

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

Which boundingRect should be used here? Using the boundingRect of all clientRects goes back to the first commit of #10716, but getNodeDetails also didn't exist back then, so that might have just been the most convenient way to do it at the time. Should it be the just node.boundingRect? When can the two differ? getBoundingClientRect should normally contain all child rects, but does it ever differ (transforms or something?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll followup with this in a different PR.

nodeLabel: target.node.nodeLabel,
};
}

Expand Down
9 changes: 2 additions & 7 deletions lighthouse-core/audits/unsized-images.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,15 +96,10 @@ class UnsizedImages extends Audit {

if (isFixedImage || UnsizedImages.isSizedImage(image)) continue;
const url = URL.elideDataURI(image.src);

unsizedImages.push({
url,
node: {
type: /** @type {'node'} */ ('node'),
path: image.node.devtoolsNodePath,
selector: image.node.selector,
nodeLabel: image.node.nodeLabel,
snippet: image.node.snippet,
},
node: Audit.makeNodeItem(image.node),
});
}

Expand Down
22 changes: 17 additions & 5 deletions lighthouse-core/gather/gatherers/seo/embedded-content.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,24 @@
*/
'use strict';

/* globals getElementsInDocument */
/* globals getElementsInDocument getNodeDetails */

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

/**
* @return {LH.Artifacts.EmbeddedContentInfo[]}
*/
function getEmbeddedContent() {
const functions = /** @type {typeof pageFunctions} */ ({
// @ts-expect-error - getElementsInDocument put into scope via stringification
getElementsInDocument,
// @ts-expect-error - getNodeDetails put into scope via stringification
getNodeDetails,
});

const selector = 'object, embed, applet';
/** @type {HTMLElement[]} */
// @ts-expect-error - getElementsInDocument put into scope via stringification
const elements = getElementsInDocument(selector);
const elements = functions.getElementsInDocument(selector);
return elements
.map(node => ({
tagName: node.tagName,
Expand All @@ -28,6 +36,7 @@ function getEmbeddedContent() {
name: el.getAttribute('name') || '',
value: el.getAttribute('value') || '',
})),
node: functions.getNodeDetails(node),
}));
}

Expand All @@ -39,7 +48,10 @@ class EmbeddedContent extends Gatherer {
afterPass(passContext) {
return passContext.driver.evaluate(getEmbeddedContent, {
args: [],
deps: [pageFunctions.getElementsInDocument],
deps: [
pageFunctions.getElementsInDocument,
pageFunctions.getNodeDetailsString,
],
});
}
}
Expand Down
9 changes: 5 additions & 4 deletions lighthouse-core/lib/page-functions.js
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ function isPositionFixed(element) {
* strings like the innerText or alt attribute.
* Falls back to the tagName if no useful label is found.
* @param {Element} node
* @return {string|null}
* @return {string}
*/
/* istanbul ignore next */
function getNodeLabel(node) {
Expand Down Expand Up @@ -461,8 +461,9 @@ function wrapRequestIdleCallback(cpuSlowdownMultiplier) {

/**
* @param {HTMLElement} element
* @return {LH.Artifacts.NodeDetails}
*/
function getNodeDetailsImpl(element) {
function getNodeDetails(element) {
// This bookkeeping is for the FullPageScreenshot gatherer.
if (!window.__lighthouseNodesDontTouchOrAllVarianceGoesAway) {
window.__lighthouseNodesDontTouchOrAllVarianceGoesAway = new Map();
Expand Down Expand Up @@ -507,8 +508,7 @@ const getNodeDetailsString = `function getNodeDetails(element) {
${getBoundingClientRect.toString()};
${getOuterHTMLSnippet.toString()};
${getNodeLabel.toString()};
${getNodeDetailsImpl.toString()};
return getNodeDetailsImpl(element);
return (${getNodeDetails.toString()})(element);
}`;

module.exports = {
Expand All @@ -522,6 +522,7 @@ module.exports = {
computeBenchmarkIndex: computeBenchmarkIndex,
computeBenchmarkIndexString: computeBenchmarkIndex.toString(),
getNodeDetailsString,
getNodeDetails,
getNodePathString: getNodePath.toString(),
getNodeSelectorString: getNodeSelector.toString(),
getNodePath,
Expand Down
Loading