1055548 - With native APZ enabled clicking on stuff sometimes doesn't work quite right

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

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

I don't really have proper STR but there were a couple of small bugs in the APZ glue code that were sometimes preventing proper touch event and gesture flow. Patches coming.

Comment 1

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

Attachment: Respond to long-tap notifications

The APZ code requires a prevent-default response for long-tap events in addition to touch-start-or-first-touch-move events. Without this the touch balance thing gets out of whack and we start ignoring legitimate prevent-default notifications from content.

Comment 2

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

Attachment: Round the points to ints before using %d to serialize them

Turns out using %d on floats gives garbage.

Comment 3

[Wesley Johnston (:wesj)]

Comment on attachment 8475171 [details] [diff] [review]
Respond to long-tap notifications

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

I'm debating this in my head a bit, so I won't mark review. You have any opinions?

::: widget/android/APZCCallbackHandler.cpp
@@ +147,5 @@
>              NS_LITERAL_CSTRING("Gesture:LongPress"), data));
> +    // The browser.js listener for LongPress almost always consumes the event if the
> +    // contextmenu dispatched to content is not consumed. So regardless, we want to
> +    // notify APZ that further touches in this this touch block should have no effect.
> +    NotifyDefaultPrevented(aGuid, true);

We don't always fire a contextmenu event on long press (if you're just in whitespace), so in that case we'll probably have a bug here where panning won't work for no discernible reason. Do people touch their screen for no reason sometimes? I'm not sure if I like that. Maybe we are better to send something from JS back out to notify the PanZoomController to stop....

Comment 4

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

(In reply to Wesley Johnston (:wesj) from comment #3)
> We don't always fire a contextmenu event on long press (if you're just in
> whitespace), so in that case we'll probably have a bug here where panning
> won't work for no discernible reason. Do people touch their screen for no
> reason sometimes? I'm not sure if I like that.

True. An alternative I was considering was that instead of dispatching the contextmenu event from browser.js we do it directly from the widget/android code. That way we'll know whether or not it was consumed, and we can take a bit more stuff out of browser.js. This approach should also fixing bug 1021804 pretty easily. We'd just dispatch an event which gets handled in the nsAppShell/nsWindow event loop, similar to MOTION_EVENT except it dispatches a contextmenu event and, if that was consumed, also dispatches a touch-cancel right after. Thoughts?

Comment 5

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

Specifically the flow I propose after looking at _sendToContent is this:

nsWindow (or nsAppShell) is set up with some code that runs on long-press notification. Also browser.js has a capture-mode listener for contextmenu, as well as a bubble-mode listener.

1. nsWindow (or nsAppShell) gets the longpress
2. Dispatch a before-build-contextmenu notification
3. Dispatch a contextmenu event
4. browser.js capture listener for contextmenu runs _buildMenu.
If shouldShow() is true, then:
  5. browser.js does nothing, the contextmenu continues dispatch to content.
  6. If content consumes the event, nothing further happens in browser.js
  7. If content doesn't consume the event, browser.js bubble listener for contextmenu runs and shows the context menu
If shouldShow() is false, then:
  5. browser.js dispatches context-menu-not-shown
  6. If text selection can be activated, browser.js does a prevent-default on the context menu and does text selection
  7. Otherwise, it does nothing
8. If the contextmenu was prevent-defaulted, then nsWindow/nsAppShell dispatches a touch-cancel.

Comment 6

[Mark Capella [:capella]]

I follow the thought generally. The Gesture:LongPress from APZCCallbackHandler (and JavaPanZoomController for bug 1021804) ... goes through nsAppShell::PostEvent() -> nsAppShell::ProcessNextNativeEvent() ...

Doing the obsServ->NotifyObservers(before-build-contextmenu) is easy enough, but generating a MouseEvent(contextmenu) from nsAppShell hangs me up. We'd have to somehow perform the equivalent of browser._findTarget(x,y) there, right?

Or send the event without a target and fill it in during the capture listener?

Comment 7

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

You don't need to do the findTarget stuff when dispatching from nsAppShell; just dispatching the event with the right coordinates will run it through the gecko hit testing code and do that for you. For some sample code you can use as inspiration, see [1]. For the contextmenu event you'd have to create a WidgetMouseEvent with type NS_CONTEXTMENU instead of the WidgetTouchEvent that code does. And then dispatch it by calling the DispatchEvent on nsWindow.

[1] http://mxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp?rev=d60356c72b78&mark=1016-1019#1016

Comment 8

[Mark Capella [:capella]]

:) I've actually gotten that far since the last post ... still playing

Comment 9

[Mark Capella [:capella]]

Attachment: bug1055548.diff

Thought I'd post this WIP to see if I'm too far off from what you have in mind.

My only outstanding issue is the NS_CANCEL_TOUCH event that I issue fails to actually prevent the "touchend" event that got me looking at this in the first place (via bug 1021804).

Small detail ... ;-)

Comment 10

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

Comment on attachment 8477634 [details] [diff] [review]
bug1055548.diff

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

::: widget/android/nsAppShell.cpp
@@ +225,5 @@
>      // this is valid to be called from any thread, so do so.
>      PostEvent(AndroidGeckoEvent::MakeNativePoke());
>  }
>  
> +#define GECKO_LONGPRESS_MOTIONEVENT "Gesture:LongPress"

I would simplify things by not using a broadcast event for the Gesture:LongPress and instead define a new event type in GeckoEvent.java. Then you don't have to deal with all the JSON parsing goop and special-casing the broadcast event. You'll also need info about the touch point that generates the long press (see my comments later).

I would recommend adding a new LONG_PRESS type to GeckoEvent that is identical to MOTION_EVENT except for the type. It will also need to take a MotionEvent (you have access to one in JavaPanZoomController.onLongPress)

::: widget/android/nsWindow.cpp
@@ +1042,5 @@
>      DispatchEvent(&event);
>  }
>  
> +void
> +nsWindow::OnContextmenuEvent(AndroidGeckoEvent *ae, nsIntPoint pt)

Just use a LayoutDeviceIntPoint instead of an nsIntPoint as the argument here. We're trying to phase out use of nsIntPoint in favour of the more self-documenting types.

@@ +1047,5 @@
> +{
> +    nsRefPtr<nsWindow> kungFuDeathGrip(this);
> +
> +    // Send the contextmenu event.
> +    WidgetMouseEvent contextMenuEvent = ae->MakeMouseEvent(this);

The MakeMouseEvent isn't really meant for making events out of broadcast events. You can just copy the line at http://mxr.mozilla.org/mozilla-central/source/widget/android/AndroidJavaWrappers.cpp?rev=55d21a229ee2#804 to make your event since that function is probably returning just after that anyway.

@@ +1060,5 @@
> +    // element behaviour such as link-clicks.
> +    if (contextMenuStatus == nsEventStatus_eConsumeNoDefault) {
> +        // Send the canceltouch event.
> +        WidgetTouchEvent canceltouchEvent(true, NS_TOUCH_CANCEL, this);
> +        canceltouchEvent.refPoint = LayoutDeviceIntPoint(pt.x, pt.y);

This cancel event also needs to include the touch point that triggered the long-press, so that gecko knows which touch point is getting cancelled. That will fix the touchend problem you were having. You have to add a touch point using code similar to http://mxr.mozilla.org/mozilla-central/source/widget/android/AndroidJavaWrappers.cpp?rev=55d21a229ee2&mark=714-719#714

Again using a new event type rather than a broadcast message for the LongPress event will allow you to pass in a full MotionEvent object that has all this data.

Comment 11

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

Btw, you should probably put this patch on bug 1021804, and then I can do the native-APZ bits on this bug.

Comment 12

[Mark Capella [:capella]]

Ok, and thanks. I was thinking one patch would fix both, not entirely being sure what's left over here :-)

Comment 13

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

I'll have to change the LongPress event dispatched from APZCCallbackHandler and some of the code in AsyncPanZoomController to match these changes. That code is disabled currently so we don't need to worry about it in your patch.

Comment 14

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

Comment on attachment 8477634 [details] [diff] [review]
bug1055548.diff

A newer version of this was posted on bug 1021804, so obsoleting this one.

Comment 15

[Botond Ballo [:botond]]

Attachment: Round the points to ints before using %d to serialize them

Rebased logging patch to apply directly to trunk. Carrying r+.

Comment 16

[Botond Ballo [:botond]]

And landed it: https://hg.mozilla.org/integration/mozilla-inbound/rev/536aaaee3ea8

Marking leave-open until the other patch lands.

Comment 17

[Botond Ballo [:botond]]

Attachment: Respond to long-tap notifications

For convenience, rebased the other patch to apply cleanly on top of the patch that landed. (I feel like getting Mercurial to do this was way more difficult thatn it should have been...)

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/536aaaee3ea8

Comment 19

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

I'll move the remaining work to another bug. Closing this one out to avoid splitting the fix across multiple trains.

Comment 20

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

Comment on attachment 8479165 [details] [diff] [review]
Respond to long-tap notifications

This is not the right approach anyway now that bug 1021804 moved around a bunch of the code.