Closed Bug 1153135 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Widget: Win32, defect)

x86_64
Windows 8
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: alessarik, Assigned: alessarik)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.
Blocks: pointerevent
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
Attached patch pen_and_apz_ver1.diff (obsolete) — Splinter Review
+ 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 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))

?
(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)
(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?
(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.
Attached patch pen_and_apz_ver2.diff (obsolete) — Splinter Review
+ 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)
(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.
Attachment #8596542 - Attachment is obsolete: true
Attachment #8596542 - Flags: review?(jmathies)
Attachment #8595332 - Attachment is obsolete: false
Attachment #8595332 - Flags: review+
(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 on attachment 8595332 [details] [diff] [review]
pen_and_apz_ver1.diff

oook
Attachment #8595332 - Attachment is obsolete: true
Attachment #8596542 - Attachment is obsolete: false
Attachment #8596542 - Flags: review?(jmathies)
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+
(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 ?
(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.
+ 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)
Attachment #8597115 - Flags: review?(jmathies) → review+
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
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.