1251346 - Fennec should not generate touch events from mouse events.

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Keyboards and IME, defect)

Description

[Randall Barker [:rbarker]]

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.

Comment 1

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1251346-Fennec-should-not-generate-touch-events-from-mouse-events-16031115-42dcbb7.patch

Comment 2

[Kartikaya Gupta (email:[e-mail])]

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?

Comment 3

[Randall Barker [:rbarker]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

(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 5

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 6

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1251346-Fennec-should-not-generate-touch-events-from-mouse-events.-r-kats-16031411-1c15114.patch

carry forward r+ from :kats.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/64aa2a8a7e73

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/64aa2a8a7e73