Closed Bug 1587845 Opened 6 months ago Closed 2 months ago

Add "clip" argument to Page.captureScreenshot

Categories

(Remote Protocol :: Page, enhancement, P1)

enhancement

Tracking

(firefox74 fixed)

RESOLVED FIXED
Firefox 74
Tracking Status
firefox74 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

(Depends on 3 open bugs, Blocks 3 open bugs, )

Details

(Whiteboard: [puppeteer-beta-mvp])

Attachments

(3 files)

The clip option itself is of type Viewport.

No longer blocks: 1549466
Blocks: 1549466
Priority: P3 → P2
Blocks: 1588432
Depends on: 1549472
No longer blocks: 1588432
Depends on: 1588432
Depends on: 1602926
Blocks: 1571424

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.

Depends on: 1588622
No longer depends on: 1549472, 1588432, 1602926
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Priority: P2 → P1
Whiteboard: [puppeteer-beta-mvp]

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.

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?

Flags: needinfo?(matt.woodrow)

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

(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.

Flags: needinfo?(matt.woodrow)
Blocks: 1604723
Depends on: 1604756

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

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.

Depends on: 1544417

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

Assignee: hskupin → nobody
Status: ASSIGNED → NEW
Priority: P1 → P2
No longer blocks: 1571424
Depends on: 1610285

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: nobody → hskupin
Status: NEW → ASSIGNED
Priority: P2 → P1
Depends on: 1610827
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
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 74

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

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/30ff42ad6a0d
[remote] ESLint fix for browser_captureScreenshot.js.
Status: REOPENED → RESOLVED
Closed: 2 months ago2 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.