Closed
Bug 1393831
Opened 7 years ago
Closed 7 years ago
pointerMove action calculates wrong default element centre point
Categories
(Remote Protocol :: Marionette, defect, P3)
Tracking
(firefox58 wontfix, firefox59 fixed)
RESOLVED
FIXED
mozilla59
People
(Reporter: ato, Assigned: aw1231, Mentored)
References
(Blocks 1 open bug, )
Details
(Keywords: good-first-bug, Whiteboard: [lang=js])
Attachments
(2 files, 2 obsolete files)
1.84 KB,
patch
|
impossibus
:
review+
|
Details | Diff | Splinter Review |
1.85 KB,
patch
|
Details | Diff | Splinter Review |
As is shown in https://github.com/mozilla/geckodriver/issues/901, the pointerMove action uses an incorrect target coordinate if x/y coordinates are not given as parameters. When no parameters are given, the default should be the element’s in-view centre point. getElementCenter in testing/marionette/action.js currently relies on element.coordinates which gives the _viewport relative_ element coordinates, whereas it should be using element.getInViewCentrePoint.
Reporter | ||
Updated•7 years ago
|
Reporter | ||
Updated•7 years ago
|
Keywords: good-first-bug
Mentor: mjzffr
Whiteboard: [lang=js]
Updated•7 years ago
|
Priority: -- → P3
Hey can i take care of this bug? I'm a Computer programming student looking for a good-first-bug.
Yep, go ahead and submit a patch for initial feedback. If you have questions, set "need more information from" maja_zf on this bug, or ask questions on IRC in #ateam (good people to ask are: maja_zf, ato)
Assignee | ||
Comment 3•7 years ago
|
||
(In reply to Parth from comment #1) > Hey can i take care of this bug? I'm a Computer programming student looking > for a good-first-bug. Are you still working on this bug? I'd like to work on it if you aren't.
Flags: needinfo?(ppatel221)
Assignee | ||
Comment 4•7 years ago
|
||
Given no response, I am going to start working on this bug.
Flags: needinfo?(ppatel221)
Comment 5•7 years ago
|
||
Alan, that sounds good. If you have questions let Maja know about. Thanks for working on it!
Assignee | ||
Comment 6•7 years ago
|
||
I have uploaded the patch. Let me know if I need to fix anything.
Attachment #8931885 -
Flags: feedback?(mjzffr)
Comment on attachment 8931885 [details] [diff] [review] patch for bug 1393831 Review of attachment 8931885 [details] [diff] [review]: ----------------------------------------------------------------- One small change. ::: testing/marionette/action.js @@ +1423,2 @@ > if (element.isDOMElement(el)) { > + return element.getInViewCentrePoint(el.getClientRects(), window); This needs to be `el.getClientRects()[0]` because getClientRects returns a collection of DOMRect objects, not just one. https://developer.mozilla.org/en-US/docs/Web/API/Element/getClientRects
Attachment #8931885 -
Flags: feedback?(mjzffr) → feedback-
QA Contact: astropiloto
Assignee: nobody → astropiloto
QA Contact: astropiloto
To build on what you've fixed here, would you be interested in also writing a WebDriver spec test like [1] to verify that the situation described in [2] works correctly? Let me know and I'll give you more info. I don't know your prior experience, but there might be a bit of a learning curve for writing this type of test. No pressure, either way :) [1] https://searchfox.org/mozilla-central/source/testing/web-platform/tests/webdriver/tests/actions/mouse.py#77 [2] https://github.com/mozilla/geckodriver/issues/901
Flags: needinfo?(astropiloto)
Test results with fix-up: https://treeherder.mozilla.org/#/jobs?repo=try&revision=556bec6b2d6f2c34d68f5e9471b87d1566708634
Assignee | ||
Comment 10•7 years ago
|
||
This fixes the call to getClientRects().
Attachment #8931885 -
Attachment is obsolete: true
Attachment #8932100 -
Flags: feedback?(mjzffr)
Assignee | ||
Comment 11•7 years ago
|
||
To be honest, that seems like a little too difficult for me at the moment. Thanks for the offer though.
Flags: needinfo?(astropiloto)
Attachment #8932100 -
Flags: feedback?(mjzffr) → review+
Keywords: checkin-needed
Comment 12•7 years ago
|
||
Please attach a patch that includes the necessary commit information (i.e. name, email, commit message). http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html#write-detailed-commit-messages
Flags: needinfo?(astropiloto)
Keywords: checkin-needed
Alan, a commit message like "Bug 1393831 - pointerMove action calculates wrong default element centre point; r=maja_zf" will do. If you're using hg, use `hg bzexport` to attach your commit to Bugzilla. (See ./mach mercurial-setup, enable bzexport extension) https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Assignee | ||
Comment 14•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8932100 -
Attachment is obsolete: true
Attachment #8932584 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(astropiloto)
Keywords: checkin-needed
Comment 15•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3599d43b43ad pointerMove action calculates wrong default element centre point. r=maja_zf
Keywords: checkin-needed
Comment 16•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3599d43b43ad
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Could we uplift this test-only change to Firefox 58? It would help out external users of geckodriver.
Whiteboard: [lang=js] → [lang=js][checkin-needed-beta]
Comment 18•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/6dc619f7460d
status-firefox58:
--- → fixed
Whiteboard: [lang=js][checkin-needed-beta] → [lang=js]
Comment 19•7 years ago
|
||
Given that this change will break the old existing API and other drivers still don't behave that way, would it be ok to you maja if the patch gets backed out from Firefox 58? Then we have a bit more time to discuss if that is the correct approach to do or not.
Flags: needinfo?(mjzffr)
Comment 20•7 years ago
|
||
We talked about the problem and agreed that a backout of the patch for Firefox 58 makes the most sense. With that we will not introduce a regression for people in the upcoming release. Sheriffs, can you please backout https://hg.mozilla.org/releases/mozilla-beta/rev/6dc619f7460d? Thanks.
Flags: needinfo?(mjzffr)
Comment 21•7 years ago
|
||
Backed out as requested Backout : https://hg.mozilla.org/releases/mozilla-beta/rev/3f6382962d9764c65e5bfac415f42799e8f2ee71 Push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=fb9ff0d537b9f1106ef6269bb1becc160d106e77&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception
Flags: needinfo?(hskupin)
Updated•7 years ago
|
Flags: needinfo?(hskupin)
Updated•2 years ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•