WebDriver:TakeScreenshot uses document element’s clientWidth/clientHeight instead of viewport dimensions

RESOLVED FIXED in Firefox 66

Status

defect
P1
normal
RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: kristian, Assigned: kristian, Mentored)

Tracking

(Blocks 2 bugs)

Trunk
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

()

Attachments

(2 attachments, 5 obsolete attachments)

Assignee

Description

2 years ago
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170730100307

Steps to reproduce:

Take a screenshot the selenium python sdk, like so:

from selenium import webdriver

capabilities=webdriver.DesiredCapabilities.FIREFOX.copy()
# Upstream bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1338771
capabilities['moz:firefoxOptions'] = {'prefs': {"browser.tabs.remote.autostart.2": False}}
driver = webdriver.Remote(desired_capabilities=capabilities, command_executor='http://127.0.0.1:4444')

driver.set_window_size(1920, 1080)

driver.get('http://google.com')
driver.save_screenshot('google.png')
driver.get('http://facebook.com')
driver.save_screenshot('facebook.png')
driver.quit()


Actual results:

I got two screenshot which was off by a different number of pixels:

google.png:   PNG image data, 1920 x 1009, 8-bit/color RGBA, non-interlaced
facebook.png: PNG image data, 1907 x 1009, 8-bit/color RGBA, non-interlaced



Expected results:

Both screenshot should have a size of 1920x1080.

See also: https://github.com/mozilla/geckodriver/issues/465 for more info, especially https://github.com/mozilla/geckodriver/issues/465#issuecomment-283345342 .
Blocks: webdriver
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Unspecified → All
Hardware: Unspecified → All
Summary: Screenshot is smaller than viewPoint → Returned screenshot from Take Screenshot is smaller than viewport
Priority: -- → P3

Comment 1

a year ago
May I take this as my first bug?
Sure, lets see if Andreas would be happy to mentor you on this one.
Flags: needinfo?(ato)
(In reply to jitajina from comment #1)

> May I take this as my first bug?

Yes, please attach a patch and have me review it.  You will find
advice on contributing to Marionette here:

  https://searchfox.org/mozilla-central/source/testing/marionette/CONTRIBUTING.md
Flags: needinfo?(ato)

Comment 4

a year ago
Posted patch patchFileForTheFix (obsolete) — Splinter Review
I hope this is the correct file. :) I generated it using "hg export tip -o patch.diff"
Attachment #8940774 - Flags: review?(ato)
Comment on attachment 8940774 [details] [diff] [review]
patchFileForTheFix

Review of attachment 8940774 [details] [diff] [review]:
-----------------------------------------------------------------

This change makes a couple of tests in FAIL
testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py
fail:

  TestScreenCaptureChrome.test_capture_viewport
  TestScreenCaptureContent.test_capture_viewport

This is because the viewport dimensions are defined as such:

>     @property
>     def viewport_dimensions(self):
>         return self.marionette.execute_script("""
>             return [arguments[0].clientWidth,
>                     arguments[0].clientHeight];
>             """, script_args=[self.document_element])

I triggered a full try run:

	https://treeherder.mozilla.org/#/jobs?repo=try&revision=879b780b136be8696ea21f4a5bcf3f8339209513

I’d also like you to write a better commit message that explains
what it is you’re changing and why.

I incidentally had a look at how the specification says this should
be implemented and concluded it is all wrong [1].  The intention
of the the Take Screenshot command is to capture the viewport
and I can’t make the math of ‘draw a bounding box from the
framebuffer’ add up.  In any case I don’t think this should
block us from fixing this bug in Marionette.

  [1] https://w3c.github.io/webdriver/webdriver-spec.html#take-screenshot
Attachment #8940774 - Flags: review?(ato) → review-

Comment 6

a year ago
I hope this commit message is better. I don't really know how to phrase it differently. I will add an patch for the test as soon as I saw that these two are the only ones failing, so we will only have one patch for the tests.
Attachment #8940774 - Attachment is obsolete: true
Attachment #8940799 - Flags: review?(ato)
Comment on attachment 8940799 [details] [diff] [review]
Patch with updated Commit message

Review of attachment 8940799 [details] [diff] [review]:
-----------------------------------------------------------------

You need to address the failing tests.
Attachment #8940799 - Flags: review?(ato) → review-

Comment 8

a year ago
Posted patch Patch fixing the testes (obsolete) — Splinter Review
As the last tests of the try run don't seem to finish: Here's the fix for the testes. I ran them locally and they worked :)
Attachment #8941601 - Flags: review?(ato)
Comment on attachment 8941601 [details] [diff] [review]
Patch fixing the testes

Review of attachment 8941601 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, but can you combine this commit with the previous?
Note that the first line of the commit message should be relatively
short.  You can use additional lines in the commit message if you
have more to say.

New try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0035895a5ba39fe26742bcc85ba74f30a14d083

::: testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py
@@ +62,4 @@
>  
>      @property
>      def viewport_dimensions(self):
> +        return self.marionette.execute_script("return [window.innerWidth,window.innerHeight];")

Nit: There’s a missing space after the comma.
Attachment #8941601 - Flags: review?(ato) → review-

Comment 10

a year ago
Posted patch completePatch.diff (obsolete) — Splinter Review
I now exported both revisions. (And added the space ;) )
Attachment #8940799 - Attachment is obsolete: true
Attachment #8941601 - Attachment is obsolete: true
Attachment #8942458 - Flags: review?(ato)
Comment on attachment 8942458 [details] [diff] [review]
completePatch.diff

Review of attachment 8942458 [details] [diff] [review]:
-----------------------------------------------------------------

The patch itself looks good now, but there are still test failures
in Wr on Windows 10 only.
Attachment #8942458 - Flags: review?(ato) → review-
Also reported in https://github.com/mozilla/geckodriver/issues/1129.
Summary: Returned screenshot from Take Screenshot is smaller than viewport → WebDriver:TakeScreenshot uses document element’s clientWidth/clientHeight instead of viewport dimensions
Assignee: nobody → jitajina
Status: NEW → ASSIGNED
Priority: P3 → P2
Duplicate of this bug: 1398087
jitajina, are you interested to continue on this bug? Please let us know. Thanks.
Flags: needinfo?(jitajina)

Comment 17

a year ago
Sry that I take so long, but I wrote exams for the last couple weeks and only recently got around to setting up a development environment on my Win 10 installation so I would be capable to reproduce the failing tests.
Flags: needinfo?(jitajina)
That sounds great. Thanks! I hope also that everything went well for you. 

In the case of questions or remaining issues don't hesitate to ask us.
Haven't heard from jitajina for a while. So putting bug back so others could work on it.
Assignee: jitajina → nobody
Status: ASSIGNED → NEW
Assignee

Comment 20

5 months ago
(In reply to Andreas Tolfsen ❲:ato❳ from comment #11)
> Comment on attachment 8942458 [details] [diff] [review]
> completePatch.diff
> 
> Review of attachment 8942458 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The patch itself looks good now, but there are still test failures
> in Wr on Windows 10 only.

The test result seems to be gone now. Could you trigger a try run?
Adding needinfo for Andreas given that he is the mentor.
Flags: needinfo?(ato)
Assignee

Comment 23

5 months ago
(In reply to Andreas Tolfsen ⦗:ato⦘ from comment #22)
> Here we are:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=17dd29c39575ca28c12cd2c2c660b9457ac6ee09
> 
> Let’s see what try says now.

Could you do a new try run with this updated patch? I think it should fix the screenshot related tests.
Attachment #8942458 - Attachment is obsolete: true
Assignee: nobody → kristian
Status: NEW → ASSIGNED
Priority: P2 → P1
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6b30ace98f808f0f7cd22435e9ea206a635b422

Kristian, we ought to get you commit access level 1 so you can run
your own try runs.  If you wish, please follow the instructions on
https://www.mozilla.org/en-US/about/governance/policies/commit/ to
get it.  If you needinfo me on that bug, I will vouch for you.
Assignee

Comment 25

5 months ago
Posted patch patch.patchSplinter Review

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbed28982da433676530c25dc2f70b8f248da066&selectedJob=220678389

Tested with:
from selenium import webdriver

capabilities=webdriver.DesiredCapabilities.FIREFOX.copy()
capabilities['moz:firefoxOptions'] = {'args': ['--headless']}
driver = webdriver.Remote(desired_capabilities=capabilities, command_executor='http://127.0.0.1:4444')

driver.set_window_size(1920, 1080)

driver.get('http://google.com')
driver.save_screenshot('google.png')
driver.get('http://facebook.com')
driver.save_screenshot('facebook.png')
driver.quit()

$ python test.py
$ file google.png facebook.png
google.png: PNG image data, 1920 x 1006, 8-bit/color RGBA, non-interlaced
facebook.png: PNG image data, 1920 x 1006, 8-bit/color RGBA, non-interlaced

Attachment #9034630 - Attachment is obsolete: true

Comment 27

5 months ago
Pushed by ato@sny.no:
https://hg.mozilla.org/integration/mozilla-inbound/rev/76be2a9fde9e
marionette: fix WebDriver:TakeScreenshot to use viewport bounds; r=ato

Comment on attachment 9035314 [details] [diff] [review]
patch.patch

Review of attachment 9035314 [details] [diff] [review]:

Thanks for the patch! I’ve applied it to inbound.

Attachment #9035314 - Flags: review+

Backed out changeset 76be2a9fde9e (Bug 1385706) per developer's request CLOSED TREE

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/e7ed31dd6f7faa874ba0dc718e8a3c34fd650dc0

Flags: needinfo?(ato)

Comment 30

5 months ago
Pushed by ato@sny.no:
https://hg.mozilla.org/integration/mozilla-inbound/rev/50d18f1d0ab6
marionette: fix WebDriver:TakeScreenshot to use viewport bounds; r=ato

I pushed the patch with a lint fix in my own name. Backed it out,
and re-applied it with correct patch author name and email.

Flags: needinfo?(ato)
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/14776 for changes under testing/web-platform/tests

Comment 33

5 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.