1571424 - Full screenshot only captures viewport even with horizontal and/or vertical scrollbar present

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

Originally filed as https://github.com/mozilla/geckodriver/issues/1580.

And here the HTML testcase: https://gist.github.com/takeya0x86/f8f1e61e6d5785a872081864ee96c714

The reason here is that even with the table having a width of 3000px, the document element's bounding rect only returns the intersection with the viewport, which means at maximum the width/height of the current viewport.

And getBoundingClientRect() is what we currently make use of to do the fullscreen screenshot. Maybe we should use the Element.scrollWidth and Element.scrollHeight values instead?

Olli, do you have any suggestion?

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Layout folks might have the CSS box model better in mind.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

I replied on IRC, but let me know if I can help more: https://mozilla.logbot.info/developers/20190805#c16519116

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

When trying to capture the screenshot with body.width and body.height which should give us the full width and height Firefox always crashes in GFX with transferring the data. Maybe this is a bug in the new fission-compatible screenshot API. I will investigate on bug 1598731.

Comment 5

[matt]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #4)

When trying to capture the screenshot with body.width and body.height which should give us the full width and height Firefox always crashes in GFX with transferring the data. Maybe this is a bug in the new fission-compatible screenshot API. I will investigate on bug 1598731.

Henrik,

I don't know if this will help you.. but here is a snippet of Javascript I use to get the full page size and it works great with scrollbars and other weird page configurations. I found body.width & height to be unreliable due to overflow handling and a few other edge cases. Maybe something like the following could be used to find the true page width & height?

var pageWidth = Math.max(
document.documentElement.clientWidth,
document.documentElement.scrollWidth,
document.documentElement.offsetWidth,
);

var pageHeight = Math.max(
document.documentElement.clientHeight,
document.documentElement.scrollHeight,
document.documentElement.offsetHeight,
);

Hope this helps!

Thanks

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

Thanks, we will have a look once the underlying crash has been fixed.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

While reading into bug 1588622 it's now more clear that a full screenshot is related to the content size of the page. A good explanation of it can be found at:

https://github.com/bokand/bokand.github.io/blob/master/web_viewports_explainer.md#content-size

So basically documentElement.scrollWidth and documentElement.scrollHeight are the properties we would have to rely on.

Matt, which cases caused you to use the other ones? Can you remember?

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

With the investigation and implementation as done for the CDP end-point (see bug 1587845) it should be easy to get this updated for Marionette.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

(In reply to matt from comment #5)

var pageWidth = Math.max(
document.documentElement.clientWidth,
document.documentElement.scrollWidth,
document.documentElement.offsetWidth,
);

var pageHeight = Math.max(
document.documentElement.clientHeight,
document.documentElement.scrollHeight,
document.documentElement.offsetHeight,
);

Matt, do you have cases where documentElement.scrollWidth/scrollHeight is not enough? I would really appreciate your feedback here. Otherwise we would go with that usage as we also do in our CDP implementation, and what Puppeteer is using.

Comment 10

[matt]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #9)

(In reply to matt from comment #5)

var pageWidth = Math.max(
document.documentElement.clientWidth,
document.documentElement.scrollWidth,
document.documentElement.offsetWidth,
);

var pageHeight = Math.max(
document.documentElement.clientHeight,
document.documentElement.scrollHeight,
document.documentElement.offsetHeight,
);

Matt, do you have cases where documentElement.scrollWidth/scrollHeight is not enough? I would really appreciate your feedback here. Otherwise we would go with that usage as we also do in our CDP implementation, and what Puppeteer is using.

Henrik,

Happy to help. This page is maybe not a fair example but it's interesting none the less given it's a real world example of the weird pages out there on the web: https://floodgate.com/

On this page you can't actually get the correct page height at all using the snippet I suggested above. scrollHeight returns an incorrect value, which is technically correct, but incorrect given there is a inner scroll bar. I don't even know if the full screenshot command would be able to fully capture this page and maybe this is something we don't worry about.

I tried to find some other examples I've ran into as to why I moved to that snippet above but haven't seen anything that provides good evidence for why we should have backup methods for finding the true page height. My only concern I suppose would be with pages using custom javascript scroll bars.

What are your thoughts on this?

Thanks.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

(In reply to matt from comment #10)

Happy to help. This page is maybe not a fair example but it's interesting none the less given it's a real world example of the weird pages out there on the web: https://floodgate.com/

On this page you can't actually get the correct page height at all using the snippet I suggested above. scrollHeight returns an incorrect value, which is technically correct, but incorrect given there is a inner scroll bar. I don't even know if the full screenshot command would be able to fully capture this page and maybe this is something we don't worry about.

Yes, doing a full capture of this page won't be possible. It's the same as Bugzilla, like this page. This overlay div which gets scrolled instead, makes it that complicated. A way more complex logic would have to be implemented, which we are clearly not going to do.

I tried to find some other examples I've ran into as to why I moved to that snippet above but haven't seen anything that provides good evidence for why we should have backup methods for finding the true page height. My only concern I suppose would be with pages using custom javascript scroll bars.

If you have some examples please send them over. Otherwise I will go ahead with the above proposal.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

Attachment: Bug 1571424 - [marionette] Full screenshot has to capture the whole content size.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b39ccb444775
[marionette] Full screenshot has to capture the whole content size. r=marionette-reviewers,ato

Comment 14

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/b39ccb444775