Closed
Bug 959242
Opened 11 years ago
Closed 11 years ago
Make mozbrowser's sendTouchEvent work with APZC enabled iframes
Categories
(Core :: Panning and Zooming, defect)
Core
Panning and Zooming
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: etienne, Assigned: vingtetun)
References
Details
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
fabrice
:
review+
smaug
:
review+
kats
:
feedback+
|
Details | Diff | Splinter Review |
No description provided.
Comment 1•11 years ago
|
||
Can you provide some explanation as to what this bug is about?
Reporter | ||
Comment 2•11 years ago
|
||
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•11 years ago
|
||
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.
Reporter | ||
Comment 4•11 years ago
|
||
(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.
Comment 5•11 years ago
|
||
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.
Reporter | ||
Comment 6•11 years ago
|
||
(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.
Assignee | ||
Comment 7•11 years ago
|
||
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•11 years ago
|
||
(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.
Reporter | ||
Comment 9•11 years ago
|
||
(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.
Assignee | ||
Comment 10•11 years ago
|
||
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)
Assignee | ||
Comment 11•11 years ago
|
||
The Gaia part. It also fix touch events that where dispatched crazily.
Assignee | ||
Comment 12•11 years ago
|
||
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•11 years ago
|
||
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+
Assignee | ||
Comment 14•11 years ago
|
||
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
Assignee | ||
Updated•11 years ago
|
Attachment #8363713 -
Flags: review?(fabrice)
Attachment #8363713 -
Flags: review?(bugmail.mozilla)
Updated•11 years ago
|
Attachment #8363713 -
Flags: review?(bugs)
Comment 15•11 years ago
|
||
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+
Assignee | ||
Comment 16•11 years ago
|
||
(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 17•11 years ago
|
||
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+
Comment 18•11 years ago
|
||
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.
Assignee | ||
Comment 19•11 years ago
|
||
(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.
Updated•11 years ago
|
Attachment #8363713 -
Flags: review?(fabrice) → review+
Assignee | ||
Comment 20•11 years ago
|
||
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
Assignee | ||
Comment 21•11 years ago
|
||
OS: Mac OS X → All
Hardware: x86 → All
Comment 22•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 23•11 years ago
|
||
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+
Comment 24•11 years ago
|
||
status-b2g-v1.3:
--- → fixed
status-b2g-v1.4:
--- → fixed
status-firefox27:
--- → wontfix
status-firefox28:
--- → wontfix
status-firefox29:
--- → fixed
Reporter | ||
Comment 25•11 years ago
|
||
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)
Assignee | ||
Comment 26•11 years ago
|
||
(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)
Comment 27•11 years ago
|
||
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.
Updated•11 years ago
|
status-b2g-v1.3T:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•