Closed
Bug 1394526
Opened 7 years ago
Closed 7 years ago
mouse hover events have incorrect Y offset
Categories
(Firefox for Android Graveyard :: General, defect)
Firefox for Android Graveyard
General
Tracking
(firefox56 fixed, firefox57 fixed)
RESOLVED
FIXED
Firefox 57
People
(Reporter: eeejay, Assigned: rbarker)
References
Details
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
esawin
:
review+
gchang
:
approval-mozilla-beta+
|
Details |
STR: 1. Configure external (USB/Bluetooth) mouse. 2. Go to: https://labs.ssbbartgroup.com/index.php/Category:Elements 3. Hover over any of the links in the index. Result: The link several rows down from the hovered element gets an underline. Expected: Link under mouse cursor should get underline.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → rbarker
Assignee | ||
Comment 1•7 years ago
|
||
Hover events are correct when the toolbar is hidden so the toolbar offset is not being subtracted from the hover event position when the toolbar is visible. I don't think hover events go through APZ so this might be a little tricky to fix.
Comment hidden (mozreview-request) |
Comment 3•7 years ago
|
||
(In reply to Randall Barker [:rbarker] from comment #1) > Hover events are correct when the toolbar is hidden so the toolbar offset is > not being subtracted from the hover event position when the toolbar is > visible. I don't think hover events go through APZ so this might be a little > tricky to fix. I'm not sure about this specific case but in general any user input event that is coordinate-based (so any mouse/touch/pen inputs) needs to go through APZ in order to have any async transforms unapplied before it gets passed to Gecko. So if these hover events are triggering tooltips in Gecko without going through APZ that's a problem.
Comment 4•7 years ago
|
||
In the case of mousemove events I would expect it to go through APZ at http://searchfox.org/mozilla-central/rev/cd82cacec2cf734768827ff85ba2dba90a534c5e/widget/android/nsWindow.cpp#582
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8902032 [details] Bug 1394526 - Ensure mouse events have the toolbar offset removed on Android https://reviewboard.mozilla.org/r/173430/#review179044 ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/NativePanZoomController.java:134 (Diff revision 1) > } > > final MotionEvent.PointerCoords coords = new MotionEvent.PointerCoords(); > event.getPointerCoords(0, coords); > final float x = coords.x; > - final float y = coords.y; > + // Mouse events are not adjusted by the AndroidDyanmicToolbarAnimator so adjust the offset here. Please wrap the line at 80 chars.
Attachment #8902032 -
Flags: review?(esawin) → review+
Assignee | ||
Comment 6•7 years ago
|
||
(In reply to Kartikaya Gupta (Away Sep02-Sep10) (email:kats@mozilla.com) from comment #3) > (In reply to Randall Barker [:rbarker] from comment #1) > > Hover events are correct when the toolbar is hidden so the toolbar offset is > > not being subtracted from the hover event position when the toolbar is > > visible. I don't think hover events go through APZ so this might be a little > > tricky to fix. > > I'm not sure about this specific case but in general any user input event > that is coordinate-based (so any mouse/touch/pen inputs) needs to go through > APZ in order to have any async transforms unapplied before it gets passed to > Gecko. So if these hover events are triggering tooltips in Gecko without > going through APZ that's a problem. I misspoke. I remembered a similar issue with mouse click events. The problem was I couldn't process the mouse event in the Animator because there wasn't a good way to tell if it was an actual mouse click or one generated from a touch event. The solution was to subtract the toolbar offset in the native code so that the animator didn't need to worry about mouse clicks. For this patch I took the same approach for native mouse move events as if feels less invasive to me.
Comment 7•7 years ago
|
||
Ok, sounds good.
Pushed by rbarker@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d601a7f3f97f Ensure mouse events have the toolbar offset removed on Android r=esawin
Comment 9•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d601a7f3f97f
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Reporter | ||
Comment 11•7 years ago
|
||
I think this is a regression of bug 1197811 from 2 years ago. Any chance this can be uplifted to other channels?
Flags: needinfo?(rbarker)
Assignee | ||
Comment 12•7 years ago
|
||
This was caused by the new dynamic toolbar. I forgot to handle the toolbar offset for native mouse move events. Should be a safe fix to uplift to beta. I can request it.
Flags: needinfo?(rbarker)
Assignee | ||
Comment 13•7 years ago
|
||
Comment on attachment 8902032 [details] Bug 1394526 - Ensure mouse events have the toolbar offset removed on Android Approval Request Comment [Feature/Bug causing the regression]: Dynamic Toolbar V3 [User impact if declined]: Mouse hover events will be offset by the height of the visible toolbar. [Is this code covered by automated tests?]: no [Has the fix been verified in Nightly?]: no [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: none. [Is the change risky?]: no [Why is the change risky/not risky?]: it only subtracts an offset from native mouse move events on Android. This is already being done for native mouse scroll events in 55. [String changes made/needed]: none
Attachment #8902032 -
Flags: approval-mozilla-beta?
Updated•7 years ago
|
status-firefox56:
--- → affected
Comment 14•7 years ago
|
||
Comment on attachment 8902032 [details] Bug 1394526 - Ensure mouse events have the toolbar offset removed on Android UI polishment. Beta56+.
Attachment #8902032 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 15•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/178da21590bb
Comment 16•7 years ago
|
||
(In reply to Randall Barker [:rbarker] from comment #13) > [Needs manual test from QE? If yes, steps to reproduce]: no This won't need manual verification, based on Randall's assessment.
Flags: qe-verify-
Reporter | ||
Comment 17•7 years ago
|
||
(In reply to Randall Barker [:rbarker] from comment #12) > This was caused by the new dynamic toolbar. I forgot to handle the toolbar > offset for native mouse move events. Should be a safe fix to uplift to beta. > I can request it. mozregression shows that this bug has been around for 2 years. Don't know when the new dynamic toolbar was introduced.
Assignee | ||
Comment 18•7 years ago
|
||
(In reply to Eitan Isaacson [:eeejay] from comment #17) > (In reply to Randall Barker [:rbarker] from comment #12) > > This was caused by the new dynamic toolbar. I forgot to handle the toolbar > > offset for native mouse move events. Should be a safe fix to uplift to beta. > > I can request it. > > mozregression shows that this bug has been around for 2 years. Don't know > when the new dynamic toolbar was introduced. I fixed it originally in Bug 1248254 for the v2 dynamic toolbar. Then broke it again when I did the v3 toolbar which shipped in 55.
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•