1249915 - Add a test for tapping with touch events

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

There was a regression recently, bug 1249280, where tapping via touch events was broken because the single-tap message from APZ wasn't correctly transformed into the child process. This bug is to add a test for that code. It required a bunch of plumbing so I'm breaking it out into a separate bug.

Comment 1

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

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

Comment 2

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

In the process of doing this I discovered that touch events are force-enabled in all mochitests [1] and so using window.TouchEvent to auto-detect whether or not touch events are supported doesn't work. I tried disabling that mochitest pref and it blew things up [2] so I guess I should just disable this test on the platforms where we don't really support touch events (OS X, Windows in automation, maybe Linux in automation?).

[1] http://hg.mozilla.org/mozilla-central/annotate/69ec3dc408a2/testing/profiles/prefs_general.js#l49
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=fff91bd1fa8f

Comment 3

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

Attachment: MozReview Request: Bug 1249915 - Fix synthesized touch injection code on Windows to not apply the scale factor twice. r?jimm

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

Comment 4

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

Attachment: MozReview Request: Bug 1249915 - Add ability to synthesize native touch events on Fennec for mochitests. r?snorp

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

Comment 5

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

Attachment: MozReview Request: Bug 1249915 - Add ability to synthesize native touch events on GTK for mochitests. r?karlt

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

Comment 6

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

Attachment: MozReview Request: Bug 1249915 - Fix missing MOZ_COUNT_CTOR and some misc cleanup. r?karlt

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

Comment 7

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

Attachment: MozReview Request: Bug 1249915 - Write a test to ensure touch-driven tapping works. r?botond

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

Comment 8

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

MozReview is only showing one try push with those patches, but there are actually two. One for fennec at https://treeherder.mozilla.org/#/jobs?repo=try&revision=b721e583dd20 and one for desktop at https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b326021f556 - note that the test will actually execute only on Fennec and Linux.

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8721796 [details]
MozReview Request: Bug 1249915 - Add ability to synthesize native touch events on Fennec for mochitests. r?snorp

https://reviewboard.mozilla.org/r/35783/#review32451

Comment 10

[Jim Mathies [:jimm]]

Comment on attachment 8721795 [details]
MozReview Request: Bug 1249915 - Fix synthesized touch injection code on Windows to not apply the scale factor twice. r?jimm

https://reviewboard.mozilla.org/r/35781/#review32459

Comment 11

[Botond Ballo [:botond]]

Comment on attachment 8721799 [details]
MozReview Request: Bug 1249915 - Write a test to ensure touch-driven tapping works. r?botond

https://reviewboard.mozilla.org/r/35789/#review32589

Comment 12

[Karl Tomlinson (:karlt)]

Comment on attachment 8721797 [details]
MozReview Request: Bug 1249915 - Add ability to synthesize native touch events on GTK for mochitests. r?karlt

https://reviewboard.mozilla.org/r/35785/#review32615

::: widget/gtk/nsWindow.cpp:6874
(Diff revision 1)
> +      event.touch.sequence = (GdkEventSequence*)((size_t)aPointerId);

size_t -> uintptr_t

::: widget/gtk/nsWindow.cpp:6915
(Diff revision 1)
> +  ScreenIntPoint pointInWindow = aPointerScreenPoint
> +    - ViewAs<ScreenPixel>(WidgetToScreenOffset(),
> +                          PixelCastJustification::LayoutDeviceIsScreenForUntransformedEvent);
> +  event.touch.x = pointInWindow.x;
> +  event.touch.y = pointInWindow.y;

I don't really understand why we have ScreenIntPoint and LayoutDeviceIntPoint.
Should nsIWidget be using the former where it is using the latter?

Anyway, there should be division by GdkScaleFactor() here.  Similarly for x_root, y_root.

I don't know why these are floating point members.
I think GDK always sets them to integer values.  Something like DevicePixelsToGdkPointRoundDown() may be fine, but using non-integer values is probably fine too.

Comment 13

[Karl Tomlinson (:karlt)]

Comment on attachment 8721797 [details]
MozReview Request: Bug 1249915 - Add ability to synthesize native touch events on GTK for mochitests. r?karlt

https://reviewboard.mozilla.org/r/35785/#review32617

::: widget/gtk/nsWindow.cpp:6864
(Diff revision 1)
> +  static std::map<uint32_t, GdkEventSequence*> sKnownPointers;

It would probably be possible to use mTouches here, as the gdk_event_put() usage is already assuming that the event will be processed before the next SynthesizeNativeTouchPoint() call.

Comment 14

[Karl Tomlinson (:karlt)]

Comment on attachment 8721798 [details]
MozReview Request: Bug 1249915 - Fix missing MOZ_COUNT_CTOR and some misc cleanup. r?karlt

https://reviewboard.mozilla.org/r/35787/#review32619

Comment 15

[Karl Tomlinson (:karlt)]

Comment on attachment 8721797 [details]
MozReview Request: Bug 1249915 - Add ability to synthesize native touch events on GTK for mochitests. r?karlt

https://reviewboard.mozilla.org/r/35785/#review32621

I didn't mean to remove the r+.  Just meant to add a comment, optional as this is not really production code.

Comment 16

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

(In reply to Karl Tomlinson (ni?:karlt) from comment #12)
> ::: widget/gtk/nsWindow.cpp:6874
> (Diff revision 1)
> > +      event.touch.sequence = (GdkEventSequence*)((size_t)aPointerId);
> 
> size_t -> uintptr_t

Done

> ::: widget/gtk/nsWindow.cpp:6915
> > +  ScreenIntPoint pointInWindow = aPointerScreenPoint
> > +    - ViewAs<ScreenPixel>(WidgetToScreenOffset(),
> > +                          PixelCastJustification::LayoutDeviceIsScreenForUntransformedEvent);
> > +  event.touch.x = pointInWindow.x;
> > +  event.touch.y = pointInWindow.y;
> 
> I don't really understand why we have ScreenIntPoint and
> LayoutDeviceIntPoint.
> Should nsIWidget be using the former where it is using the latter?

No actually in this case it should probably be using LayoutDeviceIntPoint. I'll file a follow up bug to change the signature of SynthesizeNativeTouch all the way through from DOMWindowUtils to the widget implementations. The distinction between ScreenPixels and LayoutDevicePixels is only relevant when there is a resolution (such as from pinch-zoom) applied, and on e10s desktop that's only relevant in the child process. So for the parent process the LayoutDevicePixel should always be equivalent to ScreenPixels. That being said, if I try and imagine what the browser would behave like with pinch-zooming allowed on Chrome, where LayoutDevice would diverge from ScreenPixels, the widget bounds would remain the same, and still be in LayoutDevice pixels, and so I think these Synthesize functions should be in LayoutDevice as well. (I may have said something different on previous bugs...)

> Anyway, there should be division by GdkScaleFactor() here.  Similarly for
> x_root, y_root.
> 
> I don't know why these are floating point members.
> I think GDK always sets them to integer values.  Something like
> DevicePixelsToGdkPointRoundDown() may be fine, but using non-integer values
> is probably fine too.

Ok, I'll update that. Do you know if there's a way I can configure Linux to test this scenario, where GdkScaleFactor() != 1?

(In reply to Karl Tomlinson (ni?:karlt) from comment #13)
> ::: widget/gtk/nsWindow.cpp:6864
> (Diff revision 1)
> > +  static std::map<uint32_t, GdkEventSequence*> sKnownPointers;
> 
> It would probably be possible to use mTouches here, as the gdk_event_put()
> usage is already assuming that the event will be processed before the next
> SynthesizeNativeTouchPoint() call.

Not sure I follow this. Why do you say gdk_event_put usage is assuming that? IIRC the event may be pulled out of the queue with nondeterministic delay so I would like to not assume this if possible. Also I would rather leave them separate anyway, because I don't want to be interfering with production code (mTouches) from this test-only code.

Comment 17

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

I filed bug 1250505 for converting the ScreenIntPoint to a LayoutDeviceIntPoint. I'm going to go ahead and land these patches with the comments addressed, but I'd like clarification on the gdk_event_put assumption you mentioned, flagging ni for that so it doesn't get missed.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae2f426b6f25
https://hg.mozilla.org/integration/mozilla-inbound/rev/45e65369eebf
https://hg.mozilla.org/integration/mozilla-inbound/rev/31aa0286d4e7
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d17e93e228f
https://hg.mozilla.org/integration/mozilla-inbound/rev/28c2483afa0d

Comment 19

[Karl Tomlinson (:karlt)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #16)
> Ok, I'll update that. Do you know if there's a way I can configure Linux to
> test this scenario, where GdkScaleFactor() != 1?

Yes.  Put GDK_SCALE=2 in the environment.

> (In reply to Karl Tomlinson (ni?:karlt) from comment #13)
> > ::: widget/gtk/nsWindow.cpp:6864
> > (Diff revision 1)
> > > +  static std::map<uint32_t, GdkEventSequence*> sKnownPointers;
> > 
> > It would probably be possible to use mTouches here, as the gdk_event_put()
> > usage is already assuming that the event will be processed before the next
> > SynthesizeNativeTouchPoint() call.
> 
> Not sure I follow this. Why do you say gdk_event_put usage is assuming that?
> IIRC the event may be pulled out of the queue with nondeterministic delay so
> I would like to not assume this if possible.

Sorry, I was wrong.  It is not assuming that.  I thought gdk_event_put()
behaved a bit like XPutBackEvent(), but it does not.  Although gdk_event_put()
id described as "Appends a copy of the given event onto the front of the event
queue", it actually puts it on the tail of the queue of events that have been
read from xlib.

That all means that using mTouches in a read-only way is not possible.

Comment 20

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

https://hg.mozilla.org/mozilla-central/rev/ae2f426b6f25
https://hg.mozilla.org/mozilla-central/rev/45e65369eebf
https://hg.mozilla.org/mozilla-central/rev/31aa0286d4e7
https://hg.mozilla.org/mozilla-central/rev/1d17e93e228f
https://hg.mozilla.org/mozilla-central/rev/28c2483afa0d

Comment 21

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

(In reply to Karl Tomlinson (ni?:karlt) from comment #19)
> Yes.  Put GDK_SCALE=2 in the environment.

Thanks, I verified that the test still passes with that. And the browser looks comically large :)

> Sorry, I was wrong.  It is not assuming that.  I thought gdk_event_put()
> behaved a bit like XPutBackEvent(), but it does not.  Although
> gdk_event_put()
> id described as "Appends a copy of the given event onto the front of the
> event
> queue", it actually puts it on the tail of the queue of events that have been
> read from xlib.
> 
> That all means that using mTouches in a read-only way is not possible.

Sounds good, thanks.