942929 - Long pressing an element without any contextmenu does not trigger a click

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

When long pressing an element that does not have a contextmenu and releasing the finger nothing happens while a click should have been generated.

Comment 1

[Dale Harvey (:daleharvey)]

Longtap events in the gesture listener unconditionally cancel single taps

https://bugzilla.mozilla.org/show_bug.cgi?id=942929

Comment 2

[Dale Harvey (:daleharvey)]

Sorry, was meant to be a pointer to the code for reference 

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/GestureEventListener.cpp#162

Comment 3

[Dale Harvey (:daleharvey)]

Just an update, got a patch for this that mostly works, need to clean it up however will explain it just to make sure I am doing it right

On longpress in GestureEventListener we cancel any future events getting sent (ontapup), I have taken that out so every longpress is followed by a singletap event, the longpress handler in TabChild records whether the contextmenu was handled / defaultPrevented and if so the subsequent tap is ignored

There is a bit of extra work due to tapconfirmed vs tapup in zoomable content, but the above is the important part

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Dale Harvey (:daleharvey) from comment #3)
> Just an update, got a patch for this that mostly works, need to clean it up
> however will explain it just to make sure I am doing it right
> 
> On longpress in GestureEventListener we cancel any future events getting
> sent (ontapup), I have taken that out so every longpress is followed by a
> singletap event, the longpress handler in TabChild records whether the
> contextmenu was handled / defaultPrevented and if so the subsequent tap is
> ignored
> 

That sounds what we want.

Comment 5

[Dale Harvey (:daleharvey)]

Attachment: 942929.patch

So in fixing this I found a few bugs

https://bugzilla.mozilla.org/show_bug.cgi?id=922896 regressed https://bugzilla.mozilla.org/show_bug.cgi?id=915645 so that any press over 300ms was ignored as it triggered a tapconfirmed which was ignored on zoomable content

(I drew a little diagram in case it helps anyone confused by the event flow) 
------------------
Lift Finger < 300ms

  -> Tap up -> zoom disabled -> single tap (and ignore tap confirmed)
               zoom enabled -> wait for tap confirmed

  -> Tap confirm -> zoom disabled -> ignore
                    zoomenabled -> single tap


Lift Finger > 300ms 
 
  -> Tap confirm -> single singletap
------------------

I filed https://bugzilla.mozilla.org/show_bug.cgi?id=946445 as the gaia browser doesnt currently preventDefault when it handles contextmenus.

For this bug, GestureEventListener will now always fire a tap on any tap up, it may also fire a long press, if we receive longpress and content handles it, we ignore the subsequent tap

Comment 6

[Dale Harvey (:daleharvey)]

Also I based this on top of https://bugzilla.mozilla.org/show_bug.cgi?id=940706 because BenWa told me to

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8342627 [details] [diff] [review]
942929.patch

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

I did some blind comments since I don't know this code really.

::: dom/ipc/TabChild.cpp
@@ +1635,5 @@
>  
> +  if (mContextMenuHandled) {
> +    mContextMenuHandled = false;
> +    return true;
> +  }

Does AsyncPanZoomController is not the one that is supposed to not send HandleSingleTap in this case ? I assume that the issue for you is that you have to always return true for async ipdl message. What we used to do in BrowserElementPanning is to send a sync message from the child but the events triggering was different though so i assume that you can run into races if you do that here so that's why you did it like this, and in this case it sounds ok.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +790,5 @@
>  nsEventStatus AsyncPanZoomController::OnSingleTapConfirmed(const TapGestureInput& aEvent) {
>    APZC_LOG("%p got a single-tap-confirmed in state %d\n", this, mState);
>    nsRefPtr<GeckoContentController> controller = GetGeckoContentController();
>    // If zooming is disabled, we handle this in OnSingleTapUp
> +  if (controller && !mIgnoreSingleTapConfirm) {

Any reason to remove mAllowZoom ?

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +609,5 @@
>    bool mAllowZoom;
>    mozilla::CSSToScreenScale mMinZoom;
>    mozilla::CSSToScreenScale mMaxZoom;
>  
> +  // GestureDetector will send us both a tap up and tap confirmed event

GestureDetector? Sounds like a Gaia name :)

Comment 8

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

Comment on attachment 8342627 [details] [diff] [review]
942929.patch

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

I don't like adding extra state variables like this. In particular I think there are cases where the variables will be left with the wrong value.

Consider if the user does a long-press, and then without lifting the finger, puts a second finger down. In this case, GestureEventListener will dispatch a long-press, and then when the second finger goes down, it will go into state GESTURE_NONE without ever dispatching the OnSingleTapConfirm. This means that in TabChild, mContextMenuHandled could end up being set to true and not reset. Later, if another click comes in, it will get ignored.

While going through the code I also think I found another pre-existing bug: consider if the user does a short tap quickly followed by putting two fingers down (a pinch). If I'm following the logic in GestureEventListener.cpp correctly, it will do this:

1. Finger goes down -> state = GESTURE_WAITING_SINGLE_TAP
2. Finger goes up (within MAX_TIME) -> call HandleSingleTapUp, state = GESTURE_WAITING_DOUBLE_TAP
3. Finger goes down -> (no changes)
4. Second finger goes down -> call HandleTapCancel, which sets state = GESTURE_NONE
5. Eventually the TimeoutDoubleTap from step 2 fires, but state == GESTURE_NONE so nothing happens.

In this scenario, APZC::OnSingleTapUp is called as part of step 2, but OnSingleTapConfirmed is never called even though I think it should be (because the double-tap action was never actually completed). This is already a bug but with your patch it gets amplified because mIgnoreSingleTapConfirm will be set to true and is never set back to false as part of this sequence. That means if the user later does a long-press, the OnSingleTapConfirmed call that follows it will trigger a tap.

It's my experience that adding state variables like this to track which functions have been called are very error-prone, because they're hard to reason about. Unfortunately I can't think of a better suggestion here so I won't r- just based on that, but I'd like to you think about possible alternatives if at all possible, even if we defer refactoring to a follow-up bug. I *am* minusing the patch because of the first bug I described above which would be a regression from this patch. If you can add a separate patch that fixes the second bug as well that would be great.

Finally, if you stick with this state variable approach, then you will need to update all of the implementations of GeckoContentController to handle the tap-after-long-press behaviour change. In particular you will need to update the Metro implementation of GeckoContentController the same way you did TabChild.

I'm tagging Jim for feedback since he's worked with this code as well and might have some ideas about how to organize it to be less error-prone (he can also advise on and review any changes to the Metro code).

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +789,5 @@
>  
>  nsEventStatus AsyncPanZoomController::OnSingleTapConfirmed(const TapGestureInput& aEvent) {
>    APZC_LOG("%p got a single-tap-confirmed in state %d\n", this, mState);
>    nsRefPtr<GeckoContentController> controller = GetGeckoContentController();
>    // If zooming is disabled, we handle this in OnSingleTapUp

This comment needs to be updated. In particular make sure it's clear that OnSingleTapConfirmed is going to be called after OnLongPress, and in that scenario we want to pass it through regardless of zooming behaviour.

@@ +802,5 @@
>      }
>    }
> +
> +  if (mIgnoreSingleTapConfirm) {
> +    mIgnoreSingleTapConfirm = false;

Move this up to the start of this function, and have it early-return.

Comment 9

[Dale Harvey (:daleharvey)]

> Does AsyncPanZoomController is not the one that is supposed to not send HandleSingleTap in this case ?

As you mentioned I was worried about races, however I do think it would be cleaner inside APZC.cpp

> Any reason to remove mAllowZoom ?

The GestureDetector will only send a singleTapConfirmed if the press was > 300ms, these are currently unconditionally ignored, its seperate but related to this bug and as kats suggested I filed a new bug and can split out the patch https://bugzilla.mozilla.org/show_bug.cgi?id=946661

> It's my experience that adding state variables like this to track which functions have been called are very error-prone, because they're hard to reason about. Unfortunately I can't think of a better suggestion here so I won't r- just based on that, but I'd like to you think about possible alternatives if at all possible, even if we defer refactoring to a follow-up bug. I *am* minusing the patch because of the first bug I described above which would be a regression from this patch. If you can add a separate patch that fixes the second bug as well that would be great.

I agree that state flags are likely to be error prone, I couldnt think of any way around it but will try to, Ill split out the fix for > 300ms taps being ignored on zoomed content and this bug as well

Comment 10

[Jim Mathies [:jimm]]

Comment on attachment 8342627 [details] [diff] [review]
942929.patch

kicking this over to matt since he just recently got these hooked up in metro and understands our use of the events better than I do.

Comment 11

[Dale Harvey (:daleharvey)]

Also is there a bug open tracking being able to write tests for this stuff? already seeing regressions and various extra bugs with each patch and I think thats gonna continue until there are tests

Comment 12

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

You should already be able to write tests for this using the gtest framework - see gfx/tests/gtest/TestAsyncPanZoomController.cpp. Documentation at https://developer.mozilla.org/en-US/docs/GTest

Comment 13

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8342627 [details] [diff] [review]
942929.patch

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

Metro Firefox currently uses the Windows GestureRecognizer API to detect long taps, and ignores HandleLongTap calls from APZC.

If this change lands, we'll need to add logic to Metro that is similar to the logic here in TabChild, to suppress the extra "click" if the "contentmenu" event was canceled.  It might be more difficult to get this right in Metro, because the code that sends those two events is different and may not use the same timing.  (The long-tap delay on Windows is a system setting.)

It might be easier to do the reverse of this patch:  Leave APZC and GestureEventListener unchanged, and instead make TabChild::RecvHandleLongTap dispatch a "click" event if the "contextmenu" event is not processed.  (And the WinRT widget code could do something similar.)

Comment 14

[Dale Harvey (:daleharvey)]

The problem with dispatching a click event if the target contextmenu isnt handled is that you are firing a click event with the user not actually lifting their finger up, the long press interupts and happens when the users finger is lifted, there isnt any mechanism to know when that happens currently

Comment 15

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

Perhaps a better solution then is to add a HandleLongTapUp callback to GeckoContentController. That should alleviate the state-tracking problem as well since you just need to call that once when the finger goes up after a long-tap. TabChild can treat it as a click if the contextmenu event was not handled, and Metro can carry on as before.

Comment 16

[Dale Harvey (:daleharvey)]

APZC.cpp actually returns the fact it ignores the event, we never use the return values in gesture detector and its already tracking this state, so it can just check the return and not fire the confirm if apzc said it handled singletapup right?

I didnt do that before as I thought there may be a reason we dont handle returns in gestureeventdetector

Comment 17

[Dale Harvey (:daleharvey)]

That fix I mentioned would be for https://bugzilla.mozilla.org/show_bug.cgi?id=946661, the longtapup sounds like it will likely be a good solution for this bug, cheers

Comment 18

[Dale Harvey (:daleharvey)]

Attachment: 942929-2.patch

Uploaded for feedback mainly because I wasnt sure that I was doing the right thing at all here, quite a lot of files to touch.

It still adds state, TabChild needs to know whether it previously handled the context menu, however its less severe (we wont miss normal taps, I likely need to go through gestureeventlistener to ensure tapup is always fired

also fix comments + tests

Comment 19

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

Comment on attachment 8344663 [details] [diff] [review]
942929-2.patch

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

Yup, look good!

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +358,5 @@
>  
>    /**
> +   * Helper method for long press gestures.
> +   *
> +   * XXX: Implement this.

We should remove the XXX comments from this and the other functions that have been implemented. Actually we can probably just kill this entire comment since it doesn't really add much value.

::: gfx/layers/ipc/GeckoContentController.h
@@ +49,5 @@
>     */
>    virtual void HandleLongTap(const CSSIntPoint& aPoint, int32_t aModifiers) = 0;
>  
>    /**
> +   * Requests handling a long tap. |aPoint| is in CSS pixels, relative to the

Fix comment. Mention that HandleLongTapUp will _always_ be preceeded by a call to HandleLongTap. This is important for the state variable in TabChild.cpp

::: gfx/layers/ipc/GestureEventListener.h
@@ +85,5 @@
>      // may be mistaken for a tap.
>      GESTURE_WAITING_SINGLE_TAP,
>      // A single tap has happened for sure, and we're waiting for a second tap.
> +    GESTURE_WAITING_DOUBLE_TAP,
> +    GESTURE_LONG_TAP_UP

Add a comment as to when we enter the LONG_TAP_UP state

Comment 20

[Dale Harvey (:daleharvey)]

Attachment: 942929-2.patch

Fixed comments, remembered I needed to handle the |HandleTapCancel| case, I wasnt sure if I should comment in there that its ok for the tapup event to be cancelled as its state thats maintained is only dependent on the result of the longpress that happens before it.

I attempted to write a test for this, however the gtests are using fake timers for input events which arent observed by the longtaptimeout runnable, so since we arent actually waiting 500, or any milliseconds, that isnt going to trigger. Is there a decent way to test that case?

Comment 21

[Dale Harvey (:daleharvey)]

Also I based this patch on top of https://bugzilla.mozilla.org/show_bug.cgi?id=946661 and https://bugzilla.mozilla.org/show_bug.cgi?id=947931

Comment 22

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

(In reply to Dale Harvey (:daleharvey) from comment #20)
> I attempted to write a test for this, however the gtests are using fake
> timers for input events which arent observed by the longtaptimeout runnable,
> so since we arent actually waiting 500, or any milliseconds, that isnt going
> to trigger. Is there a decent way to test that case?

I think it should be possible. When the GestureEventListener registers a runnable with a timeout, that should invoke MockContentController::PostDelayedTask. So what I think you can do is define a subclass of MockContentController that overrides PostDelayedTask to hang on to the Task object, and exposes a function to get the task. Then your test could do something like this:

TEST(AsyncPanZoomController, LongTap) {
  nsRefPtr<MockContentControllerWithTaskInterceptor> mcc = new NiceMock<...>();
  ...
  // send a MULTITOUCH_START event, which will invoke mcc->PostDelayedTask
  Task* t = mcc->GetDelayedTask();
  EXPECT_NE(nullptr, t); // make sure the PostDelayedTask was called
  // run any code here you want to run inside the timeout
  t->Run();
  // run any code here you want to run after the timeout
  ...
}

Does that make sense? I'd really like to see more tests for this code and I think this approach should work reasonably well.

Comment 23

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

Comment on attachment 8345067 [details] [diff] [review]
942929-2.patch

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

This looks good but it's missing implementations of HandleLongTapUp in widget/android/AndroidBridge.cpp and widget/windows/winrt/APZController.cpp; this will cause build failures on fennec and metro.

I would also like to see a second patch with tests, if at all possible. See my above comment on how you might be able to accomplish that. Minusing for now.

::: dom/ipc/TabChild.cpp
@@ +1670,5 @@
> +  }
> +
> +  if (!mGlobal || !mTabChildGlobal) {
> +    return true;
> +  }

You can just call RecvHandleSingleTap here instead of duplicating the code.

Comment 24

[Dale Harvey (:daleharvey)]

Attachment: Use longtapup event to handle firing clicks when longtap not handled

Added a test to ensure we receive a longtap followed by a longtapup, added stub implementations to android and windows, call |RecvSingleTap| instead of duplicating the code in long tap up.

Comment 25

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

Comment on attachment 8345902 [details] [diff] [review]
Use longtapup event to handle firing clicks when longtap not handled

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

r=me with comments addressed.

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +38,5 @@
>  };
>  
> +class MockContentControllerDelayed : public MockContentController {
> +public:
> +  Task *mCurrentTask;

Make this private or protected

@@ +512,5 @@
>    apzc->Destroy();
>  }
>  
> +TEST(AsyncPanZoomController, LongPress) {
> +  nsRefPtr<MockContentControllerDelayed> mcc = new NiceMock<MockContentControllerDelayed>();

Thinking about it a bit more I think we don't want to use NiceMock here. NiceMock will make it so that calls to any of the mock functions that are not EXPECTed are ignored without warnings. However in this case we probably want to make sure that things like HandleSingleTap are not invoked, so it would be better to get rid of the NiceMock and explictly declare via EXPECT_CALL all things we expect to be called.

@@ +532,5 @@
> +  EXPECT_NE(nullptr, t);
> +  EXPECT_CALL(*mcc, HandleLongTap(CSSIntPoint(10, 10), 0)).Times(1);
> +  EXPECT_CALL(*mcc, HandleLongTapUp(CSSIntPoint(10, 10), 0)).Times(1);
> +
> +  // Manually invoke the longpress while the mouse is currently down.

s/mouse is currently down/touch is still down/

Comment 26

[Dale Harvey (:daleharvey)]

Attachment: Use longtapup event to handle firing clicks when longtap not handled

Comments addressed, removed the NiceMock and added an EXPECT for SendAsyncScrollDomEvent, carrying r+

Comment 27

[Dale Harvey (:daleharvey)]

Attachment: Use longtapup event to handle firing clicks when longtap not handled

rebased to current m-c and pushed to try

https://tbpl.mozilla.org/?tree=Try&rev=5478223d9b8e

Comment 28

[Dale Harvey (:daleharvey)]

Attachment: Use longtapup event to handle firing clicks when longtap not handled

Forgot to add header definitions, updated .h files, carrying r+, repush to try

https://tbpl.mozilla.org/?tree=Try&rev=573346b175b4

Comment 29

[Dale Harvey (:daleharvey)]

Attachment: Use longtapup event to handle firing clicks when longtap not handled

The android c++ compiler didnt like the comparison to a null pointer, switched to EXPECT_TRUE

https://tbpl.mozilla.org/?tree=Try&rev=950893c6b2cb

Comment 30

[Dale Harvey (:daleharvey)]

https://hg.mozilla.org/integration/b2g-inbound/rev/d1d7d42059c8

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d1d7d42059c8

Comment 32

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Seems like we forgot 1.3+ this bug. Let's reopen it until it lands on mozilla-aurora.

Comment 33

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

Uplift only happens after the bug is resolved, based on the status flags.

Comment 34

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

RyanVM pointed out that this doesn't apply cleanly on aurora, which made me realize that we haven't uplifted bug 946661 either. I requested blocking-b2g:1.3+ on that bug as well so we can uplift it. With that uplifted this one should apply cleanly.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/db2cc4f651e6

Comment 36

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

The patch includes some tests in the gtest unit test suite.