1143655 - Add TOUCH_CANCEL sending for pointer events

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Maksim Lebedev]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20150305021524




Expected results:

FireFox should send touch_cancel event into content, when Firefox should do default behavior.

Comment 1

[Maksim Lebedev]

Attachment: touch_cancel_ver1.diff

Comment 2

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

Comment on attachment 8578017 [details] [diff] [review]
touch_cancel_ver1.diff

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

::: widget/windows/nsWindow.cpp
@@ +6265,5 @@
> +            nsEventStatus_eConsumeDoDefault == result) {
> +        MultiTouchInput cancel(inputToDispatch);
> +        cancel.mType = MultiTouchInput::MULTITOUCH_CANCEL;
> +        cancel.mTimeStamp = TimeStamp::Now();
> +        DispatchTouchEventForAPZ(cancel, guid, inputBlockId);

You need to add touch points to this. Also I don't think you want to be dispatching this on every single touch event. You only need to do it once per input block. See how it's done in the metro code: http://mxr.mozilla.org/mozilla-central/source/widget/windows/winrt/MetroInput.cpp?rev=9afd0ba6ef0c#1430

Comment 3

[Maksim Lebedev]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)
> You need to add touch points to this.
I expect that it happens in c-tor:    MultiTouchInput cancel(inputToDispatch);
> Also I don't think you want to be dispatching this on every single touch event.
> You only need to do it once per input block. See how it's done in the metro code.
I have seen Metro, and my implementation dispatches cancel only at cumulative event
(I mean not on every single touch point).

Comment 4

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

(In reply to Maksim Lebedev from comment #3)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)
> > You need to add touch points to this.
> I expect that it happens in c-tor:    MultiTouchInput
> cancel(inputToDispatch);

Ah, sorry, I missed that.

> > Also I don't think you want to be dispatching this on every single touch event.
> > You only need to do it once per input block. See how it's done in the metro code.
> I have seen Metro, and my implementation dispatches cancel only at
> cumulative event
> (I mean not on every single touch point).

Still not clear to me how this happens. I wanted to apply this patch and try it out but it looks like it's based on the patch in bug 736048 which is obsolete now. I guess you still need this cancel event even in the new version of the code that I landed, in order to properly support pointer events. I'll attach an outline of what I think the patch should look like.

Comment 5

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

Attachment: Patch outline

This is more or less what I would expect to do the job properly in a cross-platform way. Feel free to steal this code, make it compile, and test it as needed.

Comment 6

[Maksim Lebedev]

Attachment: touch_cancel_ver2.diff

The most part taken from "Patch outline".
+ Changes: resolved some compilation issues.

Comment 7

[Maksim Lebedev]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=57df979f5720

Comment 8

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

Comment on attachment 8582289 [details] [diff] [review]
touch_cancel_ver2.diff

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

Please do a try build that builds across all platforms. As-is this patch will break the build on B2G.

::: gfx/layers/apz/util/APZEventState.cpp
@@ +285,5 @@
>    default:
>      NS_WARNING("Unknown touch event type");
>    }
> +
> +  if (sentContentResponse && aApzResponse == nsEventStatus_eConsumeDoDefault) {

Add a check for pointer events being enabled here. You can add the pref to gfxPrefs.h to make it easy.

@@ +294,5 @@
> +      if (mozilla::dom::Touch* touch = cancelEvent.touches[i]) {
> +        touch->convertToPointer = true;
> +      }
> +    }
> +    cancelEvent.widget->DispatchEvent(&cancelEvent, aApzResponse);

Don't use aApzResponse here; use a new nsEventStatus local variable

Comment 9

[Maksim Lebedev]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> > +  if (sentContentResponse && aApzResponse == nsEventStatus_eConsumeDoDefault) {
> Add a check for pointer events being enabled here.
I don't understand why it is needed in this place? 
I think that NS_TOUCH_CANCEL we should dispatch in all cases.

Comment 10

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

If we do that it will break all sorts of things in FirefoxOS. Actually thinking about it more we really only want to send a pointercancel here and not a touch cancel, for backwards compatibility. There are plenty of websites that rely on getting the full stream of touch events. As previously discussed [1] we do want to send a pointercancel though.

[1] http://lists.w3.org/Archives/Public/public-touchevents/2014Aug/0005.html

Comment 11

[Maksim Lebedev]

Attachment: touch_cancel_ver3.diff

+ Added gfxPrefs::PointerEventsEnabled()
+ Resolved compilation issue in widget/gonk

Comment 12

[Maksim Lebedev]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=31ab5adcf5ac

Comment 13

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

Comment on attachment 8583058 [details] [diff] [review]
touch_cancel_ver3.diff

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

::: widget/gonk/nsWindow.cpp
@@ +320,5 @@
>      // Dispatch the event into the gecko root process for "normal" flow.
> +    // The event might get sent to a child process,
> +    // but if it doesn't we need to notify the APZ of various things.
> +    // All of that happens in ProcessUntransformedAPZEvent
> +    ProcessUntransformedAPZEvent(&event, aGuid, aInputBlockId, nsEventStatus_eIgnore);

This is wrong, you need to pass in the status that is returned from the mAPZC->ReceiveInputEvent call in DispatchTouchInputViaAPZ

Comment 14

[Maksim Lebedev]

Attachment: touch_cancel_ver4.diff

+ Added ApzcResponse into DispatchTouchEventForAPZ (result is got from mAPZC->ReceiveInputEvent)

Comment 15

[Maksim Lebedev]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bdebef26b25e

Comment 16

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

Comment on attachment 8583097 [details] [diff] [review]
touch_cancel_ver4.diff

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

::: dom/ipc/TabChild.cpp
@@ +2391,5 @@
>      UpdateTapState(localEvent, status);
>      return true;
>    }
>  
> +  mAPZEventState->ProcessTouchEvent(localEvent, aGuid, aInputBlockId, status);

Sorry, I missed this earlier. The status at this call site should be nsEventStatus_eIgnore, because |status| is coming from a dispatch to content rather than the APZ. I can change this when I land the patch, no need to upload a new version and do another try push.

Comment 17

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1e01692260c

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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