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

Add "document" screenshots #564

Merged
merged 1 commit into from Oct 12, 2023
Merged

Add "document" screenshots #564

merged 1 commit into from Oct 12, 2023

Conversation

jrandolf
Copy link
Contributor

@jrandolf jrandolf commented Sep 29, 2023

This PR implements document screenshots (formerly called "fullscreen screenshots").

Issue #384


Preview | Diff

@jrandolf jrandolf changed the base branch from main to jrandolf/formats September 29, 2023 12:01
@jrandolf jrandolf force-pushed the jrandolf/document-screenshot branch 2 times, most recently from 8f6d9f5 to 92db245 Compare September 29, 2023 12:10
@jrandolf jrandolf force-pushed the jrandolf/document-screenshot branch 2 times, most recently from 6f504b0 to fb333b9 Compare September 29, 2023 12:15
Copy link
Member

@thiagowfx thiagowfx left a comment

Choose a reason for hiding this comment

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

LGTM(style)

@jrandolf jrandolf force-pushed the jrandolf/document-screenshot branch from fb333b9 to 7017856 Compare October 4, 2023 07:25
@jrandolf jrandolf force-pushed the jrandolf/formats branch 3 times, most recently from 08a647e to dd89c4a Compare October 4, 2023 11:38
Base automatically changed from jrandolf/formats to main October 5, 2023 11:49
@OrKoN
Copy link
Contributor

OrKoN commented Oct 6, 2023

@jrandolf could you please resolve conflicts?

@jrandolf jrandolf force-pushed the jrandolf/document-screenshot branch from 7017856 to 71100de Compare October 6, 2023 07:30
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@jrandolf jrandolf force-pushed the jrandolf/document-screenshot branch from 71100de to 861443c Compare October 6, 2023 15:04
@jrandolf jrandolf requested a review from jgraham October 6, 2023 15:05
@jrandolf jrandolf force-pushed the jrandolf/document-screenshot branch 2 times, most recently from c09b525 to 2d0aa39 Compare October 9, 2023 11:55
index.bs Outdated Show resolved Hide resolved
Copy link
Member

@jgraham jgraham left a comment

Choose a reason for hiding this comment

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

When the origin is set to viewport the clip coordinates should also be understood as viewport relative rather than document relative, and so need to be converted into document coordinates for the rest of the algorithm.

@jrandolf jrandolf force-pushed the jrandolf/document-screenshot branch from 2d0aa39 to 3d08940 Compare October 9, 2023 14:53
@jrandolf jrandolf requested a review from jgraham October 9, 2023 14:54
@jrandolf jrandolf force-pushed the jrandolf/document-screenshot branch 2 times, most recently from 3d08940 to e05f5d6 Compare October 9, 2023 15:01
@jrandolf jrandolf merged commit a3a09be into main Oct 12, 2023
4 checks passed
@jrandolf jrandolf deleted the jrandolf/document-screenshot branch October 12, 2023 10:33
github-actions bot added a commit that referenced this pull request Oct 12, 2023
SHA: a3a09be
Reason: push, by jrandolf

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@OrKoN
Copy link
Contributor

OrKoN commented Oct 12, 2023

@jrandolf please add WPT tests for this (and also for formats)

index.bs Show resolved Hide resolved
@whimboo
Copy link
Contributor

whimboo commented Oct 26, 2023

@jrandolf please add WPT tests for this (and also for formats)

We will take care of fullpage screenshot tests over on https://bugzilla.mozilla.org/show_bug.cgi?id=1840999.

@whimboo whimboo added needs-tests browsingContext Browsing Context module labels Oct 26, 2023
@jrandolf
Copy link
Contributor Author

Oh, I've created the PR for the tests here: web-platform-tests/wpt#42804

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
browsingContext Browsing Context module needs-tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants