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

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: saschanaz, Assigned: stone)

Tracking

(Blocks 1 bug)

33 Branch
mozilla53
x86_64
Windows 8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(3 attachments, 15 obsolete attachments)

4.97 KB, patch
stone
: review+
Details | Diff | Splinter Review
4.32 KB, patch
stone
: review+
Details | Diff | Splinter Review
36.25 KB, patch
stone
: review+
Details | Diff | Splinter Review
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.
Hardware: x86 → x86_64
Sorry, the Metro mode "had given" the proper value, as it seems there is no such mode now...
Component: Untriaged → DOM: Events
Product: Firefox → Core
This is expected atm, since we don't have pointer events backed for desktop FF, only for Metro.
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.
Assignee: nobody → bhsu
Blocks: 1166347
Status: UNCONFIRMED → NEW
Ever confirmed: true
Blocks: 1293129
No longer blocks: 1293129
Blocks: 1293129
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
Blocks: pointerevent
No longer blocks: 1166347
Assignee: bhsu → nobody
Assignee: nobody → sshih
Blocks: 1315676
Attachment #8814784 - Attachment is obsolete: true
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.
Attachment #8816322 - Flags: feedback?(tlin)
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.
Attachment #8816320 - Attachment is obsolete: true
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.
Attachment #8816322 - Flags: feedback?(tlin) → feedback+
To retrieve more pen related information (e.g. tiltX, tiltY, pressure), we have to fire Gecko WidgetMouseEvent by handling WM_POINTER*
Attachment #8816344 - Attachment is obsolete: true
For Win8 or later, we handle WM_POINTER* so we don't need InkCollector. Refined the implementation and only enable InkCollector on Windows7
Attachment #8816321 - Attachment is obsolete: true
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.
Attachment #8816322 - Attachment is obsolete: true
Attachment #8818457 - Flags: feedback+
Roll up popups by WM_POINTERDOWN for pen
Attachment #8816324 - Attachment is obsolete: true
Attachment #8816328 - Attachment is obsolete: true
Attachment #8818457 - Attachment is obsolete: true
Attachment #8818454 - Flags: review?(bugs)
Attachment #8818455 - Flags: review?(bugs)
Attachment #8818459 - Flags: review?(bugs)
Attachment #8818455 - Flags: review?(bugs) → review+
Do Edge and/or Chrome fire compatibility mouse events for pen input?
Yes for Edge 14 and Chrome 55, tested on: https://patrickhlauke.github.io/touch/tests/event-listener_all-no-timings.html
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.
Attachment #8818454 - Flags: review?(bugs) → review?(jmathies)
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.
Attachment #8818459 - Flags: review?(bugs) → review?(jmathies)
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?
Attachment #8818454 - Flags: review?(jmathies) → review-
Attachment #8818459 - Flags: review?(jmathies) → review+
Attachment #8818767 - Flags: review?(jmathies)
Attachment #8818767 - Flags: review?(jmathies) → review+
Attachment #8818767 - Flags: review+ → review?(bugs)
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 /
Attachment #8818767 - Flags: review?(bugs) → review+
Attachment #8818459 - Flags: review+ → review?(bugs)
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.
Attachment #8818459 - Flags: review?(bugs) → review+
Attachment #8818459 - Attachment is obsolete: true
Attachment #8820087 - Flags: review+
(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.
Fix typo
Attachment #8820087 - Attachment is obsolete: true
Attachment #8820146 - Flags: review+
Keywords: checkin-needed
Pushed by ihsiao@mozilla.com:
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
Keywords: checkin-needed
Backout by cbook@mozilla.com:
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
Refined part1 to not dispatch pending events when handling WM_POINTER* with input source != pen
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a08bf2b90a5b68f9c9309f8fb359947f725f4e54
Attachment #8820085 - Attachment is obsolete: true
Flags: needinfo?(sshih)
Attachment #8820643 - Flags: review+
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
Keywords: checkin-needed
Pushed by ihsiao@mozilla.com:
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
Keywords: checkin-needed
Depends on: 1325663
Can we get this backed out until it builds under all supported Windows environments?
My build environment is msvc 2015 community edition which is supposedly a supported build platform.
(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
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9329689b3f83
follow-up - Fix local unified compilation bustage by including required headers. r=sparky
You need to log in before you can comment on or make changes to this bug.