When APZC is enabled FireFox cannot generate PEN events into content.

RESOLVED FIXED in Firefox 40

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: alessarik, Assigned: alessarik)

Tracking

(Blocks: 1 bug)

Trunk
mozilla40
x86_64
Windows 8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20150320202338

Steps to reproduce:

Test: http://w3c-test.org/pointerevents/pointerevent_pointerout_pen-manual.html
Try to move PEN over digitizer or graphic tablet (Over "div" element).


Actual results:

If APZC is enabled, div is not got any pointerevents with pointerType = PEN.
If APZC is disabled, div get all PEN events correctly.


Expected results:

All PEN events should be fired on "div" element in any cases.
APZC preference is related with this behavior.
Maybe there are some another preferencies, which is related too.
(Assignee)

Updated

4 years ago
Blocks: 822898
(Assignee)

Comment 1

4 years ago
Some investigation:
This bug is regression from bug 970964 and bug 1144650.
Look like, check in GetIsMouseFromTouch() should be more stronger.
Depends on: 970964, 1144650
(Assignee)

Comment 2

4 years ago
Created attachment 8595332 [details] [diff] [review]
pen_and_apz_ver1.diff

+ Changed comparison in GetIsMouseFromTouch()
Attachment #8595332 - Flags: review?(bugs)
Attachment #8595332 - Flags: feedback?(jmathies)
Attachment #8595332 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 8595332 [details] [diff] [review]
pen_and_apz_ver1.diff

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

I'm not really qualified to comment on this change. But the old code definitely looks wrong because MOUSEEVENTF_FROMTOUCH is not a single-bit value. So at the very least the change to make it of the form |(GetMessageExtraInfo() & blah) == blah| seems reasonable.
Attachment #8595332 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 8595332 [details] [diff] [review]
pen_and_apz_ver1.diff

The change is really about use of Windows APIs, so jimm should review.
Attachment #8595332 - Flags: review?(jmathies)
Attachment #8595332 - Flags: review?(bugs)
Attachment #8595332 - Flags: feedback?(jmathies)
Blocks: 1122211

Comment 6

3 years ago
Comment on attachment 8595332 [details] [diff] [review]
pen_and_apz_ver1.diff

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

::: widget/windows/WinUtils.cpp
@@ +961,5 @@
> +  return ((aEventType == NS_MOUSE_MOVE ||
> +           aEventType == NS_MOUSE_BUTTON_DOWN ||
> +           aEventType == NS_MOUSE_BUTTON_UP) &&
> +         (GetMessageExtraInfo() & (TABLET_INK_CHECK | TABLET_INK_TOUCH)) ==
> +           (TABLET_INK_CHECK | TABLET_INK_TOUCH));

Why are we checking against (TABLET_INK_CHECK | TABLET_INK_TOUCH)?

Couldn't this be simplified to a boolean check:

&& (GetMessageExtraInfo() & (TABLET_INK_CHECK | TABLET_INK_TOUCH))

?
(Assignee)

Comment 7

3 years ago
(In reply to Jim Mathies [:jimm] from comment #6)
> Couldn't this be simplified to a boolean check:
> && (GetMessageExtraInfo() & (TABLET_INK_CHECK | TABLET_INK_TOUCH))
In theory that comparisons are different.
The best mathematical comparison:
(GetMessageExtraInfo() & (TABLET_INK_SIGNATURE|TABLET_INK_TOUCH))==(TABLET_INK_CHECK|TABLET_INK_TOUCH)

Comment 8

3 years ago
(In reply to Maksim Lebedev from comment #7)
> (In reply to Jim Mathies [:jimm] from comment #6)
> > Couldn't this be simplified to a boolean check:
> > && (GetMessageExtraInfo() & (TABLET_INK_CHECK | TABLET_INK_TOUCH))
> In theory that comparisons are different.
> The best mathematical comparison:
> (GetMessageExtraInfo() &
> (TABLET_INK_SIGNATURE|TABLET_INK_TOUCH))==(TABLET_INK_CHECK|TABLET_INK_TOUCH)

Ok, so you want ot test for the presence of both TABLET_INK_SIGNATURE and TABLET_INK_TOUCH?
(Assignee)

Comment 9

3 years ago
(In reply to Jim Mathies [:jimm] from comment #8)
> Ok, so you want ot test for the presence of both TABLET_INK_SIGNATURE and TABLET_INK_TOUCH?
According to https://msdn.microsoft.com/en-us/library/windows/desktop/ms703320.aspx
and with accordance of function WinUtils::GetMouseInputSource() it is the right way.
I can make comparison more simplier like 
> (GetMessageExtraInfo() & TABLET_INK_TOUCH)
It will work in my case, but it can work wrong in another cases.
(Assignee)

Comment 10

3 years ago
Created attachment 8596542 [details] [diff] [review]
pen_and_apz_ver2.diff

+ Changed Comparison into more right variant.
+ Allocated two constant for compiler optimization.
Assignee: nobody → alessarik
Attachment #8595332 - Attachment is obsolete: true
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #8595332 - Flags: review?(jmathies)
Attachment #8596542 - Flags: review?(jmathies)

Comment 11

3 years ago
(In reply to Maksim Lebedev from comment #9)
> (In reply to Jim Mathies [:jimm] from comment #8)
> > Ok, so you want ot test for the presence of both TABLET_INK_SIGNATURE and TABLET_INK_TOUCH?
> According to
> https://msdn.microsoft.com/en-us/library/windows/desktop/ms703320.aspx
> and with accordance of function WinUtils::GetMouseInputSource() it is the
> right way.
> I can make comparison more simplier like 
> > (GetMessageExtraInfo() & TABLET_INK_TOUCH)
> It will work in my case, but it can work wrong in another cases.

I wasn't trying to get you to change the previous patch, I just wanted to confirm that you want to check for both flags.

Updated

3 years ago
Attachment #8596542 - Attachment is obsolete: true
Attachment #8596542 - Flags: review?(jmathies)

Updated

3 years ago
Attachment #8595332 - Attachment is obsolete: false
Attachment #8595332 - Flags: review+
(Assignee)

Comment 12

3 years ago
(In reply to Jim Mathies [:jimm] from comment #11)
> I wasn't trying to get you to change the previous patch, I just wanted to
> confirm that you want to check for both flags.
Ok. I understand.
But now I think that pen_and_apz_ver2 is more correct than pen_and_apz_ver1
(according with our discussion and have additionally optimization)
That's why I ask You to re-review second version of patch.

Comment 13

3 years ago
Comment on attachment 8595332 [details] [diff] [review]
pen_and_apz_ver1.diff

oook
Attachment #8595332 - Attachment is obsolete: true

Updated

3 years ago
Attachment #8596542 - Attachment is obsolete: false
Attachment #8596542 - Flags: review?(jmathies)

Comment 14

3 years ago
Comment on attachment 8596542 [details] [diff] [review]
pen_and_apz_ver2.diff

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

::: widget/windows/WinUtils.cpp
@@ +959,5 @@
>  bool
>  WinUtils::GetIsMouseFromTouch(uint32_t aEventType)
>  {
> +  const int T_I_SIGNATURE = TABLET_INK_TOUCH | TABLET_INK_SIGNATURE;
> +  const int T_I_CHECK_TCH = TABLET_INK_TOUCH | TABLET_INK_CHECK;

nit - lets rename both of these to something like MOZ_XXX.
Attachment #8596542 - Flags: review?(jmathies) → review+
(Assignee)

Comment 15

3 years ago
(In reply to Jim Mathies [:jimm] from comment #14)
> nit - lets rename both of these to something like MOZ_XXX.
T_I_SIGNATURE -> MOZ_T_I_SIGNATURE
T_I_CHECK_TCH -> MOZ_T_I_CHECK_TCH
Is it Ok ?

Comment 16

3 years ago
(In reply to Maksim Lebedev from comment #15)
> (In reply to Jim Mathies [:jimm] from comment #14)
> > nit - lets rename both of these to something like MOZ_XXX.
> T_I_SIGNATURE -> MOZ_T_I_SIGNATURE
> T_I_CHECK_TCH -> MOZ_T_I_CHECK_TCH
> Is it Ok ?

perfect, thanks.
(Assignee)

Comment 17

3 years ago
Created attachment 8597115 [details] [diff] [review]
pen_and_apz_ver3.diff

+ Changes: T_I_SIGNATURE -> MOZ_T_I_SIGNATURE
+ Changes: T_I_CHECK_TCH -> MOZ_T_I_CHECK_TCH
Attachment #8596542 - Attachment is obsolete: true
Attachment #8597115 - Flags: review?(jmathies)

Updated

3 years ago
Attachment #8597115 - Flags: review?(jmathies) → review+
(Assignee)

Comment 19

3 years ago
If there are no objections, I put checkin-needed flag.
Keywords: checkin-needed
Component: Untriaged → Panning and Zooming
Product: Firefox → Core
Component: Panning and Zooming → Widget: Win32
https://hg.mozilla.org/mozilla-central/rev/d1517c8ebf72
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.