959242 - Make mozbrowser's sendTouchEvent work with APZC enabled iframes

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Comment 1

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

Can you provide some explanation as to what this bug is about?

Comment 2

[Etienne Segonzac (:etienne)]

Sorry,

The mozbrowser iframe.sendTouchEvent API [1].
Doesn't work (at all) for iframes with setAttribute('mozasyncpanzoom', 'true').

[1] https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement.sendTouchEvent

Comment 3

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

Hm. Based on my understanding of the code that function should still partly work in that it should dispatch touch events to content. However you're right that if you send touch events trying to scroll the page or click on things that won't work. That function doesn't route events through the APZ code. What are you trying to do that needs to use this function? There's probably an alternate way to do it.

Comment 4

[Etienne Segonzac (:etienne)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> What are you trying to do that needs to use this function? There's
> probably an alternate way to do it.

For the fast app switching gesture (swiping from the edges of the screen, it can be enabled in the developer settings), when the gesture doesn't look like an horizontal swipe, we start forwarding the events to the app [1].

The goal is to be able to tap on a button / scroll content vertically even on the edges of the screen with the feature enabled.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/edge_swipe_detector.js#L174

PS: the touch events themselves are indeed properly forwarded, I was testing with scrolling.

Comment 5

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

I think maybe a better solution is to do the edge-swipe gesture detection in the APZ code. This will have better performance as well.

Comment 6

[Etienne Segonzac (:etienne)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> I think maybe a better solution is to do the edge-swipe gesture detection in
> the APZ code. This will have better performance as well.

Another use-case (maybe harder to fix in the platform):
In bug 958584 we want to listen to touch events from the sytem app (to make the statusbar appear) while forwarding _all_ of them to the app below.

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Can't we simply change the code in http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementParent.jsm#513 to route the message to this._frameLoader.tabParent when the frame is an APZC frame and expose a new method in http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsITabParent.idl which is similar to sendTouchEvent and will let the TabParent redirect the events to APZC ?

It's not the best solution as we basically kill the fast path of TryCapture but it will let the system app receive the events if it needs it while still using APZC to pan ?

Comment 8

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

(In reply to Etienne Segonzac (:etienne) from comment #6)
> Another use-case (maybe harder to fix in the platform):
> In bug 958584 we want to listen to touch events from the sytem app (to make
> the statusbar appear) while forwarding _all_ of them to the app below.

I'm not sure I understand. It sounds like you're referring to step 1 in the PDF on that bug. That is, swipe down from the top to expose the Rocketbar. Are you saying that in addition to bringing down the Rocketbar (which is presumably a gesture in the root process), you want the touch events to be delivered to the app? That seems wrong to me.

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #7)
> Can't we simply change the code in
> http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/
> BrowserElementParent.jsm#513 to route the message to
> this._frameLoader.tabParent when the frame is an APZC frame and expose a new
> method in
> http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/
> nsITabParent.idl which is similar to sendTouchEvent and will let the
> TabParent redirect the events to APZC ?

I'm not too familir with BEP.jsm so I can't comment on this but I do want to point out that we have an existing bug on file (bug 928839) for allowing Gecko code to send touch events to specific APZC instances. We will need this as part of the complete hit-testing solution (bug 928833).

> It's not the best solution as we basically kill the fast path of TryCapture
> but it will let the system app receive the events if it needs it while still
> using APZC to pan ?

This overlaps with bug 920036 as well.

Comment 9

[Etienne Segonzac (:etienne)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> (In reply to Etienne Segonzac (:etienne) from comment #6)
> > Another use-case (maybe harder to fix in the platform):
> > In bug 958584 we want to listen to touch events from the sytem app (to make
> > the statusbar appear) while forwarding _all_ of them to the app below.
> 
> I'm not sure I understand. It sounds like you're referring to step 1 in the
> PDF on that bug. That is, swipe down from the top to expose the Rocketbar.
> Are you saying that in addition to bringing down the Rocketbar (which is
> presumably a gesture in the root process), you want the touch events to be
> delivered to the app?

Yes exactly.

> That seems wrong to me.

Well FWIW that's how iOS you reveal the notification tray from a full screen in iOS too.
Don't have a strong opinion, having the events forwarded to the app only once the status bar is fully displayed would make sense to me.

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Dirty Gecko wip

Kats, I was thinking of something like this. The patch is dirty but the idea is to access APZC thought TabParent.

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: forward.event.to.apzc.gaia.patch

The Gaia part. It also fix touch events that where dispatched crazily.

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

So with those patches, once the touchstart has been forward to APZC, all the touchmove are sucked by it. That should give us nice performances.

Comment 13

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

Comment on attachment 8363138 [details] [diff] [review]
Dirty Gecko wip

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

Oh interesting, I didn't know TabParent could be accessed from script. This seems like a reasonable approach to me I think. But I would prefer renaming the "sendTouchEvent" in TabParent.* to something like "injectTouchEvent" to make it more obvious that it's coming from somewhere else.

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug959242.sendTouchEvent.to.APZC.patch

Not sure who is a good reviewer for that. Kats? Fabrice? Feel free to redirect if you feel uncomfortable reviewing those.

Comment 15

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

Comment on attachment 8363713 [details] [diff] [review]
bug959242.sendTouchEvent.to.APZC.patch

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

::: dom/ipc/TabParent.cpp
@@ +1869,5 @@
> +                            float *aForces,
> +                            uint32_t aCount,
> +                            int32_t aModifiers)
> +{
> +  int32_t msg;

Do we need a check here to make sure it's only ever called from chrome code? I'm not sure how the IDL is exposed.

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #15)
> Comment on attachment 8363713 [details] [diff] [review]
> bug959242.sendTouchEvent.to.APZC.patch
> 
> Review of attachment 8363713 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/ipc/TabParent.cpp
> @@ +1869,5 @@
> > +                            float *aForces,
> > +                            uint32_t aCount,
> > +                            int32_t aModifiers)
> > +{
> > +  int32_t msg;
> 
> Do we need a check here to make sure it's only ever called from chrome code?
> I'm not sure how the IDL is exposed.

The code is mostly based on nsIDOMWindowUtils.sendTouchEvent that has this so I won't be surprised if we need to add it. On the other side I have really no idea how content can accessed this code but it may be that I don't understand many gecko pieces.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8363713 [details] [diff] [review]
bug959242.sendTouchEvent.to.APZC.patch


>+TabParent::InjectTouchEvent(const nsAString& aType,
>+                            uint32_t *aIdentifiers,
>+                            int32_t *aXs,
>+                            int32_t *aYs,
>+                            uint32_t *aRxs,
>+                            uint32_t *aRys,
>+                            float *aRotationAngles,
>+                            float *aForces,
>+                            uint32_t aCount,
>+                            int32_t aModifiers)
* goes with the type, not with the param name.


>+{
>+  int32_t msg;
uint32_t

>+  if (aType.EqualsLiteral("touchstart")) {
>+    msg = NS_TOUCH_START;
>+  } else if (aType.EqualsLiteral("touchmove")) {
>+    msg = NS_TOUCH_MOVE;
>+  } else if (aType.EqualsLiteral("touchend")) {
>+    msg = NS_TOUCH_END;
>+  } else if (aType.EqualsLiteral("touchcancel")) {
>+    msg = NS_TOUCH_CANCEL;
>+  }

You could use nsContentUtils::GetEventIdAndAtom here
nsContentUtils::GetEventIdAndAtom(aType, NS_TOUCH_EVENT, &msg);
and then perhaps
if (msg != NS_TOUCH_START && msg != ...) {
  return NS_ERROR_FAILURE;
}

>+TabParent::GetUseAsyncPanZoom(bool* useAsyncPanZoom) {
{ goes to the next line

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

We don't have classinfo or webidl bindings for nsITabParent so content can't access it.
And since content can't access frameloader either, accessing its tabParent attribute isn't possible.

Comment 19

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Olli Pettay [:smaug] from comment #17)
> Comment on attachment 8363713 [details] [diff] [review]
> bug959242.sendTouchEvent.to.APZC.patch
> 
> 
> >+TabParent::InjectTouchEvent(const nsAString& aType,
> >+                            uint32_t *aIdentifiers,
> >+                            int32_t *aXs,
> >+                            int32_t *aYs,
> >+                            uint32_t *aRxs,
> >+                            uint32_t *aRys,
> >+                            float *aRotationAngles,
> >+                            float *aForces,
> >+                            uint32_t aCount,
> >+                            int32_t aModifiers)
> * goes with the type, not with the param name.
> 
> 
Sorry about that. You already told that to me. I keep beeing inspired by old code.

Comment 20

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://tbpl.mozilla.org/?tree=Try&rev=b6c66addf8b8

I will open a separate bug to fix Gaia.

Comment 21

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://hg.mozilla.org/integration/b2g-inbound/rev/37eac32847dd

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/37eac32847dd

Comment 23

[Jason Smith [:jsmith]]

Per jst's comment in https://bugzilla.mozilla.org/show_bug.cgi?id=946164#c33, we think this resolves bug 946164, so +ing.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/9aa94277b353

Comment 25

[Etienne Segonzac (:etienne)]

I think this bug had a few follow ups, needinfoing Vivien to see if anything else needs to be uplifted with it.

Comment 26

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Jason Smith [:jsmith] from comment #23)
> Per jst's comment in
> https://bugzilla.mozilla.org/show_bug.cgi?id=946164#c33, we think this
> resolves bug 946164, so +ing.

Reading the STR in bug 946164 I will be surprised. This patch is really used only when someone is clicking on the side edges of the device. And the only code path I know that will trigger those methods are when you have Edge Gesture enabled. I suggest that you block on bug 967236 instead.

Comment 27

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

FWIW this has already been uplifted to 1.3 so I think we should uplift bug 964261 to go with it. I got that one flagged as 1.3+ as well so it gets uplifted.