Closed Bug 956644 Opened 7 years ago Closed 7 years ago

Implement pointer events at MetroWidget level

Categories

(Core Graveyard :: Widget: WinRT, defect)

All
Windows 8
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED INVALID

People

(Reporter: nl, Assigned: nl)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 6 obsolete files)

9.99 KB, patch
smaug
: review+
jimm
: review+
Details | Diff | Splinter Review
3.98 KB, patch
jimm
: review+
smaug
: review+
Details | Diff | Splinter Review
10.27 KB, patch
jimm
: review+
smaug
: review+
Details | Diff | Splinter Review
9.82 KB, patch
jimm
: review+
smaug
: review-
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/31.0.1650.63 Safari/537.36
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.
Blocks: pointerevent
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.
Attachment #8356071 - Flags: feedback?(romaxa)
Attachment #8356071 - Flags: feedback?(bugs)
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.
(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 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.
Attachment #8356071 - Flags: feedback?(bugs)
> 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)
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 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
Component: Untriaged → Widget: WinRT
Product: Firefox → Core
Hardware: x86_64 → All
Version: unspecified → Trunk
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #8356071 - Flags: feedback?(romaxa) → feedback+
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.
Attachment #8356071 - Attachment is obsolete: true
Attachment #8361650 - Flags: feedback?(bugs)
In theory we should delay only 2 pointers, (pan 1, zoom +1), if we got more pointers, then we probably should stop delaying.
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.
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 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.
Attachment #8361650 - Flags: feedback+
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
Attachment #8365905 - Flags: review?(jmathies)
Attachment #8365905 - Flags: review?(bugs)
Attachment #8365906 - Flags: review?(jmathies)
Attachment #8365906 - Flags: review?(bugs)
Attachment #8365907 - Flags: review?(jmathies)
Attachment #8365907 - Flags: review?(bugs)
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.
Attachment #8365909 - Flags: review?(jmathies)
Attachment #8365909 - Flags: review?(bugs)
Also seems like win8 touch events aren't broken by these patches: https://tbpl.mozilla.org/?tree=Try&rev=91be5850baee
Depends on: 964675
Depends on: 964716
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.
Attachment #8365904 - Flags: review?(jmathies) → review+
Attachment #8365905 - Flags: review?(jmathies) → review+
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.
Attachment #8365906 - Flags: review?(jmathies) → review+
Attachment #8365907 - Flags: review?(jmathies) → review+
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 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.
Attachment #8365909 - Flags: review?(jmathies) → review-
(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.
(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?
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 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.
Attachment #8365906 - Attachment is obsolete: true
Attachment #8365907 - Attachment is obsolete: true
Attachment #8365908 - Attachment is obsolete: true
Attachment #8365909 - Attachment is obsolete: true
Attachment #8365906 - Flags: review?(bugs)
Attachment #8365907 - Flags: review?(bugs)
Attachment #8365908 - Flags: review?(jmathies)
Attachment #8365908 - Flags: review?(bugs)
Attachment #8365909 - Flags: review?(bugs)
Attachment #8370684 - Flags: review?(jmathies)
Attachment #8370684 - Flags: review?(bugs)
Attachment #8370688 - Flags: review?(jmathies)
Attachment #8370688 - Flags: review?(bugs)
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.
Blocks: 964675
No longer depends on: 964675
Attachment #8370684 - Flags: review?(jmathies) → review+
Where are we at on dom implementation, if I apply these to mc tip, can I test this on sites that consume pointer events?
(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
Assignee: nobody → nicklebedev37
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.
Try server results with touch action and pointer events pref turned On: https://tbpl.mozilla.org/?tree=Try&rev=71610484be03
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 <nicklebedev37@gmail.com>
>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.
Attachment #8365904 - Flags: review?(bugs) → review+
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.
Attachment #8365905 - Flags: review?(bugs) → review+
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
Attachment #8370688 - Flags: review?(jmathies) → review+
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
Attachment #8370684 - Flags: review?(bugs) → review+
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 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.
Attachment #8370688 - Flags: review?(bugs) → review-
Summary: Implement pointer events at nsIWidget level → Implement pointer events at MetroWidget level
I believe we need to close this bug in favor of https://bugzilla.mozilla.org/show_bug.cgi?id=970964
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INVALID
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.