pointerMove action calculates wrong default element centre point

RESOLVED FIXED in Firefox 59

Status

defect
P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: ato, Assigned: aw1231, Mentored)

Tracking

(Blocks 1 bug, {good-first-bug})

Version 3
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 wontfix, firefox59 fixed)

Details

(Whiteboard: [lang=js], )

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

2 years ago
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

2 years ago
Blocks: webdriver
Reporter

Updated

2 years ago
Keywords: good-first-bug
Whiteboard: [lang=js]
Priority: -- → P3

Comment 1

2 years ago
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

2 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

2 years ago
Given no response, I am going to start working on this bug.
Flags: needinfo?(ppatel221)
Alan, that sounds good. If you have questions let Maja know about. Thanks for working on it!
Assignee

Comment 6

2 years ago
Posted patch patch for bug 1393831 (obsolete) — Splinter Review
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)
Assignee

Comment 10

2 years ago
Posted patch patch v2 for bug 1393831 (obsolete) — Splinter Review
This fixes the call to getClientRects().
Attachment #8931885 - Attachment is obsolete: true
Attachment #8932100 - Flags: feedback?(mjzffr)
Assignee

Comment 11

2 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+
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

Updated

2 years ago
Attachment #8932100 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Flags: needinfo?(astropiloto)
Keywords: checkin-needed

Comment 15

2 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3599d43b43ad
Status: NEW → RESOLVED
Last Resolved: 2 years ago
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

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/6dc619f7460d
Whiteboard: [lang=js][checkin-needed-beta] → [lang=js]
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)
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)
Flags: needinfo?(hskupin)
You need to log in before you can comment on or make changes to this bug.