931763 - Simplify handling of first touch start and touch move in MetroInput

Status: RESOLVED FIXED

(Core Graveyard :: Widget: WinRT, defect)

Description

[Jim Mathies [:jimm]]

Working on this today I realized we can really simplify how we handle this event processing by combining some routines to streamline where we process the result of the first touchstart/touchmove. This should also fix some bugs related to the apz not doing what it's supposed to do - ignoring input after ContentReceivedTouch is called.

Comment 1

[Jim Mathies [:jimm]]

Attachment: wip

Comment 2

[Jim Mathies [:jimm]]

Comment on attachment 823321 [details] [diff] [review]
wip

oops - munged a couple patches together

Comment 3

[Jim Mathies [:jimm]]

Attachment: wip

Comment 4

[Jim Mathies [:jimm]]

Attachment: streamline touch handling v.1

Comment 5

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 823350 [details] [diff] [review]
streamline touch handling v.1

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

We spoke about this in #windev but I'll summarize here.

The original code made a distinction between "preventDefault was called on the first touchmove" and "preventDefault was called on the touchstart" because of language in the spec:
  For touchmove - "If the preventDefault method is called on the first touchmove event of an active touch point, it should prevent any default action caused by any touchmove event associated with the same active touch point, such as scrolling."
  For touchstart - "If the preventDefault method is called on this event, it should prevent any default actions caused by any touch events associated with the same active touch point, including mouse events or scrolling."

Jimm mentioned in channel that he'll update the patch in this bug to maintain the distinction between the two cases.

Comment 7

[Jim Mathies [:jimm]]

Attachment: streamline touch handling v.2

Comment 8

[Jim Mathies [:jimm]]

Attachment: streamline touch handling v.2

Comment 9

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 823465 [details] [diff] [review]
streamline touch handling v.2

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

Sorry this review has taken so long; it's been tough wrapping my head around all the logic involved. I wish we had tests so that we could easily verify that we're keeping the behavior consistent :(

Overall I have some questions about the behavior with this patch applied. It doesn't seem to me like we would be sending input to the gesture recognizer at appropriate times. Could you respond to the questions in the review? Perhaps I'm just not reading the logic correctly.

::: widget/windows/winrt/MetroInput.cpp
@@ +440,5 @@
>                        static_cast<void*>(&aEvent->touches));
>  }
>  
> +bool
> +MetroInput::ShouldDeliverInputToRecognizer()

Note to self: This function returns true in any of the following 3 cases:
1) the touch session started over chrome instead of over content
2) preventDefault was NOT called on the first touchmove
3) we haven't yet sent a touchmove

I'm curious whether this function should take into account whether preventDefault was called on the first touchstart event of a touch session?

@@ +502,5 @@
>  
> +  InitTouchEventTouchList(touchEvent);
> +  DispatchAsyncTouchEvent(touchEvent);
> +
> +  if (ShouldDeliverInputToRecognizer()) {

If I'm reading this correctly, we will still send input to the gesture recognizer even if the first touchstart event of a touch session had preventDefault called on it. That seems wrong to me; I believe we should not send any input to the gesture recognizer in that case.

@@ +514,5 @@
>  {
>    // Only feed move input to the recognizer if the first touchstart and
> +  // subsequent touchmove return results were not eConsumeNoDefault and
> +  // the target of the event is content.
> +  if (ShouldDeliverInputToRecognizer()) {

I think the comment doesn't match the code; from what I've read, I believe that ShouldDeliverInputToRecognizer() can return true even if preventDefault was called on the touchstart (just as long as it wasn't called on the first touchmove)

@@ +1214,5 @@
>  
> +  // If we have yet to deliver the first touch start and touch move, deliver the
> +  // event to both content and the apz. Ignore the apz's return result since we
> +  // give content the option of saying it wants to consume touch for both events.
> +  if (mCancelable) {

Note to self: mCancelable == "we have delivered both a touchstart and the first touchmove for a touch session"

@@ +1219,5 @@
> +    WidgetTouchEvent transformedEvent(*event);
> +    mWidget->ApzReceiveInputEvent(event, &transformedEvent);
> +    mWidget->DispatchEvent(&transformedEvent, status);
> +    if (event->message == NS_TOUCH_START) {
> +      mContentConsumingTouch = (nsEventStatus_eConsumeNoDefault == status);

Note to self: mContentConsumingTouch == "preventDefault was called on the first touchstart or first touchmove event for a touch session"

@@ +1223,5 @@
> +      mContentConsumingTouch = (nsEventStatus_eConsumeNoDefault == status);
> +    } else if (event->message == NS_TOUCH_MOVE) {
> +      mCancelable = false;
> +      // Add this result to to our content comsuming flag
> +      mContentConsumingTouch |= (nsEventStatus_eConsumeNoDefault == status);

This is a bitwise-OR, not a boolean OR

@@ +1227,5 @@
> +      mContentConsumingTouch |= (nsEventStatus_eConsumeNoDefault == status);
> +      // Per the W3C spec, gesture events only cancel out if content calls
> +      // preventDefault on the first touch move, otherwise we are supposed
> +      // to keep sending them. Generally all this applies to are our simulated
> +      // tap gestures. Chrome currently gets all gestures no matter what.

I think this comment could be more clear:
  Calling preventDefault on the first touchmove event of a touch session should prevent any default actions arising from *just the touchmoves* sent during that session. So this will prevent (e.g.) touch scrolling.
  Calling preventDefault on the touchstart event of a new touch session should prevent any default actions arising from *any* of the touch events sent during that session. This means that it will prevent everything in the previous case, and additionally will prevent (e.g.) "tap" gestures from sending mousedown/click/mouseup.

@@ +1228,5 @@
> +      // Per the W3C spec, gesture events only cancel out if content calls
> +      // preventDefault on the first touch move, otherwise we are supposed
> +      // to keep sending them. Generally all this applies to are our simulated
> +      // tap gestures. Chrome currently gets all gestures no matter what.
> +      mRecognizerWantsEvents = !(nsEventStatus_eConsumeNoDefault == status);

Note to self: mRecognizerWantsEvents == "preventDefault was NOT called on the first touchmove event for a touch session OR we haven't yet sent the first touchmove event"

@@ +1240,5 @@
> +      }
> +    }
> +    // If content is consuming touch don't generate any gesture based
> +    // input - clear the recognizer state without sending any events.
> +    if (!ShouldDeliverInputToRecognizer()) {

This seems to only take into account whether the first touchmove had preventDefault called on it, and not whether the touchstart had preventDefault called on it. Is this intended?

Comment 10

[Jim Mathies [:jimm]]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #9)
> If I'm reading this correctly, we will still send input to the gesture
> recognizer even if the first touchstart event of a touch session had
> preventDefault called on it. That seems wrong to me; I believe we should not
> send any input to the gesture recognizer in that case.

Yes you're right. I tied taps to canceling touchmove, it needs to cancel out for touchstart as well. Will update.

> 
> @@ +514,5 @@
> >  {
> >    // Only feed move input to the recognizer if the first touchstart and
> > +  // subsequent touchmove return results were not eConsumeNoDefault and
> > +  // the target of the event is content.
> > +  if (ShouldDeliverInputToRecognizer()) {
> 
> I think the comment doesn't match the code; from what I've read, I believe
> that ShouldDeliverInputToRecognizer() can return true even if preventDefault
> was called on the touchstart (just as long as it wasn't called on the first
> touchmove)

I think I already removed this, it's not in my patch queue patch.

> @@ +1223,5 @@
> > +      mContentConsumingTouch = (nsEventStatus_eConsumeNoDefault == status);
> > +    } else if (event->message == NS_TOUCH_MOVE) {
> > +      mCancelable = false;
> > +      // Add this result to to our content comsuming flag
> > +      mContentConsumingTouch |= (nsEventStatus_eConsumeNoDefault == status);
> 
> This is a bitwise-OR, not a boolean OR

Heh, still works. :) Will update with !!.

Comment 11

[Jim Mathies [:jimm]]

Attachment: streamline touch handling v.3

Ok I think I have this straightened out. The one thing I fudge here is move related gestures that would go to chrome, those get canceled out if touchstart gets canceled. But I don't think that's a big deal.

Comment 12

[Jim Mathies [:jimm]]

Comment on attachment 824256 [details] [diff] [review]
streamline touch handling v.3

crap, I just noticed another issue.

Comment 13

[Jim Mathies [:jimm]]

Attachment: streamline touch handling v.3

Ok, added support for chrome consuming touch. We don't currently do this in front end, but support would seem pretty broken if someone tried to without the change I added.

Comment 14

[Jim Mathies [:jimm]]

Comment on attachment 824257 [details] [diff] [review]
streamline touch handling v.3

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

::: widget/windows/winrt/MetroInput.cpp
@@ +1203,5 @@
>    }
>  
> +  // If this event is destined for chrome, deliver it directly there bypassing
> +  // the apz.
> +  if (!mCancelable && mChromeHitTestCacheForTouch) {

specifically, only dropping in here if mCancelable is false, this way we fall down to the touchstart/touchmove checks below for chrome and content.

@@ +1214,5 @@
> +  // give content the option of saying it wants to consume touch for both events.
> +  if (mCancelable) {
> +    WidgetTouchEvent transformedEvent(*event);
> +    mWidget->ApzReceiveInputEvent(event, &transformedEvent);
> +    mWidget->DispatchEvent(mChromeHitTestCacheForTouch ? event : &transformedEvent, status);

and here, making sure we send the untransformed event to chrome.

Comment 15

[Jim Mathies [:jimm]]

Attachment: streamline touch handling v.4

This brings back the bit of code in input.js. Without it MetroInput sends a touchcancel when chrome doesn't consume (per the added bits I added in the last patch) and that cancels our front end scrolling for things like context menus and the tab strip.

Comment 16

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 824312 [details] [diff] [review]
streamline touch handling v.4

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

I don't see any issues with the new patch. There's a page out there somewhere that is used for testing preventDefault on touchstarts and touchmoves (I think mbrubeck might know where it is). Might be nice to test our metro browser out there!

::: widget/windows/winrt/MetroInput.cpp
@@ +1204,5 @@
>  
> +  // If this event is destined for chrome, deliver it directly there bypassing
> +  // the apz.
> +  if (!mCancelable && mChromeHitTestCacheForTouch) {
> +    WinUtils::Log("!mCancelable && mChromeHitTestCacheForTouch");

I'm assuming this won't go into the final patch? It seems like this might be a lot of logging for touchmove events that fire in chrome.

Comment 17

[Jim Mathies [:jimm]]

Attachment: updated for landing

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e26cc9966548