1225936 - Support "pointer" input types in APZ on Fennec

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

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

Bug 1206872 will switch Fennec over from using the JavaPanZoomController to the C++ APZ code. Right now the widget/android glue for C++ APZ only deals with touch events, so we're going to lose support for wheel/joystick/other input types that JPZC currently supports. We will need to add support for that stuff back.

(This will also help with implementing the native wheel synthesization for the android widget as required by bug 1225869)

Comment 1

[Liam Quinn]

The lack of wheel support prevents scrolling with the touch-sensitive keyboard on PRIV by BlackBerry. It would be good to add back wheel support before APZ ships in a release build.

Comment 2

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

Agreed, it would be a regression if we shipped APZ without fixing this. In general we plan on fixing or somehow addressing all the issues hanging off bug 1206874 before shipping APZ.

Comment 3

[Randall Barker [:rbarker]]

I have a patch for this that was bit rotted by GeckoView refactor work. Once I have Bug 1229752 in hand I'll un-bit rot it.

Comment 4

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1225936-Support-wheel-input-type-in-APZ-on-Fennec-16011113-29b698e.patch

Work in progress.

Comment 5

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1225936-Support-wheel-input-type-in-APZ-on-Fennec-16021010-29b698e.patch

Comment 6

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1225936-Support-scroll-events-in-APZ-on-Fennec-r-16021210-fe83ee2.patch

Comment 7

[Randall Barker [:rbarker]]

This patch only adds support for wheel events. I have a bluetooth gamepad but right now it looks like all gamepad events are intercepted and sent to the Javascript gamepad API. I can work on that but seems to be a lower priority than the scroll event.

Comment 8

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

Comment on attachment 8718943 [details] [diff] [review]
0001-Bug-1225936-Support-scroll-events-in-APZ-on-Fennec-r-16021210-fe83ee2.patch

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

LGTM with comments addressed, thanks!

::: mobile/android/base/java/org/mozilla/gecko/gfx/NativePanZoomController.java
@@ +10,5 @@
>  import org.mozilla.gecko.annotation.WrapForJNI;
>  import org.mozilla.gecko.mozglue.JNIObject;
>  import org.mozilla.gecko.util.ThreadUtils;
> +import org.mozilla.gecko.GeckoAppShell;
> +import org.mozilla.gecko.PrefsHelper;

The imports might need alphabetizing, I don't know what the style du jour is in Java-land any more.

::: widget/android/nsWindow.cpp
@@ +538,5 @@
> +            return false;
> +        }
> +
> +        const nsIntPoint& offset = mWindow->WidgetToScreenOffset().ToUnknownPoint();
> +        nsIntPoint point = nsIntPoint(int32_t(floorf(aX)), int32_t(floorf(aY))) - offset;

I see this is mostly copied from the touch function, but that code is not that great an example to copy from. There's too much conversion to unknown/untyped points which we should avoid, and also WidgetToScreenOffset is not threadsafe for non-main-thread access.

Please update this code to be like so:

ScreenIntPoint offset = ViewAs<ScreenPixel>(mWindow->WidgetToScreenOffset(), PixelCastJustification::LayoutDeviceIsScreenForBounds);
ScreenPoint origin = ScreenPoint(aX, aY) - offset;

and then pass |origin| as the argument to the ScrollWheelInput constructor. Also please file a follow-up for not using WidgetToScreenOffset or making it thread-safe, as that affects both this and the touch code.

@@ +552,5 @@
> +        uint64_t blockId;
> +        nsEventStatus status = controller->ReceiveInputEvent(input, &guid, &blockId);
> +
> +        if (status == nsEventStatus_eConsumeNoDefault) {
> +            return  true;

nit: extra space after return

@@ +555,5 @@
> +        if (status == nsEventStatus_eConsumeNoDefault) {
> +            return  true;
> +        }
> +
> +

remove extra line

Comment 9

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8718943 [details] [diff] [review]
0001-Bug-1225936-Support-scroll-events-in-APZ-on-Fennec-r-16021210-fe83ee2.patch

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

JNI bits LGTM

::: mobile/android/base/java/org/mozilla/gecko/gfx/NativePanZoomController.java
@@ +122,5 @@
> +        String[] prefs = { "ui.scrolling.negate_wheel_scroll" };
> +        mPrefsObserver = new PrefsHelper.PrefHandlerBase() {
> +            @Override public void prefValue(String pref, boolean value) {
> +                if (pref.equals("ui.scrolling.negate_wheel_scroll")) {
> +                    mNegateWheelScroll = value;

You should make mNegateWheelScroll package-private because you're accessing it from an inner class.

Comment 10

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1225936-Support-scroll-events-in-APZ-on-Fennec-r-kats-jchen-16021212-78673f7.patch

Carry forward r+ from :kats and :jchen

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2730ed299327

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2730ed299327