Closed Bug 1258808 Opened 4 years ago Closed 4 years ago

Pointer event ids are always 0

Categories

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

44 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: nzolghadr, Assigned: bhsu)

References

Details

(Whiteboard: [tw-dom] )

Attachments

(2 files, 8 obsolete files)

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
Component: General → Event Handling
Whiteboard: [tw-dom]
Assignee: nobody → sshih
Whiteboard: [tw-dom] → [tw-dom] btpp-active
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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
Ben, seems you're working on it. I set assignee to you. Thanks!
Assignee: sshih → bhsu
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
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
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.
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?
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
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.
Priority: -- → P1
Whiteboard: [tw-dom] btpp-active → [tw-dom]
Attachment #8767977 - Attachment is obsolete: true
Thanks for the patches, Ben. Are they ready for review?
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 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 :)
Attachment #8779307 - Flags: review?(btseng)
Attachment #8779308 - Flags: review?(btseng)
Attachment #8779307 - Flags: review?(btseng) → feedback?(btseng)
Attachment #8779308 - Flags: review?(btseng) → feedback?(btseng)
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".
Attachment #8779308 - Flags: feedback?(btseng)
Comment on attachment 8779307 [details] [diff] [review]
Part 1: Passing PointerId (Gecko)

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

LGTM, thanks!
Attachment #8779307 - Flags: feedback?(btseng) → feedback+
Attachment #8779308 - Attachment is obsolete: true
Attachment #8781002 - Attachment is obsolete: true
Attachment #8781003 - Attachment is obsolete: true
Hi Bevis,

Do you mind giving feedback on this revision? In addition to your comment, I also add some comment to this patch.
Attachment #8781005 - Attachment is obsolete: true
Attachment #8781006 - Flags: feedback?(btseng)
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???
Attachment #8781006 - Flags: review+
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???
Attachment #8781006 - Flags: review+
Attachment #8781006 - Flags: feedback?(btseng)
Attachment #8781006 - Flags: feedback+
Attachment #8779307 - Attachment is obsolete: true
Attachment #8784719 - Flags: feedback+
Attachment #8781006 - Attachment is obsolete: true
Attachment #8784720 - Flags: feedback+
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 :)
Attachment #8784719 - Flags: review?(bugs)
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 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...
Attachment #8784720 - Flags: review?(bugs)
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.
Attachment #8784719 - Flags: review?(bugs) → review+
Comment on attachment 8784720 [details] [diff] [review]
Part 2: Enhance Widget for Windows (Widget)

># HG changeset patch
># User Ho-Pang Hsu <hopang.hsu@gmail.com>
>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.
Attachment #8784720 - Flags: review?(bugs) → review+
Comment addressed
Attachment #8784720 - Attachment is obsolete: true
Attachment #8786221 - Flags: review+
Attachment #8786221 - Flags: feedback+
(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.
Needs rebasing.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/66384650b3c8
https://hg.mozilla.org/mozilla-central/rev/54ef7b84b971
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.