956644 - Implement pointer events at MetroWidget level

Status: RESOLVED INVALID

(Core Graveyard :: Widget: WinRT, defect)

Description

[Nick Lebedev [:nl]]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/31.0.1650.63 Safari/537.36

Comment 1

[Nick Lebedev [:nl]]

I suggest to use this bug to track the changes required for the conversion of native or OS pointer/touch events to the widget pointer events.

Comment 2

[Nick Lebedev [:nl]]

Attachment: Part 1: Implemented conversion of the native pointer events to the widget ones for metro platform.

Added a patch that demonstrates the idea of converting native metro pointer events to the WidgetPointerEvent objects. The main thing i'm looking feedback for is the how is better to avoid additional PointerMove events between the PointerDown and PointerCancel events. Currently i'm implemented queuing of move events and dispatching them if apzc didn't triggered default touch behavior or discarding if apzc triggered it but i'm not 100% sure that it is the best approach.

Comment 3

[Oleg Romashin (MS)]

I created try build with this patch + enabled pointer events by default, 
https://tbpl.mozilla.org/?tree=Try&rev=9cbaa5fb9ebb
still having troubles with installation from try server (some dll missing)...
Built it locally, but cannot get metro version launching yet.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Nick Lebedev (MS) from comment #2)
> Added a patch that demonstrates the idea of converting native metro pointer
> events to the WidgetPointerEvent objects. The main thing i'm looking
> feedback for is the how is better to avoid additional PointerMove events
> between the PointerDown and PointerCancel events. 
I don't get this. I'd expect moves after down.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8356071 [details] [diff] [review]
Part 1: Implemented conversion of the native pointer events to the widget ones for metro platform.

Why we need mPointerMoveEventsQueue?
Can't we just go through mInputEventQueue when we in patch do mPointerMoveEventsQueue.Clear(); ?


I'm probably missing something here.

Comment 6

[Oleg Romashin (MS)]

> I'm probably missing something here.

IIUC ability to delay pointer events has been done for Touch Action (which does not allow to dispatch pointer move events until we get response from touch action)

Comment 7

[Nick Lebedev [:nl]]

According to the Pointer events specification when user scrolls (by touch) - no pointer move events are expected but only pointerdown and pointercancel.

Quote from the spec (http://www.w3.org/Submission/pointer-events/#the-touch-action-css-property):
A user agent must dispatch a pointercancel (and subsequently a pointerout event) whenever all of the following are true:
* The user agent has determined (via methods out of scope for this specification) that touch input is to be consumed for a default touch behavior,
* a pointerdown event has been sent for the pointer, and
* a pointerup or pointercancel event (following the above mentioned pointerdown) has not yet been sent for the pointer.
During the execution of the behavior (after sending the pointercancel and pointerout events), the user agent must not dispatch subsequent pointer events for the pointer.

Our current touch events implementation differs from this - we almost always have touchmove events between touchstart and touchcancel (and the started scrolling). This is the reason why we need to queue some first pointermove events and to wait whether scrolling would start or not. In case it started we discard queued pointer move events and if it didn't start we dispatch them.

Comment 8

[Oleg Romashin (MS)]

Comment on attachment 8356071 [details] [diff] [review]
Part 1: Implemented conversion of the native pointer events to the widget ones for metro platform.

Also it probably make sense prevent pointer events dispatching to content if preference "dom.w3c_pointer_events.enabled" == false

Comment 9

[Nick Lebedev [:nl]]

Attachment: Part 1: Implemented conversion of the native pointer events to the widget ones for metro platform.

Reworked previous patch to use mInputEventQueue for all events (including delayed PointerMove events). It's still WIP since it doesn't work well with multitouch.

The question about how we should delay PointerMove events per each pointer is still open. By delay i mean delay dispatching events until it's decided whether panning or zooming is started and whether we should dispatch pointer cancel event.
Possibly we could have max number (a kind of threshold) of PointerMove events per each pointer but it has its own downsides. Any other suggestions or advice about it are welcome.

Comment 10

[Oleg Romashin (MS)]

In theory we should delay only 2 pointers, (pan 1, zoom +1), if we got more pointers, then we probably should stop delaying.

Comment 11

[Oleg Romashin (MS)]

Also touch-action delay mechanism should work for touch events too (in theory).
Maybe it would make sense to implement delay queue somewhere on APZC side (where we have gesture detector and other things), also that way it will be reusable by all widget backends.

Comment 12

[Nick Lebedev [:nl]]

It would be great if we could have consistency between pointer and touch events in terms of delaying move events and dispatching cancel when touch behavior started.

But there maybe an issue with handling touch move event - if we have a default touch-action value = auto then when user scrolls he gets only touch down and touch cancel events. But some websites may prevent scrolling by calling preventDefault on touch move events. So if we do not dispatch touch move events in this case such website may be broken.

Comment 13

[Oleg Romashin (:romaxa)]

Comment on attachment 8361650 [details] [diff] [review]
Part 1: Implemented conversion of the native pointer events to the widget ones for metro platform.

patch looks good to me.

Comment 14

[Oleg Romashin (:romaxa)]

Comment on attachment 8361650 [details] [diff] [review]
Part 1: Implemented conversion of the native pointer events to the widget ones for metro platform.

>   // Clear :hover/:active states for mouse events generated by HandleTap
>-  WidgetMouseEvent* mouseEvent = event.get()->AsMouseEvent();
>+  // WidgetMouseEvent* mouseEvent = event.get()->AsMouseEvent();
>+  WidgetMouseEvent* mouseEvent = event->AsMouseEvent();

is this change really necessary?  


> 
>+#define DUMP_EVENT_QUEUE(eventQueue) DumpEventQueue(eventQueue)
>+//#define DUMP_EVENT_QUEUE(...)
>+

make sure debug changes don't go to latest push

Comment 15

[Nick Lebedev [:nl]]

Attachment: Part 1. Join handling of ignore-status and not-ignore-status events to simplify delaying pointer move events in future.

Comment 16

[Nick Lebedev [:nl]]

Attachment: Part 2. Add facilities for pointer events creation.

Comment 17

[Nick Lebedev [:nl]]

Attachment: Part 3. Add pointer events generation for non-touch inputs.

Comment 18

[Nick Lebedev [:nl]]

Attachment: Part 4. Add pointer events generation for touch inputs.

Comment 19

[Nick Lebedev [:nl]]

Attachment: Part 5. Add delay of dispatching PointerMove events to avoid dispatching PointerMove events at all in case touch behavior starts.

Comment 20

[Nick Lebedev [:nl]]

Attachment: Part 6. Add pointer events logs and stats to simplify debugging events.

Not sure whether this patch should go into production since it doesn't add any functional logic but only debugging facilities. Maybe it would be useful for those who would contribute into this part of FF in future.

Comment 21

[Nick Lebedev [:nl]]

Also seems like win8 touch events aren't broken by these patches: https://tbpl.mozilla.org/?tree=Try&rev=91be5850baee

Comment 22

[Jim Mathies [:jimm]]

Comment on attachment 8365904 [details] [diff] [review]
Part 1. Join handling of ignore-status and not-ignore-status events to simplify delaying pointer move events in future.

Review of attachment 8365904 [details] [diff] [review]:
-----------------------------------------------------------------

I like this, nice improvement.

Comment 23

[Jim Mathies [:jimm]]

Comment on attachment 8365906 [details] [diff] [review]
Part 3. Add pointer events generation for non-touch inputs.

Review of attachment 8365906 [details] [diff] [review]:
-----------------------------------------------------------------

I guess we're really going to have to send two dom events for every mouse move? :/ That sucks.

Comment 24

[Jim Mathies [:jimm]]

Comment on attachment 8365908 [details] [diff] [review]
Part 5. Add delay of dispatching PointerMove events to avoid dispatching PointerMove events at all in case touch behavior starts.

Review of attachment 8365908 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/winrt/MetroInput.cpp
@@ +67,5 @@
> +   * Upper limit of pointer move events we queuing while waiting whether apzc trigger default touch
> +   * behavior (pan/zoom/etc) or not. If apzc triggers it we should discard these pointer move
> +   * events otherwise we need to dispatch them (even with delay).
> +   */
> +  static uint32_t gPointerMoveMaxNumber = 10;

I don't understand why we do this. Can you provide some detail on why we queue up ten events, and how do we get out of this queuing mode?

Comment 25

[Jim Mathies [:jimm]]

Comment on attachment 8365909 [details] [diff] [review]
Part 6. Add pointer events logs and stats to simplify debugging events.

Review of attachment 8365909 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/winrt/MetroInput.cpp
@@ +663,5 @@
>      mPrimaryPointerId = pointerId;
>      mPointerMoveThresholdExceeded = false;
>      mSkippedDispatchCallsNumber = 0;
> +
> +    mPointerPressedReceived = 0;

Lets wrap all of this such that it's turned off in the tree. We don't want these variables defined or any of the logic associated with them in release builds.

Comment 26

[Nick Lebedev [:nl]]

(In reply to Jim Mathies [:jimm] from comment #24)
> Comment on attachment 8365908 [details] [diff] [review]
> Part 5. Add delay of dispatching PointerMove events to avoid dispatching
> PointerMove events at all in case touch behavior starts.
> 
> Review of attachment 8365908 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/winrt/MetroInput.cpp
> @@ +67,5 @@
> > +   * Upper limit of pointer move events we queuing while waiting whether apzc trigger default touch
> > +   * behavior (pan/zoom/etc) or not. If apzc triggers it we should discard these pointer move
> > +   * events otherwise we need to dispatch them (even with delay).
> > +   */
> > +  static uint32_t gPointerMoveMaxNumber = 10;
> 
> I don't understand why we do this. Can you provide some detail on why we
> queue up ten events, and how do we get out of this queuing mode?

Yes, sure. Sorry, I should have put detailed comments into the code. Will do it on next iteration.

About pointers queuing. The root reason is in the difference of touch and pointer events dispatching model. Let's consider the sequence of touch events we dispatch when user pans some scrolling div:
touchdown, touchmove (a few times), touchend. This is Ok with touch events. But pointer events model differs from it. PE spec states that when user tries to pan (or trigger some other touch behavior) we should dispatch only pointerdown and pointercancel (and no pointermove between them). If no pan happens we should dispatch pointerdown, pointermove (a few times) and pointerup as usual.

To correctly handle pointer events model (and do not affect touch model) we queuing some first N pointermove events (where N is the constant not specified in spec smwhere, now it's 10) and see whether apzc would trigger touch behavior (pan/zoom).
* If apzc triggers touch behavior we discard queued pointer move events and dispatch touch cancel.
* If user made enough touch movement (pointermove events number exceeded number N) and no touch behavior triggered we assume that no touch behavior would be triggered by this touch and therefore we unleash queued pointermove events.
* Together with this we dispatch touch events as usual and do not affect them.

Quite complex and we're making a logical assumption but not sure that there is an easier way to do it. Hope this helps.

And yes, surely i need to wrap dispatching stuff with pref.

Comment 27

[Jim Mathies [:jimm]]

(In reply to Nick Lebedev (MS) from comment #26)
> * If apzc triggers touch behavior we discard queued pointer move events and
> dispatch touch cancel.

For touch events, the apzc currently waits 300msec for a response from content to the first touchstart and touchmove before making a decision. If content doesn't call preventDefault on either of these, MetroInput tell the apzc to go ahead and consume the touch sequence.

In the case of pointer events, do we have a similar definitive response we can rely on instead of waiting an arbitrary number of input events? I don't understand why we wait 10 events, vs. 20 or 2).. shouldn't we wait for something specific to happen like content calling preventDefault?

Comment 28

[Jim Mathies [:jimm]]

From the spec it looks like we should be able to determine this from the result of the first pointerdown event - 

The pointercancel event

A user agent MUST dispatch this event in the following circumstances:

    The user agent has determined that a pointer is unlikely to continue to produce events (for example, because of a hardware event).
    After having fired the pointerdown event, the pointer is subsequently used to manipulate the page viewport (e.g. panning or zooming).

Comment 29

[Oleg Romashin (MS)]

Comment on attachment 8365908 [details] [diff] [review]
Part 5. Add delay of dispatching PointerMove events to avoid dispatching PointerMove events at all in case touch behavior starts.

>+  // Trying to dispatch 'ignore-status' events without dealing with touch
>+  // and pointer events state machine.
>+  if (nextQueueItem->mIsIgnoreStatus) {
>+    mInputEventQueue.PopFront();
>+    HandleEventIgnoreStatus(nextQueueItem->mEvent);

I tested latest stuff with these patches and found Metro FF kinda freezing on dispatching mInputEventQueue... 

Break in MetroInput::DeliverNextQueuedEvent show mInputEventQueue size == 500 or more.

Comment 30

[Nick Lebedev [:nl]]

Attachment: Part 3. Add pointer events generation for non-touch inputs.

Comment 31

[Nick Lebedev [:nl]]

Attachment: Part 4. Add pointer events generation for touch inputs.

Comment 32

[Nick Lebedev [:nl]]

Consulting with pointer events experts I got that I misinterpreted the spec and all the stuff with delaying PointerMove events before PointerCancel. Spec doesn't state that we need to discard PointerMove events before PointerCancel and therefore i'm removing two last patches.

Comment 33

[Jim Mathies [:jimm]]

Where are we at on dom implementation, if I apply these to mc tip, can I test this on sites that consume pointer events?

Comment 34

[Nick Lebedev [:nl]]

(In reply to Jim Mathies [:jimm] from comment #33)
> Where are we at on dom implementation, if I apply these to mc tip, can I
> test this on sites that consume pointer events?

Yes, pointer events mostly will work except pointerover/enter events and setPointerCapture feature.
Btw, we're tracking it in bugs https://bugzilla.mozilla.org/show_bug.cgi?id=967796 and https://bugzilla.mozilla.org/show_bug.cgi?id=968148

Comment 35

[Nick Lebedev [:nl]]

Try server results: https://tbpl.mozilla.org/?tree=Try&rev=513417096404.
Everything seems to Ok except build on windows server 2012. It seems to be a failure within the media module and doesn't relate to touch/pointer events.

Comment 36

[Nick Lebedev [:nl]]

Try server results with touch action and pointer events pref turned On: https://tbpl.mozilla.org/?tree=Try&rev=71610484be03

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8365904 [details] [diff] [review]
Part 1. Join handling of ignore-status and not-ignore-status events to simplify delaying pointer move events in future.

># HG changeset patch
># Parent bfe4ed6d47cec0efbc671cf48175e3a36b012b11
># User Nick Lebedev <[e-mail]>
>Bug 956644. Part 1. Join handling of ignore-status and not-ignore-status events to simplify delaying pointer move events in future. r=jimm, smaug
>
>diff --git a/widget/windows/winrt/MetroInput.cpp b/widget/windows/winrt/MetroInput.cpp
>--- a/widget/windows/winrt/MetroInput.cpp
>+++ b/widget/windows/winrt/MetroInput.cpp
>@@ -225,27 +225,28 @@ namespace {
>                aData->mRotationAngle,
>                aData->mForce);
>     touches->AppendElement(copy);
>     aData->mChanged = false;
>     return PL_DHASH_NEXT;
>   }
> 
>   // Helper for making sure event ptrs get freed.
>-  class AutoDeleteEvent
>+  template<class Type>
>+  class AutoDeleteObject
>   {
>   public:
>-    AutoDeleteEvent(WidgetGUIEvent* aPtr) :
>+    AutoDeleteObject(Type aPtr) :
>       mPtr(aPtr) {}
>-    ~AutoDeleteEvent() {
>+    ~AutoDeleteObject() {
>       if (mPtr) {
>         delete mPtr;
>       }
>     }
>-    WidgetGUIEvent* mPtr;
>+    Type mPtr;
>   };
> }

Hrm, whaat. What on earth is AutoDeleteEvent.
We have nsAutoPtr for this stuff.
So, please use that, and feel free to change the current user of 
AutoDeleteEvent to use it too.

>-MetroInput::DeliverNextQueuedEventIgnoreStatus()
>+MetroInput::HandleEventIgnoreStatus(WidgetInputEvent* inputEvent)
In Gecko arguments should have a prefix. So, aInputEvent.

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8365905 [details] [diff] [review]
Part 2. Add facilities for pointer events creation.

>+   * Creates and returns a new instance of the WidgetPointerEvent.
>+   * Note: caller is responsible for freeing the memory for the Touch
>+   * returned from this function.
>+   */
>+  WidgetPointerEvent*
>+  CreatePointerEvent(const unsigned short aPointerType,
>+                     uint32_t aMessage,
>+                     nsIWidget* aWidget,
>+                     UI::Input::IPointerPoint* aPoint,
>+                     bool aIsPrimary = false) {
{ goes to the next line

>+    WidgetPointerEvent *event = new WidgetPointerEvent(true, aMessage, aWidget);
* goes with the type, not with variable name.

Comment 39

[Jim Mathies [:jimm]]

Comment on attachment 8370688 [details] [diff] [review]
Part 4. Add pointer events generation for touch inputs.

Review of attachment 8370688 [details] [diff] [review]:
-----------------------------------------------------------------

Applied these locally, don't see any issues with touch events / input.

::: widget/windows/winrt/MetroInput.cpp
@@ +1653,5 @@
>    }
>  }
>  
>  void
> +MetroInput::HandlePointerEvent(WidgetInputEvent* inputEvent)

nit - variable name

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8370684 [details] [diff] [review]
Part 3. Add pointer events generation for non-touch inputs.

>+  enum penButtonType
>+  {
>+    eNoPenButton  = 0,
>+    eBarrelButton = 2,
>+    eEraserButton = 5
>+  };
>+
>+  enum penButtonsFlag {
{ to the next line as above enum

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8370688 [details] [diff] [review]
Part 4. Add pointer events generation for touch inputs.


>   // Dispatch 'ignore-status' events without dealing with touch
>   // and pointer events state machine.
>   if (queueItem->mIsIgnoreStatus) {
>     HandleEventIgnoreStatus(queueItem->mEvent);
>-  } else {
>+  } else if (queueItem->mEvent->eventStructType == NS_TOUCH_EVENT) {
>     HandleTouchEvent(queueItem->mEvent);
>+  } else if (queueItem->mEvent->eventStructType == NS_POINTER_EVENT &&
>+             !mApzConsumingTouch) {
>+    HandlePointerEvent(queueItem->mEvent);
>   }
This needs some comment. Why are we checking mApzConsumingTouch only for pointer events and not for touch, 
yet the flag name is xxxxTouch


Trying to figure that out...

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8370688 [details] [diff] [review]
Part 4. Add pointer events generation for touch inputs.

I assume this patch relies on something not in m-c yes, since
MetroInput::HandleTouchEvent can't be found in mxr

Could you explain mApzConsumingTouch usage.

Comment 43

[Nick Lebedev [:nl]]

I believe we need to close this bug in favor of https://bugzilla.mozilla.org/show_bug.cgi?id=970964