core(full-page-screenshot): revise logic for determining dimensions #14920
Conversation
| screenshotData = []; | ||
| mockContext = createMockContext(); | ||
| mockContext.driver.defaultSession.sendCommand.mockImplementation((method) => { | ||
| if (method === 'Page.getLayoutMetrics') { | ||
| return { | ||
| cssContentSize: contentSize, | ||
| // See comment within _takeScreenshot() implementation | ||
| cssLayoutViewport: {clientWidth: screenSize.width, clientHeight: screenSize.height}, | ||
| cssVisualViewport: {clientWidth: contentSize.width, clientHeight: contentSize.height}, |
There was a problem hiding this comment.
Honestly I think we should just remove this unit test file. It requires us to effectively simulate how chrome handles all these different width/height scales. We're better off using smoke tests for this gatherer IMO.
|
Interesting I'm getting a report error on this page: <!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8">
<meta http-equiv="X-UA-Compatible" content="IE=edge">
<meta name="viewport" content="width=device-width, initial-scale=0.5">
<title>Document</title>
</head>
<body style="overflow: scroll;">
<h1>AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA</h1>
</body>
</html>EDIT: looks like a rounding error. |
Thanks! That explains the Math.round() in the old code. I'll add that back and add a comment for future. |
alexnj
force-pushed
the
fullpage-screenshot-recalc
branch
from
e2159b0
to
2e04700
Compare
alexnj
force-pushed
the
fullpage-screenshot-recalc
branch
from
d36d43c
to
5b71aa9
Compare
alexnj
force-pushed
the
fullpage-screenshot-recalc
branch
from
5b71aa9
to
e582a12
Compare
| // Since we resized emulated viewport to match the desired screenshot size, | ||
| // it is safe to rely on scaled visual viewport css dimensions. | ||
| width: Math.round(metrics.cssVisualViewport.clientWidth * metrics.cssVisualViewport.scale), | ||
| height: Math.round(metrics.cssVisualViewport.clientHeight * metrics.cssVisualViewport.scale), |
There was a problem hiding this comment.
Why isn't this set to the above height variable, the one given the setDeviceMetricsOverride?
There was a problem hiding this comment.
Ah, it should be exactly the same I guess. Seems good.
|
We must do a lot of manual testing whenever we update this code, since we can't really automate "does this line up ok?" . It'd be good to collect a list of test URLs and write down some methodology (look at desktop, mobile, under userflow w/ pupeteer emulation provided, etc...), so we are all looking at the same thing in review (and for future changes). And really, we ought to just have a script to generate all such reports so we can just peruse them. I could make this script as part of reviewing. Please share any URLs / thoughts you have on how to best verify the change. |
|
@connorjclark so the reason for the minor mismatch in lining up horizontal coordinates was macOS scrollbar setting, depending on if you set The scenarios that we need to cover are (that I tested manually today):
The |
connorjclark
changed the title
Fixes #14824. More details on the issue.