Closed Bug 1251370 Opened 4 years ago Closed 4 years ago

In Fennec with APZ, after a real touch event, mouse wheel events are ignored.

Categories

(Firefox for Android :: Toolbar, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: rbarker, Assigned: eflores)

References

Details

Attachments

(1 file)

To reproduce, tap the screen with a finger. Attempt to scroll when a mouse wheel. The result is the mouse wheel has no effect. Click any mouse button once and the mouse wheel will work again.
Summary: In Fennec with APZ, after a touch real touch event, mouse wheel events are ignored. → In Fennec with APZ, after a real touch event, mouse wheel events are ignored.
This bug is caused by Bug 1246984. What is going on is that downtime is per pointer, so a finger tap sets the mLastDownTime to the current time, but the mouse pointer device did not have a down event so it's event.getDownTime() is now before mLastDownTime so that any new scroll events from the mouse are ignored until the mouse pointer device has a new down event (from a mouse click) so that its event.getDownTime() is not the same as mLastDownTime.
That should read: "so that its event.getDownTime() is *NOW* the same as mLastDownTime."
Assignee: nobody → edwin
Attached patch 1251370.patchSplinter Review
Attachment #8730733 - Flags: review?(bugmail.mozilla)
Comment on attachment 8730733 [details] [diff] [review]
1251370.patch

Review of attachment 8730733 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/java/org/mozilla/gecko/gfx/NativePanZoomController.java
@@ +180,5 @@
>          final int action = event.getActionMasked();
> +        if (action == MotionEvent.ACTION_SCROLL) {
> +            if (event.getDownTime() >= mLastDownTime) {
> +                mLastDownTime = event.getDownTime();
> +            } else if ((InputDevice.getDevice(event.getDeviceId()).getSources() & InputDevice.SOURCE_TOUCHPAD) == InputDevice.SOURCE_TOUCHPAD) {

I think you can just do event.getSource() == InputDevice.SOURCE_TOUCHPAD.

I actually don't know if using bitwise & on getSources() is correct; I thought that only made sense if you used SOURCE_CLASS_* rather than SOURCE_*. (i.e. if you checked for SOURCE_CLASS_POSITION it might make sense, but in this case you really only care about SOURCE_TOUCHPAD).

That being said rbarker is probably a better person to review this patch at this point.
Attachment #8730733 - Flags: review?(bugmail.mozilla) → review?(rbarker)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> Comment on attachment 8730733 [details] [diff] [review]
> 1251370.patch
> 
> Review of attachment 8730733 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> mobile/android/base/java/org/mozilla/gecko/gfx/NativePanZoomController.java
> @@ +180,5 @@
> >          final int action = event.getActionMasked();
> > +        if (action == MotionEvent.ACTION_SCROLL) {
> > +            if (event.getDownTime() >= mLastDownTime) {
> > +                mLastDownTime = event.getDownTime();
> > +            } else if ((InputDevice.getDevice(event.getDeviceId()).getSources() & InputDevice.SOURCE_TOUCHPAD) == InputDevice.SOURCE_TOUCHPAD) {
> 
> I think you can just do event.getSource() == InputDevice.SOURCE_TOUCHPAD.
> 
> I actually don't know if using bitwise & on getSources() is correct; I
> thought that only made sense if you used SOURCE_CLASS_* rather than
> SOURCE_*. (i.e. if you checked for SOURCE_CLASS_POSITION it might make
> sense, but in this case you really only care about SOURCE_TOUCHPAD).
> 

This patch fixes the issue for me. According to the docs, the bitwise & is correct, however I think the suggestion to use "event.getSource() == InputDevice.SOURCE_TOUCHPAD" instead is cleaner if that works (I tried it on my end with a blue tooth mouse and seemed to work).
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> This patch fixes the issue for me. According to the docs, the bitwise & is
> correct, however I think the suggestion to use "event.getSource() ==
> InputDevice.SOURCE_TOUCHPAD" instead is cleaner if that works (I tried it on
> my end with a blue tooth mouse and seemed to work).

Tried this earlier, but sadly the bb keyboard reports SOURCE_MOUSE on its events. Eugh.
Comment on attachment 8730733 [details] [diff] [review]
1251370.patch

Review of attachment 8730733 [details] [diff] [review]:
-----------------------------------------------------------------

Please try :kats suggestion of "event.getSource() == InputDevice.SOURCE_TOUCHPAD" but otherwise looks good to me.
Attachment #8730733 - Flags: review?(rbarker) → review+
(In reply to Edwin Flores [:eflores] [:edwin] from comment #6)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> > This patch fixes the issue for me. According to the docs, the bitwise & is
> > correct, however I think the suggestion to use "event.getSource() ==
> > InputDevice.SOURCE_TOUCHPAD" instead is cleaner if that works (I tried it on
> > my end with a blue tooth mouse and seemed to work).
> 
> Tried this earlier, but sadly the bb keyboard reports SOURCE_MOUSE on its
> events. Eugh.

Sorry, didn't see this before posting review. Okay, then land it as is then.
https://hg.mozilla.org/mozilla-central/rev/6da26abc6e24
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.