Add a test for tapping with touch events

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

Trunk
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(5 attachments)

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.
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
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 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
Attachment #8721796 - Flags: review?(snorp) → review+
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
Attachment #8721795 - Flags: review?(jmathies) → review+
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
Attachment #8721799 - Flags: review?(botond) → review+
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.
Attachment #8721797 - Flags: review?(karlt) → review+
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.
Attachment #8721797 - Flags: review+
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
Attachment #8721798 - Flags: review?(karlt) → review+
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.
Attachment #8721797 - Flags: review+
(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.
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.
Flags: needinfo?(karlt)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Flags: needinfo?(karlt)
(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.
You need to log in before you can comment on or make changes to this bug.