Closed Bug 1385706 Opened 7 years ago Closed 5 years ago

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

Categories

(Remote Protocol :: Marionette, defect, P1)

defect

Tracking

(firefox66 fixed)

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: kristian, Assigned: kristian, Mentored)

References

(Blocks 2 open bugs, )

Details

Attachments

(2 files, 5 obsolete files)

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
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)
Attached 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-
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-
Attached 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-
Attached 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
jitajina, are you interested to continue on this bug? Please let us know. Thanks.
Flags: needinfo?(jitajina)
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
(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)
(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.
Attached 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
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)
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
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: