Closed
Bug 1143655
Opened 9 years ago
Closed 9 years ago
Add TOUCH_CANCEL sending for pointer events
Categories
(Core :: Widget: Win32, defect)
Core
Widget: Win32
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: alessarik, Assigned: alessarik)
References
Details
Attachments
(2 files, 3 obsolete files)
6.48 KB,
patch
|
Details | Diff | Splinter Review | |
16.43 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•9 years ago
|
||
Attachment #8578017 -
Flags: feedback?(bugs)
Attachment #8578017 -
Flags: feedback?(bugmail.mozilla)
Updated•9 years ago
|
Component: Untriaged → Widget: Win32
Product: Firefox → Core
Comment 2•9 years ago
|
||
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•9 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).
Comment 4•9 years ago
|
||
(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.
Comment 5•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=57df979f5720
Comment 8•9 years ago
|
||
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•9 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.
Comment 10•9 years ago
|
||
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•9 years ago
|
||
+ Added gfxPrefs::PointerEventsEnabled() + Resolved compilation issue in widget/gonk
Attachment #8582289 -
Attachment is obsolete: true
Attachment #8583058 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 12•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=31ab5adcf5ac
Comment 13•9 years ago
|
||
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•9 years ago
|
||
+ 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•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bdebef26b25e
Comment 16•9 years ago
|
||
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+
Updated•9 years ago
|
Assignee: nobody → alessarik
Updated•9 years ago
|
OS: Windows 8 → All
Hardware: x86_64 → All
Summary: Add TOUCH_CANCEL sending in Windows widget → Add TOUCH_CANCEL sending for pointer events
Comment 17•9 years ago
|
||
landing |
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1e01692260c
Comment 18•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f1e01692260c
Status: UNCONFIRMED → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Updated•9 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•