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)

x86_64
Windows 8
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: alessarik, Assigned: alessarik)

References

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: 822898
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
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: