-
Notifications
You must be signed in to change notification settings - Fork 9.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
core(full-page-screenshot): leave emulated width unchanged #13643
Conversation
...observedDeviceMetrics, | ||
}); | ||
} | ||
// Best effort to reset emulation to what it was. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's gonna take me awhile to validate these changes, but before I do that I wanted to highlight this: the first thing that jumps out to me is that the intention/comments behind the observedDeviceMetrics
are no longer reflected in this PR. We wanted to redo the exact emulation via the emulation
module if we controlled emulation, otherwise did a best-effort to restore what it was before. Now, it just always does the "best-effort", which is likely worse than doing it ourself if we're in control.
Could you describe the reasoning behind these particular changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to controlled which emulation variables were being changed as much as possible. We should only be changing the viewport height, and then changing it back to its original value.
In hindsight, I think I was paranoid that emulation
would do more than reset the height
because we give it positive values for width
and deviceScaleFactor
and it calls other protocol functions as well. I'm fine changing this back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so far this looks good and useful!
i only tested with screenemulation ON and w/ mobile. but i'd also want to test with it disabled and w/ desktop.
it looks like our screenshot smoketest is a page WITHOUT a meta viewport. (kinda weird as that's so atypical). we should validate this is fine in that scenario.. and .. maybe add a similar test for a page WITH a viewport
side note: from all this it does seem like our dpr of 2.645 gets treated as just 2 internally. i can imagine chrome just floor()s it to simplify.
deviceScaleFactor: 1, | ||
scale: 1, | ||
screenOrientation: {angle: 0, type: 'portraitPrimary'}, | ||
width: 0, // Don't change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using the 0 to not set width and dpr is INTERESTING. the protocol sez 0 disables the override, and there is already the override we applied in emulation.js ... so i'm kinda unclear on if the 0 is clearing out any width/dpr override or its just not changing them on this call. Do you have a sense?
i found this which seems to indicate the don't-set behavior, at least for dpr. i don't see a similarish path for width.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely think it's the "don't change on this call" behavior. If it was the "clear current emulation" behavior, the viewport would revert back to some default at this point and I don't see that happening. The viewport width remains the same in all my testing.
height, | ||
width, | ||
deviceScaleFactor: 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the https://output.jsbin.com/koqamuh/4/quiet test case does find a related problem in this branch..
on master the colorcontrast results work all the way up to 16300 px
but in this branch it wraps around at 16300/2:
this wrapping around at maxtexturesize/dpr is behavior we worked around in #11688 (i think)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah setting a DPR of 1 will also reduce the image "filesize". Going off #11121 (comment) I'm fine keeping the DPR at 1 for the FPS. The content change in #13642 appears to be from us changing the viewport width anyway.
// Height should be as tall as the content. | ||
// Scale the emulated height to reach the content height. | ||
const fullHeight = Math.round( | ||
deviceMetrics.height * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i definitely don't follow how this results in a meaningful number.... but.... i can confirm that it does indeed result in something good. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
metrics.contentSize.height
represents the desired metrics.layoutViewport.clientHeight
for the FPS, and we control metrics.layoutViewport.clientHeight
by setting the emulated height over CDP.
However, the emulated height (deviceMetrics.height
) is not necessarily equivalent to metrics.layoutViewport.clientHeight
(because of DPR maybe).
So we use the ratio of metrics.contentSize.height/metrics.layoutViewport.clientHeight
to determine how much we need to scale the emulated height to arrive at the desired metrics.layoutViewport.clientHeight
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed on a few URLs, they still seem good. And the url in the linked issue looks good too.
Fixes #13642
Setting
width
anddeviceScaleFactor
to 0 will disable the override. Using a value of 0 does not appear to remove any preexisting overrides.ref #11688