core(iframe-elements): Include new IFrameElements gatherer #8979
Conversation
jburger424
requested review from
brendankenny,
patrickhulce and
paulirish
as code owners
|
TODO: unit tests |
There was a problem hiding this comment.
awesome stuff thank you so much for bringing all this work over @jburger424! 🎉
|
for the travis failure, will need to update the |
jburger424
changed the title
There was a problem hiding this comment.
I think I'm pretty much there other than tests!
unit tests can mock out the connection of driver to exercise the toVisit frame tree traversal, smoke tests can just assert that the whole flow works. the OOPIF smoketest should already have some nested iframes you can assert the artifacts on.
There was a problem hiding this comment.
Thanks @jburger424! Functionality all LGTM, just some style and tests left for me :)
Update getElementsInDocument function to find child elements within IFrames.
|
@patrickhulce I'm ensuring that frame traversal is working properly in the smoke test, by ensuring that the nested iframe has a |
|
@paulirish @brendankenny do you have any desire to take another look at this one? |
There was a problem hiding this comment.
the downside here is that we gotta manually match up collectIFrameElements() results with getFrameTree().
as noted below, we can't match up with name vs id.
frameTree's only DOM-mutable property is name, but that isn't supposed to be unique and indeed I see google_osd_static_frame as the name frequently, so I don't think we want to rely on it as a unique key.
I looked at DOMSnapshot.captureSnapshot which has style data, clientRects, and attributes, so we can 100% replace collectIFrameElements() with this protocol call. The downside is that I think we still don't have anything to bind to our frameTree objects. I wish backendNodeId was available on the frameTree, then we'd be peachy.
... looks around...
One option... DOM.getFrameOwner accepts a frameId and returns a backendNodeId.
So that would allow us to get a frameTree, send the frameIds within into DOM.getFrameOwner and then we use the resulting BackendNodeId's to match up against the captureSnapshot output.
the snapshot output is a little wild, but workable.
I know this is a big curveball, but as it stands I don't think the iframe id/name matching is reliable for most iframes.
| @@ -98,6 +98,10 @@ function getElementsInDocument(selector) { | |||
| if (el.shadowRoot) { | |||
| _findAllElements(el.shadowRoot.querySelectorAll('*')); | |||
| } | |||
| // If the element has a contentDocument (IFrame), dig deeper. | |||
There was a problem hiding this comment.
only some iframes have contentDocuments... I honestly dont understand why some do and some dont (even within google ads iframes...)
- can we make this comment more specific to which iframes are included
- i think we need this behind a
pierceIframesoption that defaults to off, otherwise ourdom-sizeaudit is about to go pretty crazy.
There was a problem hiding this comment.
Going to go ahead and remove this functionality for the time being.
In the future it may:
- Become a part of Warren's PR ([WIP] core(driver): Implement a "pierce iframes" option in settings & CLI #9566)
- Be a follow-up PR after his is in.
- Not be included at all.
There was a problem hiding this comment.
update: getFrameOwner gets the DOM details of the frame's parent frame. That's not what we want. :/
Also.. when I was just testing getFrameTree() on theverge.. i'm only seeing 5 frames, whereas qSA returns 12 and I see ~26 iframes in the DOM tree. It feels like there's something else going on here leading to frames missing from the frameTree, or maybe i'm holding it wrong.
|
@paulirish Sorry for the late response, have been OOO for past week. Will start digging into these today. |
Summary
This moves https://github.com/googleads/pub-ads-lighthouse-plugin/blob/master/lighthouse-plugin-ad-speed-insights/gatherers/iframe-elements.js to core Lighthouse repo.
Related Issues/PRs
This resolves googleads/publisher-ads-lighthouse-plugin#58.