Closed Bug 1162009 Opened 9 years ago Closed 9 years ago

POINTER_CANCEL does not work in e10s

Categories

(Core :: Panning and Zooming, defect)

Unspecified
Windows
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
e10s + ---
firefox39 --- wontfix
firefox40 --- affected
firefox41 --- fixed

People

(Reporter: alessarik, Assigned: alessarik)

References

Details

Attachments

(1 file, 2 obsolete files)

POINTER_CANCEL event was not sended into content.
Some investigation:
> APZEventState::ProcessTouchEvent(..., nsEventStatus aApzResponse)
> ...
>   if (... && aApzResponse == nsEventStatus_eConsumeDoDefault && ...) {
>     WidgetTouchEvent cancelEvent(aEvent);
>     cancelEvent.message = NS_TOUCH_CANCEL;
> ...
>     cancelEvent.widget->DispatchEvent(&cancelEvent, status);
>   }
> }
And
> TabChild::RecvRealTouchEvent()
> {
> ...
>   mAPZEventState->ProcessTouchEvent(localEvent, aGuid, aInputBlockId, nsEventStatus_eIgnore);
> }
As result NS_TOUCH_CANCEL cannot be sended.
Blocks: 822898, 1122211
Depends on: 1154739, 1143655
Product: Firefox → Core
Look's like, obvious place for solution is
> TabChild::RecvRealTouchEvent()
> {
> ...
>   nsEventStatus status = APZCCallbackHelper::DispatchWidgetEvent(localEvent);
> ...
>   mAPZEventState->ProcessTouchEvent(localEvent, aGuid, aInputBlockId, status); // Using status
> ...
> }
But, unfortunately it does not help.
Flags: needinfo?(bugmail.mozilla)
Comment 1 makes no sense to me. It's not at all obvious to me why this is happening and what bug 1154739 has to do with it. Either you are getting POINTER_CANCEL with e10s and APZ enabled or you are not. Either way, the same should have been happening before 1154739, it's just that APZ was taking effect in non-e10s windows as well.
Flags: needinfo?(bugmail.mozilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3)
> Comment 1 makes no sense to me. It's not at all obvious to me why this is
> happening and what bug 1154739 has to do with it. Either you are getting
> POINTER_CANCEL with e10s and APZ enabled or you are not. Either way, the
> same should have been happening before 1154739, it's just that APZ was
> taking effect in non-e10s windows as well.
Before bug 1154739 we tested at non-e10s window, and all worked correctly.
After bug 1154739 the most part of pointer events tests have became failed.
POINTER_CANCEL event stoped dispatching and comment 1 shows why it happens.
Maybe bug 1154739 is not once reason, but looks like, it is first reason.
Component: General → Panning and Zooming
Summary: POINTER_CANCEL was not sended at e10s → POINTER_CANCEL does not work at e10s
(In reply to Maksim Lebedev from comment #4)
> Before bug 1154739 we tested at non-e10s window, and all worked correctly.

Were you testing with APZ enabled?
Ok, I understand what's going on now. It only works when APZ is enabled, and previously it worked in non-e10s code paths and didn't work in e10s. Since APZ is now only enabled in e10s codepaths, there's no working code path.
Blocks: 1143655
No longer depends on: 1143655, 1154739
Summary: POINTER_CANCEL does not work at e10s → POINTER_CANCEL does not work in e10s
Assignee: nobody → alessarik
tracking-e10s: --- → +
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #7)
> To fix this you need to add aApzResponse to the InputAPZContext at [1], pull it out 
> of the context at [2], send it over IPC at [3], and then use it in the call at [4].
Thanks, it helps a lot.
Attached patch touch_cancel_e10s_ver1.diff (obsolete) — Splinter Review
+ Add using correct response from APZ to send TOUCH_CANCEL event on e10s
Attachment #8603229 - Flags: review?(bugmail.mozilla)
Comment on attachment 8603229 [details] [diff] [review]
touch_cancel_e10s_ver1.diff

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

This looks pretty good but please use nsEventStatus as the type all the way through instead of int32_t.
Attachment #8603229 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #11)
> This looks pretty good but please use nsEventStatus as the type all the way
> through instead of int32_t.
I don't know IPDL very well, but looks like, it cannot use nsEventStatus. I got issues in this case:
> ...\ipc\chromium\src\chrome/common/ipc_message_utils.h(121) : error C2039: 'Read' : 
> is not a member of 'IPC::ParamTraits<P>'
> with
> [
>     P=nsEventStatus
> ]
> ...\obj-i686-pc-mingw32\ipc\ipdl\_ipdlheaders\mozilla/dom/PBrowserChild.h(1170)
> : see reference to function template instantiation 'bool IPC::ReadParam<T>(const IPC::Message *,
> void **,P *)' being  compiled with
> [
>     T=nsEventStatus
>     P=nsEventStatus
> ]
> ...\obj-i686-pc-mingw32\ipc\ipdl\PBrowserChild.cpp(3146) : see reference to function template
> instantiation 'bool mozilla::dom::PBrowserChild::Read<nsEventStatus>(T *,
> const mozilla::dom::PBrowser Child::Message *,void **)' being compiled with
> [
>     T=nsEventStatus
> ]
Flags: needinfo?(bugmail.mozilla)
You need to add support for serializing nsEventStatus using ContiguousEnumSerializer. See how it's done for other enumerations here [1]. (You may need to add a sentinel value to nsEventStatus.)

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/ipc/GfxMessageUtils.h?rev=f3bc38042efe#193
Flags: needinfo?(bugmail.mozilla)
(In reply to Botond Ballo [:botond] from comment #13)
> See how it's done for other enumerations here [1].
Thanks, it helped.
Attached patch touch_cancel_e10s_ver2.diff (obsolete) — Splinter Review
+ Added SENTINEL_VALUE into enum nsEventStatus
+ Added ContiguousEnumSerializer for nsEventStatus
+ (Send/Recv)RealTouch(Move)Event uint32_t aApzResponse -> nsEventStatus

Suggestions and comments and objections are very welcome.
Attachment #8603229 - Attachment is obsolete: true
Attachment #8605117 - Flags: review?(bugmail.mozilla)
Attachment #8605117 - Flags: feedback?(botond)
Tested on win8 opt. Pointercancel tests (https://github.com/w3c/web-platform-tests/tree/master/pointerevents) all passes
Comment on attachment 8605117 [details] [diff] [review]
touch_cancel_e10s_ver2.diff

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

::: widget/EventForwards.h
@@ +24,5 @@
>    nsEventStatus_eConsumeNoDefault,
>    // The event is consumed, but do default processing
> +  nsEventStatus_eConsumeDoDefault,
> +  // Value is not for use, only for serialization
> +  SENTINEL_VALUE

Please name it nsEventStatus_eSentinel, for consistency with the other names (also, prefixing with nsEventStatus_ is important because this isn't an enum class, so otherwise we'd be adding a name like SENTINEL_VALUE to the global namespace).
Attachment #8605117 - Flags: feedback?(botond) → feedback+
Comment on attachment 8605117 [details] [diff] [review]
touch_cancel_e10s_ver2.diff

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

Just a couple of minor comment changes, and what botond said.

::: dom/ipc/TabParent.h
@@ +502,5 @@
>      // null.
>      // |aOutInputBlockId| will contain the identifier of the input block
>      // that this event was added to, if there was one. aOutInputBlockId may
>      // be null.
>      void ApzAwareEventRoutingToChild(ScrollableLayerGuid* aOutTargetGuid,

Update the comment to also say:

// |aOutApzResponse| will contain the response that the APZ gave
// when processing the input block; this is used for generating
// appropriate pointercancel events.

::: gfx/layers/apz/util/InputAPZContext.h
@@ +12,5 @@
>  namespace mozilla {
>  namespace layers {
>  
> +// InputAPZContext is used to communicate the ScrollableLayerGuid and
> +// input block ID and ApzResponse from nsIWidget to RenderFrameParent.

"...ScrollableLayerGuid, input block ID, and APZ response from nsIWidget...."
Attachment #8605117 - Flags: review?(bugmail.mozilla) → review+
+ nsEventStatus::SENTINEL_VALUE -> nsEventStatus::nsEventStatus_eSentinel
+ Changed some comments
Attachment #8605117 - Attachment is obsolete: true
Attachment #8605865 - Flags: review?(bugmail.mozilla)
Attachment #8605865 - Flags: feedback?(botond)
Comment on attachment 8605865 [details] [diff] [review]
touch_cancel_e10s_ver3.diff

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

Thanks!
Attachment #8605865 - Flags: feedback?(botond) → feedback+
Comment on attachment 8605865 [details] [diff] [review]
touch_cancel_e10s_ver3.diff

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

::: dom/ipc/TabParent.h
@@ +511,2 @@
>      bool mMarkedDestroying;
> +    // When true, the TabParent is invalid and we should not send IPC messages anymore.

Try to avoid changing stuff not related to your changes.
Attachment #8605865 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta from comment #23)
> Try to avoid changing stuff not related to your changes.
It was very nearest code.
Should I change patch again?
Flags: needinfo?(bugmail.mozilla)
No, don't bother
Flags: needinfo?(bugmail.mozilla)
If there are no objections, I put checkin-needed flag.
Status: NEW → ASSIGNED
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8c5b4478608d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.