1016481 - preventing default action of touchend event should cancel synthetic click event

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Mike Taylor [:miketaylr]]

This bug is to track the change for Firefox OS.

+++ This bug was initially created as a clone of Bug #1016480 +++

Currently calling preventDefault() on a touchend event does not cancel the synthetic click event that comes after. We should add that for compatibility with current Chrome Mobile and iOS Safari.

This breaks sharing videos on Vimeo.com and playback for tablet users (also on vimeo).

Comment 1

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

Attachment: WIP

This works to prevent clicks. However, if you tap on something and the touch-end is cancelled, the thing you tapped on remains in the active state. This seems to happen because TabChild receives the NotifyAPZStateChange callback before the touch-end event, and so my change to the AEM::HandleTouchEnd callsite is a no-op. Trying to figure out what makes the most sense here.

Comment 2

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

Attachment: Patch

This works, but it's a little ugly, so I'll sit on it for a bit to see if I can think of anything better.

Comment 3

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

Attachment: Patch

The thing that makes this tricky is the ordering in which TabChild receives notifications about things.
(1) First it receives the APZStateChange notification, with aArg = true indicating the APZ thought there was a click
(2) Then it gets the touch-end event to process, which can be preventDefault'ed to cancel the click
(3) Then it gets the HandleSingleTap call from APZ to dispatch the click

In order to properly clear the active element, I had to move the call to ActiveElementManager::OnTouchEnd from (1) to (2) because it isn't until (2) that we actually know whether or not a click will be dispatched at (3).

Note also that if I leave the OnTouchEnd call at (1) and just call ActiveElementManager::ResetActive at (2) that doesn't work, because the call at (1) clears ActiveElementManager::mTarget, and so the ResetActive function doesn't have any way to find the root element.

So this is the best that I could come up with; open to suggestions.

Comment 4

[Botond Ballo [:botond]]

Comment on attachment 8458201 [details] [diff] [review]
Patch

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

Looks reasonable to me.

::: dom/ipc/TabChild.h
@@ +590,5 @@
>      bool mPendingTouchPreventedResponse;
>      ScrollableLayerGuid mPendingTouchPreventedGuid;
>      void FireSingleTapEvent(LayoutDevicePoint aPoint);
>  
> +    enum ClickState {

I would prefer using MOZ_BEGIN_NESTED_ENUM_CLASS (see GeckoContentController::APZStateChange for an example). Also, I believe we use CamelCase rather than UPPER_CASE for enumerators elsewhere.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

So do we have a spec bug about this? Since nothing in the spec hints about this.
(But based on https://bugzilla.mozilla.org/show_bug.cgi?id=1016480#c4 we should do this)

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8458201 [details] [diff] [review]
Patch

A bit regression risky, but I assume this will land trunk only, and not
to branches.

Comment 7

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

Matt? (comment 5)

Comment 8

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

(In reply to Botond Ballo [:botond] from comment #4)
> > +    enum ClickState {
> 
> I would prefer using MOZ_BEGIN_NESTED_ENUM_CLASS (see
> GeckoContentController::APZStateChange for an example). Also, I believe we
> use CamelCase rather than UPPER_CASE for enumerators elsewhere.

I changed it to CamelCase, but it looks like I can only use MOZ_BEGIN_NESTED_ENUM_CLASS if the enum is public. Otherwise all the operator-delete code that MOZ_FINISH_NESTED_ENUM_CLASS spits out won't compile. So I left it using a private enum because I'd rather it stay private than be strongly-typed.

Try pushes that include this patch are at:
https://tbpl.mozilla.org/?tree=Try&rev=4ad71eff0b94
https://tbpl.mozilla.org/?tree=Try&rev=3c760d184c0d

Waiting for the tree to reopen before I can land.

Comment 9

[Matt Brubeck (:mbrubeck)]

(In reply to Olli Pettay [:smaug] from comment #5)
> So do we have a spec bug about this? Since nothing in the spec hints about
> this.

Section 5.4 of the W3C Touch Events spec says that "touchend" is cancelable and includes "click" as one of its default behaviors of:
http://www.w3.org/TR/2013/REC-touch-events-20131010/#list-of-touchevent-types

So it is consistent with the spec that preventDefault on "touchend" will prevent "click" from firing, even though Section 7 does not explicitly require this.  I'll open a spec bug to clarify this in the Touch Events errata.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

5.6 should say something about preventDefault.

Comment 11

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> it looks like I can only use
> MOZ_BEGIN_NESTED_ENUM_CLASS if the enum is public. Otherwise all the
> operator-delete code that MOZ_FINISH_NESTED_ENUM_CLASS spits out won't
> compile. 

I filed bug 1040384 for this.

> So I left it using a private enum because I'd rather it stay
> private than be strongly-typed.

Sounds reasonable.

Comment 12

[Matt Brubeck (:mbrubeck)]

Proposed a spec change to clarify this behavior:
http://lists.w3.org/Archives/Public/public-touchevents/2014Jul/0011.html

Comment 13

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/568b73c5bd31

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/568b73c5bd31

Comment 15

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

I backed this out to fix bug 1043022. I'll request an uplift of the backout to mozilla33 (now on aurora) as well, and then hopefully fix this properly on 34.

Comment 16

[Botond Ballo [:botond]]

For reference, the m-i backout revision is https://hg.mozilla.org/integration/mozilla-inbound/rev/bf3d32fac6b3

Comment 17

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

Attachment: Part 1 - Allow cancelling touch-end

A fundamental problem in my last approach was conflating the code to prevent clicks with the code to handle :active state. After much head-banging I think that approach is unworkable. So I split the patch into two parts; one to deal with the actual functional click-canceling change (part 1) and one to deal with the active state management (part 2). This seems to work better.

Comment 18

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

Attachment: Part 2 - Deal with active state manager

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8463623 [details] [diff] [review]
Part 1 - Allow cancelling touch-end

I guess this is fine since mTouchEndCancelled value is just checked in case
of RecvHandleSingleTap.

Comment 20

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/e31206802483
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac8248c5b891

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/e31206802483
https://hg.mozilla.org/mozilla-central/rev/ac8248c5b891