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.

Sign up for GitHub

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

Jump to bottom

core: add devtools path to DOMStats #11578

Merged
merged 12 commits into from
Nov 9, 2020
Merged

core: add devtools path to DOMStats #11578

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
938ec86
domstats gatherer: using node path instead of custom
adrianaixba Oct 19, 2020
8850d72
small update
adrianaixba Oct 19, 2020
23ccc4b
small update
adrianaixba Oct 19, 2020
e8c87ee
small update
adrianaixba Oct 19, 2020
de5bd61
removed table element remnants
adrianaixba Oct 22, 2020
7071ceb
Merge branch 'domstats-devtools' of https://github.com/GoogleChrome/l…
adrianaixba Oct 22, 2020
e1e9d98
reimplemented getNodeDetails for shadowRoot case, adjusted smoke test…
adrianaixba Oct 22, 2020
ce2fdde
cleanup artifacts & domstats
adrianaixba Oct 22, 2020
002fc49
cleanup
adrianaixba Oct 22, 2020
16221a9
moved shadowroot logic
adrianaixba Oct 23, 2020
a7b8fa4
cleanup
adrianaixba Oct 23, 2020
cf7a4bf
cleanups
adrianaixba Nov 9, 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
2 changes: 1 addition & 1 deletion lighthouse-cli/test/smokehouse/test-definitions/dobetterweb/dbw-expectations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,7 +406,7 @@ const expectations = [
{
statistic: 'Maximum Child Elements',
value: 100,
element: {value: '<div id="shadow-root-container">'},
node: {snippet: '<div id="shadow-root-container">'},
},
],
},
Expand Down
25 changes: 15 additions & 10 deletions lighthouse-core/audits/dobetterweb/dom-size.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,31 +87,36 @@ class DOMSize extends Audit {
/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
{key: 'statistic', itemType: 'text', text: str_(UIStrings.columnStatistic)},
{key: 'element', itemType: 'code', text: str_(i18n.UIStrings.columnElement)},
{key: 'node', itemType: 'node', text: str_(i18n.UIStrings.columnElement)},
{key: 'value', itemType: 'numeric', text: str_(UIStrings.columnValue)},
];

/** @type {LH.Audit.Details.Table['items']} */
const items = [
{
statistic: str_(UIStrings.statisticDOMElements),
element: '',
value: stats.totalBodyElements,
},
{
node: /** @type {LH.Audit.Details.NodeValue} */ ({
type: 'node',
path: stats.depth.devtoolsNodePath,
snippet: stats.depth.snippet,
adamraine marked this conversation as resolved.
Show resolved Hide resolved
selector: stats.depth.selector,
nodeLabel: stats.depth.nodeLabel,
}),
statistic: str_(UIStrings.statisticDOMDepth),
element: {
type: 'code',
value: stats.depth.snippet,
},
value: stats.depth.max,
},
{
node: /** @type {LH.Audit.Details.NodeValue} */ ({
type: 'node',
path: stats.width.devtoolsNodePath,
snippet: stats.width.snippet,
selector: stats.width.selector,
nodeLabel: stats.width.nodeLabel,
}),
statistic: str_(UIStrings.statisticDOMWidth),
element: {
type: 'code',
value: stats.width.snippet,
},
value: stats.width.max,
},
];
Expand Down
73 changes: 4 additions & 69 deletions lighthouse-core/gather/gatherers/dobetterweb/domstats.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,73 +9,13 @@
* and total number of elements used on the page.
*/

/* global ShadowRoot, getOuterHTMLSnippet */
/* global getNodeDetails */

'use strict';

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

/**
* Constructs a pretty label from element's selectors. For example, given
* <div id="myid" class="myclass">, returns 'div#myid.myclass'.
* @param {Element} element
* @return {string}
*/
/* istanbul ignore next */
function createSelectorsLabel(element) {
let name = element.localName || '';
const idAttr = element.getAttribute && element.getAttribute('id');
if (idAttr) {
name += `#${idAttr}`;
}
// svg elements return SVGAnimatedString for .className, which is an object.
// Stringify classList instead.
if (element.classList) {
const className = element.classList.toString();
if (className) {
name += `.${className.trim().replace(/\s+/g, '.')}`;
}
} else if (ShadowRoot.prototype.isPrototypeOf(element)) {
name += '#shadow-root';
}

return name;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

love it

/**
* @param {Node} element
* @return {Array<string>}
*/
/* istanbul ignore next */
function elementPathInDOM(element) {
const visited = new Set();
const path = [createSelectorsLabel(element)];
let node = element;
while (node) {
visited.add(node);

// Anchor elements have a .host property. Be sure we've found a shadow root
// host and not an anchor.
if (ShadowRoot.prototype.isPrototypeOf(node)) {
const isShadowHost = node.host && node.localName !== 'a';
node = isShadowHost ? node.host : node.parentElement;
} else {
const isShadowHost = node.parentNode && node.parentNode.host &&
node.parentNode.localName !== 'a';
node = isShadowHost ? node.parentNode.host : node.parentElement;
}

if (visited.has(node)) {
node = null;
}

if (node) {
path.unshift(createSelectorsLabel(node));
}
}
return path;
}

/**
* Calculates the maximum tree depth of the DOM.
Expand Down Expand Up @@ -124,14 +64,11 @@ function getDOMStats(element, deep = true) {
return {
depth: {
max: result.maxDepth,
pathToElement: elementPathInDOM(deepestElement),
// ignore style since it will provide no additional context, and is often long
snippet: getOuterHTMLSnippet(deepestElement, ['style']),
...getNodeDetails(deepestElement),
},
width: {
max: result.maxWidth,
pathToElement: elementPathInDOM(parentWithMostChildren),
snippet: getOuterHTMLSnippet(parentWithMostChildren, ['style']),
...getNodeDetails(parentWithMostChildren),
},
totalBodyElements: result.numElements,
};
Expand All @@ -146,9 +83,7 @@ class DOMStats extends Gatherer {
const driver = passContext.driver;

const expression = `(function() {
${pageFunctions.getOuterHTMLSnippetString};
${createSelectorsLabel.toString()};
${elementPathInDOM.toString()};
${pageFunctions.getNodeDetailsString};
return (${getDOMStats.toString()}(document.body));
})()`;
await driver.sendCommand('DOM.enable');
Expand Down
8 changes: 4 additions & 4 deletions lighthouse-core/lib/page-functions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,6 @@ function getNodeLabel(node) {
}
return str.slice(0, maxLength - 1) + '…';
}

const tagName = node.tagName.toLowerCase();
// html and body content is too broad to be useful, since they contain all page content
if (tagName !== 'html' && tagName !== 'body') {
Expand Down Expand Up @@ -453,12 +452,13 @@ const getNodeDetailsString = `function getNodeDetails(elem) {
${getBoundingClientRect.toString()};
${getOuterHTMLSnippet.toString()};
${getNodeLabel.toString()};
const htmlElem = elem instanceof ShadowRoot ? elem.host : elem;
return {
devtoolsNodePath: getNodePath(elem),
selector: getNodeSelector(elem),
boundingRect: getBoundingClientRect(elem),
selector: getNodeSelector(htmlElem),
boundingRect: getBoundingClientRect(htmlElem),
snippet: getOuterHTMLSnippet(elem),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

decided to leave these as is, since they take care of the shadowRoot case. @adamraine @patrickhulce what do y'all think?

Copy link
Member

@adamraine adamraine Oct 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

getNodePath will return different results for ShadowRoot and its host element, so leaving that as is was a good call.

@patrickhulce Would you prefer keeping the ShadowRoot logic in getOuterHTMLSnippet even though it won't be used anywhere after this patch?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree, I wouldn't go out of our way to remove ShadowRoot support from anything just yet, even if the path is currently being covered by our other logic. If we also removed the functions from the public interface of this module, then I'd feel less strongly, but seems like that step would benefit from a more targeted and comprehensive plan for what to do with shadow elements across all of lighthouse that feels out of scope for this PR.

nodeLabel: getNodeLabel(elem),
nodeLabel: getNodeLabel(htmlElem),
};
}`;

Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/test/audits/dobetterweb/dom-size-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ describe('DOMSize audit', () => {
const artifact = {
DOMStats: {
totalBodyElements: numElements,
depth: {max: 1, pathToElement: ['html', 'body', 'div', 'span']},
width: {max: 2, pathToElement: ['html', 'body']},
depth: {max: 1},
width: {max: 2},
},
};
const context = {options, settings: {locale: 'en'}};
Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/test/lib/i18n/swap-locale-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ describe('swap-locale', () => {
expect(lhrDe.audits.plugins.title).toEqual('Dokument verwendet keine Plug-ins');

// With ICU string argument values
expect(lhrEn.audits['dom-size'].displayValue).toEqual('31 elements');
expect(lhrDe.audits['dom-size'].displayValue).toEqual('31 Elemente');
expect(lhrEn.audits['dom-size'].displayValue).toEqual('148 elements');
expect(lhrDe.audits['dom-size'].displayValue).toEqual('148 Elemente');

// Renderer formatted strings
expect(lhrEn.i18n.rendererFormattedStrings.labDataTitle).toEqual('Lab Data');
Expand Down
42 changes: 27 additions & 15 deletions lighthouse-core/test/results/artifacts/artifacts.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2395,24 +2395,36 @@
"AppCacheManifest": "clock.appcache",
"DOMStats": {
"depth": {
"max": 3,
"pathToElement": [
"html",
"body",
"div",
"h2"
],
"snippet": "<h2>"
"max": 4,
"devtoolsNodePath": "3,HTML,1,BODY,7,DIV,3,svg,0,title",
"selector": "body > div > svg.social-facebook > title#social-facebook-5",
"boundingRect": {
"top": 0,
"bottom": 0,
"left": 0,
"right": 0,
"width": 0,
"height": 0
},
"snippet": "<title id=\"social-facebook-5\">",
"nodeLabel": "title"
},
"width": {
"max": 29,
"pathToElement": [
"html",
"body"
],
"snippet": "<body>"
"max": 100,
"devtoolsNodePath": "3,HTML,1,BODY,4,DIV,a,#document-fragment",
"selector": "body > div#shadow-root-container",
"boundingRect": {
"top": 316,
"bottom": 316,
"left": 8,
"right": 352,
"width": 344,
"height": 0
},
"snippet": "<div id=\"shadow-root-container\">",
"nodeLabel": "div"
},
"totalBodyElements": 31
"totalBodyElements": 148
},
"Stacks": [
{
Expand Down
6 changes: 2 additions & 4 deletions lighthouse-core/test/results/artifacts/defaultPass.trace.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14505,10 +14505,8 @@
{"pid":75994,"tid":17667,"ts":185608246435,"ph":"X","cat":"toplevel","name":"MessageLoop::RunTask","args":{"src_file":"../../ipc/ipc_mojo_bootstrap.cc","src_func":"SendMessage"},"dur":31,"tdur":30,"tts":78713},
{"pid":75994,"tid":17667,"ts":185608246474,"ph":"X","cat":"toplevel","name":"MessageLoop::RunTask","args":{"src_file":"../../ipc/ipc_mojo_bootstrap.cc","src_func":"SendMessage"},"dur":16,"tdur":16,"tts":78752},
{"pid":75994,"tid":17667,"ts":185608247189,"ph":"X","cat":"toplevel","name":"MessageLoop::RunTask","args":{"src_file":"../../ipc/ipc_mojo_bootstrap.cc","src_func":"SendMessage"},"dur":30,"tdur":29,"tts":78797},

{"_comment": "Manually added event to make sample lhr not error", "name":"largestContentfulPaint::Candidate", "pid":75994,"tid":17667,"ts":185608247190,"ph":"R","cat":"loading,rail,devtools.timeline", "args": {"frame": "0x44d2861df8"}},
{"_comment": "Manually added event to make test CLS", "name":"LayoutShift", "pid":75994,"tid":775,"ts":185608247190,"ph":"R","cat":"loading,rail,devtools.timeline", "args": {"frame": "0x44d2861df8", "data": {"is_main_frame": true, "cumulative_score": 0.42}}},

Copy link
Member

Choose a reason for hiding this comment

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

This seems unrelated. I don't mind the style change, but is it related to this patch somehow?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@adamraine This is unrelated. It's just a style change that has continued to pop up for me for some reason, I could revert.

Copy link
Member

Choose a reason for hiding this comment

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

I think the change is good, its probably too small for its own PR so I'm ok leaving it in this one :)

{"_comment":"Manually added event to make sample lhr not error","name":"largestContentfulPaint::Candidate","pid":75994,"tid":17667,"ts":185608247190,"ph":"R","cat":"loading,rail,devtools.timeline","args":{"frame":"0x44d2861df8"}},
{"_comment":"Manually added event to make test CLS","name":"LayoutShift","pid":75994,"tid":775,"ts":185608247190,"ph":"R","cat":"loading,rail,devtools.timeline","args":{"frame":"0x44d2861df8","data":{"is_main_frame":true,"cumulative_score":0.42}}},
{"pid":75994,"tid":17667,"ts":185608250604,"ph":"X","cat":"toplevel","name":"MessagePumpLibevent::OnLibeventNotification","args":{"src_file":"../../mojo/edk/system/channel_posix.cc","src_func":"StartOnIOThread"},"dur":29,"tdur":30,"tts":78849},
{"pid":75994,"tid":17667,"ts":185608250671,"ph":"X","cat":"toplevel","name":"MessagePumpLibevent::OnLibeventNotification","args":{"src_file":"../../mojo/edk/system/channel_posix.cc","src_func":"StartOnIOThread"},"dur":25,"tdur":25,"tts":78903},
{"pid":75994,"tid":17667,"ts":185608251964,"ph":"X","cat":"toplevel","name":"MessageLoop::RunTask","args":{"src_file":"../../ipc/ipc_mojo_bootstrap.cc","src_func":"SendMessage"},"dur":29,"tdur":28,"tts":78957},
Expand Down
39 changes: 22 additions & 17 deletions lighthouse-core/test/results/sample_v2.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3600,9 +3600,9 @@
"description": "A large DOM will increase memory usage, cause longer [style calculations](https://developers.google.com/web/fundamentals/performance/rendering/reduce-the-scope-and-complexity-of-style-calculations), and produce costly [layout reflows](https://developers.google.com/speed/articles/reflow). [Learn more](https://web.dev/dom-size/).",
"score": 1,
"scoreDisplayMode": "numeric",
"numericValue": 31,
"numericValue": 148,
"numericUnit": "element",
"displayValue": "31 elements",
"displayValue": "148 elements",
"details": {
"type": "table",
"headings": [
Expand All @@ -3612,8 +3612,8 @@
"text": "Statistic"
},
{
"key": "element",
"itemType": "code",
"key": "node",
"itemType": "node",
"text": "Element"
},
{
Expand All @@ -3625,24 +3625,29 @@
"items": [
{
"statistic": "Total DOM Elements",
"element": "",
"value": 31
"value": 148
},
{
"statistic": "Maximum DOM Depth",
"element": {
"type": "code",
"value": "<h2>"
"node": {
"type": "node",
"path": "3,HTML,1,BODY,7,DIV,3,svg,0,title",
"snippet": "<title id=\"social-facebook-5\">",
"selector": "body > div > svg.social-facebook > title#social-facebook-5",
"nodeLabel": "title"
},
"value": 3
"statistic": "Maximum DOM Depth",
"value": 4
},
{
"statistic": "Maximum Child Elements",
"element": {
"type": "code",
"value": "<body>"
"node": {
"type": "node",
"path": "3,HTML,1,BODY,4,DIV,a,#document-fragment",
"snippet": "<div id=\"shadow-root-container\">",
"selector": "body > div#shadow-root-container",
"nodeLabel": "div"
},
"value": 29
"statistic": "Maximum Child Elements",
"value": 100
}
]
}
Expand Down Expand Up @@ -7671,7 +7676,7 @@
"lighthouse-core/audits/dobetterweb/dom-size.js | displayValue": [
{
"values": {
"itemCount": 31
"itemCount": 148
},
"path": "audits[dom-size].displayValue"
}
Expand Down
4 changes: 2 additions & 2 deletions types/artifacts.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,8 +205,8 @@ declare global {
export interface DOMStats {
/** The total number of elements found within the page's body. */
totalBodyElements: number;
width: {max: number, pathToElement: Array<string>, snippet: string};
depth: {max: number, pathToElement: Array<string>, snippet: string};
width: NodeDetails & {max: number;};
depth: NodeDetails & {max: number;};
}

export interface EmbeddedContentInfo {
Expand Down