Closed Bug 1394526 Opened 2 years ago Closed 2 years ago

mouse hover events have incorrect Y offset

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: eeejay, Assigned: rbarker)

References

Details

Attachments

(1 file)

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: nobody → rbarker
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.
(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 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+
(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.
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
https://hg.mozilla.org/mozilla-central/rev/d601a7f3f97f
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Duplicate of this bug: 1396484
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)
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)
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?
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+
(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-
(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.
(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.
You need to log in before you can comment on or make changes to this bug.