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

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Maksim Lebedev]

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.

Comment 1

[Maksim Lebedev]

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

Comment 2

[Maksim Lebedev]

Attachment: pen_and_apz_ver1.diff

+ Changed comparison in GetIsMouseFromTouch()

Comment 3

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Perhaps useful: https://msdn.microsoft.com/en-us/library/windows/desktop/ms703320%28v=vs.85%29.aspx

Comment 5

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

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.

Comment 6

[Jim Mathies [:jimm]]

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))

?

Comment 7

[Maksim Lebedev]

(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

[Jim Mathies [:jimm]]

(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?

Comment 9

[Maksim Lebedev]

(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.

Comment 10

[Maksim Lebedev]

Attachment: pen_and_apz_ver2.diff

+ Changed Comparison into more right variant.
+ Allocated two constant for compiler optimization.

Comment 11

[Jim Mathies [:jimm]]

(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.

Comment 12

[Maksim Lebedev]

(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

[Jim Mathies [:jimm]]

Comment on attachment 8595332 [details] [diff] [review]
pen_and_apz_ver1.diff

oook

Comment 14

[Jim Mathies [:jimm]]

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.

Comment 15

[Maksim Lebedev]

(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

[Jim Mathies [:jimm]]

(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.

Comment 17

[Maksim Lebedev]

Attachment: pen_and_apz_ver3.diff

+ Changes: T_I_SIGNATURE -> MOZ_T_I_SIGNATURE
+ Changes: T_I_CHECK_TCH -> MOZ_T_I_CHECK_TCH

Comment 18

[Maksim Lebedev]

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

Comment 19

[Maksim Lebedev]

If there are no objections, I put checkin-needed flag.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d1517c8ebf72

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d1517c8ebf72