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

Proposing a get endpoint for Take Screenshot that with fullpage option #1536

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

k7z45
Copy link
Contributor

@k7z45 k7z45 commented Jul 16, 2020

Due to the high demand from
https://bugs.chromium.org/p/chromedriver/issues/detail?id=294,
come up with initial proposal for the spec


Preview | Diff

@jgraham
Copy link
Member

jgraham commented Jul 17, 2020

geckodriver supports this, but it's a new endpoint rather than a parameter (…/screenshot/full). Since the existing endpoint is a GET and we don't have other cases that use parameters on the GET, that design would make more sense to me.

@k7z45
Copy link
Contributor Author

k7z45 commented Jul 17, 2020

Thanks for the comment, made the change accordingly.

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.

Note that merging this also requires some web-platform-tests to be submitted/approved for the feature.

index.html Outdated
<li><p>Return {<var>x</var>, <var>y</var>, <var>width</var>, <var>height</var>}.
</ol>

<p>In order to <dfn>draw a bounding box from the fullpage region</dfn>,
Copy link
Member

Choose a reason for hiding this comment

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

Could this just reuse the "draw a bounding box from the framebuffer" steps? It seems to be mostly duplicated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The definition of "draw a bounding box from the framebuffer" uses initial viewport to define the width and height of the paint which is not the case here since we want the width and height of the entire page instead of just visible area.

index.html Outdated

<li><p>Let <var>width</var> be the value
returned by
<p><a>max</a>(document.body.scrollWidth, document.documentElement.scrollWidth,
Copy link
Member

Choose a reason for hiding this comment

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

Are there really no defined relations that enforce an ordering between any of these properties?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure what you mean regarding ordering of the properties. I took this "heuristics" from https://javascript.info/size-and-scroll-window. In reality, ChromeDriver implementation will probably utilize CDP https://chromedevtools.github.io/devtools-protocol/tot/Page/#method-getLayoutMetrics and use the contentSize width and height directly.

@k7z45
Copy link
Contributor Author

k7z45 commented Jul 20, 2020

Note that merging this also requires some web-platform-tests to be submitted/approved for the feature.

Will work on web-platform-tests once I have "verbal" approval for this.

@k7z45 k7z45 requested a review from jgraham July 20, 2020 22:15
@sideshowbarker
Copy link
Contributor

Hi @k7z45 — in order to accept this PR we need to make our tooling recognize that you’re already a participant in the Browser Testing and Tools Working Group.

You can do that by using https://www.w3.org/users/myprofile to log into your W3C account — and from there, follow the Connected Accounts link in the sidebar to connect your W3C account with your GitHub account.

If you don’t remember your W3C username or password, you can use https://www.w3.org/accounts/recover to recover them.

@k7z45
Copy link
Contributor Author

k7z45 commented Jul 21, 2020

Thanks Michael! Just did that.

@sideshowbarker
Copy link
Contributor

Thanks @k7z45https://labs.w3.org/repo-manager/pr/id/w3c/webdriver/1536 shows this is now green for the IPR check

@k7z45 k7z45 changed the title Proposing a post endpoint for Take Screenshot that with fullpage option Proposing a get endpoint for Take Screenshot that with fullpage option Nov 26, 2020
k7z45 added a commit to k7z45/wpt that referenced this pull request Nov 26, 2020
This commit is to add tests using GET screenshot/full for WebDriver
proposed at [1]. The tests are copied from take_screenshot/ and
modified to use the full page screenshot endpoint. In iframe.py
test_source_origin test, the cross origin test full page screenshot
does not generate the same png as the same origin one. This behavior
has been verified by taking the full page screenshot in browser
DevTools directly using Capture full size screenshot[2].

Added a function to get full page dimensions using
document.scrollingElement.scroll{Width,Height}.

[1] w3c/webdriver#1536
[2] https://www.deconetwork.com/blog/how-to-take-full-webpage-screenshots-instantly/#:~:text=Click%20Command%2BShift%2BP%20on,Select%20Capture%20full%20size%20screenshot.
@k7z45
Copy link
Contributor Author

k7z45 commented Nov 26, 2020

Hi @jgraham ,
PTAL pending review web-platform-tests: web-platform-tests/wpt#26652 for the feature.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@@ -9082,6 +9090,45 @@ <h2>Screen capture</h2>
<li><p>Return <a>success</a> with <var>canvas</var>.
</ol>

<p>In order to <dfn>draw a scrollable region from the framebuffer</dfn>,
Copy link
Contributor

Choose a reason for hiding this comment

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

What exactly is that part about? Is it for the document element, or a child node? For child nodes we have Take Element Screenshot. Otherwise check you do below we already have at the top of the former list.

I feel we don't have to repeat all the steps again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for describing taking the screenshot for the full page instead of only the visible viewport.
It's for document.scrollingElement.
I did not use "draw a bounding box from the framebuffer" because I think step 2 and step 3
do not satisfy the requirement for full page screenshot.

"2. Let paint width be the initial viewport’s width – min(rectangle x coordinate, rectangle x coordinate + rectangle width dimension).
3. Let paint height be the initial viewport’s height – min(rectangle y coordinate, rectangle y coordinate + rectangle height dimension)."

Notice the - denotes the minus sign, so I think for Take Screenshot and Take Element Screenshot, the width and height will not be greater than the visible viewport's width and height but the full page screenshot takes the document.scrollingElement.scrollWidth and scrollHeight which can be greater. So I removed step 2 and 3 and uses the scrollWidth and Height directly for paint width and height.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@whimboo Could you take a look, please?

index.html Outdated Show resolved Hide resolved
@jugglinmike
Copy link
Contributor

I'm interested in bringing this functionality to WebDriver BiDi. Whether it belongs here in WebDriver "classic" is not a topic I can comment on, but I'm interested in learning if the normative text proposed here is viable for either spec.

Specifically, I haven't found any implementer's perspective on @jgraham's 2021 comment:

One challenge is that the fullPage option may not be trivial to specify; I think we skipped it for WebDriver Classic but in practice implementations have it, although I don't know they all work the same The typical kind of problem is "what if you haven't already rendered the whole page, but just some tiles. Then you might need to simulate scrolling to render the whole page, but what about position:fixed headers, or content that changes duirng scroll". So really you want "render the entire current layout tree as if the viewport length was unbounded", but that might not actually be a thing that's supported in engines.

@chrishtr you and I investigated a Chrome rendering bug a number of years ago; is there any chance you could weigh in here or perhaps refer me to someone more appropriate?

@juliandescottes could you direct me to a subject-matter expert at Mozilla?

@whimboo
Copy link
Contributor

whimboo commented Sep 12, 2023

For Chrome there is https://bugs.chromium.org/p/chromium/issues/detail?id=770769&desc=2 which at least for headless seems to still limit the maximum height to 16384px.

For Firefox we currently have the limit set to 32767px for both height and width, and a maximum texture size of 472907776.

As of now we do not have problems in capturing full page screenshots for pages exceeding the visible viewport.

@whimboo
Copy link
Contributor

whimboo commented Sep 12, 2023

Note that we will discuss the full screenshot feature for BiDi during the TPAC sessions on Thursday or Friday this week.

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

Successfully merging this pull request may close these issues.

5 participants