core(full-page-screenshot): get screenshot, nodes concurrently #14763
Conversation
| @@ -222,9 +222,11 @@ class FullPageScreenshot extends FRGatherer { | |||
| await this._resizeViewport(context, deviceMetrics); | |||
| } | |||
|
|
|||
| const [screenshot, nodes] = | |||
| await Promise.all([this._takeScreenshot(context), this._resolveNodes(context)]); | |||
There was a problem hiding this comment.
should include a comment, otherwise this appears unnecessary
There was a problem hiding this comment.
I looked at the CDP activity here and this change looks good
(i bet if we just swapped the order of the two original lines, it'd be mostly equivalent. but.. this is fine.)
the screenshot has extra latency in encoding. we don't want to delay collecting nodes while waiting for the webp encoder... here's the cdp activity with the PR applied
for added bonus we could make the two resolveNodesInPage in parallel.
This brings up a good point for maintenance: all it takes is some future change putting an async call ahead of I don't think it's necessary to land this change since we want it for 10.0, but it probably makes sense to merge |
|
Thanks for digging in @paulirish. I spent so long bisecting (to no avail) and on a whim thought to try this small change and was happy it worked out, but in the interest of time just wanted to move on. I opened #14764 to track the additional work here. |
connorjclark
changed the title
connorjclark
changed the title
This change seems to resolve an issue where sometimes LH would produce element screenshots that did not line up (test w/ cnn.com). It was flaky behavior, so it's hard to know for sure if this is truly solved now. Please run a few times yourself. The
listaudit in a11y is what I was checking. Ifuser-syncfile is downloaded, trash the run and try again.I ran this patch roughly 20 times with no incident, whereas before I could reproduce this problem at least twice every 5 runs.