-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Remove /deep/ usage #2371
Remove /deep/ usage #2371
Conversation
@@ -152,7 +152,7 @@ module.exports = [ | |||
score: 0, | |||
extendedInfo: { | |||
value: { | |||
0: {value: '6,025'}, | |||
0: {value: '6,037'}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL that document.querySelector('html, html /deep/ *')
returns different results for shadow dom v0 vs shadow dom v1! I've used it in the past for v0 (e.g. chromestatus returns correct results), but the dbw tester uses shadow dom v1, so we weren't catching all the nodes that we should have been. No idea why v0 and v1 are different, but they deprecated /deep/
before v1 came out. So may be it was buggy with the new API and they never fixed it.
@@ -38,7 +39,10 @@ function collectImageElementInfo() { | |||
}; | |||
} | |||
|
|||
const htmlImages = [...document.querySelectorAll('img')].map(element => { | |||
const allElements = getElementsInDocument(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small perf one on the reduced qSA calls :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait, what's the perf improvement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, just that we went from 2 qSA to 1. Before we had a sep query just for document.querySelectorAll('img')
. But now, allElements
is inclusive of all the img elements on the page. Filtering allElements
is probably not much faster, but it's nice to combine the collection calls.
lighthouse-core/gather/driver.js
Outdated
* True by default. | ||
* @return {!Promise<!Array<!Element>>} The found elements, or [], resolved in a promise | ||
*/ | ||
getAllNodesInDocument(pierce = true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you filter on nodetype and then return our element instance.
and this pierces iframes whereas the dom helper doesnt.
getAllElementsInDocumentAndFrames
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -139,7 +144,12 @@ class ImageUsage extends Gatherer { | |||
return map; | |||
}, {}); | |||
|
|||
return driver.evaluateAsync(`(${collectImageElementInfo.toString()})()`) | |||
const expression = `(function() { | |||
${DOMHelpers.getElementsInDocument.toString()}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
took me a moment to figure out what you're doing here.
we should use this library consistently. If this makes the most sense, then in anchors-with-no-rel-noopener.js
we should use it similar rather than ${fn.toString()}(selector)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
lighthouse-core/lib/dom-helpers.js
Outdated
} | ||
|
||
module.exports = { | ||
getElementsInDocument |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this is only going to be consumed as a string, i think we should toString it at this point. We can append a suffix to help clarify that this is for clientside. perhaps getElementsInDocumentFn
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
lighthouse-core/gather/driver.js
Outdated
getAllNodesInDocument(pierce = true) { | ||
return this.sendCommand('DOM.getFlattenedDocument', {depth: -1, pierce}) | ||
.then(result => { | ||
const elements = result.nodes.filter(node => node.nodeType === 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this use the Node.ELEMENT_NODE
enum here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, why not. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is driver code. No Node.ELEMENT_NODE
:(
results.push(el); | ||
} | ||
// If the element has a shadow root, dig deeper. | ||
if (el.shadowRoot) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmmm should this be using root
here instead of shadowRoot
or shadyRoot
? Didn't really find any documentation on root
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.shadowRoot
is the standard property. root
and shadyRoot
are Polymer APIs
lighthouse-core/gather/driver.js
Outdated
* True by default. | ||
* @return {!Promise<!Array<!Element>>} The found elements, or [], resolved in a promise | ||
*/ | ||
getAllNodesInDocument(pierce = true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paulirish @ebidel this is what I had come up with for querying through shadow dom.
queryAllElements(query, includeUserAgentShadowDOM = true) {
return this.sendCommand('DOM.performSearch', {query, includeUserAgentShadowDOM})
.then(({searchId, resultCount}) => {
if (resultCount === 0) {
return [];
}
return this.sendCommand('DOM.getSearchResults', {
searchId,
fromIndex: 0,
toIndex: resultCount
});
})
.then(nodeList => {
const elementList = [];
nodeList.nodeIds.forEach(nodeId => {
if (nodeId !== 0) {
elementList.push(new Element({nodeId}, this));
}
});
return elementList;
});
}
Quick testing was giving me accurate results like the previous html, html /deep/ *
=> html, html *
} | ||
} | ||
}; | ||
_findAllElements(document.querySelectorAll('*')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you support passing in a starting node? Defaulting to document? Should be pretty easy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure we need that flexibility quite yet. The method name is getElementsInDocument
, so it's obvious that it acts on the entire DOM.
PTAL |
@paulirish wdyt? |
I will be merging this today if there are no comments :) |
10 other PRs will be happy to hear about our new policy :P |
lighthouse-core/lib/dom-helpers.js
Outdated
} | ||
|
||
module.exports = { | ||
getElementsInDocumentFn: getElementsInDocument.toString() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be too much to call it getElementsInDocumentFnString
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's unfortunate not to have a driver-answer to "how do I get all the elements matching a selector, including in shadow trees". Any way we can wrap that up in a usable driver method?
lighthouse-core/gather/driver.js
Outdated
* True by default. | ||
* @return {!Promise<!Array<!Element>>} The found elements, or [], resolved in a promise | ||
*/ | ||
getAllElementsInDocumentAndFrames(pierce = true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
avoid the boolean trap for now by just not making pierce
optional until it's needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was modeling after the DT protocol APIs, which make pierce
optional. With the exception that they default to false
. We want to default to true
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, but getAllElementsInDocumentAndFrames(false)
is confusing on its own. Don't get all elements?
We could change it somehow to avoid the boolean trap, but I was just suggesting that we don't need to because we don't actually have any uses for not piercing.
If you really think it should stay, though, we should rename somehow (if pierce
is false it won't be looking in the "frames" part of getAllElementsInDocumentAndFrames
), and maybe make the parameter noPierce
so that undefined
can just act as false as is usually expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, I think we should go back to what I had originally: getAllElementsInDocument(pierce = true)
.
Closest to the DT protocol for both func and param naming. The docs already explain what pierce
does ("pierces through iframes and shadow roots when true"). It doesn't need to be in the method name.
@@ -743,6 +743,20 @@ class Driver { | |||
} | |||
|
|||
/** | |||
* Returns the flattened list of all DOM nodes within the document. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all DOM element nodes maybe, to be specific?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think the @return
value covers us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant in regards to the "nodes" part...it's only returning nodes with nodeType
1. Could also just be "Returns the flattened list of all DOM elements within the document."
@@ -38,7 +39,10 @@ function collectImageElementInfo() { | |||
}; | |||
} | |||
|
|||
const htmlImages = [...document.querySelectorAll('img')].map(element => { | |||
const allElements = getElementsInDocument(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait, what's the perf improvement?
Agreed. Pavel wasn't a fan of supporting this at the protocol level: |
So is there anything we can do on LH's side? I guess one issue would be is it possible for |
Fixes #1913
Also enables shadow dom on the rel="noopener" audit 👊