Add "clip" argument to Page.captureScreenshot
Categories
(Remote Protocol :: CDP, enhancement, P1)
Tracking
(firefox74 fixed)
Tracking | Status | |
---|---|---|
firefox74 | --- | fixed |
People
(Reporter: whimboo, Assigned: whimboo)
References
(Depends on 3 open bugs, Blocks 2 open bugs, )
Details
(Whiteboard: [puppeteer-beta-mvp])
Attachments
(3 files)
The clip
option itself is of type Viewport.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 1•5 years ago
|
||
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.
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 2•5 years ago
|
||
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.
Assignee | ||
Comment 3•5 years ago
|
||
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?
Assignee | ||
Comment 4•5 years ago
|
||
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•5 years ago
|
||
(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.
Assignee | ||
Comment 6•5 years ago
|
||
I filed bug 1604756 for the rendering issue with fixed elements. It actually shouldn't block us from landing a patch for this bug.
Assignee | ||
Comment 7•5 years ago
|
||
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.
Assignee | ||
Comment 8•5 years ago
|
||
Assignee | ||
Comment 9•5 years ago
|
||
Unassigning until bug 1544417 has been fixed completely or parts as needed by this option.
Assignee | ||
Comment 10•5 years ago
|
||
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.
Assignee | ||
Comment 11•5 years ago
|
||
Comment 12•5 years ago
|
||
Pushed by mjzffr@gmail.com: 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•5 years ago
|
||
bugherder |
Assignee | ||
Comment 14•5 years ago
|
||
No, this is not fixed. It misses the main patch, which hasn't been reviewed yet.
Comment 15•5 years ago
|
||
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e584a14a5a4e [remote] Add "clip" argument to Page.captureScreenshot. r=remote-protocol-reviewers,maja_zf
Assignee | ||
Comment 16•5 years ago
|
||
Comment 17•5 years ago
|
||
Pushed by apavel@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/30ff42ad6a0d [remote] ESLint fix for browser_captureScreenshot.js.
Comment 18•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e584a14a5a4e
https://hg.mozilla.org/mozilla-central/rev/30ff42ad6a0d
Updated•4 years ago
|
Description
•