core(full-page-screenshot): wait for doubleraf, network quiet #13663
Conversation
…re taking screenshot
| isIdle: recorder => recorder.is2Idle(), | ||
| }); | ||
| await Promise.race([ | ||
| new Promise(resolve => setTimeout(resolve, 1000 * 10)), |
There was a problem hiding this comment.
thoughts on how long timeout should be?
There was a problem hiding this comment.
how long did the networkidle promise take on CNN? maybe double that?
There was a problem hiding this comment.
IMO it shouldn't be that long. FPS isn't critical information and I don't want to wait an extra 10s just so everything lines up. Quickly eyeballing it I would say 2s.
There was a problem hiding this comment.
cnn.com took ~1-1.5s to reach isCritical network quiet time on my machine
Settled on 5s because 1-2s is way too short to expect a network response in the extreme case
There was a problem hiding this comment.
5s seems reasonable to me!
Quick FYI, some pages have carousels that animate so the full screen screenshot may end up out of sync with the last screenshot from the timeline screenshots. I don't think that's a big deal, but it may lead to confusion of eagle-eyed report viewers :-)
| // Now that the viewport is taller, give the page some time to fetch new resources that | ||
| // are now in view. | ||
| const networkMonitor = new NetworkMonitor(context.driver.defaultSession); | ||
| const waitForNetworkIdleResult = waitForNetworkIdle(session, networkMonitor, { |
There was a problem hiding this comment.
Don't have great reasons for 1000ms or 2-idle vs 0-idle.
There was a problem hiding this comment.
you can pull networkQuietThresholdMs from the pass settings. the other two are correct.
There was a problem hiding this comment.
let's do network-critical-busy because we don't want to wait for tracking crap
I feel that option as used for the page load isn't quite the same as this use case... besides, that would be 5s by default which is the same time as the timeout we chose :P
| const networkMonitor = new NetworkMonitor(context.driver.defaultSession); | ||
| const waitForNetworkIdleResult = waitForNetworkIdle(session, networkMonitor, { | ||
| networkQuietThresholdMs: 1000, | ||
| busyEvent: 'network-2-busy', |
There was a problem hiding this comment.
hmm.. should the waitfor be called before setDeviceMetricsOverride (but only awaited later like it is now)?
connorjclark
changed the title
We don't give the page time to load new resources / perform a layout between changing the viewport and grabbing a screenshot. Sometimes, it would just work, but usually stuff is shifted a lot or many images are not loaded yet.
before
after