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 .

May I take this as my first bug?

Sure, lets see if Andreas would be happy to mentor you on this one.

(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

I hope this is the correct file. :) I generated it using "hg export tip -o patch.diff"

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

I now exported both revisions. (And added the space ;) )

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.

try run to confirm: https://treeherder.mozilla.org/#/jobs?repo=try&revision=238c9cf5f576762376b37015350c3fa68fbb359b

Sorry, wrong try link earlier: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e27402954de3c7d70f9dc392b3e0f6cb588aae78

Also reported in https://github.com/mozilla/geckodriver/issues/1129.

jitajina, are you interested to continue on this bug? Please let us know. Thanks.

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.

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.

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

Here we are: https://treeherder.mozilla.org/#/jobs?repo=try&revision=17dd29c39575ca28c12cd2c2c660b9457ac6ee09 Let’s see what try says now.

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

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.

Pushed by ato@sny.no: https://hg.mozilla.org/integration/mozilla-inbound/rev/76be2a9fde9e marionette: fix WebDriver:TakeScreenshot to use viewport bounds; r=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

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/14776 for changes under testing/web-platform/tests