preventing default action of touchend event should cancel synthetic click event

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: miketaylr, Assigned: kats)

Tracking

({dev-doc-needed})

unspecified
mozilla34
ARM
Android
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox33 wontfix, firefox34 affected)

Details

Attachments

(3 attachments, 2 obsolete attachments)

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).
No longer depends on: 1016480
See Also: → 1016480
Component: Panning and Zooming → DOM: Events
Assignee: nobody → bugmail.mozilla
Posted patch WIP (obsolete) — Splinter Review
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.
Posted patch Patch (obsolete) — Splinter Review
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.
Attachment #8457965 - Attachment is obsolete: true
Posted patch PatchSplinter Review
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.
Attachment #8458010 - Attachment is obsolete: true
Attachment #8458201 - Flags: review?(bugs)
Attachment #8458201 - Flags: review?(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.
Attachment #8458201 - Flags: review?(botond) → review+
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 on attachment 8458201 [details] [diff] [review]
Patch

A bit regression risky, but I assume this will land trunk only, and not
to branches.
Attachment #8458201 - Flags: review?(bugs) → review+
Matt? (comment 5)
Flags: needinfo?(mbrubeck)
(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.
(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.
Flags: needinfo?(mbrubeck)
5.6 should say something about preventDefault.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
https://hg.mozilla.org/mozilla-central/rev/568b73c5bd31
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla33 → ---
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.
Attachment #8463623 - Flags: review?(bugs)
Attachment #8463625 - Flags: review?(botond) → review+
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.
Attachment #8463623 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/e31206802483
https://hg.mozilla.org/mozilla-central/rev/ac8248c5b891
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.