Closed Bug 947892 Opened 8 years ago Closed 7 years ago
Single tap isnt fired when followed by multiple touches
As explained in https://bugzilla.mozilla.org/show_bug.cgi?id=942929#c8
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).
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.
(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  or put down a second finger , 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.
Assignee: nobody → ameyabap
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
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.
Assignee: ameyabap → nobody
Depends on: 985541
7 years ago
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
Whiteboard: [mentor=botond][lang=c++] → [lang=c++]
Gtests passes well: https://tbpl.mozilla.org/?tree=Try&rev=56b464015291, but i need some more testing on the real device.
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
Done. Btw, I didn't run separate 'try' but run GTests locally and they are fine.
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.)
Attachment #8498827 - Flags: review?(botond) → review+
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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.