1264017 - Add a test_click.html to test mouse clicks

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

We have a test_tap.html file that tests touch-tapping in APZ. We should add a similar one for mouse clicking. We'll need to ensure the native mouse event sythesization works everywhere and add some helpers in apz_test_native_event_utils.js.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=39d9fcb63709

Comment 2

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=818fd93e99d2

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Also https://treeherder.mozilla.org/#/jobs?repo=try&revision=4dc482341101 for android since 'add jobs' doesn't work.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

I canceled the push in comment 3 since it turns out 'add jobs' on comment 2 did work, just delayed. Tests are looking green, I'll put these up for review.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: MozReview Request: Bug 1264017 - Add an APZ test API to synthesize a mouse click. r=botond

Review commit: https://reviewboard.mozilla.org/r/51993/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51993/

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: MozReview Request: Bug 1264017 - Add a basic sanity mouse click synthesization test. r=botond

Review commit: https://reviewboard.mozilla.org/r/51995/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51995/

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: MozReview Request: Bug 1264017 - Add synthesized mouse support to Android. r=rbarker

Review commit: https://reviewboard.mozilla.org/r/51997/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51997/

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Attachment: MozReview Request: Bug 1264017 - Add another mouse event synthesization sanity test. r=botond

Review commit: https://reviewboard.mozilla.org/r/51999/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51999/

Comment 9

[Randall Barker [:rbarker]]

Comment on attachment 8751389 [details]
MozReview Request: Bug 1264017 - Add synthesized mouse support to Android. r=rbarker

https://reviewboard.mozilla.org/r/51997/#review48877

Comment 10

[Botond Ballo [:botond]]

Comment on attachment 8751388 [details]
MozReview Request: Bug 1264017 - Add a basic sanity mouse click synthesization test. r=botond

https://reviewboard.mozilla.org/r/51995/#review48879

Comment 11

[Botond Ballo [:botond]]

Comment on attachment 8751387 [details]
MozReview Request: Bug 1264017 - Add an APZ test API to synthesize a mouse click. r=botond

https://reviewboard.mozilla.org/r/51993/#review48881

I'm not very well-versed in the GTK widget code, but this looks simple enough, and it's only used by tests, so I'll r+ it.

::: widget/gtk/nsWindow.cpp:6837
(Diff revision 1)
>  
> +    event.button.x_root = aPoint.x;
> +    event.button.y_root = aPoint.y;
> +
> +    LayoutDeviceIntPoint pointInWindow = aPoint - WidgetToScreenOffset();
> +    event.button.x = pointInWindow.x;

Similar code for touch events calls DevicePixelsToGdkCoordRoundDown().

Comment 12

[Botond Ballo [:botond]]

Comment on attachment 8751390 [details]
MozReview Request: Bug 1264017 - Add another mouse event synthesization sanity test. r=botond

https://reviewboard.mozilla.org/r/51999/#review48887

::: gfx/layers/apz/test/mochitest/helper_drag_click.html:28
(Diff revision 1)
> +
> +function clicked(e) {
> +  // The mouse down at (5, 5) should not have generated a click, but the up
> +  // at (8, 8) should have.
> +  window.opener.is(e.target, document.getElementById('b'), "Clicked on button, yay! (at " + e.clientX + "," + e.clientY + ")");
> +  window.opener.is(e.clientX, 8 + Math.floor(document.getElementById('b').getBoundingClientRect().left), 'x-coord of click event looks sane');

coordinatesRelativeToWindow() also takes into account devicePixelRatio and mozInnerScreen{X,Y}. If those are not 1 / 0, will this calculation still work?

Perhaps we could instead write a coordinatesRelativeToElement() function that does the inverse of coordinatesRelativeToWindow(), and the check that coordinatesRelativeToElement(clientX, clientY) is (8, 8)?

Comment 13

[Kartikaya Gupta (email:[e-mail])]

ni? myself as a reminder to do what I said in https://bugzilla.mozilla.org/show_bug.cgi?id=1270955#c20

Comment 14

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #11)
> > +    LayoutDeviceIntPoint pointInWindow = aPoint - WidgetToScreenOffset();
> > +    event.button.x = pointInWindow.x;
> 
> Similar code for touch events calls DevicePixelsToGdkCoordRoundDown().

Good catch! I fixed this here, and filed bug 1272152 for fixing it in the mouse-scroll event synthesization wherefrom I copied this.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #12)
> coordinatesRelativeToWindow() also takes into account devicePixelRatio and
> mozInnerScreen{X,Y}. If those are not 1 / 0, will this calculation still
> work?

Good question - I checked and yes, it does. The parameters passed to synthesizeNativeMouseEvent are in CSS pixels relative to the element's bounding rect (this is documented above coordinatesRelativeToWindow). They get converted into some GDK coordinate space when they are sent off to GDK, but then gecko does most of the reverse conversion when processing the event. Inside the click handler, the event's clientX and clientY coordinates are in CSS pixels, relative to the client area of the window. Then I use the button's bounding client rect to shift it into CSS pixels, relative to the button's bounding rect. This is the same coordinate space that the original values passed to synthesizeNativeMouseEvent were in, so they are comparable.

I verified this by running the test with --setpref layout.css.devPixelsPerPx=2.0 and 4.0. If I run it with 0.5 the test does fail, because of float to int truncation somewhere along the way (the click coordinates are one pixel less than the expected coordinates). I didn't dig into where exactly that's happening.

> Perhaps we could instead write a coordinatesRelativeToElement() function
> that does the inverse of coordinatesRelativeToWindow(), and the check that
> coordinatesRelativeToElement(clientX, clientY) is (8, 8)?

Given the above, I don't think this is needed, since it's basically what Gecko does for us already.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Attachment: MozReview Request: Bug 1264017 - Refactor machinery to run tests serially in new windows into a helper function. r?botond

Review commit: https://reviewboard.mozilla.org/r/52207/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52207/

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Attachment: MozReview Request: Bug 1264017 - Rename the meta-tests to have more meaningful names. r?botond

Review commit: https://reviewboard.mozilla.org/r/52209/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52209/

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8751387 [details]
MozReview Request: Bug 1264017 - Add an APZ test API to synthesize a mouse click. r=botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51993/diff/1-2/

Comment 19

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8751388 [details]
MozReview Request: Bug 1264017 - Add a basic sanity mouse click synthesization test. r=botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51995/diff/1-2/

Comment 20

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8751389 [details]
MozReview Request: Bug 1264017 - Add synthesized mouse support to Android. r=rbarker

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51997/diff/1-2/

Comment 21

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8751390 [details]
MozReview Request: Bug 1264017 - Add another mouse event synthesization sanity test. r=botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51999/diff/1-2/

Comment 22

[Kartikaya Gupta (email:[e-mail])]

Parts 1-4 are the same as before, with any review comments addressed.

Part 5 extracts the machinery to run the tests in new windows from test_tap, test_click, and test_scroll_window into a helper function in apz_test_utils. I cleaned it up a bit to use a Promise, remove the !!, documented it a bit, and so on. I also modified it to inject a few things into the subtests so they don't have to use window.opener all over the place.

Part 6 just renames these meta-tests to more appropriate names.

Comment 23

[Kartikaya Gupta (email:[e-mail])]

Uh whoops, apparently starting test file names with "testgroup_" is verboten, and the harness just refuses to run them. Renaming to test_group_, sorry for the spew MozReview is about to generate.

Comment 24

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8751387 [details]
MozReview Request: Bug 1264017 - Add an APZ test API to synthesize a mouse click. r=botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51993/diff/2-3/

Comment 25

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8751388 [details]
MozReview Request: Bug 1264017 - Add a basic sanity mouse click synthesization test. r=botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51995/diff/2-3/

Comment 26

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8751389 [details]
MozReview Request: Bug 1264017 - Add synthesized mouse support to Android. r=rbarker

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51997/diff/2-3/

Comment 27

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8751390 [details]
MozReview Request: Bug 1264017 - Add another mouse event synthesization sanity test. r=botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51999/diff/2-3/

Comment 28

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8751775 [details]
MozReview Request: Bug 1264017 - Refactor machinery to run tests serially in new windows into a helper function. r?botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52207/diff/1-2/

Comment 29

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8751776 [details]
MozReview Request: Bug 1264017 - Rename the meta-tests to have more meaningful names. r?botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52209/diff/1-2/

Comment 30

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8751775 [details]
MozReview Request: Bug 1264017 - Refactor machinery to run tests serially in new windows into a helper function. r?botond

Looks like these patches are also failing on try, I need to debug it.

Comment 31

[Kartikaya Gupta (email:[e-mail])]

New try push which includes the patch on bug 1272757. https://treeherder.mozilla.org/#/jobs?repo=try&revision=7811b76c4608

Comment 32

[Kartikaya Gupta (email:[e-mail])]

I rebased to latest master, try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec248f9f7116 is still green, patches coming up.

Comment 33

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8751387 [details]
MozReview Request: Bug 1264017 - Add an APZ test API to synthesize a mouse click. r=botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51993/diff/3-4/

Comment 34

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8751388 [details]
MozReview Request: Bug 1264017 - Add a basic sanity mouse click synthesization test. r=botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51995/diff/3-4/

Comment 35

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8751389 [details]
MozReview Request: Bug 1264017 - Add synthesized mouse support to Android. r=rbarker

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51997/diff/3-4/

Comment 36

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8751390 [details]
MozReview Request: Bug 1264017 - Add another mouse event synthesization sanity test. r=botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51999/diff/3-4/

Comment 37

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8751775 [details]
MozReview Request: Bug 1264017 - Refactor machinery to run tests serially in new windows into a helper function. r?botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52207/diff/2-3/

Comment 38

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8751776 [details]
MozReview Request: Bug 1264017 - Rename the meta-tests to have more meaningful names. r?botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52209/diff/2-3/

Comment 39

[Botond Ballo [:botond]]

Comment on attachment 8751775 [details]
MozReview Request: Bug 1264017 - Refactor machinery to run tests serially in new windows into a helper function. r?botond

https://reviewboard.mozilla.org/r/52207/#review49730

Nice :)

Comment 40

[Botond Ballo [:botond]]

Comment on attachment 8751776 [details]
MozReview Request: Bug 1264017 - Rename the meta-tests to have more meaningful names. r?botond

https://reviewboard.mozilla.org/r/52209/#review49732

Comment 41

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd9e7767d112
https://hg.mozilla.org/integration/mozilla-inbound/rev/425633c8f994
https://hg.mozilla.org/integration/mozilla-inbound/rev/e07d0d4457bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/393b895ef10e
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d96e3a7b84c
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b108c0fa4c9

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/dd9e7767d112
https://hg.mozilla.org/mozilla-central/rev/425633c8f994
https://hg.mozilla.org/mozilla-central/rev/e07d0d4457bd
https://hg.mozilla.org/mozilla-central/rev/393b895ef10e
https://hg.mozilla.org/mozilla-central/rev/7d96e3a7b84c
https://hg.mozilla.org/mozilla-central/rev/4b108c0fa4c9

Comment 43

[:Gijs (he/him)]

This went perma-orange on all desktop platforms as well as android when merged to fx-team, which is now closed because of it. Any chance you have any idea why that would be? :-)

Comment 44

[Kartikaya Gupta (email:[e-mail])]

See bug 1273759