Add TOUCH_CANCEL sending for pointer events

RESOLVED FIXED in Firefox 39

Status

()

Core
Widget: Win32
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Maksim Lebedev, Assigned: Maksim Lebedev)

Tracking

Trunk
mozilla39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Updated

3 years ago
Depends on: 736048
(Assignee)

Comment 1

3 years ago
Created attachment 8578017 [details] [diff] [review]
touch_cancel_ver1.diff
Attachment #8578017 - Flags: feedback?(bugs)
Attachment #8578017 - Flags: feedback?(bugmail.mozilla)
Component: Untriaged → Widget: Win32
Product: Firefox → Core
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
Attachment #8578017 - Flags: feedback?(bugs)
Attachment #8578017 - Flags: feedback?(bugmail.mozilla)
(Assignee)

Comment 3

3 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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).
(In reply to Maksim Lebedev from comment #3)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Created attachment 8580043 [details] [diff] [review]
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.
(Assignee)

Comment 6

3 years ago
Created attachment 8582289 [details] [diff] [review]
touch_cancel_ver2.diff

The most part taken from "Patch outline".
+ Changes: resolved some compilation issues.
Attachment #8578017 - Attachment is obsolete: true
Attachment #8582289 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 7

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=57df979f5720
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
Attachment #8582289 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 9

3 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
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
(Assignee)

Comment 11

3 years ago
Created attachment 8583058 [details] [diff] [review]
touch_cancel_ver3.diff

+ Added gfxPrefs::PointerEventsEnabled()
+ Resolved compilation issue in widget/gonk
Attachment #8582289 - Attachment is obsolete: true
Attachment #8583058 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 12

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=31ab5adcf5ac
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
Attachment #8583058 - Flags: review?(bugmail.mozilla) → review-
(Assignee)

Comment 14

3 years ago
Created attachment 8583097 [details] [diff] [review]
touch_cancel_ver4.diff

+ Added ApzcResponse into DispatchTouchEventForAPZ (result is got from mAPZC->ReceiveInputEvent)
Attachment #8583058 - Attachment is obsolete: true
Attachment #8583097 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 15

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bdebef26b25e
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.
Attachment #8583097 - Flags: review?(bugmail.mozilla) → review+
Assignee: nobody → alessarik
OS: Windows 8 → All
Hardware: x86_64 → All
Summary: Add TOUCH_CANCEL sending in Windows widget → Add TOUCH_CANCEL sending for pointer events
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1e01692260c
https://hg.mozilla.org/mozilla-central/rev/f1e01692260c
Status: UNCONFIRMED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
(Assignee)

Updated

3 years ago
Blocks: 1162009
No longer blocks: 1162009
Depends on: 1162009
(Assignee)

Updated

3 years ago
Blocks: 1122211
Blocks: 1251915
You need to log in before you can comment on or make changes to this bug.