Closed Bug 1571424 Opened 5 years ago Closed 4 years ago

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

Categories

(Remote Protocol :: Marionette, defect, P1)

Version 3
defect

Tracking

(firefox73 fixed)

RESOLVED FIXED
mozilla73
Tracking Status
firefox73 --- fixed

People

(Reporter: whimboo, Assigned: whimboo, NeedInfo)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file)

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?

Flags: needinfo?(bugs)

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

Flags: needinfo?(bugs) → needinfo?(emilio)

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

Flags: needinfo?(emilio)
Summary: Full screenshot only captures viewport even with horizontal scrollbar present → Full screenshot only captures viewport even with horizontal and/or vertical scrollbar present

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.

Depends on: 1598731

(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

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

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?

Flags: needinfo?(matt)

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.

Depends on: 1587845

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

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

Flags: needinfo?(matt)
Blocks: 1604777

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

Flags: needinfo?(matt)
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
No longer depends on: 1587845
Priority: P3 → P1
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b39ccb444775
[marionette] Full screenshot has to capture the whole content size. r=marionette-reviewers,ato
Blocks: 1581494
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla73
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.