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

No WebDriver\Session::moveto() call when dragging onto itself #359

Merged
merged 2 commits into from
Nov 2, 2024

Conversation

mvorisek
Copy link
Contributor

@mvorisek mvorisek commented Oct 17, 2022

This fixes dragging over itself, ie. source element = destination element, in such usecase, extra WebDriver\Session::moveto() call is redundant.

Drag library like https://github.com/Shopify/draggable needs this fix as the source element is very often transformed when dragging and WebDriver\Session::moveto() is failing otherwise.

image

also remove Selenium2Driver::withSyn() call as completely useless for dragging, the current impl. of Selenium2Driver::dragTo() does not use it

@mvorisek mvorisek changed the title No WebDriver\Session::moveto() call when dragging onto itself No WebDriver\Session::moveto() call when dragging over itself Oct 17, 2022
@codecov
Copy link

codecov bot commented Oct 17, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.64%. Comparing base (a3a5370) to head (afa138b).

❗ Current head afa138b differs from pull request most recent head 663bfb2. Consider uploading reports for the commit 663bfb2 to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #359      +/-   ##
============================================
+ Coverage     90.19%   90.64%   +0.45%     
+ Complexity      168      151      -17     
============================================
  Files             1        1              
  Lines           469      449      -20     
============================================
- Hits            423      407      -16     
+ Misses           46       42       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mvorisek mvorisek changed the title No WebDriver\Session::moveto() call when dragging over itself No WebDriver\Session::moveto() call when dragging onto itself Oct 17, 2022
@stof
Copy link
Member

stof commented Oct 17, 2022

is this something that could be covered by a test in the mink driver testsuite ?

@mvorisek
Copy link
Contributor Author

is this something that could be covered by a test in the mink driver testsuite ?

probably by hiding the dragged element on dragstart event, but not sure if it is worth doing so, I would call this an implementation detail

@stof
Copy link
Member

stof commented Oct 17, 2022

Well, if we don't have a test covering it, we risk regressing on that case.
And if this is necessary to be compatible with the behavior of the draggable library of Shopify, it means there is a real-world use case for that, not just an implementation detail.

@mvorisek
Copy link
Contributor Author

if this will imply the fix will be merged then, I can code the test

probably by hiding the dragged element on dragstart event

simply like this or use native https://github.com/Shopify/draggable lib? (about 100KB)

@stof
Copy link
Member

stof commented Oct 17, 2022

if we can do it with a simple implementation, it will be better for the driver testsuite IMO.

and yes, once we have tests for it, I would happily accept that patch.

@mvorisek
Copy link
Contributor Author

@stof please merge, tested in minkphp/driver-testsuite#59

I will then address minkphp/driver-testsuite#59 (comment) and #354

@mvorisek mvorisek force-pushed the no_moveto_when_dragging_over_itself branch from afbf746 to afa138b Compare May 3, 2023 09:57
@mvorisek mvorisek force-pushed the no_moveto_when_dragging_over_itself branch from afa138b to 663bfb2 Compare April 1, 2024 08:43
@mvorisek
Copy link
Contributor Author

mvorisek commented Apr 1, 2024

How to skip the Behat\Mink\Tests\Driver\Js\JavascriptTest::testDragDropOntoHiddenItself test on Selenium 2? Or any idea how to fix the driver for Selenium 2?

@aik099
Copy link
Member

aik099 commented Apr 1, 2024

@mvorisek , Test skipping can be done in the \Behat\Mink\Tests\Driver\Selenium2Config::skipMessage method.

The only purpose of the test you've mentioned (added by you) was to confirm, that code changes from this PR (added by you) will continue to work as expected.

Then, why do you need that test to be skipped?

@mvorisek
Copy link
Contributor Author

mvorisek commented Apr 1, 2024

In Selenium 3 (and with regular mouser/keyboard) the dragging works. In Selenium 2, it seems it does not, IDK why. Help welcomed. I did some experiments with Selenium 2 and it seems the dragging is not emulated well enough.

Please advise what to do and how to detect Selenium 2. Or how to fix Selenium 2 of course if you have an idea.

@aik099
Copy link
Member

aik099 commented Apr 1, 2024

I've checked on Selenium 2 + Chrome and it works, but Selenium 2 + Firefox doesn't. Visually it looks like once the element is clicked without releasing a mouse button (to perform drag) it disappears somewhere and never drops back.

Maybe it is a bug in Selenium or geckodriver itself.

@aik099
Copy link
Member

aik099 commented Apr 3, 2024

I've checked with different Firefox versions on Selenium 2 and :

  • on Firefox 47 it doesn't work (used in GitHub Actions)
  • on Firefox 92 it does work

I wish we could use the more recent browser versions on GitHub Actions. Maybe used docker image page can help in detecting what docker image version should get us desired Selenium Server + Firefox version: https://hub.docker.com/r/selenium/standalone-firefox .

@aik099
Copy link
Member

aik099 commented Apr 7, 2024

@uuf6429 , do you know what tag of the selenium-firefox Dockerimage needs to be used to get the Selenium 2 and modern Firefox version?

Tests for this PR are failing only because of outdated Firefox version usage in the Selenium Dockerimage.

The same test failure also occurs on the minkphp/webdriver-classic-driver#27 .

@uuf6429
Copy link
Member

uuf6429 commented Apr 7, 2024

@aik099 selenium/standalone-firefox:2 typically means "the latest selenium 2 with the latest firefox available at that time".
In other words, I don't think they are building images for with selenium 2 and updated firefox since they stopped maintaining selenium 2.

That being said, we could probably reverse engineer it (the docker layers are publicly readable) to have an up-to-date firefox.

@aik099
Copy link
Member

aik099 commented Apr 8, 2024

@aik099 selenium/standalone-firefox:2 typically means "the latest selenium 2 with the latest firefox available at that time". In other words, I don't think they are building images for with selenium 2 and updated firefox since they stopped maintaining selenium 2.

That being said, we could probably reverse engineer it (the docker layers are publicly readable) to have an up-to-date firefox.

If that can be done, then both Selenium-based drivers would benefit from this. I've created a #391 about this.

@aik099 aik099 force-pushed the no_moveto_when_dragging_over_itself branch from 663bfb2 to 696ed94 Compare November 2, 2024 12:41
@aik099 aik099 merged commit db44be5 into minkphp:master Nov 2, 2024
2 of 10 checks passed
@aik099
Copy link
Member

aik099 commented Nov 2, 2024

Merging, because test from the minkphp/driver-testsuite#59 is already merged. Thanks @mvorisek .

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.

4 participants