1324956 - [Pointer Event] Fill pen attributes on Linux

Status: NEW

(Core :: DOM: UI Events & Focus Handling, enhancement, P3)

Comment 1

[Ming-Chou Shih [:stone]]

Attachment: [Pointer Event] Fill pen attributes on Linux

Comment 2

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

Comment on attachment 8820672 [details] [diff] [review]
[Pointer Event] Fill pen attributes on Linux

This is so gtk specific so karlt might want to take a look.

Why the GtkPointerEvents class, especially as a member variable.
All the methods in it could be just static and then code would be a bit easier to read if one could do something like GtkPointerEvents::GetEventSource(aEvent); and not mPointerEvents.GetEventSource(aEvent)


>+bool
>+GtkPointerEvents::ShouldIgnoreButtonReleaseEvent(GdkEventButton *aEvent)
Nit, * should go with the type.

+  uint16_t ConvertInputSource(GdkInputSource aGdkInputSource) {
Nit, { should be on its own line

>     WidgetMouseEvent event(true, eMouseEnterIntoWidget, this,
>                            WidgetMouseEvent::eReal);
> 
>     event.mRefPoint = GdkEventCoordsToDevicePixels(aEvent->x, aEvent->y);
>     event.AssignEventTime(GetWidgetEventTime(aEvent->time));
>-
>+    mPointerEvents.GetEventSource((GdkEvent*)aEvent);
Why code like this when we don't use the return value of the method at all?



>     event.mExitFrom = is_top_level_mouse_exit(mGdkWindow, aEvent)
>         ? WidgetMouseEvent::eTopLevel : WidgetMouseEvent::eChild;
>+    mPointerEvents.GetEventSource((GdkEvent*)aEvent);
ditto.

Are you missing to assign the return value to event's inputSource?

Comment 3

[Ming-Chou Shih [:stone]]

Attachment: [Pointer Event] Fill pen attributes on Linux

Comment 4

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

Comment on attachment 8820991 [details] [diff] [review]
[Pointer Event] Fill pen attributes on Linux


>+GtkPointerEvents::ShouldIgnoreButtonPressEvent(GdkEventButton* aEvent,
>+                                               guint aButtonState)
>+{
>+#if GTK_CHECK_VERSION(3,4,0)
>+  GdkDevice* device = gdk_event_get_source_device((GdkEvent*)aEvent);
>+  uint16_t source = ConvertInputSource(gdk_device_get_source(device));
>+  if (aButtonState && source != sLastPressButtonSource) {
>+    // This could be happened when press button on different input devices.
>+    // (e.g. press and hold mouse left button, and press button on pen). In that
>+    // case, we should ignore the button press and release events.
>+    return true;
>+  }
So why do we need or want this behavior? I assume
aButtonState is 0 when there aren't existing buttons pressed so that we at least some times end up processing events from other sources.
Could we have several gButtonState variable, one for mouse and one for pen?

>+  sLastPressButtonSource = source;

And this method has weird side effect, it sets sLastPressButtonSource. But don't have good suggestion how to improve this.



I think I need some more information before reviewing this.

Comment 5

[Ming-Chou Shih [:stone]]

Tested multiple (mouse and pen) input devices simultaneously on following platforms, and the results are
1) Mac 10.11.5
   button up from another input device can cancel the button pressed state
   drag item can be moved by another input device

2) Linux 16.04
   button up from another input device can't cancel the button pressed state
   drag item can be moved by another input device

3) Windows 10
   button up from another input device can cancel the button pressed state
   drag item can be moved by another input device

Tested Chrome Canary on Windows10 and it behaved the same as Mac and Windows10.
Tested Edge on Windows10 and the dnd is not impacted by another input device. In that case, dnd is not working with the 2nd input device but click is working.

Created https://github.com/w3c/pointerevents/issues/164 on github

Comment 6

[Ming-Chou Shih [:stone]]

Comment on attachment 8820991 [details] [diff] [review]
[Pointer Event] Fill pen attributes on Linux

Cancel the review request since the behavior of simultaneously controlling multiple pointer devices is unclear.

Comment 7

[Ming-Chou Shih [:stone]]

Although we didn't have the conclusion about how to handle compatibility mouse events in the case of simultaneously controlling multiple pointer devices, it makes sense to me to handle pointer events of each device independently. I'd try to refine the patch to meet it.

Comment 8

[Ming-Chou Shih [:stone]]

Attachment: [Pointer Event] Fill pen attributes on Linux

Comment 9

[Ming-Chou Shih [:stone]]

Comment on attachment 8822603 [details] [diff] [review]
[Pointer Event] Fill pen attributes on Linux

About the behaviors of compatibility mouse events for multiple pointer devices, I'd handle it in the follow up once the behavior is clarified.

Comment 10

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

Comment on attachment 8822603 [details] [diff] [review]
[Pointer Event] Fill pen attributes on Linux

>     // Check before is_parent_ungrab_enter() as the button state may have
>     // changed while a non-Gecko ancestor window had a pointer grab.
>-    DispatchMissedButtonReleases(aEvent);
>+    DispatchMissedButtonReleases(aEvent, nsIDOMMouseEvent::MOZ_SOURCE_MOUSE);
>+    DispatchMissedButtonReleases(aEvent, nsIDOMMouseEvent::MOZ_SOURCE_PEN);
> 
>     if (is_parent_ungrab_enter(aEvent))
>         return;
> 
>     WidgetMouseEvent event(true, eMouseEnterIntoWidget, this,
>                            WidgetMouseEvent::eReal);
> 
>     event.mRefPoint = GdkEventCoordsToDevicePixels(aEvent->x, aEvent->y);
>     event.AssignEventTime(GetWidgetEventTime(aEvent->time));
>-
>+    event.inputSource = GtkPointerEvents::GetEventSource((GdkEvent*)aEvent);
>     LOG(("OnEnterNotify: %p\n", (void *)this));
> 
>     DispatchInputEvent(&event);
> }
> 
> // XXX Is this the right test for embedding cases?
> static bool
> is_top_level_mouse_exit(GdkWindow* aWindow, GdkEventCrossing *aEvent)
>@@ -2584,16 +2586,17 @@ nsWindow::OnLeaveNotifyEvent(GdkEventCro
>     WidgetMouseEvent event(true, eMouseExitFromWidget, this,
>                            WidgetMouseEvent::eReal);
> 
>     event.mRefPoint = GdkEventCoordsToDevicePixels(aEvent->x, aEvent->y);
>     event.AssignEventTime(GetWidgetEventTime(aEvent->time));
> 
>     event.mExitFrom = is_top_level_mouse_exit(mGdkWindow, aEvent)
>         ? WidgetMouseEvent::eTopLevel : WidgetMouseEvent::eChild;
>+    event.inputSource = GtkPointerEvents::GetEventSource((GdkEvent*)aEvent);
> 
>     LOG(("OnLeaveNotify: %p\n", (void *)this));
> 
>     DispatchInputEvent(&event);
> }
> 
> template <typename Event> static LayoutDeviceIntPoint
> GetRefPoint(nsWindow* aWindow, Event* aEvent)
>@@ -2670,35 +2673,39 @@ nsWindow::OnMotionNotifyEvent(GdkEventMo
>         event.mRefPoint = GetRefPoint(this, aEvent);
> 
>         modifierState = aEvent->state;
> 
>         event.AssignEventTime(GetWidgetEventTime(aEvent->time));
>     }
> 
>     KeymapWrapper::InitInputEvent(event, modifierState);
>+    GtkPointerEvents::FillPointerInfo((GdkEvent*)aEvent, event);
> 
>     DispatchInputEvent(&event);
> }
> 
> // If the automatic pointer grab on ButtonPress has deactivated before
> // ButtonRelease, and the mouse button is released while the pointer is not
> // over any a Gecko window, then the ButtonRelease event will not be received.
> // (A similar situation exists when the pointer is grabbed with owner_events
> // True as the ButtonRelease may be received on a foreign [plugin] window).
> // Use this method to check for released buttons when the pointer returns to a
> // Gecko window.
> void
>-nsWindow::DispatchMissedButtonReleases(GdkEventCrossing *aGdkEvent)
>-{
>-    guint changed = aGdkEvent->state ^ gButtonState;
>+nsWindow::DispatchMissedButtonReleases(GdkEventCrossing *aGdkEvent,
>+                                       uint16_t aSource)
>+{
>+    guint& buttonState = aSource == nsIDOMMouseEvent::MOZ_SOURCE_MOUSE ?
>+                           gMouseButtonState : gPenButtonState;
>+    guint changed = aGdkEvent->state ^ buttonState;
>     // Only consider button releases.
>     // (Ignore button presses that occurred outside Gecko.)
>-    guint released = changed & gButtonState;
>-    gButtonState = aGdkEvent->state;
>+    guint released = changed & buttonState;
>+    buttonState = aGdkEvent->state;
> 
>     // Loop over each button, excluding mouse wheel buttons 4 and 5 for which
>     // GDK ignores releases.
>     for (guint buttonMask = GDK_BUTTON1_MASK;
>          buttonMask <= GDK_BUTTON3_MASK;
>          buttonMask <<= 1) {
> 
>         if (released & buttonMask) {
>@@ -2721,16 +2728,17 @@ nsWindow::DispatchMissedButtonReleases(G
> 
>             // Dispatch a synthesized button up event to tell Gecko about the
>             // change in state.  This event is marked as synthesized so that
>             // it is not dispatched as a DOM event, because we don't know the
>             // position, widget, modifiers, or time/order.
>             WidgetMouseEvent synthEvent(true, eMouseUp, this,
>                                         WidgetMouseEvent::eSynthesized);
>             synthEvent.button = buttonType;
>+            synthEvent.inputSource = aSource;
>             DispatchInputEvent(&synthEvent);
>         }
>     }
> }
> 
> void
> nsWindow::InitButtonEvent(WidgetMouseEvent& aEvent,
>                           GdkEventButton* aGdkEvent)
>@@ -2835,32 +2843,36 @@ nsWindow::OnButtonPressEvent(GdkEventBut
>         return;
>     case 9:
>         DispatchCommandEvent(nsGkAtoms::Forward);
>         return;
>     default:
>         return;
>     }
> 
>-    gButtonState |= ButtonMaskFromGDKButton(aEvent->button);
>+    uint16_t source = GtkPointerEvents::GetEventSource((GdkEvent*)aEvent);
>+    guint& buttonState = source == nsIDOMMouseEvent::MOZ_SOURCE_MOUSE ?
>+                           gMouseButtonState : gPenButtonState;
>+    buttonState |= ButtonMaskFromGDKButton(aEvent->button);
> 
>     WidgetMouseEvent event(true, eMouseDown, this, WidgetMouseEvent::eReal);
>     event.button = domButton;
>     InitButtonEvent(event, aEvent);
>     event.pressure = mLastMotionPressure;
>-
>+    GtkPointerEvents::FillPointerInfo((GdkEvent*)aEvent, event);
>     DispatchInputEvent(&event);
> 
>     // right menu click on linux should also pop up a context menu
>     if (domButton == WidgetMouseEvent::eRightButton &&
>         MOZ_LIKELY(!mIsDestroyed)) {
>         WidgetMouseEvent contextMenuEvent(true, eContextMenu, this,
>                                           WidgetMouseEvent::eReal);
>         InitButtonEvent(contextMenuEvent, aEvent);
>         contextMenuEvent.pressure = mLastMotionPressure;
>+        GtkPointerEvents::FillPointerInfo((GdkEvent*)aEvent, contextMenuEvent);
>         DispatchInputEvent(&contextMenuEvent);
>     }
> }
> 
> void
> nsWindow::OnButtonReleaseEvent(GdkEventButton *aEvent)
> {
>     LOG(("Button %u release on %p\n", aEvent->button, (void *)this));
>@@ -2875,25 +2887,29 @@ nsWindow::OnButtonReleaseEvent(GdkEventB
>         break;
>     case 3:
>         domButton = WidgetMouseEvent::eRightButton;
>         break;
>     default:
>         return;
>     }
> 
>-    gButtonState &= ~ButtonMaskFromGDKButton(aEvent->button);
>+    uint16_t source = GtkPointerEvents::GetEventSource((GdkEvent*)aEvent);
>+    guint& buttonState = source == nsIDOMMouseEvent::MOZ_SOURCE_MOUSE ?
>+                           gMouseButtonState : gPenButtonState;
>+    buttonState &= ~ButtonMaskFromGDKButton(aEvent->button);
> 
>     WidgetMouseEvent event(true, eMouseUp, this,
>                            WidgetMouseEvent::eReal);
>     event.button = domButton;
>     InitButtonEvent(event, aEvent);
>     gdouble pressure = 0;
>     gdk_event_get_axis ((GdkEvent*)aEvent, GDK_AXIS_PRESSURE, &pressure);
>     event.pressure = pressure ? pressure : mLastMotionPressure;
>+    GtkPointerEvents::FillPointerInfo((GdkEvent*)aEvent, event);
> 
>     DispatchInputEvent(&event);
>     mLastMotionPressure = pressure;
> }
> 
> void
> nsWindow::OnContainerFocusInEvent(GdkEventFocus *aEvent)
> {
>@@ -3475,17 +3491,17 @@ nsWindow::OnTouchEvent(GdkEventTouch* aE
> 
>     LayoutDeviceIntPoint touchPoint = GetRefPoint(this, aEvent);
> 
>     int32_t id;
>     RefPtr<dom::Touch> touch;
>     if (mTouches.Remove(aEvent->sequence, getter_AddRefs(touch))) {
>         id = touch->mIdentifier;
>     } else {
>-        id = ++gLastTouchID & 0x7FFFFFFF;
>+        id = GtkPointerEvents::GetPointerId(GDK_SOURCE_TOUCHSCREEN);
>     }
> 
>     touch = new dom::Touch(id, touchPoint, LayoutDeviceIntPoint(1, 1),
>                            0.0f, 0.0f);
> 
>     WidgetTouchEvent event(true, msg, this);
>     KeymapWrapper::InitInputEvent(event, aEvent->state);
>     event.mTime = aEvent->time;
>diff --git a/widget/gtk/nsWindow.h b/widget/gtk/nsWindow.h
>--- a/widget/gtk/nsWindow.h
>+++ b/widget/gtk/nsWindow.h
>@@ -537,17 +537,18 @@ private:
>     float              mLastMotionPressure;
> 
>     // Remember the last sizemode so that we can restore it when
>     // leaving fullscreen
>     nsSizeMode         mLastSizeMode;
> 
>     static bool DragInProgress(void);
> 
>-    void DispatchMissedButtonReleases(GdkEventCrossing *aGdkEvent);
>+    void DispatchMissedButtonReleases(GdkEventCrossing* aGdkEvent,
>+                                      uint16_t aSource);
> 
>     // nsBaseWidget
>     virtual LayerManager* GetLayerManager(PLayerTransactionChild* aShadowManager = nullptr,
>                                           LayersBackend aBackendHint = mozilla::layers::LayersBackend::LAYERS_NONE,
>                                           LayerManagerPersistence aPersistence = LAYER_MANAGER_CURRENT) override;
> 
>     void CleanLayerManagerRecursive();
>

Comment 11

[Karl Tomlinson (:karlt)]

Comment on attachment 8822603 [details] [diff] [review]
[Pointer Event] Fill pen attributes on Linux

In order to be able to review this, I'll need answers to some questions.

(In reply to Ming-Chou Shih [:stone] from comment #7)
> Although we didn't have the conclusion about how to handle compatibility
> mouse events in the case of simultaneously controlling multiple pointer
> devices, it makes sense to me to handle pointer events of each device
> independently. I'd try to refine the patch to meet it.

The system can be be configured either with different physical/slave devices
attached to single virtual/master pointer device or with different slave
devices connected to different master pointer devices.

It is going to be easier to go with the way the system is configured than to
try to undo attachment to single master pointer device, for example, and
following the system configuration is more likely to provide what the user expects.

What is the configuration on your system?  i.e what does "xinput list" show?

> // Sometimes this actually also includes the state of the modifier keys, but
> // only the button state bits are used.
>-static guint gButtonState;
>+//
>+// We have to keep the button states for pen and mouse when multiple input
>+// devices are supported
>+static guint gMouseButtonState;
>+static guint gPenButtonState;

Usually different hardware/slave devices are attached to a single
virtual/master device, and so share the same button state.
Are pens typically different?

If slave devices are connected to a single master device, then the button
state in an event triggered by one device is affected by the buttons on other
devices attached to the same master.  The current approach of comparing button
states on events from a single master device is not going to work well if the
intention is to detect releases on particular slave devices, but I doubt that
is what we want anyway.

Please don't change the existing single button state in this patch.
If that is something that needs to be changed, then please deal with that in a
separate patch and explain the motivation.

Gecko doesn't currently make any effort to detect different master devices,
but if that is required, then that should be addressed separately,
distinguishing by master device, rather than source kind.

>     // Check before is_parent_ungrab_enter() as the button state may have
>     // changed while a non-Gecko ancestor window had a pointer grab.
>-    DispatchMissedButtonReleases(aEvent);
>+    DispatchMissedButtonReleases(aEvent, nsIDOMMouseEvent::MOZ_SOURCE_MOUSE);
>+    DispatchMissedButtonReleases(aEvent, nsIDOMMouseEvent::MOZ_SOURCE_PEN);

It is not correct, for example, to dispatch synth events for both mouse and
pen source kinds if/when the button state may be from only one kind.

>@@ -2721,16 +2728,17 @@ nsWindow::DispatchMissedButtonReleases(G
> 
>             // Dispatch a synthesized button up event to tell Gecko about the
>             // change in state.  This event is marked as synthesized so that
>             // it is not dispatched as a DOM event, because we don't know the
>             // position, widget, modifiers, or time/order.
>             WidgetMouseEvent synthEvent(true, eMouseUp, this,
>                                         WidgetMouseEvent::eSynthesized);
>             synthEvent.button = buttonType;
>+            synthEvent.inputSource = aSource;

This code exists for the sake of NPAPI plugins and setCapture().  Does the
setCapture() implementation capture only a single kind of source?  If not, I
assume the actual source type here is not important?

As you have considered, the button may have been held down on two slave
devices and released in an unknown order, and so MOZ_SOURCE_UNKNOWN sounds
appropriate here.

Do synthesized events from geometry changes use MOZ_SOURCE_UNKNOWN?

If there are multiple master devices, then it may be necessary to distinguish
between master devices.

> #include "mozilla/layers/APZCTreeManager.h"
>+#include "gtkPointerEvents.h"

Please add this include beside an existing include for another file from this directory such as that for NativeKeyBindings.h.

>+++ b/widget/gtk/gtkPointerEvents.h

>+#ifndef __GTK_POINTER_EVENTS_H__
>+#define __GTK_POINTER_EVENTS_H__
>+
>+#include <gtk/gtk.h>
>+#include "mozilla/MouseEvents.h"
>+#include "nsIDOMMouseEvent.h"
>+
>+class GtkPointerEvents final

The "Gtk" prefix is used like a namespace for libgtk names, and so please
don't use the same prefix elsewhere.
Please name the class and files PointerEventsGTK(|.cpp|.h).

>@@ -3475,17 +3491,17 @@ nsWindow::OnTouchEvent(GdkEventTouch* aE
>-        id = ++gLastTouchID & 0x7FFFFFFF;
>+        id = GtkPointerEvents::GetPointerId(GDK_SOURCE_TOUCHSCREEN);

Is there something that ensures that touch events are generated only by
touchscreens?

Is there something that prevents touchscreens from generating regular motion
or button events (i.e. being pointer-controlling), or how do pointerIds map
between motion events and touch events from the same device?

DispatchPointerFromMouseOrTouch() uses tiltx/y from Touch events.
Should those be set?

>+  case GDK_SOURCE_MOUSE:
>+  case GDK_SOURCE_TOUCHPAD:
>+    // Mouse's pointerId is always 0
>+    return 0;
>+  case GDK_SOURCE_PEN:
>+  case GDK_SOURCE_ERASER:
>+    // Pen's pointerId is always 1
>+    return 1;

I wonder whether master device is the abstraction corresponding to "Any touch
contact, pen stylus, mouse cursor, or other pointer that can produce events"?

"A mouse connected to the device is always active" implies that each mouse has
a separate pointer id, but "mouse cursor" implies that all mice moving the
same cursor act as a single pointer.

>     WidgetMouseEvent event(true, eMouseEnterIntoWidget, this,
>                            WidgetMouseEvent::eReal);
> 
>     event.mRefPoint = GdkEventCoordsToDevicePixels(aEvent->x, aEvent->y);
>     event.AssignEventTime(GetWidgetEventTime(aEvent->time));
>-
>+    event.inputSource = GtkPointerEvents::GetEventSource((GdkEvent*)aEvent);

>     KeymapWrapper::InitInputEvent(event, modifierState);
>+    GtkPointerEvents::FillPointerInfo((GdkEvent*)aEvent, event);

"Use pointers instead of references for function out parameters, even for
primitive types."
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

Please reverse the order of parameters for consistency with InitInputEvent.

>+GtkPointerEvents::FillPointerInfo(GdkEvent* aEvent,

const GdkEvent* should work here, I expect.

>+  if (!IsPointerEventsEnabled()) {
>+    // Fill PointerEvent attributes when it's enabled.
>+    return;
>+  }

This comment is confusing because it describes the other code path, I assume.
I think it can just be removed because the behavior and intention of the code
are clear enough.

>+  aMouseEvent.tiltX = xtilt == 0 ? 0 : NS_round(xtilt * 90);
>+  aMouseEvent.tiltY = ytilt == 0 ? 0 : NS_round(ytilt * 90);

Is the special case for zero necessary?

>+GtkPointerEvents::GetEventSource(GdkEvent* aEvent)

const GdkEvent* should work, I expect.

Comment 12

[Ming-Chou Shih [:stone]]

(In reply to Karl Tomlinson (:karlt) from comment #11)
> The system can be be configured either with different physical/slave devices
> attached to single virtual/master pointer device or with different slave
> devices connected to different master pointer devices.
I have no idea it's configurable on Linux. Thanks to the information.

> What is the configuration on your system?  i.e what does "xinput list" show?
Here is the configuration on my system.
⎡ Virtual core pointer                      id=2  [master pointer  (3)]
⎜   ↳ Virtual core XTEST pointer                id=4  [slave  pointer  (2)]
⎜   ↳ SYNA2B29:00 06CB:77C6                     id=12 [slave  pointer  (2)]
⎜   ↳ ELAN21EF:00 04F3:21EF                     id=13 [slave  pointer  (2)]
⎜   ↳ Wacom Intuos Pro S Pen stylus             id=9  [slave  pointer  (2)]
⎜   ↳ Wacom Intuos Pro S Pen eraser             id=10 [slave  pointer  (2)]
⎜   ↳ Wacom Intuos Pro S Pen cursor             id=16 [slave  pointer  (2)]
⎜   ↳ Wacom Intuos Pro S Pad pad                id=17 [slave  pointer  (2)]
⎜   ↳ Wacom Intuos Pro S Finger touch           id=18 [slave  pointer  (2)]

> >+static guint gMouseButtonState;
> >+static guint gPenButtonState;
> 
> Usually different hardware/slave devices are attached to a single
> virtual/master device, and so share the same button state.
> Are pens typically different?
This is because I'd handle button states for multiple slave devices. According to the spec discussion in [1], it's expected that each input should act independently and fire the relevant pointer events. And pointer events is generated from mouse events. So that I have to keep the button state for each input type (mouse and pen). Touch is different because it doesn't support hover.

> Gecko doesn't currently make any effort to detect different master devices,
> but if that is required, then that should be addressed separately,
> distinguishing by master device, rather than source kind.
Sorry for that I have no idea about how to distinguish events by master device. Could you kindly give me more hints?

> >@@ -3475,17 +3491,17 @@ nsWindow::OnTouchEvent(GdkEventTouch* aE
> >-        id = ++gLastTouchID & 0x7FFFFFFF;
> >+        id = GtkPointerEvents::GetPointerId(GDK_SOURCE_TOUCHSCREEN);
> 
> Is there something that ensures that touch events are generated only by
> touchscreens?
I thought only touch screen can trigger callback of 'touch-event'. Is there any other devices may generate touch events?

> Is there something that prevents touchscreens from generating regular motion
> or button events (i.e. being pointer-controlling), or how do pointerIds map
> between motion events and touch events from the same device?
I found [2] says if we set event mask 'GDK_TOUCH_MASK' and doesn't chain up on GtkWidget::touch-event, only touch events will be received. 

There are some questions I don't have answers now but I'll try to sort it out.

[1] https://github.com/w3c/pointerevents/issues/164
[2] https://developer.gnome.org/gtk3/3.14/chap-input-handling.html

Comment 13

[Ming-Chou Shih [:stone]]

Tested on GTK with single master device is configured and found that the button states of the master device are shared by all attached slave devices. GTK won't fire two button-press-event when pressing mouse left button and pen down on the tablet. So it's unnecessary to define multiple button states for different slave devices.

Comment 14

[Ming-Chou Shih [:stone]]

Attachment: [Pointer Event] Fill pen attributes on Linux

> Is there something that ensures that touch events are generated only by
> touchscreens?
I think only touch screen can generate touch-event. Touchpad generates regular button / motion event with device type = GDK_SOURCE_TOUCHPAD

> Is there something that prevents touchscreens from generating regular motion
> or button events 
When we handle touch-event and stop event propagation, GTK won't fire regular motion / button events

> DispatchPointerFromMouseOrTouch() uses tiltx/y from Touch events.
> Should those be set?
As I know, only events generate with pen carry tiltx/y information.

Comment 15

[Ming-Chou Shih [:stone]]

Attachment: Part1 Fill pen attributes on Linux

Comment 16

[Ming-Chou Shih [:stone]]

Attachment: Part2: Add include headers to fix compile errors

Comment 17

[Ming-Chou Shih [:stone]]

(In reply to Ming-Chou Shih [:stone] from comment #15)
> Created attachment 8910169 [details] [diff] [review]
> Part1 Fill pen attributes on Linux

Rebased the patch.

Comment 18

[Karl Tomlinson (:karlt)]

Comment on attachment 8910170 [details] [diff] [review]
Part2: Add include headers to fix compile errors

>Bug 1324956 Part2: Add include headers to fix compile errors

Probably helpful to explain that this is due to new files changing groups of unified sources, if that is accurate.

Comment 19

[Karl Tomlinson (:karlt)]

(In reply to Ming-Chou Shih [:stone] from comment #12)
> (In reply to Karl Tomlinson (:karlt) from comment #11)
> > // Sometimes this actually also includes the state of the modifier keys, but
> > // only the button state bits are used.
> >-static guint gButtonState;
> >+//
> >+// We have to keep the button states for pen and mouse when multiple input
> >+// devices are supported
> > >+static guint gMouseButtonState;
> > >+static guint gPenButtonState;
> > 
> > Usually different hardware/slave devices are attached to a single
> > virtual/master device, and so share the same button state.
> > Are pens typically different?
>
> This is because I'd handle button states for multiple slave devices.
> According to the spec discussion in [1], it's expected that each input
> should act independently and fire the relevant pointer events. And pointer
> events is generated from mouse events. So that I have to keep the button
> state for each input type (mouse and pen). Touch is different because it
> doesn't support hover.

Should each physical input device act independently, or each input source
type?

If you want the state of each button on the physical device, then I expect
listening for events from that device is the best way to get that.
But that need not be in the first patch, I assume.

gtk_widget_set_device_enabled() can register for events from physical devices,
or gtk_widget_add_device_events() may be better as that can be more specific.

Compatibility mouse events should still behave like the virtual/master device,
I assume.  I wouldn't try to change what is normal for the platform.
I assume drags are usually based on the virtual device state.

> > Gecko doesn't currently make any effort to detect different master devices,
> > but if that is required, then that should be addressed separately,
> > distinguishing by master device, rather than source kind.
>
> Sorry for that I have no idea about how to distinguish events by master
> device. Could you kindly give me more hints?

I don't know, but I assume we don't need to sort that out in the first patch,
at least.  Your system has all slave pointer devices connected to a single
master device, and that is the usual configuration AFAIK.

> > >@@ -3475,17 +3491,17 @@ nsWindow::OnTouchEvent(GdkEventTouch* aE
> > >-        id = ++gLastTouchID & 0x7FFFFFFF;
> > >+        id = GtkPointerEvents::GetPointerId(GDK_SOURCE_TOUCHSCREEN);
> > 
> > Is there something that ensures that touch events are generated only by
> > touchscreens?
>
> I thought only touch screen can trigger callback of 'touch-event'. Is there
> any other devices may generate touch events?

I don't know.  What made you think that only a touch screen could trigger
touch-event?

In XI2proto, 9.2. Touch device modes says "An example of a DependentTouch
device is a trackpad", and seems to indicate that trackpads generate touch
events.  However, I'm not seeing any touch events in xinput test-xi2 from my
trackpad, and I don't know why that is.

How would tablets send multi-touch events without Touch events?

Does your Wacom tablet support multiple touches?
What events does your Wacom tablet generate with multiple finger touches?

> > Is there something that prevents touchscreens from generating regular motion
> > or button events (i.e. being pointer-controlling), or how do pointerIds map
> > between motion events and touch events from the same device?
>
> I found [2] says if we set event mask 'GDK_TOUCH_MASK' and doesn't chain up
> on GtkWidget::touch-event, only touch events will be received. 

Thank you.  That is helpful.

Comment 20

[Karl Tomlinson (:karlt)]

Comment on attachment 8910169 [details] [diff] [review]
Part1 Fill pen attributes on Linux

This is somewhat simpler now thanks, and I think that is the best way to
progress. i.e. land a satisfactory first version and then add complex
features, such as separate button state for some or all hardware devices, or
drag events, later or as required.

Why is FillPointerInfo() not called on the WidgetMouseEvents for enter and
leave events?

Why is inputSource not set for scroll events?

WidgetDragEvent also derives from WidgetMouseEvent.
Does that need pointer info?

>+// Cached pointer events enabler value, True if pointer events are enabled.
>+static bool sIsPointerEventsEnabled = false;
>+
>+/* static */ bool
>+PointerEventsGTK::IsPointerEventsEnabled()
>+{
>+  static bool initialized = false;
>+  if (initialized) {
>+    return sIsPointerEventsEnabled;
>+  }
>+  initialized = true;
>+
>+#if GTK_CHECK_VERSION(3,4,0)
>+  mozilla::Preferences::AddBoolVarCache(&sIsPointerEventsEnabled,
>+                                       "dom.w3c_pointer_events.enabled",
>+                                        false);
>+#endif
>+  return sIsPointerEventsEnabled;

Can PointerEventHandler::IsPointerEventEnabled() or
gfxPrefs::PointerEventsEnabled() be used instead of adding another look-up and
cache?

BTW, GTK 3.4 is a requirement in configure.in now, and so the
GTK_CHECK_VERSION(3,4,0) tests are not needed.

>+/* static */ uint32_t
>+PointerEventsGTK::GetPointerId(GdkInputSource aSource)

Touch::mIdentifier is int32_t.
WidgetPointerHelper::pointerId is uint32_t.

PointerEvent has:

pointerId of type long, readonly

    A unique identifier for the pointer causing the event. This identifier
    MUST be unique from all other active pointers at the time. A user agent
    MAY recycle previously retired values for pointerId from previous active
    pointers, if necessary.

WidgetPointerHelper seems to be the one that is inconsistent with all others,
and so int32_t seems more appropriate, to indicate the limited range.

>+  case GDK_SOURCE_MOUSE:
>+  case GDK_SOURCE_TOUCHPAD:
>+    // Mouse's pointerId is always 0
>+    return 0;
>+  case GDK_SOURCE_PEN:
>+  case GDK_SOURCE_ERASER:
>+    // Pen's pointerId is always 1
>+    return 1;

Please adjust the comment to indicate arbitrary id.
e.g. "The virtual core pointers produce mouse events as a single pointer, with
pointerId arbitrarily chosen as 0." if that is accurate.

active pointer

    Any touch contact, pen stylus, mouse cursor, or other pointer that can
    produce events. If it is possible for a given pointer (identified by a
    unique pointerId) to produce additional events within the document, then
    that pointer is still considered active. Examples:

        A mouse connected to the device is always active.
        A touch contact on the screen is considered active.
        If a touch contact or pen stylus is lifted beyond the range of the
        digitizer, then it is no longer considered active.

I don't know exactly how to interpret this.  The pen events handled here are
connected to the same virtual "mouse cursor", as are most "[mice] connected to
the device".  The fact that a "mouse cursor" is an active pointer implies that
a virtual device can be an active pointer, and so all its events would have
the same id regardless of physical device.

However the "A mouse connected to the device is always active" part implies
that this is about devices (hardware), not cursors (graphics), and so each
mouse device would be a different active pointer.

I can't see any way to interpret this so that the pointer id for a pen would
differ from that of a mouse controlling the same cursor and yet not differ
from the pointer id of other pens.

The definition of pointer is also not helpful.  It says it represents multiple
"devices", but then gives individual hardware examples:

pointer
    A hardware agnostic representation of input devices that can target a
    specific coordinate (or set of coordinates) on a screen, such as a mouse,
    pen, or touch contact.

https://www.w3.org/TR/pointerevents/#h4_implicit-release-of-pointer-capture
seems to expect that a pointerup (or pointercancel) event will be received
with the same pointerId as used for setPointerCapture.

As the code is currently only listening for pointer events on the virtual
device, there will not necessarily be matching down and up events for each
physical device.  Consider a button pressed on two different physical devices
and released in the same order.

It seems that for the sake of sane capture, different pointerIds should only
be assigned to devices that can expect down and up events to be in pairs.
With the current state of the code, that is only the virtual pointer and touch
events.  Button events are received for changes in the virtual pointer buttons.
Mouse and pen devices share the same virtual pointer, and so should have the
same pointerId.

>+  case GDK_SOURCE_TOUCHSCREEN:

The meaning of GetPointerId() is quite different for touch events as it is
really just getting a new or unused id.  For mouse events, the purpose is to
return the same id for devices involved in previous calls.  I'd be inclined to
use separate functions (in the same file), which causes no problem because
there is no call site which wants support for both mouse and touchscreen.

>+  // The range of xtilt and ytilt are -1 to 1. Normalize it to -90 to 90.
>+  aMouseEvent->tiltX = NS_round(xtilt * 90);
>+  aMouseEvent->tiltY = NS_round(ytilt * 90);

This seems very odd given that tiltX and tiltY are unsigned.
https://searchfox.org/mozilla-central/rev/30ead7d1ae5bf95b8bc0fd67b950cd46ca05e32c/widget/MouseEvents.h#47

>+PointerEventsGTK::GetEventSource(const GdkEvent* aEvent)
>+{
>+  if (IsPointerEventsEnabled()) {
>+#if GTK_CHECK_VERSION(3,4,0)
>+    if (IsPointerEventsEnabled()) {

No need to check twice, but what is the motivation for checking at all?
What would be the consequences of not using MOZ_SOURCE_UNKNOWN when pointer
events are disabled?
I assume the pref need only disable what is exposed to content.

>+  return nsIDOMMouseEvent::MOZ_SOURCE_UNKNOWN;

This currently defaults to nsIDOMMouseEvent::MOZ_SOURCE_MOUSE.
What is the effect of changing it?

>+  static bool IsPointerEventsEnabled();

>+  static uint16_t ConvertInputSource(GdkInputSource aGdkInputSource)

IsPointerEventsEnabled() and ConvertInputSource() are used only from
PointerEventsGTK.cpp.  Please move to file static methods.

I assume nsIDOMMouseEvent.h will no longer be needed in the header.

>+    event.inputSource = PointerEventsGTK::GetEventSource((GdkEvent*)aEvent);

The event signal signatures actually have GdkEvent* and so please adjust the
parameters of the handlers to that and use union members elsewhere rather
than the C cast here.

Comment 21

[[e-mail]]

linuxwacom driver developer reporting in...

(In reply to Karl Tomlinson (:karlt) from comment #19)
> (In reply to Ming-Chou Shih [:stone] from comment #12)
> > Sorry for that I have no idea about how to distinguish events by master
> > device. Could you kindly give me more hints?
> 
> I don't know, but I assume we don't need to sort that out in the first patch,
> at least.  Your system has all slave pointer devices connected to a single
> master device, and that is the usual configuration AFAIK.
> 
This is indeed the usual configuration for a tablet under X11. Wayland is shaping up to be different, however. Pens are not treated the same as mice at the protocol level, and current implementations (GNOME's "Mutter" compositor and an out-of-tree set of patches to the "Weston" compositor) provide an independent cursor for each stylus.

> > > >@@ -3475,17 +3491,17 @@ nsWindow::OnTouchEvent(GdkEventTouch* aE
> > > >-        id = ++gLastTouchID & 0x7FFFFFFF;
> > > >+        id = GtkPointerEvents::GetPointerId(GDK_SOURCE_TOUCHSCREEN);
> > > 
> > > Is there something that ensures that touch events are generated only by
> > > touchscreens?
> >
> > I thought only touch screen can trigger callback of 'touch-event'. Is there
> > any other devices may generate touch events?
> 
> I don't know.  What made you think that only a touch screen could trigger
> touch-event?
> 
> In XI2proto, 9.2. Touch device modes says "An example of a DependentTouch
> device is a trackpad", and seems to indicate that trackpads generate touch
> events.  However, I'm not seeing any touch events in xinput test-xi2 from my
> trackpad, and I don't know why that is.
> 
> How would tablets send multi-touch events without Touch events?
> 
> Does your Wacom tablet support multiple touches?
> What events does your Wacom tablet generate with multiple finger touches?
> 
The default behavior of the xf86-input-wacom driver is to consume multi-touch events from the hardware and emit synthetic gestures. Performing a two-finger scroll, for example, is consumed by the driver and a series of mouse scrollwheel events is sent instead. You need to run `xsetwacom set <id> gesture off` to have the driver forward XI2 multi-touch events along to applications.

The driver does distinguish touchscreens from touchpads and /should/ use XIDirectTouch or XIDependentTouch as necessary[1]. That said, we have had some mysterious of our "gesture off" behavior not matching that of other touchscreens. Its possible that we have a bug that is causing incorrect multi-touch events to be sent... I could really use some `xinput test-xi2` multitouch logs from known-good touchscreens and touchpads for comparison.

[1]: https://github.com/linuxwacom/xf86-input-wacom/blob/master/src/xf86Wacom.c#L413

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:hsinyi, could you have a look please?
For more information, please visit auto_nag documentation.