988494 - [B2G] a defaultPrevented touchstart should not trigger a click when doubletapping

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Julien Wajsberg [:julienw]]

Here is a testcase: http://everlong.org/mozilla/testcase-dblclick/

In this testcase the touchstart event handler calls "preventDefault".

Single tapping the interaction area prevents correctly the click event. However, double tapping generates a click event, which is wrong.

We think it's part of the APZC behavior, that's why I'm filing this under Panning and Zooming, but please change the component if it's wrong :)

Comment 1

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

Bug 985541 is rearranging a lot of this code so we should retest this after that lands to see if it still happens. If it does it should be easy to track down and fix.

Comment 2

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

Attachment: WIP

Looks like the APZC code to handle preventDefault() was pretty crappy, and would still process some events. Consider the simple sequence of events: touchstart, touchmove, touchend, where the touchstart is prevent-default'ed by content.

- The touchstart goes through ReceiveInputEvent, sets the state to WAITING_CONTENT_RESPONSE, starts the content response timer, gets added to mTouchQueue, and returns.
- Then we get the ContentReceivedTouch(true) call from content. This drops everything in mTouchQueue but doesn't find the end/cancel event, so leaves the APZC in state WAITING_CONTENT_RESPONSE.
- Then we get the move event. Since we are in state WAITING_CONTENT_RESPONSE this gets added to mTouchQueue and also starts a new content response timer.
- This timer eventually expires, and calls ContentReceivedTouch(false)
- The move event is then sent into APZC::HandleInputEvent (and into the GestureEventListener) from the loop in CheckContentResponse. This probably shouldn't happen.
- Following touchend event also gets dispatched into the APZC and GestureEventListener for the same reason, but then finally sets the state back to NOTHING.

The above flow shows that the preventDefault codepath was only working by accident - that is, the touchstart event (plus anything else that might have made it into mTouchQueue before content responded) would get dropped, but everything after that in the touch input block would get processed still. It just so happens that most of our gesture code will ignore events if they don't get the touchstart event first.

I haven't fully confirmed what was happening in the double-tap case, but I suspect that one of the content response timers from the first move/end was triggering before the content response from the second touchstart was received, and so the entirety of the second touchstart/move/end events were getting processed. This resulted in the click event getting fired.

My attached patch cleans up the codepaths by adding a new DEFAULT_PREVENTED state. We enter this state once we get a call to ContentReceivedTouch(true) and drop all events upto the end of the input block. It also gets rid of the mHandlingTouchQueue variable which is at best useless (at worst it deludes somebody into thinking it fixes a thread-safety issue which it doesn't).

The patch needs some more testing before I'm satisfied with it.

Comment 3

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

Attachment: WIP v2

Updated test code and docs because now we're not gonna fire a LongTapUp if the LongTap was consumed.

Comment 4

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

After trying to write some tests for the new code I discovered a tricky problem - the loop in CheckContentResponse stops upon encountering a END or CANCEL event, but there might be another input block in the queue after it, and that will not get processed properly. Not sure what the best way to deal with this is. Probably some variant of what I did in Java-land for Fennec but that's a bigger change than I want to try at the moment.

Comment 5

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

Unassigning for now. I'll pick this up again once 1009733 is done.

Comment 6

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

Attachment: Patch to test this behaviour

With the patch on bug 1009733, these tests exercise the behaviour described in this bug, and pass.

Comment 7

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

Attachment: Tests

This depends on the patches in bug 1009733, which fix the actual bug.

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/80bfdd5622b4

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/80bfdd5622b4