Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve compatibility with GeckoDriver when using Selenium 3 #255

Merged
merged 1 commit into from
Feb 6, 2017

Conversation

stof
Copy link
Member

@stof stof commented Jan 3, 2017

moveto is not part of the W3C Webdriver spec, and so GeckoDriver does not implement it.
Our click() implementation uses it to ensure that the element is scrolled into the viewport before clicking on it, due to Selenium requirements.
Instead of forbidding the click entirely when using Geckodriver, this now attempts the click directly when moving is unsupported, hoping that the driver has not implemented the restriction when they don't have a way to force the move yet.

Refs #254

Other methods using moveto are not changed like this as they rely on the cursor position too (when using other non-standardized APIs).

Before:
Tests: 171, Assertions: 300, Errors: 46, Failures: 4, Skipped: 14.

After:
Tests: 172, Assertions: 375, Errors: 11, Failures: 6, Skipped: 13.

(and one of the failures and one of the errors are the one from #239)

moveto is not part of the W3C Webdriver spec, and so GeckoDriver does not
implement it.
Our click() implementation uses it to ensure that the element is scrolled into
the viewport before clicking on it, due to Selenium requirements.
Instead of forbidding the click entirely when using Geckodriver, this now
attempts the click directly when moving is unsupported, hoping that the
driver has not implemented the restriction when they don't have a way to
force the move yet.
@aik099
Copy link
Member

aik099 commented Jan 3, 2017

Maybe we should do JS scrolling if moveto call fails just to make test pass?

@stof
Copy link
Member Author

stof commented Jan 3, 2017

@aik099 failing tests are not related to the element being clicked out of view, but to other unimplemented APIs:

  • usage of non-W3C webdriver methods for drag&drop, right click and double click (which all rely on moveto btw)
  • usage of moveto for mouseOver (which is exactly what moveto is about)
  • JS errors when calling WebDriver\Session::source() in some cases (meaning an issue in Selenium/GeckDriver probably)
  • buggy handling of popup windows (the GeckoDriver status page documents the popup window support as being only partially implemented)
  • issue in cookie resetting with path (not sure where the issue belongs)
  • failures from Failing test suite, Selenium 2.53 #239
  • failures about window name tests (see Fix the window name tests for GeckoDriver #256)

For the non-W3C APIs, we have to wait a bit for now, as replacement for the Selenium low-level (and badly documented) APIs we are using has been discussed only recently in the W3C WebDriver spec, and Firefox has not yet implemented this part.

@aik099
Copy link
Member

aik099 commented Jan 3, 2017

failing tests are not related to the element being clicked out of view, but to other unimplemented APIs:

Yes, I know. But if somebody is to run these tests locally on a driver, that doesn't implement moveto for element outside of viewport, then we need to make sure, that just clicking would also pass the test.

@stof
Copy link
Member Author

stof commented Jan 3, 2017

@aik099 the W3C Webdriver spec says that Element::click must handle the scrolling part internally. So there is 3 solutions here:

  • it is an implementation of the Selenium WebDriver protocol, and then moveto is part of it
  • it is an implementation of the W3C protocol and then moving is useless
  • it is an implementation of an unknown protocol or a buggy implementation of the above => we cannot know what it is the wau to support it.

The only thing I would do here is adding a test in the driver testsuite with a huge Lorem ipsum text and a link below it, so that the link is out of view for almost all screen sizes, to ensure that we can click on it through Mink.

But if somebody is to run these tests locally on a driver, that doesn't implement moveto for element outside of viewport, then we need to make sure, that just clicking would also pass the test.

GeckoDriver is such a driver (as it implements the W3C protocol). And clicking works in it with this patch. This is why 35 tests changed status with this patch (33 from error to passing, and 2 form error to failing)

@aik099
Copy link
Member

aik099 commented Jan 3, 2017

Then why moveto was there in the first place if test should have worked just fine without moveto even on Selenium 2?

@stof
Copy link
Member Author

stof commented Jan 3, 2017

@aik099 Selenium 2.x is about the Selenium protocol, not the W3C protocol (Selenium 2 was written years before the standardization effort started at the W3C). and in the Selenium version of the protocol, moving into the view is not implied by Element::click (but moveto is available for that)

@aik099
Copy link
Member

aik099 commented Jan 3, 2017

I see now. I wasn't aware, that moveto became part of click only in Selenium 3.

@stof
Copy link
Member Author

stof commented Jan 3, 2017

@aik099 to be clear, Selenium 3 can run in both Selenium and W3C mode depending on the driver being used. The ChromeDriver is still in Selenium mode for instance

@BenjaminStuermer
Copy link

@stof and @aik099 Hi. I'm blocked in a project that uses this library after an upgrade to Selenium 3 due to this issue with moveto in GeckoDriver. I would be very happy if this PR could be accepted - from reading the conversation it sounds like all that's still possibly required would be the test that stof suggests above?

Would it be possible to accept this PR without that test? If not, I'd be happy to develop the test myself if that would help get the wheels turning on this issue.

@stof stof merged commit 739b757 into minkphp:master Feb 6, 2017
@stof stof deleted the better_geckodriver_support branch February 6, 2017 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants