-
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): use layoutViewport width #11402
Conversation
Co-authored-by: Paul Irish <paulirish@google.com>
you got a smoke failure. at first i thought it was a flake.. #11428 but its not. under emulation this page looks like this funnily the "full-size screenshot" that devtools takes looks like this: as we know.. devtools still uses content size.. and you can tell by the TINY lil text so.. the smoke failure is actually indiciative that the 5000px attempt to try to get a TOO HUGE data uri does't work because we're taking a much smaller screenshot: 1440px instead of 5000px. and thus the datauri comes in under the threshold. yay. i still think my changes in #11428 are good.. wanna pull them in here? |
It's passing locally for me. you?
your changes are just deleting the test? |
(though since screenshot size is dependent on DPR, i wouldn't be surprised if i locally had diff results than the CI bot)
|
While the specific too big error case isn't necessarily super critical. The general flow of handling too large screenshots in a non-super-fatal way is very compelling to me. Like with most gatherers, the chance for failure is unlikely to be in our specific internal ifs and far more likely in a misinterpretation or expectation of how the integration with the protocol actually works. And the failure mode for pages that are too tall is both significant and very common. Any tall page on desktop easily sails passed 2MB. If we convert to a test on the handling a tall page by cutting down to <5000px tall instead of the error to reduce flakes that seems fine to me but something in smokes to cover this too big handling case seems important. |
there's two situations here and i'm not sure which you're talking about.. perhaps both?
are you saying our test would assert that we reattempted at 5000px? and it worked or didnt or both? On the data uri max size.. I see it in our code, the chromium bug comments, and the chromium source. However... I'm not sure if this limit is a problem for us. I logged out the size of the data uri that devtools is creating when it makes a fullsize screenshot of our test page.. and it hits 10MB and saves just fine. perhaps that 2MB is for top-level URLs being navigated to? |
I would give slight preference to a test that it worked after being reattempted, then the version we currently have, then one higher than the chrome limit but a high priority that at least one of these exists that it doesn't fail to handle the tall case entirely. (with the below, the first two might be moot)
My understanding was that the concern isn't creating and saving, it's rendering the screenshot using that data URL. That being said, I did just open up a page with a 2.5MB data URL image and it worked (the only effect seems to be that Chrome disables the menu option for opening the image in a new tab) sooooooooo maybe neither of those first two cases are necessary at all? I guess we never double checked those constraints were necessary after the PR was revived from the dead, sorry @connorjclark 🤦 |
So just delete the too-big smoke test? |
found while investigating #11118
You can test the before/after on http://localhost:10200/perf/trace-elements.html
note that the image doesn't fit the screen size–
this means the
scrollWidth
of the html element is 488, but we expected the width to be 360 (width of moto g4). This resulted in the x-axis being mis-scaled in rendering the thumbnails.before (note the highlight rectangle should cover the width of the page, but doesnt)
after