core(full-page-screenshot): handle ShadowRoots #11852
Conversation
brendankenny
requested review from
connorjclark
and removed request for
a team
|
LGTM this didn't change in this PR but I noticed this:
weird... that is highlighting an image, not a |
| width: 120, | ||
| height: 15, | ||
| }, | ||
| // And then many more nodes. |
There was a problem hiding this comment.
open to expectation suggestions :)
| htmlElement.tagName, | ||
| ].join('-'); | ||
| window.__lighthouseNodesDontTouchOrAllVarianceGoesAway.set(element, lhId); | ||
| window.__lighthouseNodesDontTouchOrAllVarianceGoesAway.set(htmlElement, lhId); |
There was a problem hiding this comment.
the other option is to do another ShadowRoot check here:
but this seems in keeping with the intent of getNodeDetails (see the getBoundingClientRect(htmlElement), below)
connorjclark
changed the title
yeah, this is actually really weird. That text ("This domain is for use in illustrative examples in documents...") is from example.com 🙃 Is the saved trace for sample_v2 mixed up somehow? |
|
or wait, only the element reference comes from the trace. lighthouse/lighthouse-core/test/results/artifacts/artifacts.json Lines 3053 to 3070 in 966a206 I don't see anywhere in |
|
I believe it was manually added https://github.com/GoogleChrome/lighthouse/pull/10517/files#diff-fccf071866992ae51684ed3c5edc57deda156a9ef8bbf41caeeeb5ba8931d205R2086 my b. I didnt know about wanna rebaseline that artifact? |
ok, I sort of rebaselined it. I updated the LCP trace element (because I think a lot of people are looking to that for checking that the fp screenshot is working), but fully rebaselining removes the cls and animation elements, because there aren't any on the page (apparently). So I kept those manually added ones so We can figure out some sort of policy on that some other day :) |
4 participants
if you look at existing deployments, you'll notice the
shadow_v2screenshot bounding boxes are a little weird. Trying toyarn update:sample-artifacts FullPageScreenshot, however, will give a gatherer error (element.getBoundingClientRect is not a function).Turns out we were trying to
ShadowRoot.getBoundingRect, which isn't a thing. FullPageScreenshot just needs to use thehost, likegetNodeDetailsdoes when taking the original rect.We didn't notice because #11536 didn't update the artifact, and the screenshot renderer falls back the audits' bounding rects if there isn't one bundled with the screenshot:
lighthouse/lighthouse-core/report/html/renderer/details-renderer.js
Lines 526 to 527 in 966a206
and since dbw_tester gets so weird with an extended height while taking the screenshot, the the original bounding rects on the full screenshot were pretty little weird :)