Closed Bug 959242 Opened 6 years ago Closed 6 years ago

Make mozbrowser's sendTouchEvent work with APZC enabled iframes

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
firefox27 --- wontfix
firefox28 --- wontfix
firefox29 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: etienne, Assigned: vingtetun)

References

Details

Attachments

(2 files, 1 obsolete file)

No description provided.
Blocks: 958584
Blocks: 959243
Can you provide some explanation as to what this bug is about?
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
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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
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 ?
(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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Attached patch Dirty Gecko wip (obsolete) — Splinter Review
Kats, I was thinking of something like this. The patch is dirty but the idea is to access APZC thought TabParent.
Attachment #8363138 - Flags: feedback?(bugmail.mozilla)
The Gaia part. It also fix touch events that where dispatched crazily.
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 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.
Attachment #8363138 - Flags: feedback?(bugmail.mozilla) → feedback+
Not sure who is a good reviewer for that. Kats? Fabrice? Feel free to redirect if you feel uncomfortable reviewing those.
Attachment #8363138 - Attachment is obsolete: true
Attachment #8363713 - Flags: review?(fabrice)
Attachment #8363713 - Flags: review?(bugmail.mozilla)
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.
Attachment #8363713 - Flags: review?(bugmail.mozilla) → feedback+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 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
Attachment #8363713 - Flags: review?(bugs) → review+
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.
(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.
Attachment #8363713 - Flags: review?(fabrice) → review+
https://tbpl.mozilla.org/?tree=Try&rev=b6c66addf8b8

I will open a separate bug to fix Gaia.
Assignee: nobody → 21
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla29
https://hg.mozilla.org/mozilla-central/rev/37eac32847dd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Per jst's comment in https://bugzilla.mozilla.org/show_bug.cgi?id=946164#c33, we think this resolves bug 946164, so +ing.
blocking-b2g: --- → 1.3+
Blocks: 946164
I think this bug had a few follow ups, needinfoing Vivien to see if anything else needs to be uplifted with it.
Flags: needinfo?(21)
(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.
blocking-b2g: 1.3+ → ---
Flags: needinfo?(21)
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.
Depends on: 973107
You need to log in before you can comment on or make changes to this bug.