1587845 - Add "clip" argument to Page.captureScreenshot

Status: RESOLVED FIXED

(Remote Protocol :: Page, enhancement, P1)

Description

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

The clip option itself is of type Viewport.

Comment 1

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

We actually don't have to depend on the device emulation and visible viewport bugs given that the clip argument has all the details to work with here.

Comment 2

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

I got the full screenshots working! It looks great for huge pages. And similar to Chrome we do not include the scrollbars!

Downside is that our page navigation is somewhat broken and returns too early. As such a lot of images in the screenshot are just placeholders. This wont change until we were able to fix all the dependencies on bug 1603776. I added the dependency on the captureScreenshot meta bug.

On Monday I will do some x-checks with Chrome in how it handles various invalid parameters, and will also write tests. It's very likely that a final patch will be up by late Monday.

Comment 3

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

Here a sample full screenshot: https://snipboard.io/j6utLs.jpg

Matt, there is a cookie banner which is fixed at the bottom of the page. But in the full screenshot it appears at the bottom of the currently visible viewport. I assume this is a misrendering which needs to be fixed in drawSnapshot?

Comment 4

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

This sounds actually similar to what the Chrome and Puppeteer team had to solve for the last 2.0.0 release:
https://github.com/puppeteer/puppeteer/issues/5080

Comment 5

[Matt Woodrow (:mattwoodrow)]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #3)

Here a sample full screenshot: https://snipboard.io/j6utLs.jpg

Matt, there is a cookie banner which is fixed at the bottom of the page. But in the full screenshot it appears at the bottom of the currently visible viewport. I assume this is a misrendering which needs to be fixed in drawSnapshot?

The current code for taking a full page screenshot is a bit of a hack, where it still has the normal viewport, we just don't clip to that rectangle (and omit the scrollbars).

If you want banners that are attached to the bottom edge of the viewport to be at the bottom of the page, then we'd have to re-layout using the page size as the viewport (and then un-do it since we don't want that layout to make it to the screen).

That's a fair big bit of new functionality to implement, so if it's needed, we should try schedule a meeting with the layout team in Berlin.

Comment 6

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

I filed bug 1604756 for the rendering issue with fixed elements. It actually shouldn't block us from landing a patch for this bug.

Comment 7

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

To properly test full screenshots with Puppeteer I would need bug 1544417 first. Reason is the scale factor as part of clip. It gets multiplied with the devicePixelRatio. And because Puppeteer emulates that value to 1, the final dimensions of the screenshots will be different. I could write todo() tests but would like to have it properly tested.

I will just add a WiP patch for now if someone wants to experiment.

Comment 8

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

Attachment: Bug 1587845 - [remote] Add "clip" argument to Page.captureScreenshot. r=#remote

Comment 9

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

Unassigning until bug 1544417 has been fixed completely or parts as needed by this option.

Comment 10

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

This can be implemented now. For mobile it has a different width/height due to overlay scrollbars not taking into account. But that is already added as dependency and can be fixed later.

Comment 11

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

Attachment: Bug 1587845 - [remote] Use _contentRect as name for internal helper that returns the content bounding box.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/135183e3f91f
[remote] Use _contentRect as name for internal helper that returns the content bounding box. r=remote-protocol-reviewers,maja_zf

Comment 13

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/135183e3f91f

Comment 14

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

No, this is not fixed. It misses the main patch, which hasn't been reviewed yet.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e584a14a5a4e
[remote] Add "clip" argument to Page.captureScreenshot. r=remote-protocol-reviewers,maja_zf

Comment 16

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

Attachment: Bug 1587845 - [remote] ESLint fix for browser_captureScreenshot.js.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/30ff42ad6a0d
[remote] ESLint fix for browser_captureScreenshot.js.

Comment 18

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/e584a14a5a4e
https://hg.mozilla.org/mozilla-central/rev/30ff42ad6a0d