core(full-page-screenshot): resolve inaccuracies related to dpr #11532
Conversation
connorjclark
requested review from
patrickhulce
and removed request for
a team
| @@ -31,7 +31,6 @@ class FullPageScreenshot extends Gatherer { | |||
| async _takeScreenshot(passContext, maxScreenshotHeight) { | |||
| const driver = passContext.driver; | |||
| const metrics = await driver.sendCommand('Page.getLayoutMetrics'); | |||
| const deviceScaleFactor = await driver.evaluateAsync('window.devicePixelRatio'); | |||
There was a problem hiding this comment.
taking another look at our reset, shouldn't we collect DPR, width, height, etc before we muck with all of it using our emulation override?
There was a problem hiding this comment.
yeah that was an oversight. I'll open a separate PR for that.
| @@ -48,7 +47,7 @@ class FullPageScreenshot extends Gatherer { | |||
| screenHeight: height, | |||
| width, | |||
| screenWidth: width, | |||
| deviceScaleFactor, | |||
| deviceScaleFactor: 1, | |||
There was a problem hiding this comment.
Prior to this moment, the page has a DPR of 2.625, at least in our own mobile emulation.
Switching the page to be DPR of 1 is a significant change and could trigger all sorts of different media queries and srcset rules that weren't used before.
There was a problem hiding this comment.
Oh kinda felt like all bets were off on reacting to a page that's 16000px tall to me. Do you suspect this is going to be fundamentally worse? (I was also under the impression the screenshot would happen after the first render not necessarily waiting for many images to load from the network, is that not the case?)
|
Closing, I think we have a different problem to solve. see #11121 |
Fixes #11121