Fennec should not generate touch events from mouse events.

RESOLVED FIXED in Firefox 48

Status

()

Firefox for Android
Keyboards and IME
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: rbarker, Assigned: rbarker)

Tracking

unspecified
Firefox 48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
Currently in Fennec, all pointer event events are sent as touch events first before being sent and synthesized mouse events. Mouse event should not be sent as touch events and instead sent as real mouse events.
(Assignee)

Updated

2 years ago
Assignee: nobody → rbarker
(Assignee)

Comment 1

2 years ago
Created attachment 8729727 [details] [diff] [review]
0001-Bug-1251346-Fennec-should-not-generate-touch-events-from-mouse-events-16031115-42dcbb7.patch
(Assignee)

Updated

2 years ago
Attachment #8729727 - Flags: review?(nchen)
Attachment #8729727 - Flags: review?(bugmail.mozilla)
Comment on attachment 8729727 [details] [diff] [review]
0001-Bug-1251346-Fennec-should-not-generate-touch-events-from-mouse-events-16031115-42dcbb7.patch

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

Looks good overall but a few minor things I'd like to see addressed (in particular what happens if you are dragging with the mouse - see last comment below).

::: mobile/android/base/java/org/mozilla/gecko/gfx/NativePanZoomController.java
@@ +173,5 @@
>      public boolean onTouchEvent(MotionEvent event) {
> +        if (event.getToolType(0) == MotionEvent.TOOL_TYPE_MOUSE) {
> +            return handleMouseEvent(event);
> +        } else {
> +            return handleMotionEvent(event, /* keepInViewCoordinates */ true);

Unrelated to this patch, we should throw out the keepInViewCoordinates goop in NativePanZoomController - this is the only callsite and it's hard-coded to true. This flag was useful back in the day when there was generic code in GeckoEvent used by both NPZC and JPZC but it's not useful any more now that that code has been inlined.

::: widget/InputData.cpp
@@ +160,5 @@
>    event.timeStamp = mTimeStamp;
>    event.refPoint =
>      RoundedToInt(ViewAs<LayoutDevicePixel>(mOrigin,
>        PixelCastJustification::LayoutDeviceIsScreenForUntransformedEvent));
> +  event.clickCount += clickCount;

For consistency please use = instead of +=. I'm assuming they're equivalent at this point since this is a freshly created event with 0 clickCount.

::: widget/android/nsWindow.cpp
@@ +581,5 @@
>  
>          return true;
>      }
>  
> +    MouseInput::ButtonType GetButtonType(int buttons)

Rename this to GetAndUpdateButtonType or something that makes it obvious it mutates state. Otherwise it sounds like a static conversion function which it isn't really. Alternatively (and I might prefer this), make this static, pass in mPreviousButtons as an extra parameter to this function, and move the |mPreviousButtons = buttons| thing to the callsites.

@@ +603,5 @@
> +        mPreviousButtons = buttons;
> +        return result;
> +    }
> +
> +    int16_t ConvertButtons(int buttons) {

Make this static

@@ +653,5 @@
> +                mouseType = MouseInput::MOUSE_UP;
> +                buttonType = GetButtonType(buttons);
> +                break;
> +            case AndroidMotionEvent::ACTION_MOVE:
> +                mouseType = MouseInput::MOUSE_MOVE;

What happens if you're dragging with the mouse? You might have a button down here, right?
Attachment #8729727 - Flags: review?(bugmail.mozilla) → review-
(Assignee)

Comment 3

2 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2)
> Comment on attachment 8729727 [details] [diff] [review]
> 0001-Bug-1251346-Fennec-should-not-generate-touch-events-from-mouse-events-
> 16031115-42dcbb7.patch
> 
> Review of attachment 8729727 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +653,5 @@
> > +                mouseType = MouseInput::MOUSE_UP;
> > +                buttonType = GetButtonType(buttons);
> > +                break;
> > +            case AndroidMotionEvent::ACTION_MOVE:
> > +                mouseType = MouseInput::MOUSE_MOVE;
> 
> What happens if you're dragging with the mouse? You might have a button down
> here, right?

The button could be down, but they can't change state. I was under the impression the button field (as opposed to the buttons field) was to indicate which button had changed state and that can only happen in an up or down event I thought.
(In reply to Randall Barker [:rbarker] from comment #3)
> The button could be down, but they can't change state. I was under the
> impression the button field (as opposed to the buttons field) was to
> indicate which button had changed state and that can only happen in an up or
> down event I thought.

Ah that's true, I keep forgetting the distinction between button and buttons. That's fine then.
Comment on attachment 8729727 [details] [diff] [review]
0001-Bug-1251346-Fennec-should-not-generate-touch-events-from-mouse-events-16031115-42dcbb7.patch

Changing to r+ but I'd still like you to rename that function or make it static like I suggested before landing.
Attachment #8729727 - Flags: review- → review+
(Assignee)

Comment 6

2 years ago
Created attachment 8730308 [details] [diff] [review]
0001-Bug-1251346-Fennec-should-not-generate-touch-events-from-mouse-events.-r-kats-16031411-1c15114.patch

carry forward r+ from :kats.
Attachment #8729727 - Attachment is obsolete: true
Attachment #8729727 - Flags: review?(nchen)

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/64aa2a8a7e73
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Duplicate of this bug: 1251365
You need to log in before you can comment on or make changes to this bug.