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

Don't pass x, y to test_driver_internal.click #48098

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jgraham
Copy link
Contributor

@jgraham jgraham commented Sep 11, 2024

This is because the semantics are supposed to be defined by WebDriver, and that doesn't allow setting the x,y coordinates for the click command (you have to use Actions for that instead). Therefore implementations are expected to compute the coordinates rather than use passed-in values.

By the same argument we could remove the scrollIntoView and initial check, since those just replicate checks WebDriver already does. Since that comes with the possibility of timing changes it isn't included in this change.

This is because the semantics are supposed to be defined by WebDriver,
and that doesn't allow setting the x,y coordinates for the click
command (you have to use Actions for that instead). Therefore
implementations are expected to compute the coordinates rather than
use passed-in values.

By the same argument we could remove the scrollIntoView and initial
check, since those just replicate checks WebDriver already does. Since
that comes with the possibility of timing changes it isn't included in
this change.
@wpt-pr-bot
Copy link
Collaborator

There are no reviewers for this pull request besides its author. Please reach out on the chat room to get help with this. Thank you!

@jgraham
Copy link
Contributor Author

jgraham commented Sep 11, 2024

@WeizhongX @gsnedders there's a decent chance this will break your internal testdriver implementations if they aren't using webdriver. You need to move the coordinate calculation down into test_driver_internal.click. However since this doesn't match WebDriver, and the mismatch has caused people to write broken tests that pass coordinates down to the testdriver internals directly, I think it's worth fixing.

Copy link
Contributor

@Ms2ger Ms2ger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rubber stamp

@whimboo
Copy link
Contributor

whimboo commented Sep 11, 2024

Please note that I'm going to fix obvious broken input event tests over on https://bugzilla.mozilla.org/show_bug.cgi?id=1918049.

Copy link
Member

@gsnedders gsnedders left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given this has implications for downstream consumers, this probably needs an RFC.

Also, given this was found because we have tests using test_driver_internal, we probably need to add a lint for that.

(On the face of it, this seems fine — I think originally I was trying to avoid non-WebDriver implementations having to reimplement everything from scratch.)

@whimboo
Copy link
Contributor

whimboo commented Sep 12, 2024

Also, given this was found because we have tests using test_driver_internal, we probably need to add a lint for that.

The linter gets actually added via #48097 but we need my patches landed and upstreamed first.

@WeizhongX
Copy link
Contributor

@WeizhongX @gsnedders there's a decent chance this will break your internal testdriver implementations if they aren't using webdriver. You need to move the coordinate calculation down into test_driver_internal.click. However since this doesn't match WebDriver, and the mismatch has caused people to write broken tests that pass coordinates down to the testdriver internals directly, I think it's worth fixing.

Thanks for the notice. We need make a change to test_driver_internal.click but seems to be a rather small change. I am fine with this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants