970964 - Implement generic mouse/touch -> Pointer events converter

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Oleg Romashin (MS)]

I think it would make sense to implement generic converter of Mouse/Touch events into Pointer events in Cross-Platform space, like nsView.cpp.

Idea is to catch all mouse/touch WidgetEvents and if those allowed to be converted  into pointer events, then Dispatch Pointer event.

nsIWidget implementation which may have original pointer events dispatching should set special flag on Touch/Mouse events in order to prevent mouse/touch conversion in cross-platform space.

Comment 1

[Oleg Romashin (MS)]

Attachment: Mouse/Touch -> pointers conversion.

Tested this mostly on Win32 - non metro backend. This works ok with Touch/Mouse/Pen. also dispatching pointer events by default in this mode does not break any tests

Comment 2

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

Comment on attachment 8374096 [details] [diff] [review]
Mouse/Touch -> pointers conversion.

Could you move this stuff to PresShell. I'd like to get rid of 
all the view/* stuff. It is after all just quite a thin layer between
widget and presshell.

NS_MOUSEENTER/LEAVE shouldn't be handled at all, and for NS_MOUSE_ENTER/LEAVE you want something else than NS_POINTER_ENTER/LEAVE I think.

event = *mouseEvent; looks a bit scary. Could you use AssignEventData there?

Coding style is 
} else if (foo) {



But yes, I like this kind of approach.

Comment 3

[Oleg Romashin (MS)]

(In reply to Olli Pettay [:smaug] from comment #2)
> Comment on attachment 8374096 [details] [diff] [review]
> Mouse/Touch -> pointers conversion.
> 
> Could you move this stuff to PresShell. I'd like to get rid of 
> all the view/* stuff. It is after all just quite a thin layer between
> widget and presshell.
> 
> NS_MOUSEENTER/LEAVE shouldn't be handled at all, and for
hmm in Android Widget we do this: http://mxr.mozilla.org/mozilla-central/source/widget/android/AndroidJavaWrappers.cpp#758

why do we dispatch it here?

> NS_MOUSE_ENTER/LEAVE you want something else than NS_POINTER_ENTER/LEAVE I
> think.

What should it be then?
I saw you set r+ on making NS_POINTER_LEAVE when NS_MOUSE_EXIT dispatched...
https://bug956644.bugzilla.mozilla.org/attachment.cgi?id=8370684

Comment 4

[Oleg Romashin (MS)]

Attachment: Mouse/Touch -> pointers conversion.

Comment 5

[Oleg Romashin (MS)]

Comment on attachment 8375866 [details] [diff] [review]
Mouse/Touch -> pointers conversion.


>+Touch::Touch(int32_t aIdentifier,
>+             nsIntPoint aPoint,
>+             nsIntPoint aRadius,
>+             float aRotationAngle,
>+             float aForce,
>+             float aTiltX,
>+             float aTiltY)
>+  : Touch(aIdentifier, aPoint, aRadius, aRotationAngle, aForce)
>+  , tiltX(aTiltX)
>+  , tiltY(aTiltY)
>+
>+{

this should go here:
  Touch(aIdentifier, aPoint, aRadius, aRotationAngle, aForce)
  tiltX = aTiltX;
  tiltY = aTiltY;
>+}
>+

Comment 6

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

Comment on attachment 8375866 [details] [diff] [review]
Mouse/Touch -> pointers conversion.

>+static nsresult
>+DispatchWrappedPointerEvent(PresShell* aShell,
Odd name for the method. Nothing is wrapped here.

>+                            nsIFrame* aFrame,
>+                            WidgetGUIEvent* aEvent,
>+                            bool aDontRetargetEvents,
>+                            nsEventStatus* aEventStatus)
>+{
>+  uint32_t pointerMessage = 0;
>+  if (aEvent->eventStructType == NS_MOUSE_EVENT) {
>+    WidgetMouseEvent* mouseEvent = aEvent->AsMouseEvent();
>+    // if it is not mouse then it is likely will come as touch event
>+    switch (mouseEvent->message) {
>+    case NS_MOUSE_MOVE:
>+      pointerMessage = NS_POINTER_MOVE;
>+      break;
>+    case NS_MOUSE_BUTTON_UP:
>+      pointerMessage = NS_POINTER_UP;
>+      break;
>+    case NS_MOUSE_BUTTON_DOWN:
>+      pointerMessage = NS_POINTER_DOWN;
>+      break;
>+    default:
>+      break;
>+    }
>+    if (pointerMessage && mouseEvent->convertToPointer) {
>+      WidgetPointerEvent event(*mouseEvent);
>+      event.message = pointerMessage;
>+      return aShell->HandleEvent(aFrame, &event, aDontRetargetEvents, aEventStatus);
>+    }
>+  }
>+  else if (aEvent->eventStructType == NS_TOUCH_EVENT) {
} else if (...) {


I don't yet quite see what guarantees that we get the same order of event in all the platforms.
But I guess there will be tests to check that on all the platform
(in case we have touch event or only mouse event)

>+  nsEventStatus pointerEventStatus; 
Initialize to some value

Comment 7

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

Comment on attachment 8375866 [details] [diff] [review]
Mouse/Touch -> pointers conversion.

>+      case NS_TOUCH_ENTER:
>+        pointerMessage = NS_POINTER_ENTER;
>+        break;
>+      case NS_TOUCH_LEAVE:
>+        pointerMessage = NS_POINTER_LEAVE;
>+        break;
Hmm, so enter/leave should be dispatched by EventStateManager, since only that
knows where to dispatch them all.

Comment 8

[Oleg Romashin (MS)]

Attachment: Mouse/Touch -> pointers conversion

Comment 9

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

Comment on attachment 8376847 [details] [diff] [review]
Mouse/Touch -> pointers conversion

>+WinUtils::GetIsMouseFromTouch(uint32_t aEventType)
>+{
>+#define MOUSEEVENTF_FROMTOUCH 0xFF515700
>+  return (aEventType == NS_MOUSE_BUTTON_DOWN ||
>+          aEventType == NS_MOUSE_BUTTON_UP ||
>+          aEventType == NS_MOUSE_MOVE)
>+          && (GetMessageExtraInfo() & MOUSEEVENTF_FROMTOUCH) == MOUSEEVENTF_FROMTOUCH;
&& to the previous line


(And huh, MOUSEEVENTF_FROMTOUCH is horrible name, but coming from Windows)


> NS_METHOD nsWindow::RegisterTouchWindow() {
>-  if (Preferences::GetInt("dom.w3c_touch_events.enabled", 0)) {
>+  if (Preferences::GetInt("dom.w3c_touch_events.enabled", 0) || Preferences::GetBool("dom.w3c_pointer_events.enabled", false)) {
overlong line


>+  event.convertToPointer = aInputSource == nsIDOMMouseEvent::MOZ_SOURCE_PEN || !(WinUtils::GetIsMouseFromTouch(aEventType) && mTouchWindow);
ditto

>+  if (Preferences::GetBool("dom.w3c_pointer_events.enabled", false)) {
Could we please add some pref cache and just check static boolean value.
That way we don't have to copy-paste the pref name everywhere.

But I want to see some tests, for the event ordering and stuff like pointerenter/leave (which should be generated from pointermove) etc.
So, don't land this patch without tests.

Comment 10

[Oleg Romashin (MS)]

Attachment: Mouse/Touch -> pointers conversion. Tests

Added test for Mouse->Pointer, Touch->Pointer. events order check

Comment 11

[Oleg Romashin (MS)]

Attachment: Mouse/Touch -> pointers conversion. Tests

Added more tests for Pointer Enter Leave

Comment 12

[Nick Lebedev [:nl]]

A note to the Oleg's patch:

currently we don't set Primary attribute properly and i would advice to set it in the following way:
1. set isPrimary attribute to true for pointer events generated by mouse and pen (in the nsPresShell level).
2. check TouchStart events and if TouchStart event has only one touch - remember its pointerId as primary and set primary attribute for all future touches with that pointerId.

I'm not sure how we should behave in mixed scenarious (when both mouse and touch are available) and whether such scenarios are possible. I believe mouse should be preferable in such cases.

Comment 13

[Matt Brubeck (:mbrubeck)]

(In reply to Nick Lebedev [:nl] from comment #12)
> I'm not sure how we should behave in mixed scenarious (when both mouse and
> touch are available) and whether such scenarios are possible. I believe
> mouse should be preferable in such cases.

When multiple device types are active, there should be one primary pointer for each device type.  For example, if the mouse and touch screen are both generating events, both the mouse pointer *and* the first touch point should have isPrimary = true.

Comment 14

[Oleg Romashin (:romaxa)]

(In reply to Nick Lebedev [:nl] from comment #12)
> A note to the Oleg's patch:
> 
> currently we don't set Primary attribute properly and i would advice to set
> it in the following way:
> 1. set isPrimary attribute to true for pointer events generated by mouse and
> pen (in the nsPresShell level).

primary is set by default for all Pointer events in CTOR. so mouse will get it by default.

> 2. check TouchStart events and if TouchStart event has only one touch -
> remember its pointerId as primary and set primary attribute for all future
> touches with that pointerId.

this approach not very effective because we may get Touch event with > 1 touches in first place... current approach looks right to me because we rely on widget code which supposed to place first(primary) touch in first place inside touch array.

> I'm not sure how we should behave in mixed scenarious (when both mouse and
> touch are available) and whether such scenarios are possible. I believe
> mouse should be preferable in such cases.
I think we should not... on Win32 when touch event generated, we have mouse event generated by Win32 from primary touch. and we exclude pointer conversion from such mouse event.

Comment 15

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

Comment on attachment 8376847 [details] [diff] [review]
Mouse/Touch -> pointers conversion

Unless I'm missing something, this needs still right conversion for .button and .buttons
https://bugzilla.mozilla.org/show_bug.cgi?id=970199#c17

Comment 16

[[e-mail]]

(In reply to Nick Lebedev [:nl] from comment #12)
> I'm not sure how we should behave in mixed scenarious (when both mouse and
> touch are available) and whether such scenarios are possible. I believe
> mouse should be preferable in such cases.
Android will happily send mouse, touch, and (if you've got a third hand free, pen) events simultaneously all as MotionEvents. X11/GTK+ is also theoretically capable, sending both tool-specific events (which are currently disabled: bug 469756) and generic mouse events (which we can ignore if that bug is fixed).

I'm not sure if simultaneous input from different types is important enough to explicitly support, but we shouldn't box ourselves into a corner in case some intrepid developer decides to provide a patch in the future.

Comment 17

[Oleg Romashin (MS)]

Attachment: Mouse/Touch -> pointers conversion

Added fix for button conversion.

Comment 18

[Oleg Romashin (MS)]

Attachment: 8377837: Mouse/Touch -> pointers conversion. Tests

Added small test for button type for pointer converted from mousemove

Comment 19

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

Comment on attachment 8379361 [details] [diff] [review]
Mouse/Touch -> pointers conversion

>+      WidgetPointerEvent event(touchEvent->mFlags.mIsTrusted, pointerMessage, touchEvent->widget);
>+      event.isPrimary = i == 0;
>+      event.pointerId = touch->Identifier();
>+      event.refPoint.x = touch->mRefPoint.x;
>+      event.refPoint.y = touch->mRefPoint.y;
>+      event.modifiers = touchEvent->modifiers;
>+      event.width = touch->RadiusX();
>+      event.height = touch->RadiusY();
>+      event.tiltX = touch->tiltX;
>+      event.tiltY = touch->tiltY;
>+      event.time = touchEvent->time;
>+      event.mFlags = touchEvent->mFlags;
>+      event.inputSource = nsIDOMMouseEvent::MOZ_SOURCE_TOUCH;
>+      event.convertToPointer = touch->convertToPointer = false;
>+      aShell->HandleEvent(aFrame, &event, aDontRetargetEvents, aStatus);
>+    }

I don't understand why this sets the right .button and .buttons.
Doesn't this end up to .button == 0 and .buttons == 0 ?

Do we not have tests for that yet?

Comment 20

[Oleg Romashin (MS)]

Attachment: Mouse/Touch -> pointers conversion

Added buttons initialization for touch genenrated event.

Comment 21

[Oleg Romashin (MS)]

Attachment: Mouse/Touch -> pointers conversion. Tests

Added simple test for touch generated pointer event button[s]

Comment 22

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

Comment on attachment 8381164 [details] [diff] [review]
Mouse/Touch -> pointers conversion

DispatchPointerFromMouseOrTouch should btw check the pref whether pointer events are enabled.
(looking at this more in a moment.)

Comment 23

[Oleg Romashin (MS)]

> DispatchPointerFromMouseOrTouch should btw check the pref whether pointer
Ok I added it into my queue, already. http://hg.mozilla.org/users/romaxa_gmail.com/pointer-events-queue/file/default/b970964_mouse_touch_to_pointer_basic.diff#l72

Comment 24

[Oleg Romashin (MS)]

Comment on attachment 8381164 [details] [diff] [review]
Mouse/Touch -> pointers conversion

Jimm, could you check widget part.

Comment 25

[Oleg Romashin (MS)]

Attachment: Mouse/Touch -> pointers conversion

Added preference

Comment 26

[Oleg Romashin (MS)]

Comment on attachment 8381702 [details] [diff] [review]
Mouse/Touch -> pointers conversion

Carry over smaug review from previous version of the patch

Comment 27

[Jim Mathies [:jimm]]

Comment on attachment 8381702 [details] [diff] [review]
Mouse/Touch -> pointers conversion

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

::: widget/windows/WinUtils.cpp
@@ +564,5 @@
> +#define MOUSEEVENTF_FROMTOUCH 0xFF515700
> +  return (aEventType == NS_MOUSE_BUTTON_DOWN ||
> +          aEventType == NS_MOUSE_BUTTON_UP ||
> +          aEventType == NS_MOUSE_MOVE) &&
> +          (GetMessageExtraInfo() & MOUSEEVENTF_FROMTOUCH) == MOUSEEVENTF_FROMTOUCH;

Checking (GetMessageExtraInfo() & MOUSEEVENTF_FROMTOUCH) should suffice, no need for the added |== MOUSEEVENTF_FROMTOUCH|.

::: widget/windows/nsWindow.cpp
@@ +282,5 @@
>  // we will always display a resize cursor in, regardless of the underlying
>  // content.
>  static const int32_t kResizableBorderMinSize = 3;
>  
> +static bool gIsPointerEventsEnabled = false;

nit - A short comment here explaining what this pertains to might be helpful.

Comment 28

[Oleg Romashin (MS)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d58be18b53a
https://hg.mozilla.org/integration/mozilla-inbound/rev/788141812826

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/788141812826
https://hg.mozilla.org/mozilla-central/rev/3d58be18b53a