-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
base: master
Are you sure you want to change the base?
Conversation
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.
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! |
@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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rubber stamp
Please note that I'm going to fix obvious broken input event tests over on https://bugzilla.mozilla.org/show_bug.cgi?id=1918049. |
There was a problem hiding this 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.)
The linter gets actually added via #48097 but we need my patches landed and upstreamed first. |
Thanks for the notice. We need make a change to |
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.