Andreas Tolfsen ❲:ato❳ Reporter
	Description • 7 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.

	Parth
	Comment 1 • 7 years ago

Hey can i take care of this bug? I'm a Computer programming student looking for a good-first-bug.

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 2 • 7 years ago

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)

	Alan Williams [:aw1231] 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.

	Alan Williams [:aw1231] Assignee
	Comment 4 • 7 years ago

Given no response, I am going to start working on this bug.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 5 • 7 years ago

Alan, that sounds good. If you have questions let Maja know about. Thanks for working on it!

	Alan Williams [:aw1231] Assignee
	Comment 6 • 7 years ago

I have uploaded the patch.  Let me know if I need to fix anything.

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 7 • 7 years ago

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

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 8 • 7 years ago

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

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 9 • 7 years ago

Test results with fix-up: https://treeherder.mozilla.org/#/jobs?repo=try&revision=556bec6b2d6f2c34d68f5e9471b87d1566708634

	Alan Williams [:aw1231] Assignee
	Comment 10 • 7 years ago

This fixes the call to getClientRects().

	Alan Williams [:aw1231] 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.

	Ryan VanderMeulen [:RyanVM]
	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

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 13 • 7 years ago

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

	Pulsebot
	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

	Dorel Luca [:dluca]
	Comment 16 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/3599d43b43ad

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 17 • 7 years ago

Could we uplift this test-only change to Firefox 58? It would help out external users of geckodriver.

	Ryan VanderMeulen [:RyanVM]
	Comment 18 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/6dc619f7460d

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 19 • 6 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.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 20 • 6 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.

	Natalia Csoregi [:nataliaCs]
	Comment 21 • 6 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