1258808 - Pointer event ids are always 0

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P1)

Description

[Navid Zolghadr]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/49.0.2623.87 Safari/537.36

Steps to reproduce:


Steps to reproduce:

1. Enable pointer_event feature in about:config.
2. Goto http://output.jsbin.com/kobada and left click on the green div
3. Try interacting with the green div with mouse and touch



Actual results:

The events that are logged all have pointerId of 0.


Expected results:

There should be different ids for the active pointers based on the spec.

Particularly mouse is always active. So if 0 is used for the mouse pointer event it cannot be re-used for touch or pen pointer events.

Related links:
https://w3c.github.io/pointerevents/#attributes-1

Comment 1

[Ben Hsu [:HoPang]]

Though this might be not so related to how to fix this issue, I found there is a mistake in EventUtils.js which is used to synthesize pointer events to do automation tests. The `synthesized` flag is put to the place for aPointerId. Maybe we can fix it when writing a testcase for this issue ;)

[0] https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/EventUtils.js#418
[1] https://dxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIDOMWindowUtils.idl#446

Comment 2

[Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]]

Ben, seems you're working on it. I set assignee to you. Thanks!

Comment 3

[Ben Hsu [:HoPang]]

After investigating this issue, we soon found out that we always enter this branch[0] despite the type of the triggering input device. As a result, the pointerId is always assigned 0, which is the default value of WidgetPointerHelper [1].


[0]. https://dxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#7132
[1]. https://dxr.mozilla.org/mozilla-central/source/widget/MouseEvents.h#52

Comment 4

[Ben Hsu [:HoPang]]

Attachment: Part 1: Implement PointerEvent.pointerId. (WIP)

This is a WIP patch, which still need to be polished. At this moment, with this patch, we can have different values for pointers derived from different input devices such as mouse, pen and touch on a Windows machine. The values now are acquired from the extra information of window messages [0], which is given by Windows OS. However, applying this patch signifies two other issues, PointerEvent.isPrimary and multiple pointer case.

Current PointerEvent.isPrimary is decided by the order within WidgetTouchEvent event which is triggered by WM_TOUCH, but I cannot receive any WM_TOUCH on my Surface Pro 4. Thus, we must come up with a mechanism to deal with it.

Multiple pointer case is that I won't get a new pointer when I touch the screen with another finger when there is already one finger attached to the screen.
Now, I am suspecting that it is caused by the current implementation of Pointer Events which is based on mouse window messages, but I have no evidence now.

[0]. https://msdn.microsoft.com/en-us/library/windows/desktop/ms703320(v=vs.85).aspx

Comment 5

[Kartikaya Gupta (email:[e-mail])]

You only get WM_TOUCH events if e10s and APZ are enabled. On touch-enabled windows devices e10s is disabled by default, you need to set browser.tabs.remote.force-enable to true to enable it.

Comment 6

[Ben Hsu [:HoPang]]

Hi Kats, and thanks for the information.

After setting "browser.tabs.remote.force-enable" to true, I can receive WM_TOUCH now, but there are two thing still to fix. One is that, we'll get duplicated pointer events (with different pointerId, where one is 0). The other is the pointerId of a pen is still zero. At this moment, stone and I are trying to remove the duplicated pointer events.

By the way, I still cannot find why enabling e10s will make WM_TOUCH receivable after tracing the source code, do you mind providing any clue for it?

Comment 7

[Kartikaya Gupta (email:[e-mail])]

If e10s is enabled, then APZ is enabled, which means we hit the code at [1]. That code in the windows widget will call [2] which is a Window API that enables the WM_TOUCH events.

[1] http://searchfox.org/mozilla-central/rev/a7c8e9f3cc323fd707659175a46826ad12899cd1/widget/nsBaseWidget.cpp#1029
[2] http://searchfox.org/mozilla-central/rev/a7c8e9f3cc323fd707659175a46826ad12899cd1/widget/windows/nsWinGesture.cpp#75

Comment 8

[Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]]

Ben, given your patch is almost ready for review, and according to our plan for PE, I map btpp-active to P1 here. Please feel free to modify it.

Comment 9

[Ben Hsu [:HoPang]]

Attachment: Part 1: Passing PointerId (Gecko)

Comment 10

[Ben Hsu [:HoPang]]

Attachment: Part 2: Enhance Widget for Windows (Widget)

Comment 11

[Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]]

Thanks for the patches, Ben. Are they ready for review?

Comment 12

[Ben Hsu [:HoPang]]

There are two patches for this issue, gecko and widget for windows. Since we focus only on Windows Desktop at this moment, I only enhance the widget code for Windows. In the gecko part, I pass pointerId alone with widget mouse events, which includes modifying the related IPC serializer. In the Windows widget part, I retrieve the pointerId from a platform API[0] provided by Windows in the widget layer, and save the pointerId in the InkCollecter which also resides in the widget layer.

[0]: https://msdn.microsoft.com/en-us/library/windows/desktop/ms703320(v=vs.85).aspx

Comment 13

[Ben Hsu [:HoPang]]

Comment on attachment 8779307 [details] [diff] [review]
Part 1: Passing PointerId (Gecko)

Hello Bevis,

I know you might be overly busy at this moment, do you mind pre-reviewing these patches? The design concept is described in Comment 12. If there anything I can do to make the process smoother, do please feel free to let me know :)

Comment 14

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8779308 [details] [diff] [review]
Part 2: Enhance Widget for Windows (Widget)

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

Please see my questions below, thanks!

::: widget/windows/InkCollector.cpp
@@ +148,5 @@
>      if (mInkCollector) {
>        Enable(false);
>        if (SUCCEEDED(mInkCollector->put_hWnd((LONG_PTR)aTargetWindow))) {
>          mTargetWindow = aTargetWindow;
> +        mPointerId = aPointerId;

When shall we clear this mPointerId?

::: widget/windows/InkCollector.h
@@ +54,5 @@
>    ~InkCollector();
>    void Shutdown();
>  
>    HWND GetTarget();
> +  void SetTarget(HWND aTargetWindow, uint16_t aPointerId);

It'll be more clear to have Get/Set/Clear functions for PointerId.
Or we should rename this to something more generic to update/clear both the mTargetWindow and mPointerId if there life-cycles are strictly the same.

::: widget/windows/WinUtils.h
@@ +383,5 @@
>    static uint16_t GetMouseInputSource();
>  
> +  /**
> +   * Windows also fires mouse window messages for pens and touches, so we should
> +   * retrieve their pointer ID on recieving mouse events as well. Please refer to 

nit: trailing space after "to".

Comment 15

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8779307 [details] [diff] [review]
Part 1: Passing PointerId (Gecko)

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

LGTM, thanks!

Comment 16

[Ben Hsu [:HoPang]]

Attachment: Part 2: Enhance Widget for Windows (Widget)

Comment 17

[Ben Hsu [:HoPang]]

Attachment: Part 2: Enhance Widget for Windows (Widget)

Comment 18

[Ben Hsu [:HoPang]]

Attachment: Part 2: Enhance Widget for Windows (Widget)

Comment 19

[Ben Hsu [:HoPang]]

Attachment: Part 2: Enhance Widget for Windows (Widget)

Hi Bevis,

Do you mind giving feedback on this revision? In addition to your comment, I also add some comment to this patch.

Comment 20

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8781006 [details] [diff] [review]
Part 2: Enhance Widget for Windows (Widget)

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

LGTM, thanks!

::: widget/windows/InkCollector.h
@@ +89,5 @@
> +  // window message, please refer to
> +  // https://msdn.microsoft.com/en-us/library/windows/desktop/hh454916(v=vs.85).aspx
> +
> +  // NOTE: The pointerId of a pen shouldn't be 0 on a Windows platform, since 0
> +  // is reserved of the mouse, please to

nit: please to???

Comment 21

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8781006 [details] [diff] [review]
Part 2: Enhance Widget for Windows (Widget)

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

LGTM, thanks!

::: widget/windows/InkCollector.h
@@ +89,5 @@
> +  // window message, please refer to
> +  // https://msdn.microsoft.com/en-us/library/windows/desktop/hh454916(v=vs.85).aspx
> +
> +  // NOTE: The pointerId of a pen shouldn't be 0 on a Windows platform, since 0
> +  // is reserved of the mouse, please to

nit: please to???

Comment 22

[Ben Hsu [:HoPang]]

Attachment: Part 1: Passing PointerId (Gecko)

Comment 23

[Ben Hsu [:HoPang]]

Attachment: Part 2: Enhance Widget for Windows (Widget)

Comment 24

[Ben Hsu [:HoPang]]

Comment on attachment 8784719 [details] [diff] [review]
Part 1: Passing PointerId (Gecko)

Hi Olli,

In this patch, I pass `pointerId` as wall when we are passing or creating WidgetMouseEvent and WidgetPointerEvent, including the IPC serializer.

In fact, in addition to `pointerId`, `tiltX` and `tiltY` should also be updated in the IPC serializer. Otherwise, we'll lost some information when crossing IPC channels. I'll file another bug for it, and leave this bug dedicated to `pointerId`.

BTW, this is not an urgent issue, feel free to delay its review and enjoy your vacation :)

Comment 25

[Ben Hsu [:HoPang]]

Comment on attachment 8784720 [details] [diff] [review]
Part 2: Enhance Widget for Windows (Widget)

There are two thing I've done in this bug. One is retrieving the `pointerId` of an windows mouse message from its ExtraInfo. The other is to save the `pointerId` of the pen for MOZ_WM_PEN_LEAVES_HOVER_OF_DIGITIZER (a custom window message representing the vanashing of the pen).

Comment 26

[Ben Hsu [:HoPang]]

Comment on attachment 8784720 [details] [diff] [review]
Part 2: Enhance Widget for Windows (Widget)

Forgot to set the reviewer... 

Please refer to comment 25 for more information...

Comment 27

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

Comment on attachment 8784719 [details] [diff] [review]
Part 1: Passing PointerId (Gecko)

This is fine, though I wonder why we need this. It is totally fine to have 0 as the pointer id of Mousevents. Just need to guarantee Pointer events generated from touch don't use 0 too.

Maybe the next patch explains.

Comment 28

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

Comment on attachment 8784720 [details] [diff] [review]
Part 2: Enhance Widget for Windows (Widget)

># HG changeset patch
># User Ho-Pang Hsu <[e-mail]>
>Bug 1258808 - Part 2: Enhance Widget for Windows (Widget). r=smaug,
>f=btseng
>
>
>diff --git a/widget/windows/InkCollector.cpp b/widget/windows/InkCollector.cpp
>index 3cee40f..65398d1 100644
>--- a/widget/windows/InkCollector.cpp
>+++ b/widget/windows/InkCollector.cpp
>@@ -164,16 +164,28 @@ void InkCollector::ClearTarget()
>     if (SUCCEEDED(mInkCollector->put_hWnd(0))) {
>       mTargetWindow = 0;
>     } else {
>       NS_WARNING("InkCollector did not clear window property successfully");
>     }
>   }
> }
> 
>+uint16_t InkCollector::GetPointerId() {
Nit, { should be on its own line

>+  return mPointerId;
>+}
>+
>+void InkCollector::SetPointerId(uint16_t aPointerId) {
ditto


>+void InkCollector::ClearPointerId() {
and here.



>+  /**
>+   * Windows also fires mouse window messages for pens and touches, so we should
>+   * retrieve their pointer ID on recieving mouse events as well. Please refer to
nit, receiving

> #define TABLET_INK_SIGNATURE 0xFFFFFF00
> #define TABLET_INK_CHECK     0xFF515700
> #define TABLET_INK_TOUCH     0x00000080
>+#define TABLET_INK_ID_MASK   0x0000007F
> #define MOUSE_INPUT_SOURCE() WinUtils::GetMouseInputSource()
>+#define MOUSE_POINTERID()    WinUtils::GetMousePointerID()
ok, tiny bit odd to have #define for the method, but looks like similar setup is used already for other stuff, so fine.

Comment 29

[Ben Hsu [:HoPang]]

Attachment: (V2) Part 2: Enhance Widget for Windows (Widget)

Comment addressed

Comment 30

[Ben Hsu [:HoPang]]

(In reply to Olli Pettay [:smaug] from comment #27)
> Comment on attachment 8784719 [details] [diff] [review]
> Part 1: Passing PointerId (Gecko)
> 
> This is fine, though I wonder why we need this. It is totally fine to have 0
> as the pointer id of Mousevents. Just need to guarantee Pointer events
> generated from touch don't use 0 too.
> 
> Maybe the next patch explains.

In case, the other patch doesn't explain this well enough.
Here is more wordy explanation:)

Windows fires mouse platform messages for mouse, pen, and even touch for the sake of Compatibility. On receiving those platform messages, Gecko wraps all of them into WidgetMouseEvents, and then proceed to next steps. At this point of view, WidgetMouseEvents do not only represent mouse but also pen and touch. Thus, we should pass pointerId along with WidgetMouseEvents, though it's pretty weird :(

Though it's not so related to this issue, there is another issue caused by this fact. When APZ is enabled, Gecko handles both mouse platform messages and touch platform messages, so Gecko might fire two pointerdown when touching the screen, where one is derived from mouse platform message and the other is derived from touch platform message.

Comment 31

[Ben Hsu [:HoPang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6bba0c41b79a&selectedJob=26830882

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/66384650b3c8
Part 1: Passing PointerId (Gecko). r=smaug, f=btseng
https://hg.mozilla.org/integration/mozilla-inbound/rev/54ef7b84b971
Part 2: Enhance Widget for Windows (Widget). r=smaug, f=btseng

Comment 34

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/66384650b3c8
https://hg.mozilla.org/mozilla-central/rev/54ef7b84b971

Comment 35

[florent.ct]

I can reproduce this bug on recent Firefox versions:
Firefox 103.0, and Firefox Developer Edition 104.0b4 (User agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:104.0) Gecko/20100101 Firefox/104.0)

Using the original example in http://output.jsbin.com/kobada, the pointerId is always zero. I expect it to differ (at least increment) for each new PointerEvent.

Can you confirm this is a regression? I don't know in which version it has been reintroduced.