Add Screenshot function to Selection.

[johanbrandhorst]

Fixes #104. Adds Screenshot function to Selection and GetScreenshot to the element API.

I had a quick stab at implementing this, and when I tried it in my own tests I'm seeing the following error:

request unsuccessful: unknown command: session/ca367dbfa9385ab2a0166deb0d87dcbf/element/0.535781163001626-5/screenshot

I'm slightly confused as that seems to conform to the API description in https://w3c.github.io/webdriver/webdriver-spec.html#take-element-screenshot. Am I doing something obviously wrong?

[sclevine]

Hi @johanbrandhorst, thanks for contributing. Your PR looks fantastic!

If you have time, you could add an integration test here that looks like this. Doing so would give us a test to drive out a fix.

It seems like the per-element screenshot API is newer, and isn't implemented for every webdriver yet. The webdriver.io docs seem to suggest this. If you can use a webdriver that supports per-element screenshots, I'm happy to merge this with your current implementation. You can filter out the unsupported webdrivers in your integration test like this.

If you need to use a webdriver that doesn't support per-element screenshots, you could fallback to taking a screenshot of the whole page and cropping it to the selected element's rect. Here's an extension to webdriver.io that employs that strategy successfully.

[johanbrandhorst]

@sclevine Thanks for the feedback! I'll get on those integration tests. I'd really like to have this functionality with ChromeDriver myself so I'll try and implement the fallback as well.

[johanbrandhorst]

Adding the integration test was easy, and after some struggling I managed to get PhantomJS, ChromeDriver and Selenium running. ChromeDriver reports the error I reported before, and while PhantomJS does seem to have implemented the element screenshot endpoint, the screenshot in question is still of the whole page.

Selenium (with firefox) returns an extremely unhelpful error:

failed to retrieve screenshot: request unsuccessful: GET /session/476a5fc9-eaa0-4aef-8b46-85f147d48006/element/0/screenshot
        Build info: version: '3.4.0', revision: 'unknown', time: 'unknown'
        System info: host: '<redacted>', ip: '<redacted>', os.name: 'Linux', os.arch: 'amd64', os.version: '4.11.9-1-ARCH', java.version: '1.8.0_131'
        Driver info: driver.version: unknown

This is in the debug log:

21:12:27.470 INFO - Executing: [new session: Capabilities [{acceptSslCerts=true, browserName=firefox}]])
21:12:27.503 INFO - Creating a new session for Capabilities [{acceptSslCerts=true, browserName=firefox}]
1500235947873   geckodriver     INFO    Listening on 127.0.0.1:22365
1500235947945   geckodriver::marionette INFO    Starting browser /usr/bin/firefox with args ["-marionette"]

(firefox:22938): Gtk-WARNING **: Locale not supported by C library.
        Using the fallback 'C' locale.

(/usr/lib/firefox/firefox:22999): Gtk-WARNING **: Locale not supported by C library.
        Using the fallback 'C' locale.
1500235949583   Marionette      INFO    Listening on port 33883
1500235949884   Marionette      DEBUG   loaded listener.js
21:12:29.938 INFO - Detected dialect: W3C
21:12:29.962 INFO - Done: [new session: Capabilities [{acceptSslCerts=true, browserName=firefox}]]
21:12:29.965 INFO - Executing: [get current window handle])
21:12:29.984 INFO - Done: [get current window handle]
21:12:29.990 INFO - Executing: [set window size])
21:12:30.110 INFO - Done: [set window size]
21:12:30.115 INFO - Executing: [get: http://127.0.0.1:40049])
1500235950128   Marionette      DEBUG   Received DOM event "beforeunload" for "about:blank"
1500235950144   Marionette      DEBUG   Received DOM event "pagehide" for "about:blank"
1500235950145   Marionette      DEBUG   Received DOM event "unload" for "about:blank"
1500235950200   Marionette      DEBUG   Received DOM event "DOMContentLoaded" for "http://127.0.0.1:40049/"
1500235950475   Marionette      DEBUG   Received DOM event "DOMContentLoaded" for "http://example.com/"
1500235950576   Marionette      DEBUG   Received DOM event "pageshow" for "http://example.com/"
1500235950578   Marionette      DEBUG   Received DOM event "pageshow" for "http://127.0.0.1:40049/"
21:12:30.587 INFO - Done: [get: http://127.0.0.1:40049]
21:12:30.590 INFO - Executing: [find elements: By.cssSelector: a])
21:12:30.648 INFO - Done: [find elements: By.cssSelector: a]
21:12:30.688 INFO - Executing: [delete session: 476a5fc9-eaa0-4aef-8b46-85f147d48006])
1500235950692   Marionette      INFO    New connections will no longer be accepted
[Child 22999] WARNING: pipe error (19): Connection reset by peer: file /build/firefox/src/mozilla-unified/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 346
[Child 22999] WARNING: pipe error (3): Connection reset by peer: file /build/firefox/src/mozilla-unified/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 346
21:12:31.264 INFO - Done: [delete session: 476a5fc9-eaa0-4aef-8b46-85f147d48006]

As for fallback strategies, do you prefer it to "give it a go" and do the fallback as error handling or to have a whitelist/blacklist of browsers in the function?

[johanbrandhorst]

I've pushed up the integration tests anyway, you can test run them yourself if you have a large setup of drivers. I expect the Travis integration tests will fail because the PhantomJS screenshot is of the whole page.

[sclevine]

As a general rule, Agouti should implement the w3c spec, and bugs and missing functionality should be fixed upstream in the webdrivers.

At the agouti package level, this is achieved by:

1. Following the specification first (by calling the appropriate method implemented in agouti/api)
2. Using other methods implemented in agouti/api as a fallback when any webdriver-specific (non-agouti/api-defined) error is encountered, without attempting to parse it
3. Avoiding whitelisting/blacklisting browsers

This strategy reduces maintenance costs for Agouti. When bugs are fixed and additional endpoints are implemented upstream, the fallback behavior becomes unnecessary, but no changes are needed in Agouti.

That said, browsers should be whitelisted/blacklisted in the integration tests when functionality is broken and we don't have a sane fallback behavior. For instance, I'm fine with per-element PhantomJS screenshots not working due to the whole-page bug you mentioned, as long as the integration test blacklists PhantomJS.

[johanbrandhorst]

Okay, so if I'm understanding that correctly, what I should be doing is implement a new method on the agouti/api level which would take a full-page screenshot and narrow down the image to the element bounding box. This function should then be used in agouti.Selection.Screenshot should the agouti/api.Element.GetScreenshot call error.

Obviously, this means that PhantomJS will still exhibit the wron behaviour because it wouldn't trigger the fallback but it also wouldn't capture only the element in the screenshot. Therefore it should be blacklisted from the agouti.Selection.Screenshot integration tests.

Am I understanding that correctly?

[sclevine]

Almost. The agouti/api package has a single responsibility -- implement the w3c spec verbatim. So for the fallback, your agouti.Selection.Screenshot method should call api.Session.GetScreenshot and do the cropping at the agouti level.

[johanbrandhorst]

Cool, can probably do. I'm going to have to implement https://www.w3.org/TR/webdriver/#get-element-rect at the same time, but that appears simple enough.

How do you feel about 3rd party dependencies? I'm loathe to introduce one unless it's needed but for cropping an image.Image I thought https://github.com/oliamb/cutter looked really good (it doesn't add any 3rd party deps itself and all the logic is 1 file).

[sclevine]

With the exception of tests (which depend on Ginkgo) and the Gomega matchers package (which depends on Gomega), Agouti has no third party dependencies. Given that cutter just seems to add ~100 lines of glue code top of the standard library image package, I'm not convinced that including it is the best solution.

I leave it up to you though -- I'll accept the PR either way. If you do include it, please vendor it using the /vendor/ directory and remove Go 1.5 from the .travis.yml file.

[johanbrandhorst]

So, I've implemented GetRect on api.Element, and written tests and stuff... and the workaround isn't really working either. It's a nasty can of worms of unsupported API endpoints.

ChromeDriver doesn't support the element screenshot, and also doesn't support the get element rect endpoint.

PhantomJS as mentioned supports the element screenshot endpoint but still draws the entire page.

Firefox supports get element rect, but I'm not entirely sure how to translate the CSS reference pixels to actual image coordinates. In my tests the cropped image resulting from using Firefox with the workaround is way off-element.

I will push the stuff I've written, but I'm not sure how to proceed here. We might just have to close this an blame poor driver support. I could also limit the scope of this PR to just be adding the element screenshot and element rect endpoints to the agouti/api package. Let me know how you think I should proceed.

[sclevine]

I'm hesitant to add the endpoints to agouti/api if they don't work for a significant number of webdrivers, especially if they might be implemented inconsistently in the webdrivers they do work for. Unsupported endpoints can already be queried with page.Session().Send.

If you can make this work with more than one webdriver, I'll merge it to master, and we can blacklist the others in the integration tests.

If not, how about we:

1. Add some final touch-ups to the PR (I see at least one fmt.Println, the crop package should be internal, and the Cropper interface shouldn't be exposed at the agouti level)
2. Keep this PR open until things improve upstream
3. Add these commits to a element-screenshots branch on sclevine/agouti, in case your fork goes away, and so I can rebase it against master occasionally

I appreciate the work you've put into this, and I hope that we can merge it eventually :)

[johanbrandhorst]

So I think it might be possible to get this functionality (the cropping technique) working with Firefox, since it does support GetRect. PhantomJS is out of the question since it supports the element screenshot endpoint but still returns the whole page. Chromedriver supports neither the element screenshot nor the GetRect endpoint in my testing so is also out of the question.

Given that, I've decided not to pursue this any further. I have made the changes you suggested and squashed the commit history a bit, so we can retarget this PR to a branch in this repo and open a new PR to merge that into master (is that the easiest way?).

Thanks for being such a great maintainer! It's always exciting to have such good and prompt feedback on PRs. Lets hope this could be solved with proper driver support down the line 😞 .

[johanbrandhorst]

Test failures seems to be due to a difference of encoding in image/png between 1.6 and 1.7.

[sclevine]

Leave this PR open, and I'll take care of moving the branch over when I have time. Github doesn't support changing the source repo for PRs, so I'll just mention the local branch name here and keep it rebased against master, if that works for you.

I really appreciate your efforts, and I'm disappointed that we can't merge this (yet). I spend a significant portion of my time reviewing PRs, and I rarely see first-time contributions of this quality 😄

[johanbrandhorst]

Happy to contribute 👍. I've automated the testing of my GopherJS framework courtesy of agouti and I wanted to give back. Good luck with the project!

[shanshanzhu]

Any updates on this feature?

[shanshanzhu]

From the logic, there is no need to create maxBounds / computeSize functions.
just need to use
size := image.Point{width, height}

[shanshanzhu]

Rather than adding a fallback, it could be more explicit if we have two functions, one directly use the screenshot, the other use the fallback. And after detecting the browser version, we directly calls one function or the other.

[johanbrandhorst]

@shanshanzhu Thanks for picking up on this PR, unfortunately I have not touched it since last year (as you can see). I found at the time that support for this in the web drivers was in the range of lacking to non-existent. If that has changed in the meantime you're welcome to pick this up, but I currently don't have the time to invest in this right now.

[sulthonzh]

Hello any update for this Feature

[Flagon00]

Hey,
Function return me white image without elements.
Someone can help?