Open Bug 1434313 Opened 6 years ago Updated 1 year ago

Full screenshot captures document element, but should include initial viewport dimensions

Categories

(Remote Protocol :: Marionette, defect, P3)

Product:
Remote Protocol
Component:
Marionette
Type:
defect

Tracking

(Not tracked)

Status:
REOPENED

People

(Reporter: code_mozilla, Unassigned)

References

Details

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.132 Safari/537.36

Steps to reproduce:

1. use selenium
2. use a patched geckodriver (https://github.com/mguentner/geckodriver/commit/df37bf4d1837fb6b049f0a1819fadbb59bf4d0ea)
3. take a screenshot of a page with an absolute element *under* the bounding box of <html></html>
    which means that it's outside the dimensions of win.document.documentElement
    Example: https://gist.github.com/mguentner/c975c0d1aa12b1c6a846d615fb292484

4: take a screenshot of the same page using
    $ firefox -headless -screenshot

-> I know this usecase is *currently* not supported but if https://bugzilla.mozilla.org/show_bug.cgi?id=1431148 makes progress, this will become an issue.


Actual results:

3: The screenshot obtained through selenium / webdriver / marionette only includes the <html>, but not the <div> that is "position: absolute" under it.
Relevant Code:

https://hg.mozilla.org/mozilla-central/file/9746e0a0a81c/testing/marionette/driver.js#l2865

4: the screenshot includes everything

Album: https://imgur.com/a/9OBXS


Expected results:

steps 3 and 4 produce identical results.
As far as I can tell from the screenshot attached generated using
"firefox -headless -screenshot", it takes a screenshot of the viewport.
If the document element expands beyond the viewport, is that included?

If that is the case it means that the -screenshot flag uses the
dimensions of the initial viewport as the minimum capture area and
that it grows beyond that.  That’s not the behaviour currently
defined in Marionette, so I’m not sure why you think that will
be an issue when bug 1431148 gets implemented.

Conversely, you will have the same problem as you describe above if
the absolutely positioned element falls below the document element
as it expands beyond the bottom edge of the viewport because it
will not be visible.  Maybe this is intended, but it seems like
what you are actually asking for here is _a change_ to the way
Marionette takes full document screenshots.
OS: Unspecified → All
Hardware: Unspecified → All
Summary: marionette → Full screenshot captures document element, but should include initial viewport dimensions
Thank you for clarifying :).
Basically, I am asking that the marionette command produces the same pixels as `firefox -headless -screenshot`

I think that when taking a "fullscreen" screenshot, using `win.document.documentElement` is wrong.
This can be demonstrated using the inspector using the above example html: https://imgur.com/a/7ZlxR

There are some (poorly) written pages where the content is "position: absolute; top: x" where x is
where the header (navbar, breadcrumbs etc.) stops vertically.
Taking a screenshot of such a page using the selector `win.document.documentElement` results in just
the header being included in the screenshot but not the actual content.
In the above html example, the green element would be the header and the red element the content.

Also, I just tried to move the absolute div outside the viewport by changing the top value from 200 to 2200.
`firefox -headless -screenshot` produces this (this is what I expect from marionette when bug 1431148 is implemented)
https://imgur.com/a/RjioK

As for the actual code in firefox, I am completely new to the code base. The closest thing I found is this:
https://hg.mozilla.org/mozilla-central/file/9746e0a0a81c/gfx/wrench/src/main.rs#l590
I tend to sympathise we should have the same behaviour as -screenshot
in Marionette.

The implementation of -screenshot in Firefox can be found here:

    https://searchfox.org/mozilla-central/rev/97cb0aa64ae51adcabff76fb3b5eb18368f5f8ab/browser/components/shell/HeadlessShell.jsm#141-162

They appear to use these dimensions:

> let width = fullWidth ? contentWindow.innerWidth + contentWindow.scrollMaxX - contentWindow.scrollMinX
>                       : contentWindow.innerWidth;
> let height = fullHeight ? contentWindow.innerHeight + contentWindow.scrollMaxY - contentWindow.scrollMinY
>                         : contentWindow.innerHeight;

fullWidth and fullHeight are booleans, which if true (default),
will select the content window’s innerWidth/innerHeight +
scrollMaxX/scrollMaxY - scrollMinX/scrollMinY.  That seems reasonable
to me.

Since taking a full document screenshot is not a WebDriver defined
concept we can change the semantics of WebDriver:TakeScreenshot
when passed {full: true} without ramifications.

That said, the command is awfully overloaded and we might want to
review it and split it up into separate commands.  Whether that is
done as part of this bug or separately I don’t particularly have
an opinion about.
I have started to write with a patch, however scrollM{in,ax}{X,Y} return 0 for the `win`
obtained through `getCurrentWindow()`.
Any ideas why that might be, or how to get the scroll context for a ChromeWindow?
On further investigation it looks like the getWindowDimensions function
in devtools/shared/layout/utils.js might do exactly what we want [1].

WindowProxy.{scrollMinY,scrollMinX} appear to only be exposed
to chrome [2] which makes it tricky to get at these values from
the content script.  I wonder though if it might not be possible
to capture the screenshot of the content window entirely from
chrome space.  It looks like it might be possible.

This would entail a lot more work than just updating the algorithm
for what the full document is, however.

  [1] https://searchfox.org/mozilla-central/rev/f41017f5cd7b5f3835d282fbe011d2899574fc2b/devtools/shared/layout/utils.js#661-685
  [2] https://searchfox.org/mozilla-central/source/dom/webidl/Window.webidl#265-268
Priority: -- → P3

See also https://bugzilla.mozilla.org/show_bug.cgi?id=1559592 about
drawWindow going away.

See Also: → 1559592
See Also: → 1560113
Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE

I think duping this bug was a mistake. Reopening for now.

Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: DUPLICATE → ---
Severity: normal → S3
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.