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

feat: use captureBeyondViewport in Page.captureScreenshot #6805

Merged
merged 4 commits into from
Feb 3, 2021

Conversation

sadym-chromium
Copy link
Collaborator

BREAKING CHANGE:

  • page.screenshot makes a screenshot with the clip dimensions, not cutting it by the ViewPort size.

@google-cla google-cla bot added the cla: yes label Feb 2, 2021
@sadym-chromium sadym-chromium changed the title Capture beyond viewport feat: use captureBeyondViewport in Page.captureScreenshot Feb 2, 2021
@mathiasbynens
Copy link
Member

page.screenshot makes a screenshot with the clip dimensions, not cutting it by the ViewPort size.

Are you saying this restores the pre-v6.0.0 behavior?

@@ -50,26 +50,8 @@ describe('Screenshots', function () {
});
expect(screenshot).toBeGolden('screenshot-clip-rect.png');
});
// TODO: enable after the screenshot is fixed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this test being removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bug https://crbug.com/1173457 was closed with the motivation: if you need a screenshot bigger than the ViewPort, you have to use the flag captureBeyondViewport .

This change uses the flag, screenshot works in the new way, but in the way can easily be called "correctly", the same way Firefox does it. No further TODO needed.

It used to be 2 similar tests with the different expectations:

  • skipped should clip elements to the viewport size without artefacts
  • should clip elements to the viewport size with artefacts

I removed one of them and adjusted the expectations. It covers the issue point and the breaking change, verifying the screenshot works correctly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack, thanks!

@sadym-chromium
Copy link
Collaborator Author

@mathiasbynens

Are you saying this restores the pre-v6.0.0 behavior?

  • Before 90, the screenshot is cut based on the ViewPort position and size.
  • In 90 without captureBeyondViewport, the screenshot is cut based on the ViewPort size and clip position with some unwilling artefacts out of the ViewPort.
  • In 90 with captureBeyondViewport the screenshot is cut based on the clip position and size.

From this point, using captureBeyondViewport is still a breaking change, but it makes the screenshots work properly.

@google-cla
Copy link

google-cla bot commented Feb 3, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Feb 3, 2021
@mahnunchik
Copy link

Why captureBeyondViewport option doesn't documented? https://github.com/puppeteer/puppeteer/blob/v8.0.0/docs/api.md#pagescreenshotoptions

@mahnunchik
Copy link

@sadym-chromium could you please make captureBeyondViewport configurable. To be able to not use this patch #7043 (comment)

@sadym-chromium
Copy link
Collaborator Author

sadym-chromium commented Apr 6, 2021

PR #7063 should address the issue.

Copy link
Contributor

@jschfflr jschfflr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants