Closed
Bug 1153135
Opened 10 years ago
Closed 10 years ago
When APZC is enabled FireFox cannot generate PEN events into content.
Categories
(Core :: Widget: Win32, defect)
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: alessarik, Assigned: alessarik)
References
Details
Attachments
(1 file, 2 obsolete files)
1.49 KB,
patch
|
jimm
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•10 years ago
|
||
Some investigation:
This bug is regression from bug 970964 and bug 1144650.
Look like, check in GetIsMouseFromTouch() should be more stronger.
Assignee | ||
Comment 2•10 years ago
|
||
+ Changed comparison in GetIsMouseFromTouch()
Attachment #8595332 -
Flags: review?(bugs)
Attachment #8595332 -
Flags: feedback?(jmathies)
Attachment #8595332 -
Flags: feedback?(bugmail.mozilla)
Comment 3•10 years ago
|
||
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 4•10 years ago
|
||
Comment 5•10 years ago
|
||
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)
![]() |
||
Comment 6•10 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•10 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•10 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•10 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•10 years ago
|
||
+ 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•10 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•10 years ago
|
Attachment #8596542 -
Attachment is obsolete: true
Attachment #8596542 -
Flags: review?(jmathies)
![]() |
||
Updated•10 years ago
|
Attachment #8595332 -
Attachment is obsolete: false
Attachment #8595332 -
Flags: review+
Assignee | ||
Comment 12•10 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•10 years ago
|
||
Comment on attachment 8595332 [details] [diff] [review]
pen_and_apz_ver1.diff
oook
Attachment #8595332 -
Attachment is obsolete: true
![]() |
||
Updated•10 years ago
|
Attachment #8596542 -
Attachment is obsolete: false
Attachment #8596542 -
Flags: review?(jmathies)
![]() |
||
Comment 14•10 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•10 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•10 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•10 years ago
|
||
+ 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)
Assignee | ||
Comment 18•10 years ago
|
||
![]() |
||
Updated•10 years ago
|
Attachment #8597115 -
Flags: review?(jmathies) → review+
Assignee | ||
Comment 19•10 years ago
|
||
If there are no objections, I put checkin-needed flag.
Keywords: checkin-needed
Updated•10 years ago
|
Component: Untriaged → Panning and Zooming
Product: Firefox → Core
Updated•10 years ago
|
Component: Panning and Zooming → Widget: Win32
Comment 20•10 years ago
|
||
Keywords: checkin-needed
Comment 21•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 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.
Description
•