If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Implement generic mouse/touch -> Pointer events converter

RESOLVED FIXED in mozilla30

Status

()

Core
Widget
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Oleg Romashin (MS), Assigned: Oleg Romashin (MS))

Tracking

(Blocks: 1 bug)

Trunk
mozilla30
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 8 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 8374096 [details] [diff] [review]
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
Attachment #8374096 - Flags: feedback?(mbrubeck)
Attachment #8374096 - Flags: feedback?(bugs)
(Assignee)

Updated

4 years ago
Blocks: 822898

Comment 2

4 years ago
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+
(Assignee)

Comment 3

4 years ago
(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
(Assignee)

Comment 4

4 years ago
Created attachment 8375866 [details] [diff] [review]
Mouse/Touch -> pointers conversion.
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
(Assignee)

Comment 5

4 years ago
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;
>+}
>+
(Assignee)

Updated

4 years ago
Attachment #8375866 - Flags: review?(jmathies)

Comment 6

4 years ago
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

4 years ago
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-
(Assignee)

Comment 8

4 years ago
Created attachment 8376847 [details] [diff] [review]
Mouse/Touch -> pointers conversion
Attachment #8375866 - Attachment is obsolete: true
Attachment #8375866 - Flags: review?(jmathies)
Attachment #8376847 - Flags: review?(bugs)

Comment 9

4 years ago
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+
(Assignee)

Comment 10

4 years ago
Created attachment 8377776 [details] [diff] [review]
Mouse/Touch -> pointers conversion. Tests

Added test for Mouse->Pointer, Touch->Pointer. events order check
Attachment #8377776 - Flags: review?(bugs)
(Assignee)

Comment 11

4 years ago
Created attachment 8377837 [details] [diff] [review]
Mouse/Touch -> pointers conversion. Tests

Added more tests for Pointer Enter Leave
Attachment #8377776 - Attachment is obsolete: true
Attachment #8377776 - Flags: review?(bugs)
Attachment #8377837 - Flags: review?(bugs)

Comment 12

4 years ago
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.

Updated

4 years ago
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.
(Assignee)

Comment 17

4 years ago
Created attachment 8379361 [details] [diff] [review]
Mouse/Touch -> pointers conversion

Added fix for button conversion.
Attachment #8376847 - Attachment is obsolete: true
Attachment #8379361 - Flags: review?(bugs)
(Assignee)

Comment 18

4 years ago
Created attachment 8379362 [details] [diff] [review]
8377837: Mouse/Touch -> pointers conversion. Tests

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-
(Assignee)

Comment 20

4 years ago
Created attachment 8381164 [details] [diff] [review]
Mouse/Touch -> pointers conversion

Added buttons initialization for touch genenrated event.
Attachment #8379361 - Attachment is obsolete: true
Attachment #8381164 - Flags: review?(bugs)
(Assignee)

Comment 21

4 years ago
Created attachment 8381165 [details] [diff] [review]
Mouse/Touch -> pointers conversion. Tests

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.)
(Assignee)

Comment 23

4 years ago
> 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

Updated

4 years ago
Attachment #8381164 - Flags: review?(bugs) → review+

Updated

4 years ago
Attachment #8381165 - Flags: review?(bugs) → review+
(Assignee)

Comment 24

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

Jimm, could you check widget part.
Attachment #8381164 - Flags: review?(jmathies)
(Assignee)

Comment 25

4 years ago
Created attachment 8381702 [details] [diff] [review]
Mouse/Touch -> pointers conversion

Added preference
Attachment #8381164 - Attachment is obsolete: true
Attachment #8381164 - Flags: review?(jmathies)
(Assignee)

Updated

4 years ago
Attachment #8381702 - Flags: review?(jmathies)
(Assignee)

Comment 26

4 years ago
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 27

4 years ago
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+
(Assignee)

Comment 28

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d58be18b53a
https://hg.mozilla.org/integration/mozilla-inbound/rev/788141812826
https://hg.mozilla.org/mozilla-central/rev/788141812826
https://hg.mozilla.org/mozilla-central/rev/3d58be18b53a
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30

Updated

4 years ago
Depends on: 979497

Updated

4 years ago
Blocks: 979314

Updated

4 years ago
No longer blocks: 979314

Updated

3 years ago
Blocks: 1153135
You need to log in before you can comment on or make changes to this bug.