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(dom-size): collect only body nodes for DOM stats #7241

Merged
merged 6 commits into from
Apr 10, 2019
Merged

core(dom-size): collect only body nodes for DOM stats #7241

merged 6 commits into from
Apr 10, 2019

Conversation

midzer
Copy link
Contributor

@midzer midzer commented Feb 13, 2019

Hi :)

Summary

Respect only nodes within <body> for dom-size calculation audit

This has been tagged as bug, but it appears it's more like an adjustment how calculation of DOM size is done

More accurate results whether there are too many DOM elements which might result in bad performance for layouting and rendering

Related Issues/PRs

#6806

Have a nice day
midzer

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

Thanks for jumping on this @midzer!

We'll probably want to add a smoketest for this one to make sure we're test that DOM stats function since we're changing it.

@@ -148,8 +148,8 @@ class DOMStats extends Gatherer {
})()`;
return passContext.driver.sendCommand('DOM.enable')
.then(() => passContext.driver.evaluateAsync(expression, {useIsolation: true}))
.then(results => passContext.driver.getElementsInDocument().then(allNodes => {
results.totalDOMNodes = allNodes.length;
.then(results => passContext.driver.getElementsInDocument('body').then(bodyNodes => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this will do what we want it to do. The implementation of getElementsInDocument looks like it doesn't accept a querySelector

/**
* Returns the flattened list of all DOM elements within the document.
* @param {boolean=} pierce Whether to pierce through shadow trees and iframes.
* True by default.
* @return {Promise<Array<Element>>} The found elements, or [], resolved in a promise
*/
getElementsInDocument(pierce = true) {
return this.getNodesInDocument(pierce)
.then(nodes => nodes
.filter(node => node.nodeType === 1)
.map(node => new Element({nodeId: node.nodeId}, this))
);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, I misleadingly thought I can pick getElementsInDocument() from those page-functions...

@@ -148,8 +148,8 @@ class DOMStats extends Gatherer {
})()`;
return passContext.driver.sendCommand('DOM.enable')
.then(() => passContext.driver.evaluateAsync(expression, {useIsolation: true}))
Copy link
Collaborator

Choose a reason for hiding this comment

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

we'll probably want to piggyback on our getDOMStats function to compute the total body nodes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, sounds reasonable to do calculation right there :)

Copy link
Member

@exterkamp exterkamp left a comment

Choose a reason for hiding this comment

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

I'm gonna +1 @patrickhulce and say these changes LGTM but I think there should be a smoke test that has bodyNodesCount != totalNodesCount so we can make sure this doesn't break.

@patrickhulce
Copy link
Collaborator

Just checking in here @midzer do you think you'd be able to make the smoketest change we're talking about? You'll basically add an assertion in the expectations file and run yarn smoke dbw to make sure it passes with your fix and fails without it :)

@midzer
Copy link
Contributor Author

midzer commented Mar 8, 2019

Right now I'm kinda stuck with lighthouse due #7246
Yeah, I can try to finish those tests anyway @patrickhulce

@patrickhulce
Copy link
Collaborator

Oh bummer :( I wonder if there's another solution. Could downloading Chromium straight from https://download-chromium.appspot.com/ make any difference?

@midzer
Copy link
Contributor Author

midzer commented Mar 8, 2019

Thanks for your support @patrickhulce
Meanwhile I've installed regular Google Chrome and can run LH properly via node lighthouse-cli http://example.com --view --chrome-flags="--no-sandbox --headless --disable-gpu"

Running yarn smoke dbw fails though:

Unable to connect to Chrome

even if I add those chrome-flags in runSmokehouse() in run-smoke.js

@brendankenny
Copy link
Member

ok, since this is a breaking change in the artifacts, I wanted to get this in for 5.0 :) I added an entry in the expectations file for dom-size as requested.

I also realized we're really counting elements, not nodes, so I changed all names to reflect that fact.

@exterkamp, @patrickhulce are you still good with this with the extra smoke test?

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

LGTM other than that!

failureTitle: 'Avoid an excessive DOM size',
/** Description of a Lighthouse audit that tells the user *why* they should reduce the size of the web page's DOM. The size of a DOM is characterized by the total number of DOM nodes and greatest DOM depth. This is displayed after a user expands the section to see more. No character length limits. 'Learn More' becomes link text to additional documentation. */
description: 'Browser engineers recommend pages contain fewer than ' +
`~${MAX_DOM_NODES.toLocaleString()} DOM nodes. The sweet spot is a tree ` +
`~${MAX_DOM_ELEMENTS.toLocaleString()} DOM nodes. The sweet spot is a tree ` +
Copy link
Collaborator

Choose a reason for hiding this comment

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

shall we change the text here then to to read DOM elements?

Copy link
Member

Choose a reason for hiding this comment

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

shall we change the text here then to to read DOM elements?

definitely, thanks for catching that

@exterkamp exterkamp added the i18n internationalization thangs label Apr 10, 2019
Copy link
Member

@exterkamp exterkamp left a comment

Choose a reason for hiding this comment

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

LGTM, labeling i18n b/c it has some strings to translate. Sorry for being the long pull!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
i18n internationalization thangs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants