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

RESOLVED FIXED in Firefox 28

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: vingtetun, Assigned: daleharvey)

Tracking

Trunk
mozilla29
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:1.3+, firefox27 wontfix, firefox28 fixed, firefox29 fixed, b2g-v1.2 unaffected, b2g-v1.3 fixed)

Details

Attachments

(1 attachment, 7 obsolete attachments)

When long pressing an element that does not have a contextmenu and releasing the finger nothing happens while a click should have been generated.
Longtap events in the gesture listener unconditionally cancel single taps

https://bugzilla.mozilla.org/show_bug.cgi?id=942929
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
Assignee: nobody → dale
Summary: Long pressing an element without any contextmeny does not trigger a click → Long pressing an element without any contextmenu does not trigger a click
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
(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.
Posted patch 942929.patch (obsolete) — Splinter Review
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
Attachment #8342627 - Flags: review?(bugmail.mozilla)
Also I based this on top of https://bugzilla.mozilla.org/show_bug.cgi?id=940706 because BenWa told me to
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 :)
Attachment #8342627 - Flags: feedback+
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.
Attachment #8342627 - Flags: review?(bugmail.mozilla)
Attachment #8342627 - Flags: review-
Attachment #8342627 - Flags: feedback?(jmathies)
> 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 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.
Attachment #8342627 - Flags: feedback?(jmathies) → feedback?(mbrubeck)
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
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 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.)
Attachment #8342627 - Flags: feedback?(mbrubeck) → feedback-
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
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.
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
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
Posted patch 942929-2.patch (obsolete) — Splinter Review
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
Attachment #8344663 - Flags: feedback?(bugmail.mozilla)
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
Attachment #8344663 - Flags: feedback?(bugmail.mozilla) → feedback+
Posted patch 942929-2.patch (obsolete) — Splinter Review
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?
Attachment #8344663 - Attachment is obsolete: true
Attachment #8345067 - Flags: review?(bugmail.mozilla)
Depends on: 946661, 947931
(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 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.
Attachment #8345067 - Flags: review?(bugmail.mozilla) → review-
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.
Attachment #8342627 - Attachment is obsolete: true
Attachment #8345067 - Attachment is obsolete: true
Attachment #8345902 - Flags: review?(bugmail.mozilla)
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/
Attachment #8345902 - Flags: review?(bugmail.mozilla) → review+
Comments addressed, removed the NiceMock and added an EXPECT for SendAsyncScrollDomEvent, carrying r+
Attachment #8345902 - Attachment is obsolete: true
Attachment #8345929 - Flags: review+
rebased to current m-c and pushed to try

https://tbpl.mozilla.org/?tree=Try&rev=5478223d9b8e
Attachment #8345929 - Attachment is obsolete: true
Attachment #8345941 - Flags: review+
Forgot to add header definitions, updated .h files, carrying r+, repush to try

https://tbpl.mozilla.org/?tree=Try&rev=573346b175b4
Attachment #8345941 - Attachment is obsolete: true
Attachment #8345994 - Flags: review+
The android c++ compiler didnt like the comparison to a null pointer, switched to EXPECT_TRUE

https://tbpl.mozilla.org/?tree=Try&rev=950893c6b2cb
Attachment #8345994 - Attachment is obsolete: true
Attachment #8346191 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/d1d7d42059c8
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Seems like we forgot 1.3+ this bug. Let's reopen it until it lands on mozilla-aurora.
Status: RESOLVED → REOPENED
blocking-b2g: --- → 1.3+
Resolution: FIXED → ---
Target Milestone: mozilla29 → mozilla28
Uplift only happens after the bug is resolved, based on the status flags.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: mozilla28 → mozilla29
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.
Flags: in-testsuite?
The patch includes some tests in the gtest unit test suite.
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.