Closed Bug 988494 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Panning and Zooming, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: julienw, Assigned: kats)

References

Details

Attachments

(1 file, 3 obsolete files)

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 :)
No longer depends on: 988492
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.
Depends on: 985541
Attached patch WIP (obsolete) — Splinter Review
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.
Assignee: nobody → bugmail.mozilla
Attached patch WIP v2 (obsolete) — Splinter Review
Updated test code and docs because now we're not gonna fire a LongTapUp if the LongTap was consumed.
Attachment #8398692 - Attachment is obsolete: true
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.
Unassigning for now. I'll pick this up again once 1009733 is done.
Assignee: bugmail.mozilla → nobody
Attached patch Patch to test this behaviour (obsolete) — Splinter Review
With the patch on bug 1009733, these tests exercise the behaviour described in this bug, and pass.
Attachment #8398774 - Attachment is obsolete: true
Attached patch TestsSplinter Review
This depends on the patches in bug 1009733, which fix the actual bug.
Assignee: nobody → bugmail.mozilla
Attachment #8454518 - Attachment is obsolete: true
Attachment #8456108 - Flags: review?(botond)
Attachment #8456108 - Flags: review?(botond) → review+
https://hg.mozilla.org/mozilla-central/rev/80bfdd5622b4
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.