WebDriver:TakeScreenshot uses document element’s clientWidth/clientHeight instead of viewport dimensions
Categories
(Remote Protocol :: Marionette, defect, P1)
Tracking
(firefox66 fixed)
Tracking | Status | |
---|---|---|
firefox66 | --- | fixed |
People
(Reporter: kristian, Assigned: kristian, Mentored)
References
(Blocks 2 open bugs, )
Details
Attachments
(2 files, 5 obsolete files)
2.93 KB,
patch
|
ato
:
review+
|
Details | Diff | Splinter Review |
2.38 KB,
patch
|
Details | Diff | Splinter Review |
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 .
Updated•7 years ago
|
Updated•7 years ago
|
Updated•7 years ago
|
Comment 2•6 years ago
|
||
Sure, lets see if Andreas would be happy to mentor you on this one.
Comment 3•6 years ago
|
||
(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
Updated•6 years ago
|
I hope this is the correct file. :) I generated it using "hg export tip -o patch.diff"
Comment 5•6 years ago
|
||
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
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.
Comment 7•6 years ago
|
||
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.
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 :)
Comment 9•6 years ago
|
||
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.
Comment 10•6 years ago
|
||
I now exported both revisions. (And added the space ;) )
Comment 11•6 years ago
|
||
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.
Comment 12•6 years ago
|
||
try run to confirm: https://treeherder.mozilla.org/#/jobs?repo=try&revision=238c9cf5f576762376b37015350c3fa68fbb359b
Comment 13•6 years ago
|
||
Sorry, wrong try link earlier: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e27402954de3c7d70f9dc392b3e0f6cb588aae78
Comment 14•6 years ago
|
||
Also reported in https://github.com/mozilla/geckodriver/issues/1129.
Updated•6 years ago
|
Updated•6 years ago
|
Comment 16•6 years ago
|
||
jitajina, are you interested to continue on this bug? Please let us know. Thanks.
Comment 17•6 years 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.
Comment 18•6 years ago
|
||
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.
Comment 19•6 years ago
|
||
Haven't heard from jitajina for a while. So putting bug back so others could work on it.
Assignee | ||
Comment 20•5 years 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?
Comment 21•5 years ago
|
||
Adding needinfo for Andreas given that he is the mentor.
Comment 22•5 years ago
|
||
Here we are: https://treeherder.mozilla.org/#/jobs?repo=try&revision=17dd29c39575ca28c12cd2c2c660b9457ac6ee09 Let’s see what try says now.
Assignee | ||
Comment 23•5 years 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.
Updated•5 years ago
|
Updated•5 years ago
|
Comment 24•5 years ago
|
||
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 years ago
|
||
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
Comment 26•5 years ago
|
||
Comment 27•5 years 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 28•5 years ago
|
||
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.
Comment 29•5 years ago
|
||
Backed out changeset 76be2a9fde9e (Bug 1385706) per developer's request CLOSED TREE
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/e7ed31dd6f7faa874ba0dc718e8a3c34fd650dc0
Comment 30•5 years 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
Comment 31•5 years ago
|
||
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.
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/14776 for changes under testing/web-platform/tests
Comment 33•5 years ago
|
||
bugherder |
Updated•1 year ago
|
Description
•