[rb] add support for element screenshots

[johndouthat]

Description

Adds support for taking screenshots of individual elements via the session/:session_id/element/:id/screenshot endpoint

Motivation and Context

Selenium::WebDriver::Remote::COMMANDS already includes :take_element_screenshot but the Element class isn't wired up to use it. Python and Java Selenium bindings have this, and it would be nice to have it available in the Ruby bindings

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

[CLAassistant]

All committers have signed the CLA.

[AutomatedTester]

Please move the s/master/trunk update to a separate PR

[johndouthat]

Will do. Sorry, it wasn't my intention to include that commit in this PR.

[titusfortner]

@johndouthat this is great! thanks for noticing that it was missing and adding it.

I wonder if it would make sense to change the TakesScreenshot module not to be namespaced by DriverExtensions and instead make similar to SearchContext so it can be included by both Driver & Element? Seems weird to have a module included by all of the supported drivers at this point. I don't love the idea of a conditional to toggle between #element_screenshot and #screenshot, but it seems less ideal to have essentially the same code in two places.

Regardless this is working and better than what we have, so +1.

[johndouthat]

@titusfortner Thanks for taking a look!

OK, I've added a commit to extract TakesScreenshot from DriverExtensions and include it in both Element and Driver to reduce some of the code duplication. What do you think?

[titusfortner]

Yes, I like this. I was trying to think if it made sense to have the conditional in the TakesScreenshot module or in the Bridge, but I think it makes sense to have it where you put it.

Does it make sense to have #screenshot protected instead of private? It's in a module, so it isn't like it can be subclassed directly.

Also, please run bundle exec rubocop from the rb directory to update syntax issues.

@p0deje do you like this approach better than the first commit? A couple of the other DriverExtensions don't really make sense to me (DownloadFiles is obviously a Chrome-only implementation and can never be applied to other drivers, so not really an "extension" & HasWebStorage applies to all drivers as well), maybe we update them as well?

[p0deje]

I think the reason for driver extensions is to make driver augmentable so if new session returns "has_web_storage" in capabilities, we could include the related module in the driver. Similar to what an Augmenter does in Java. It was never really implemented so the driver extensions looked like simple modules to include.

I'm fine with the changes though there might be client code that did something like this:

driver.extend(Selenium::WebDriver::DriverExtensions::TakesScreenshot)

In regards toDownloadFiles or HasWebStorage, let's not touch unrelated files in the PR, there is no need for that.

[titusfortner]

Oh, yeah, dynamic adding of the extensions would make sense, and I didn't mean more changes in this PR. :)
As for people who have extended the module, we do have it listed as @api private and should be easy enough to change.

@johndouthat we'll also want the commits rebased/squashed and I'm not sure what Github does with the automatic actions when there is a merge commit. Let me know if you want help with the git part (you can also find me on Selenium Slack).

[diemol]

@titusfortner we only have squash enabled for the repo when merging PRs.

[titusfortner]

@diemol right, but this PR has a merge commit in it, so I'm not sure what that looks like with the automatic Github feature. Does it do the right thing automatically? I suppose I could try it out on a fork :)

[diemol]

@diemol right, but this PR has a merge commit in it, so I'm not sure what that looks like with the automatic Github feature. Does it do the right thing automatically? I suppose I could try it out on a fork :)

Yeah, a merge commit is still a commit, so it will get squashed with the other ones.

[johndouthat]

OK, I've removed the conditional toggle and protected method by moving #screenshot into Element and Driver, cleaned up the RuboCop warnings, rebased, and squashed the commits