Closed
Bug 1153135
Opened 8 years ago
Closed 8 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•8 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•8 years ago
|
||
+ Changed comparison in GetIsMouseFromTouch()
Attachment #8595332 -
Flags: review?(bugs)
Attachment #8595332 -
Flags: feedback?(jmathies)
Attachment #8595332 -
Flags: feedback?(bugmail.mozilla)
Comment 3•8 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•8 years ago
|
||
Perhaps useful: https://msdn.microsoft.com/en-us/library/windows/desktop/ms703320%28v=vs.85%29.aspx
Comment 5•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 years ago
|
Attachment #8596542 -
Attachment is obsolete: true
Attachment #8596542 -
Flags: review?(jmathies)
![]() |
||
Updated•8 years ago
|
Attachment #8595332 -
Attachment is obsolete: false
Attachment #8595332 -
Flags: review+
Assignee | ||
Comment 12•8 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•8 years ago
|
||
Comment on attachment 8595332 [details] [diff] [review] pen_and_apz_ver1.diff oook
Attachment #8595332 -
Attachment is obsolete: true
![]() |
||
Updated•8 years ago
|
Attachment #8596542 -
Attachment is obsolete: false
Attachment #8596542 -
Flags: review?(jmathies)
![]() |
||
Comment 14•8 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•8 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•8 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•8 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•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=767aaa8f9b88
![]() |
||
Updated•8 years ago
|
Attachment #8597115 -
Flags: review?(jmathies) → review+
Assignee | ||
Comment 19•8 years ago
|
||
If there are no objections, I put checkin-needed flag.
Keywords: checkin-needed
Updated•8 years ago
|
Component: Untriaged → Panning and Zooming
Product: Firefox → Core
Updated•8 years ago
|
Component: Panning and Zooming → Widget: Win32
Comment 20•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1517c8ebf72
Keywords: checkin-needed
Comment 21•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d1517c8ebf72
Status: NEW → RESOLVED
Closed: 8 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
•