947892 - Single tap isnt fired when followed by multiple touches

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Dale Harvey (:daleharvey)]

As explained in https://bugzilla.mozilla.org/show_bug.cgi?id=942929#c8

Comment 1

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

The relevant part of the above comment is reproduced below:

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).

Comment 2

[Ameya Bapat]

Hi..
I would like to take this bug. Can u help me through? I think i got the problem. I am looking into the GestureEventListener.cpp code.

Comment 3

[Botond Ballo [:botond]]

(In reply to Ameya from comment #2)
> Hi..
> I would like to take this bug. Can u help me through? I think i got the
> problem. I am looking into the GestureEventListener.cpp code.

Sure! I've assigned the bug to you.

To get you started on a solution, here are some thoughts:

  - Notice that HandleTapCancel() is called when we have a finger down and
    then we either start moving that finger [1] or put down a second finger 
    [2], telling us that there is no tap.

  - If we are in the GESTURE_WAITING_DOUBLE_TAP state when HandleTapCancel()
    is called, it means that prior to the current finger being down, there 
    was _previously_ a finger down and up. So even though we are cancelling
    the tap for the _current_ finger being down, we need to dispatch the
    tap (i.e. call HandleSingleTapConfirmedEvent()) for that previous touch.
    We are not doing that right now.

  - In the above case, we have to be careful to dispatch the correct touch
    coordinates. Right now, we use the variable 'mLastTouchInput' to keep
    track of the coordinates to dispatch when waiting for a double tap
    times out. In our case, mLastTouchInput will already be the current
    touch, whereas we want to dispatch the previous touch, so we'll have
    to change something there (perhaps add a new field).

Feel free to ping me on IRC to chat more about this - my nick is 'botond' 
and you can find me in #gfx.

Comment 4

[Dale Harvey (:daleharvey)]

So the first thing to do would be to get the tests running and to write a failing test

http://mxr.mozilla.org/mozilla-central/source/gfx/tests/gtest/TestAsyncPanZoomController.cpp is the file thats likely best to put the test in, https://developer.mozilla.org/en-US/docs/GTest is the documentation for getting the tests running

Comment 5

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

Ameya, are you still working on this bug? I'm unassigning it from you for now but if you'd like to continue working on it please let us know.

Also note that the code in GestureEventListener.cpp is changing significantly as part of bug 985541.

Comment 6

[Nick Lebedev [:nl]]

Attachment: Add dispatching of the tap event when the tap is followed by multiple touches. v1.

Gtests passes well: https://tbpl.mozilla.org/?tree=Try&rev=56b464015291, but i need some more testing on the real device.

Comment 7

[Botond Ballo [:botond]]

Comment on attachment 8498116 [details] [diff] [review]
Add dispatching of the tap event when the tap is followed by multiple touches. v1.

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

This patch seems to correctly handle the case where the user puts two fingers down simultaneously after a tap.

However, I think we should also handle the case where there is a small gap between the first finger going down and the second; that is:
  (1) finger down
  (2) finger up
  (3) finger down
  (4) second finger down

(This can often happen when the user intends to put the two fingers down simultaneously, but the hardware is fast enough to detect the small difference in time between the two fingers touching down.)

In this scenario, event (3) will put us into the GESTURE_SECOND_SINGLE_TOUCH_DOWN state. So, to handle it, we should add a TriggerSingleTapConfirmedEvent() call to the GESTURE_SECOND_SINGLE_TOUCH_DOWN case of HandleInputTouchMultiStart(), similar to the one in the GESTURE_FIRST_SINGLE_TOUCH_UP case.

We should also have a test for this scenario.

Otherwise the patch looks good - thanks for working on this!

::: gfx/layers/apz/src/GestureEventListener.h
@@ +179,5 @@
> +   * Cached copy of the last tap gesture input.
> +   * In the situation when we have a tap followed by a pinch we lose info
> +   * about tap since we keep only last input and to dispatch it correctly
> +   * we save last tap copy into this variable.
> +   * For more see bug 947892.

s/For more/For more info

Comment 8

[Nick Lebedev [:nl]]

Attachment: Add dispatching of the tap event when the tap is followed by multiple touches. v2.

Done. Btw, I didn't run separate 'try' but run GTests locally and they are fine.

Comment 9

[Botond Ballo [:botond]]

Comment on attachment 8498827 [details] [diff] [review]
Add dispatching of the tap event when the tap is followed by multiple touches. v2.

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

Thanks!

(If you haven't already, please do test on a device before landing.)

Comment 10

[Nick Lebedev [:nl]]

Tried on the real device:
* The second case (described by you) is fixed.
* I wasn't able to reproduce the first case when touches are put simultaneously which is expected i believe ;), but at least it is covered by the gtest so I hope that this is Ok.

Given this i think the bug is fixed and I'm putting checkin-needed flag.

Comment 11

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/e5b574a8db51

Thanks, Nick!

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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