1031362 - .pressure is always 0.5 on Pointer Events in Firefox Nightly desktop

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Kagami Rosylight [:saschanaz] (they/them)]

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; Trident/7.0; Touch; .NET4.0E; .NET4.0C; Tablet PC 2.0; InfoPath.3; rv:11.0) like Gecko

Steps to reproduce:

I enabled dom.w3c_pointer_events.enabled and tested pen input.
I tested this on Samsung Series 7 Slate XQ700T1A-WA30, which is Wacom-compatible tablet PC. 


Actual results:

Pen inputs always gives pressure value of 0.5. This can be simply tested here: http://labs.crosspop.in/Croquispop/croquispop.html

Metro mode Firefox gives actual pressure value here with same device.


Expected results:

Win32 build should also give the actual pen input pressure.

Comment 1

[Kagami Rosylight [:saschanaz] (they/them)]

Sorry, the Metro mode "had given" the proper value, as it seems there is no such mode now...

Comment 2

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

This is expected atm, since we don't have pointer events backed for desktop FF, only for Metro.

Comment 3

[hans]

It may be expected, but the development for the Metro App for firefox has stopped. I would be very interested in pen pressure reporting on my surface pro 3 using firefox.

Comment 4

[Ben Hsu [:HoPang]]

If APZ is enabled, Gecko listens to mouse window messages and touch windows messages, where pointer events (PE) for a pen are derived from mouse window messages. However, I cannot find a trivial way to obtain the `pressure` value from the mouse window messages. 

IMHO, the best way fixing this issue is moving our implementation to pointer window messages[0][1], from which we can get the `pressure` value easily, but I personally think that we should do this after stablizing Gecko PE implementation, since this modification might affect PE behavior significantly. 

One final thing worth mentioning is that Blink doesn't support `pressure` at this moment. I think it's mainly because of the windows message, since Blink also listens to mouse window messages and touch windows messages instead pointer windows message.

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

Comment 5

[Ming-Chou Shih [:stone]]

Attachment: Part1: Dispatching mouse events by handling pen generated WM_POINTER* messages

Comment 6

[Ming-Chou Shih [:stone]]

Attachment: Bug 1031362 Part2: Refine InkCollector to enable it only on Win7

Comment 7

[Ming-Chou Shih [:stone]]

Attachment: Part1: Dispatching mouse events by handling pen generated WM_POINTER* messages

Comment 8

[Ming-Chou Shih [:stone]]

Attachment: Part2: Refine InkCollector to enable it only on Win7

Comment 9

[Ming-Chou Shih [:stone]]

Attachment: Part3: Don't dispatch event to AccessibleCaretEventHub when the event status is nsEventStatus_eConsumeNoDefault

Comment 10

[Ming-Chou Shih [:stone]]

Attachment: Part4: StopTrackingDragGesture when preventDefault is called

Comment 11

[Ming-Chou Shih [:stone]]

Attachment: Part5: Rollup popups by WM_POINTERDOWN

Rollup popup windows by gen generated WM_POINTERDOWN when pointer events preference is on. In that case, we consume the WM_POINTER* messages to prevent Windows firing WM_LBUTTONDOWN / WM_LBUTTONUP so we don't have to add some checks if it's generated by pen and stop firing WidgetMouseEvents.

Comment 12

[Ming-Chou Shih [:stone]]

Attachment: Part1: Dispatching mouse events by handling pen generated WM_POINTER* messages

We have to handle WM_POINTER* to generate mouse events for pen because we can't get the tiltX, tiltY and some other pen attributes from WM_LBUTTONDOWN.

Comment 13

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8816322 [details] [diff] [review]
Part3: Don't dispatch event to AccessibleCaretEventHub when the event status is nsEventStatus_eConsumeNoDefault

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

AccessibleCaret is like a platform default behavior responding to touch events on Fennec and Windows with touch support. If the event is already eConsumeNoDefault, I think it makes sense to skip the event for AccessibleCaretEventHub.

Comment 14

[Ming-Chou Shih [:stone]]

Attachment: Part1: Dispatching mouse events by handling pen generated WM_POINTER* messages

To retrieve more pen related information (e.g. tiltX, tiltY, pressure), we have to fire Gecko WidgetMouseEvent by handling WM_POINTER*

Comment 15

[Ming-Chou Shih [:stone]]

Attachment: Part2: Refine InkCollector to enable it only on Win7

For Win8 or later, we handle WM_POINTER* so we don't need InkCollector. Refined the implementation and only enable InkCollector on Windows7

Comment 16

[Ming-Chou Shih [:stone]]

Attachment: Part3: Don't dispatch event to AccessibleCaretEventHub when the event status is nsEventStatus_eConsumeNoDefault

Stop dispatching the event to AccessibleCaretEventHub when it's defaultPrevented. It's happened when the user calls preventDefault on ePointerDown, we also set the subsequent mouse events as defaultPrevented until the button is released.

Comment 17

[Ming-Chou Shih [:stone]]

Attachment: Part3: Rollup popups by WM_POINTERDOWN

Roll up popups by WM_POINTERDOWN for pen

Comment 18

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

Do Edge and/or Chrome fire compatibility mouse events for pen input?

Comment 19

[Kagami Rosylight [:saschanaz] (they/them)]

Yes for Edge 14 and Chrome 55, tested on: https://patrickhlauke.github.io/touch/tests/event-listener_all-no-timings.html

Comment 20

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

Comment on attachment 8818454 [details] [diff] [review]
Part1: Dispatching mouse events by handling pen generated WM_POINTER* messages

jimm, could you perhaps take a look at this from Windows API usage point of view?
I could then review after that.

Comment 21

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

Comment on attachment 8818459 [details] [diff] [review]
Part3: Rollup popups by WM_POINTERDOWN

>+bool
>+nsWinPointerEvents::ShouldRollupOnPointerEvent(WPARAM aWParam)
>+{
>+  // Only roll up popups when we handling WM_POINTER* to fire Gecko
>+  // WidgetMouseEvent and suppres Windows WM_*BUTTONDOWN.
>+  return FireMouseEventsByPointerMessagesForPen(aWParam);
This is very confusing. FireMouseEventsByPointerMessagesForPen() doesn't fire anything.
The method is coming from the first patch. Perhaps rename it to ShouldFireCompatibilityMouseEventsForPen() or some such?

But I think some windows widget peer should look at this one too first.

Comment 22

[Jim Mathies [:jimm]]

Comment on attachment 8818454 [details] [diff] [review]
Part1: Dispatching mouse events by handling pen generated WM_POINTER* messages

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

Generally looks pretty good. I'd like to look this over with the suggested changes.

::: widget/windows/moz.build
@@ +55,5 @@
>      'nsWindowBase.cpp',
>      'nsWindowDbg.cpp',
>      'nsWindowGfx.cpp',
>      'nsWinGesture.cpp',
> +    'nsWinPointerEvents.cpp',

We no longer use 'ns', so please get rid of all references in files or class names to this.

::: widget/windows/nsWinPointerEvents.cpp
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/*
> + * nsWinGesture - Touch input handling for tablet displays.

nit - copy paste error

@@ +22,5 @@
> +bool nsWinPointerEvents::sPointerEventEnabled = true;
> +
> +nsWinPointerEvents::nsWinPointerEvents()
> +{
> +  (void)InitLibrary();

why do we return a result from InitLibrary() that we never check? Capture this return value and debug/release assert on the result or get rid of it and assert in the actual init method.

@@ +28,5 @@
> +  if (!addedPointerEventEnabled) {
> +    Preferences::AddBoolVarCache(&sPointerEventEnabled,
> +                                 "dom.w3c_pointer_events.enabled", true);
> +    addedPointerEventEnabled = true;
> +  }    

nit - ws

@@ +33,5 @@
> +}
> +
> +/* Load and shutdown */
> +bool
> +nsWinPointerEvents::InitLibrary()

lets add a assert on a process check here, make sure this only inits in the chrome process.

@@ +82,5 @@
> +  }
> +  if (pointerType != PT_PEN) {
> +    return false;
> +  }
> +  return true;

nit - 

return (pointerType == PT_PEN);

::: widget/windows/nsWinPointerEvents.h
@@ +5,5 @@
> +
> +#ifndef WinPointerEvents_h__
> +#define WinPointerEvents_h__
> +
> +//#include "nsdefs.h"

nit - commented line

@@ +95,5 @@
> + *
> + * This is a helper class to handle WM_POINTER*. It only supports Win8 or later.
> + *
> + ******************************************************************************/
> +class nsWinPointerInfo final : public mozilla::WidgetPointerHelper

nit - WinPointerInfo

@@ +115,5 @@
> +  float mPressure;
> +  int16_t mButtons;
> +};
> +
> +class nsWinPointerEvents final

WinPointerEvents

::: widget/windows/nsWindow.cpp
@@ +8011,5 @@
> +  mPointerEvents.GetPointerPenInfo(pointerId, &penInfo);
> +
> +  // Windows defines the pen pressure is normalized to a range between 0 and
> +  // 1024. Convert it to float.
> +  float pressure = (float)penInfo.pressure/1024;

any chance penInfo.pressure might be zero?

Comment 23

[Ming-Chou Shih [:stone]]

Attachment: Part1: Dispatching mouse events by handling pen generated WM_POINTER* messages

Comment 24

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

Comment on attachment 8818767 [details] [diff] [review]
Part1: Dispatching mouse events by handling pen generated WM_POINTER* messages

couple of nits

>+WinPointerEvents::GetPointerType(uint32_t aPointerId,
>+                                 POINTER_INPUT_TYPE *aPointerType)
>+{
>+  if (!getPointerType)
>+    return false;
Especially all the new code should use Mozilla coding style.
So, always {} with 'if'. Here and elsewhere.



+// Desktop builds target apis for 502. Win8 Metro builds target 602.
+#if WINVER < 0x0602
This could use some better comment. So we want to define some macros when we're building using older target, right?

>+/*
>+ * Macros to retrieve information from pointer input message parameters
>+ */
>+#define GET_POINTERID_WPARAM(wParam)                (LOWORD(wParam))
>+#define IS_POINTER_FLAG_SET_WPARAM(wParam, flag)    (((DWORD)HIWORD(wParam) & (flag)) == (flag))
>+#define IS_POINTER_NEW_WPARAM(wParam)               IS_POINTER_FLAG_SET_WPARAM(wParam, POINTER_MESSAGE_FLAG_NEW)
>+#define IS_POINTER_INRANGE_WPARAM(wParam)           IS_POINTER_FLAG_SET_WPARAM(wParam, POINTER_MESSAGE_FLAG_INRANGE)
>+#define IS_POINTER_INCONTACT_WPARAM(wParam)         IS_POINTER_FLAG_SET_WPARAM(wParam, POINTER_MESSAGE_FLAG_INCONTACT)
>+#define IS_POINTER_FIRSTBUTTON_WPARAM(wParam)       IS_POINTER_FLAG_SET_WPARAM(wParam, POINTER_MESSAGE_FLAG_FIRSTBUTTON)
>+#define IS_POINTER_SECONDBUTTON_WPARAM(wParam)      IS_POINTER_FLAG_SET_WPARAM(wParam, POINTER_MESSAGE_FLAG_SECONDBUTTON)
>+#define IS_POINTER_THIRDBUTTON_WPARAM(wParam)       IS_POINTER_FLAG_SET_WPARAM(wParam, POINTER_MESSAGE_FLAG_THIRDBUTTON)
>+#define IS_POINTER_FOURTHBUTTON_WPARAM(wParam)      IS_POINTER_FLAG_SET_WPARAM(wParam, POINTER_MESSAGE_FLAG_FOURTHBUTTON)
>+#define IS_POINTER_FIFTHBUTTON_WPARAM(wParam)       IS_POINTER_FLAG_SET_WPARAM(wParam, POINTER_MESSAGE_FLAG_FIFTHBUTTON)
>+#define IS_POINTER_PRIMARY_WPARAM(wParam)           IS_POINTER_FLAG_SET_WPARAM(wParam, POINTER_MESSAGE_FLAG_PRIMARY)
>+#define HAS_POINTER_CONFIDENCE_WPARAM(wParam)       IS_POINTER_FLAG_SET_WPARAM(wParam, POINTER_MESSAGE_FLAG_CONFIDENCE)
>+#define IS_POINTER_CANCELED_WPARAM(wParam)          IS_POINTER_FLAG_SET_WPARAM(wParam, POINTER_MESSAGE_FLAG_CANCELED)
I don't understand why we define bunch of macros here which aren't ever used.
If you want to keep also those unused macros, at least add a comment that they are there for future usage or something.
Perhaps the comment could be around the WINVER < 0x0602 check.

>+  // Windows defines the pen pressure is normalized to a range between 0 and
>+  // 1024. Convert it to float.
>+  float pressure = penInfo.pressure ? (float)penInfo.pressure/1024 : 0;
space before and after /

Comment 25

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

Comment on attachment 8818459 [details] [diff] [review]
Part3: Rollup popups by WM_POINTERDOWN

Hmm, this is a patch which jimm already r+'ed?
If so, you could have asked additional review.

FireMouseEventsByPointerMessagesForPen is still using the old name. Use the new name.

Comment 26

[Ming-Chou Shih [:stone]]

Attachment: Part1: Dispatching mouse events by handling pen generated WM_POINTER* messages. r=jimm,smaug

Comment 27

[Ming-Chou Shih [:stone]]

Attachment: Part2: Refine InkCollector to enable it only on Win7. r=smaug

Comment 28

[Ming-Chou Shih [:stone]]

Attachment: Part3: Rollup popups by WM_POINTERDOWN. r=jimm, smaug

Comment 29

[Ming-Chou Shih [:stone]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c84574ea9dac79d4cdf0c85775ee481e76169632

Comment 30

[Ming-Chou Shih [:stone]]

(In reply to Olli Pettay [:smaug] from comment #25)
> Comment on attachment 8818459 [details] [diff] [review]
> Part3: Rollup popups by WM_POINTERDOWN
> 
> Hmm, this is a patch which jimm already r+'ed?
> If so, you could have asked additional review.
Yes, it is.

Comment 31

[Ming-Chou Shih [:stone]]

Attachment: Part3: Rollup popups by WM_POINTERDOWN. r=jimm, smaug

Fix typo

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1413fa802632
Part1: Dispatching mouse events by handling pen generated WM_POINTER* messages. r=jimm,smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb1654cb6f30
Part2: Refine InkCollector to enable it only on Win7. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d3072ee3656
Part3: Rollup popups by WM_POINTERDOWN. r=jimm, smaug

Comment 33

[Carsten Book [:Tomcat]]

backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=40813250&repo=mozilla-inbound

Comment 34

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d109223b6ea
Backed out changeset 1d3072ee3656 
https://hg.mozilla.org/integration/mozilla-inbound/rev/334e83c59cab
Backed out changeset bb1654cb6f30 
https://hg.mozilla.org/integration/mozilla-inbound/rev/c59499ae612d
Backed out changeset 1413fa802632 for test failures in pointerevents/test_touch_action.html

Comment 35

[Ming-Chou Shih [:stone]]

Attachment: Part1: Dispatching mouse events by handling pen generated WM_POINTER* messages. r=jimm,smaug

Refined part1 to not dispatch pending events when handling WM_POINTER* with input source != pen
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a08bf2b90a5b68f9c9309f8fb359947f725f4e54

Comment 36

[Ming-Chou Shih [:stone]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a08bf2b90a5b68f9c9309f8fb359947f725f4e54

Comment 37

[Ming-Chou Shih [:stone]]

One failure of test_touch_action.html found in try results. Its error messages are
03:20:33     INFO - InjectTouchInput failure. GetLastError=1460
03:20:33     INFO - Flushed APZ repaints, waiting for callback...
03:20:33     INFO - InjectTouchInput failure. GetLastError=87
03:20:33     INFO - InjectTouchInput failure. GetLastError=87
These messages are the same as the messages of intermittent failure bug1309606. The error code 1460 is ERROR_TIMEOUT, which is defined in [1].

It's different from the failure addressed in comment33. The failure in comment33 is caused by DispatchPendingEvents at WM_POINTER* when input source = touch. In this case, DispatchPendingEvents may induce Windows stops firing WM_TOUCH and causing the test timeout. It should be fixed in attachment 8820643 [details] [diff] [review].

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

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f397c3444f2a
Part1: Dispatching mouse events by handling pen generated WM_POINTER* messages. r=jimm,smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bff72325005
Part2: Refine InkCollector to enable it only on Win7. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/a699d5de64dd
Part3: Rollup popups by WM_POINTERDOWN. r=jimm, smaug

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f397c3444f2a
https://hg.mozilla.org/mozilla-central/rev/9bff72325005
https://hg.mozilla.org/mozilla-central/rev/a699d5de64dd

Comment 40

[Bill Gianopoulos [:WG9s]]

Can we get this backed out until it builds under all supported Windows environments?

Comment 41

[Bill Gianopoulos [:WG9s]]

My build environment is msvc 2015 community edition which is supposedly a supported build platform.

Comment 42

[Jonathan Watt [:jwatt]]

(In reply to Bill Gianopoulos [:WG9s] from comment #41)
> My build environment is msvc 2015 community edition which is supposedly a
> supported build platform.

Not just "a" supported platform, but *the* supported platform.

I also got some weird build failures that initially looked like msvc 2015 wasn't compatible with the patches for this bug, but it turns out they're a unified build issue that only manifests locally. I've pushed the likely fix to Try and will land it later once Try confirms all is well:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=889effb9481c5a8371a3e1d6a0c32164a697af78

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9329689b3f83
follow-up - Fix local unified compilation bustage by including required headers. r=sparky

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9329689b3f83