1291381 - Enable tests with synthetic touch events on Windows

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P3)

Description

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

This is a follow-up from bug 1289435, to enable tests on Windows that exercise the APZ touch events code paths. In particular this includes test_group_touchevents in the APZ mochitest folder and the touch_action_tests in the DOM pointerevents test folder.

Right now just enabling these on Windows results in try failures because somehow a mouse button event gets put into the event stream, and that creates a mouse-drag block which interrupts the touch events. Ideally we should figure out why that's happening but if we fix bug 1289432 then it may become a moot point.

Comment 1

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

I should see if this works now with the new ImputQueue code.

Comment 2

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b0299412f95

Comment 3

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

Seems like there's still some issues.

Comment 4

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

Gonna pick this up again and dig into it.

Comment 5

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

I think the new InputQueue code did in fact fix the issues that were happening before. The new issues are just that the Windows long-tap event sequence is different from other platforms (contextmenu is fired after touchend, rather than before) and a couple of tests weren't expecting that. I have patches that appear to fix this, just waiting on some retriggers before I clean them up and push them for review.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f5425c36eaeaec8aa9ad9ae75d3ff63ff3068ee&group_state=expanded

Comment 6

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

Attachment: Bug 1291381 - Fix helper_long_tap.html and helper_tap_passive.html to work with the Windows long-press event sequence.

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

Comment 7

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

Attachment: Bug 1291381 - Enable touch tests on Windows.

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

Comment 8

[Botond Ballo [:botond]]

Comment on attachment 8798853 [details]
Bug 1291381 - Fix helper_long_tap.html and helper_tap_passive.html to work with the Windows long-press event sequence.

https://reviewboard.mozilla.org/r/84232/#review82880

Not a comment on the test per se, but isn't this difference between platform behaviours a potential headache for web developers who want to listen to these events?

::: gfx/layers/apz/test/mochitest/helper_long_tap.html:60
(Diff revision 1)
> -  if (eventsFired == 3) {
> +    if (eventsFired == 3) {
> -    synthesizeNativeTouch(document.getElementById('b'), 5, 5, SpecialPowers.DOMWindowUtils.TOUCH_REMOVE, function() {
> +      synthesizeNativeTouch(document.getElementById('b'), 5, 5, SpecialPowers.DOMWindowUtils.TOUCH_REMOVE, function() {
> -      dump("Finished synthesizing touch-end, doing an APZ flush to see if any more unexpected events come through...\n");
> +        dump("Finished synthesizing touch-end, doing an APZ flush to see if any more unexpected events come through...\n");
> -      flushApzRepaints(function() {
> +        flushApzRepaints(function() {
> -        dump("Done APZ flush, ending test...\n");
> +          dump("Done APZ flush, ending test...\n");
> -        subtestDone(); // closing the window should dismiss the context menu dialog
> +          subtestDone();

I liked this comment

Comment 9

[Botond Ballo [:botond]]

Comment on attachment 8798854 [details]
Bug 1291381 - Enable touch tests on Windows.

https://reviewboard.mozilla.org/r/84234/#review82882

::: dom/events/test/pointerevents/mochitest.ini
(Diff revision 1)
>  [test_pointerevent_suppress_compat_events_on_drag_mouse.html]
>    support-files = pointerevent_suppress_compat_events_on_drag_mouse.html
>    disabled = should be investigated
>  [test_touch_action.html]
> -  # Windows touch injection doesn't work in automation, but this test can be run locally on a windows touch device.
> -  skip-if = (toolkit == 'windows')

I like the indentation here (it makes it more clear that skip-if and such pertain to the test above, not the test below). Can we adopt it in gfx/layers/apz/test?

Comment 10

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

(In reply to Botond Ballo [:botond] from comment #8)
> Comment on attachment 8798853 [details]
> Bug 1291381 - Fix helper_long_tap.html and helper_tap_passive.html to work
> with the Windows long-press event sequence.
> 
> https://reviewboard.mozilla.org/r/84232/#review82880
> 
> Not a comment on the test per se, but isn't this difference between platform
> behaviours a potential headache for web developers who want to listen to
> these events?
> 

The mouselongtap event isn't exposed to web content at all. The web-observable difference is with the contextmenu event, but web developers shouldn't be using the contextmenu event to detect a "long-press" gesture anyway. The event can fire for other reasons, such as mouse right-clicking and keyboard shift-F10 (or whatever the platform-specific keyboard combo is). So as long as they are using the contextmenu for its intended purpose (detecting when the context menu is about to be displayed, and/or preventing it) they should be fine.

> ::: gfx/layers/apz/test/mochitest/helper_long_tap.html:60
> > +          dump("Done APZ flush, ending test...\n");
> > -        subtestDone(); // closing the window should dismiss the context menu dialog
> > +          subtestDone();
> 
> I liked this comment

Sadly the comment was wrong. The test does a preventDefault() on the contextmenu event, so the context menu dialog never actually appears. While modifying this test I inadvertently forgot to do the preventDefault() in the windows branch, and a later test hung because (a) closing the window doesn't actually dismiss the context menu dialog and (b) the test needed the firefox window to have focus, which it didn't. So if we ever actually spawn context menu dialogs in a test bad things will happen since I don't think we have any good way of dismissing them.

Comment 11

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

(In reply to Botond Ballo [:botond] from comment #9)
> I like the indentation here (it makes it more clear that skip-if and such
> pertain to the test above, not the test below). Can we adopt it in
> gfx/layers/apz/test?

SGTM. I'll add a patch to the series.

Comment 12

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

Attachment: Bug 1291381 - Add some indenting to mochitest.ini for readaibility.

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

Comment 13

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

Comment on attachment 8798853 [details]
Bug 1291381 - Fix helper_long_tap.html and helper_tap_passive.html to work with the Windows long-press event sequence.

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

Comment 14

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

Comment on attachment 8798854 [details]
Bug 1291381 - Enable touch tests on Windows.

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

Comment 15

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #10)
> > ::: gfx/layers/apz/test/mochitest/helper_long_tap.html:60
> > > +          dump("Done APZ flush, ending test...\n");
> > > -        subtestDone(); // closing the window should dismiss the context menu dialog
> > > +          subtestDone();
> > 
> > I liked this comment
> 
> Sadly the comment was wrong. The test does a preventDefault() on the
> contextmenu event, so the context menu dialog never actually appears. 

Ah, I overlooked that! Never mind then :)

Comment 16

[Botond Ballo [:botond]]

Comment on attachment 8798982 [details]
Bug 1291381 - Add some indenting to mochitest.ini for readaibility.

https://reviewboard.mozilla.org/r/84294/#review82896

Thanks!

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0412d58b754e
Fix helper_long_tap.html and helper_tap_passive.html to work with the Windows long-press event sequence. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/f88a205fc4ff
Enable touch tests on Windows. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecc7a268020f
Add some indenting to mochitest.ini for readaibility. r=botond

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0412d58b754e
https://hg.mozilla.org/mozilla-central/rev/f88a205fc4ff
https://hg.mozilla.org/mozilla-central/rev/ecc7a268020f