Closed Bug 1219898 Opened 5 years ago Closed 5 years ago

Get rid of TouchManager::gPreventMouseEvents

Categories

(Core :: DOM: Events, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: kats, Assigned: kats)

Details

Attachments

(3 files, 1 obsolete file)

The gPreventMouseEvents flag in TouchManager is not really needed any more. It seems like the original purpose of it was to help implement the "preventdefault" behaviour of touch events so that no mouse events would get dispatched. Since that time in ancient history, a couple of changes have occurred that make this no longer necessary.

One is that in general touch events don't generate corresponding mouse events, except in the case of a "tap" gesture - this will generate mousemove/mousedown/mouseup/click mouse events. These are the only mouse events that get generated from touch events now.

The other is that the APZ and related code is now responsible for detecting the "tap" gesture from touch events and issuing the request for the corresponding mouse events. This is true whenever the dom.w3c_touch_events.enabled pref is nonzero, on all platforms that support touch events.

Right now the APZ uses the nsEventStatus returned from the WidgetTouchEvent dispatch to determine if preventDefault was called on the touch event or not. However this return status is set by the presShell based on TouchManager::gPreventMouseEvents using a bunch of logic that is no longer really needed, and that logic is interfering with my solution for bug 1174532.

The specific problem I'm seeing is that given this sequence of touch inputs:
 touchstart
 touchmove <-- preventDefault is called by content
 touchend
the nsEventStatus returned is eConsumeNoDefault for the touchend, instead of eIgnore. This happens because gPreventMouseEvents get set, and this interferes with the changes I'm making.
Oh part 3 caused test_bug603008.html to fail, but the change is intentional. I'll change some of the expected "prevent" values from true to false at [1]. Specifically the ones that get set to true as because they are propagated from the touchstart/first touchmove.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/events/test/test_bug603008.html?rev=e31206802483&mark=415-422&force=1#415
So "If touchstart, touchmove, or touchend are canceled, the user agent should not dispatch any mouse event that would be a consequential result of the the prevented touch event. " per spec.
I assume the patches don't change that behavior.
looking the patches ...
(In reply to Olli Pettay [:smaug] from comment #7)
> So "If touchstart, touchmove, or touchend are canceled, the user agent
> should not dispatch any mouse event that would be a consequential result of
> the the prevented touch event. " per spec.
> I assume the patches don't change that behavior.
> looking the patches ...

Where is this quote taken from? I don't see that exact wording in http://www.w3.org/TR/touch-events/ - but anyway, this is not how other browsers behave, so the spec needs updating. Specifically, if the touchmove (and only the touchmove) is canceled, then the mouse events are still dispatched. I verified this in Chrome on Android using http://people.mozilla.org/~kgupta/touch.html - check the box that says "first point touch move". If the above text were implemented as written then after checking that box you shouldn't be able to check any other boxes since the click events would never be dispatched. However in Chrome for Android (and from what I've heard, Safari on iOS) both allow it. The patches I'll be posting to bug 1174532 will change our behaviour to match on Firefox OS.
Ah, I see the text at http://w3c.github.io/touch-events/ - I'll file a spec bug for it as per the readme in the repo.
Kats: that reasoning isn't quite right.  The reason you can still click when canceling the first touchmove is that in Chrome we NEVER send a touchmove and a click for the same sequence.  So if you see a touchmove event at all, then a mousedown/mouseup/click is not going to happen.  We've found there are pages that depend on this property, so we suppress any small movement we see as long as scrolling is possible.  Some details in here: https://docs.google.com/document/d/12k_LL_Ot9GjF8zGWP9eI_3IMbSizD72susba0frg44Y/edit

So I believe Chrome does conform to that spec language - there's just no way to test the 'touchmove' case because it'll never happen.
Oh, interesting. According to the google doc you linked Chrome Desktop has a different behaviour than Chrome Android, and Chrome Desktop matches what I'm trying to do. Do you know if there are any plans to change that?

Also the reason I don't like suppressing touchmoves during the slop region is because it can adversely impact apps like games or finger-painting where you really want to track even small moves from the touchstart point. I suppose you could dispatch touchmoves in the slop region if there's no chance of it triggering a scroll but I don't really like that inconsistency.

Do you happen to know which websites rely on the property that you mentioned in the above comment?
Ah, I should read the whole doc before commenting. Apparently Chrome Desktop was changed to match Chrome Android in https://code.google.com/p/chromium/issues/detail?id=334040, and there's a link to a website that supposedly had problems in comment 6.
Attachment #8680832 - Flags: review?(bugs) → review+
Yeah, that's exactly why we originally had a difference between android and desktop (I was on the desktop side at the time).  We compromised by enabling "touchmove slop suppression" only when scrolling is still possible, i.e. the touchstart wasn't canceled.  Games/painting apps tend to cancel the touchstart event.

Safari appears to suppress slop all the time (although I haven't tested to see if this is iOS/firmware or actually Safari itself - it's a feature of the atmel touch controllers I believe iOS devices use).
FWIW, I'm not opposed to trying to eliminate slop suppression (I agree it's ugly).  But I'd want to try to get Safari on board too (otherwise sites tested in Safari may be broken in Chrome) and honestly since we came to the compromise I haven't seen a single scenario where slop suppression caused harm.  So it seems pretty low priority to me.  

If you decide to align with Chrome here, then perhaps we should add some spec text to codify some/all of this?
The one site that's listed as broken at https://code.google.com/p/chromium/issues/detail?id=334040#c6 is http://www.dodgycoder.net/2013/02/googles-fiber-leeching-caper.html?m=1 but it's not clear to me in what way it's broken. With this proposed behaviour in Firefox OS the page seems to work fine (panning, clicking on links, etc).

Theoretically, this change would cause a problem if the page was:
(a) listening for touchmove events and calling preventDefault on them, AND
(b) not calling preventDefault on either touchstart or touchend, AND
(c) expecting mouse events to not get dispatched from tap gestures

I feel like in this case it's relatively easy for the page to implement a fix - simply listen for touchstart instead of touchmove and call preventDefault on that. So the only question is - how many sites depend on the above behaviour, and how hard would it be for them to update? I'm skeptical that many sites would choose to do a preventDefault on the touchmove rather than the touchstart when they want to cancel clicks, because it's possible to get clicks without any touchmoves at all (just a touchstart+touchend).

Note that with the current behaviour in Chrome/Safari as you described (i.e. not sending touchmoves in the slop region) it is *impossible* for an app to get the raw touch data in cases where they might want it (e.g. a fingerpainting app that is scrollable). I agree this is a rare case as well but I feel like making something impossible is undesirable, so I would still prefer implementing this change in Firefox at least and trying to get sites to update where possible.
The pattern I've seen more commonly is custom click-detection logic (usually to work around the click delay) which aborts click on touchmove.  This can make buttons subtly (or not to subtly depending on the touch controller configuration) harder to activate.  I don't have pages handy offhand, but anecdotally I've seen this enough that I'd be worried about changing our behavior and breaking a bunch of small sites.  I did get a bunch of the more major libraries (like fastclick.js) updated not to make this assumption.

> Note that with the current behaviour in Chrome/Safari as you described (i.e. not sending touchmoves in the slop region) it is *impossible* for an app to get the raw touch data in cases where they might want it 

That's true only for the still-scrollable cases.  Can you describe how a "fingerpainting app that is scrollable" would work?  Does dragging your finger scroll or paint?  The inherent conflict between these two is what makes our design make sense IMHO.  Either you care about precise positioning or you care about scrolling - the UX interaction model prevents you from being able to care about both, regardless of this implementation detail :-)
(In reply to Rick Byers from comment #16)
> The pattern I've seen more commonly is custom click-detection logic (usually
> to work around the click delay) which aborts click on touchmove.

Ah I see, that makes sense. So it's not that my proposed change would break stuff in Firefox, it's that switching to Chrome's behaviour would fix even more stuff in Firefox. That was the flaw in my argument, thanks for pointing it out :) I agree that I've seen this pattern as well and it's currently broken in Firefox because we send touchmoves in the slop. Custom click-detection code has to reimplement the slop check in order to work properly, and sometimes they don't.
Attachment #8681224 - Flags: review?(bugs) → review+
Comment on attachment 8680834 [details] [diff] [review]
Part 2 - Update APZ code to stop using gPreventMouseEvents

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

::: gfx/layers/apz/util/APZEventState.cpp
@@ +259,5 @@
>    if (aEvent.mMessage == eTouchStart && aEvent.touches.Length() > 0) {
>      mActiveElementManager->SetTargetElement(aEvent.touches[0]->GetTarget());
>    }
>  
> +  bool isTouchPrevented = aContentResponse == nsEventStatus_eConsumeNoDefault;

Is this interpretation of the content response documented somewhere? If not, please mention it in a comment, either in APZEventState.h, or perhaps in nsIWidget.h at the declaration of DispatchEvent().
Attachment #8680834 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #18)
> > +  bool isTouchPrevented = aContentResponse == nsEventStatus_eConsumeNoDefault;
> 
> Is this interpretation of the content response documented somewhere? If not,
> please mention it in a comment, either in APZEventState.h, or perhaps in
> nsIWidget.h at the declaration of DispatchEvent().

I was under the impression that eConsumeNoDefault generally meant that the default action was prevented - see comment at http://mxr.mozilla.org/mozilla-central/source/widget/EventForwards.h?rev=632ffd1d42ba#23.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #19)
> (In reply to Botond Ballo [:botond] from comment #18)
> > > +  bool isTouchPrevented = aContentResponse == nsEventStatus_eConsumeNoDefault;
> > 
> > Is this interpretation of the content response documented somewhere? If not,
> > please mention it in a comment, either in APZEventState.h, or perhaps in
> > nsIWidget.h at the declaration of DispatchEvent().
> 
> I was under the impression that eConsumeNoDefault generally meant that the
> default action was prevented - see comment at
> http://mxr.mozilla.org/mozilla-central/source/widget/EventForwards.
> h?rev=632ffd1d42ba#23.

Indeed, I suppose that's good enough for "prevent default" :)
I'm going to land the patches on this bug, since they're a good cleanup. They don't actually implement the behaviour change being discussed in comments 7-17. I had planned to put the patches for those on bug 1219898, but I'm still trying to decide what to do. We already have bug 1141127 on file for eliminating the touchmove events during the slop so we can move any further discussion there.
No longer blocks: 1174532
You need to log in before you can comment on or make changes to this bug.