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

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

34 Branch
mozilla34
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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.
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.
Attachment #8475171 - Flags: review?(wjohnston)
Turns out using %d on floats gives garbage.
Attachment #8475172 - Flags: review?(wjohnston)
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....
Attachment #8475172 - Flags: review?(wjohnston) → review+
(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?
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.
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?
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
:) I've actually gotten that far since the last post ... still playing
Posted patch bug1055548.diff (obsolete) — Splinter Review
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 ... ;-)
Attachment #8477634 - Flags: feedback?(bugmail.mozilla)
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.
Attachment #8477634 - Flags: feedback?(bugmail.mozilla) → feedback+
Btw, you should probably put this patch on bug 1021804, and then I can do the native-APZ bits on this bug.
Ok, and thanks. I was thinking one patch would fix both, not entirely being sure what's left over here :-)
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 on attachment 8477634 [details] [diff] [review]
bug1055548.diff

A newer version of this was posted on bug 1021804, so obsoleting this one.
Attachment #8477634 - Attachment is obsolete: true
Rebased logging patch to apply directly to trunk. Carrying r+.
Attachment #8475172 - Attachment is obsolete: true
Attachment #8479135 - Flags: review+
Attachment #8479135 - Flags: checkin+
Attachment #8479135 - Attachment description: Bug 1055548 - Round the APZ tap notifications to integer CSS pixels before sending to JS code. r= → Round the points to ints before using %d to serialize them
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...)
Attachment #8475171 - Attachment is obsolete: true
I'll move the remaining work to another bug. Closing this one out to avoid splitting the fix across multiple trains.
Status: NEW → RESOLVED
Closed: 5 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
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.
Attachment #8479165 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.