Closed Bug 970964 Opened 6 years ago Closed 6 years ago

Implement generic mouse/touch -> Pointer events converter

Categories

(Core :: Widget, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: oleg.romashin, Assigned: oleg.romashin)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 8 obsolete files)

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.
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
Attachment #8374096 - Flags: feedback?(mbrubeck)
Attachment #8374096 - Flags: feedback?(bugs)
Blocks: pointerevent
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.
Attachment #8374096 - Flags: feedback?(bugs) → feedback+
(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
Attachment #8374096 - Attachment is obsolete: true
Attachment #8374096 - Flags: feedback?(mbrubeck)
Attachment #8375866 - Flags: review?(bugs)
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee: nobody → oleg.romashin
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;
>+}
>+
Attachment #8375866 - Flags: review?(jmathies)
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 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.
Attachment #8375866 - Flags: review?(bugs) → review-
Attachment #8375866 - Attachment is obsolete: true
Attachment #8375866 - Flags: review?(jmathies)
Attachment #8376847 - Flags: review?(bugs)
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.
Attachment #8376847 - Flags: review?(bugs) → review+
Added test for Mouse->Pointer, Touch->Pointer. events order check
Attachment #8377776 - Flags: review?(bugs)
Added more tests for Pointer Enter Leave
Attachment #8377776 - Attachment is obsolete: true
Attachment #8377776 - Flags: review?(bugs)
Attachment #8377837 - Flags: review?(bugs)
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.
(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.
(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.
Attachment #8377837 - Flags: review?(bugs) → review+
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
Attachment #8376847 - Flags: review+ → review-
(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.
Added fix for button conversion.
Attachment #8376847 - Attachment is obsolete: true
Attachment #8379361 - Flags: review?(bugs)
Added small test for button type for pointer converted from mousemove
Attachment #8377837 - Attachment is obsolete: true
Attachment #8379362 - Flags: review+
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?
Attachment #8379361 - Flags: review?(bugs) → review-
Added buttons initialization for touch genenrated event.
Attachment #8379361 - Attachment is obsolete: true
Attachment #8381164 - Flags: review?(bugs)
Added simple test for touch generated pointer event button[s]
Attachment #8379362 - Attachment is obsolete: true
Attachment #8381165 - Flags: review?(bugs)
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.)
> 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
Attachment #8381164 - Flags: review?(bugs) → review+
Attachment #8381165 - Flags: review?(bugs) → review+
Comment on attachment 8381164 [details] [diff] [review]
Mouse/Touch -> pointers conversion

Jimm, could you check widget part.
Attachment #8381164 - Flags: review?(jmathies)
Added preference
Attachment #8381164 - Attachment is obsolete: true
Attachment #8381164 - Flags: review?(jmathies)
Attachment #8381702 - Flags: review?(jmathies)
Comment on attachment 8381702 [details] [diff] [review]
Mouse/Touch -> pointers conversion

Carry over smaug review from previous version of the patch
Attachment #8381702 - Flags: review+
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.
Attachment #8381702 - Flags: review?(jmathies) → review+
https://hg.mozilla.org/mozilla-central/rev/788141812826
https://hg.mozilla.org/mozilla-central/rev/3d58be18b53a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Depends on: 979497
Blocks: 979314
No longer blocks: 979314
Blocks: 1153135
You need to log in before you can comment on or make changes to this bug.