Skip to content
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

Discussion: page.screenshot changes in v2.0.0 #5080

Open
mathiasbynens opened this issue Oct 24, 2019 · 4 comments

Comments

@mathiasbynens
Copy link
Member

@mathiasbynens mathiasbynens commented Oct 24, 2019

Per upstream Chromium changes, page.screenshot now clips elements to the viewport as of Puppeteer v2.0.0.

81d2600 rolled Chromium, and #5079 added a test based on the new behavior.

The good news is that with this change, document and overflow scroll clipping now work the same. It fixes issues with fixed-position headers, such as the bottom-sticking support and cookie banners on https://www.theguardian.com/us which would previously render in the middle of a screenshot. Generally, it makes page.screenshot work exactly like Chrome does (literally capturing a screenshot), rather than producing a result Chrome would not render in real scenarios.

Puppeteer scripts that relied on the old behavior can be updated to resize the viewport before calling page.screenshot.

But, it could be that people relied on the old behavior in some way for which the above workaround doesn't help. This issue can be used to collect such use cases and identify alternative implementations.

@trotzig

This comment has been minimized.

Copy link

@trotzig trotzig commented Oct 29, 2019

Hi there! 👋

First of all I'd like to stress how much I appreciate the work that you are doing. Puppeteer is by far the most developer-friendly and efficient way of automating a browser. 👏

I run Happo.io, a screenshot testing service. We do cross-browser testing with Chrome being one of the targets. Other browsers (like IE11, Edge, Safari, etc) we control via Selenium but Chrome is controlled via Puppeteer.

Most of our users have test suites that consist of individual components where few span more than the height of the viewport. But there are cases where components are taller than that, and we rely on Puppeteer giving us full screenshots of the components. Some of our users have full pages in their test suites, although that's not the most common setup.

The workaround suggested where the screen size is adjusted before taking the screenshot isn't always a good solution, for a few reasons:

  • elements styled with a vh unit in CSS will probably not look right. Imagine a large heading (hero style) which is made vertically centered through a 100vh flexbox container.
  • fixed/sticky elements might not be in the right place (as mentioned in the original comment)

Viewport-cropped screenshots is a problem in other browsers as well. With IE controlled through Selenium, we only get the viewport back in the screenshot. The workaround we have implemented there involves stitching together a full screenshot by scrolling through the page. That workaround has the added side-effect of making sticky/fixed elements appear several times, which we work around by hiding them after they're first seen.

We could potentially adopt the "stitching" workaround described above for Chrome as well, but it would affect performance negatively. Since Happo is a tool people use in CI, and people don't like waiting for CI, we have a tight budget for taking screenshots. Some of our users have 10000+ examples to screenshoot for each CI run and the screenshot phase is one of the slowest (50-250ms) in the Chrome runner.

I don't have enough context to know if supporting the old screenshot behavior is even viable, but if I could be the PM here I'd love to see that happen! :)

@progers

This comment has been minimized.

Copy link

@progers progers commented Oct 29, 2019

I work on the blink paint code and was involved in this change (it was part of BlinkGenPropertyTrees). The previous behavior was more of an accident and we didn't really have a unified solution for position: fixed/vh units/etc. For example, not changing the viewport is good for hero elements but bad for pages where the background behind the text is viewport sized.

@trotzig , are there pages that you could screenshot before but no longer can using stitching and resizing?

@lencioni

This comment has been minimized.

Copy link

@lencioni lencioni commented Oct 30, 2019

Hi @progers, thanks for the fast reply!

I built Happo with @trotzig and I currently work at Airbnb where we are using it extensively.

are there pages that you could screenshot before but no longer can using stitching and resizing?

Stitching (I assume by scrolling and taking multiple screenshots) isn't very good on pages where there is anything sticky or fixed position, or anything that is triggered by scrolling (e.g. IntersectionObserver, lazy loaded content).

Resizing the viewport to accommodate the entire contents isn't very good for anything that uses height-based media queries (e.g. a list that compacts down on shorter viewports), resize events, or lazy loaded content. It probably also would throw off elements styled with vh on really tall pages and other things of that sort.

Stitching and resizing also don't work for elements that begin positioned below the viewport and then animate in--Happo will freeze the animation at the first frame and we currently are able to capture these types of elements just below the viewport.

@trotzig

This comment has been minimized.

Copy link

@trotzig trotzig commented Oct 30, 2019

@trotzig , are there pages that you could screenshot before but no longer can using stitching and resizing?

I found that few pages look right when using the resizing approach. Developers tend to assume that the height never goes above something like 1600px. Take https://api-platform.com/ and https://connect.garmin.com/ for instance, they both have content that will be 100vh tall.

Stitching is more robust, but it poses a number of challenges as well. Sticky content will be visible in each chunk, parallax effects make backgrounds look wrong, infinite scroll lists make it hard to know when you're done, content can shift position when lazy-loaded (e.g. with the use of IntersectionObserver).

While I understand that the old behavior was more of an accident, it was an accident that made our work a lot easier. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.