+++ This bug was initially created as a clone of Bug #912657 +++ As part of the dynamic toolbar implementation for B2G, bug 835152 adds a scrollable <div> grouping the toolbar and the browser iframe. To get the desired behaviour, we need overscroll hand-off (bug 898478) to work between the iframe and the <div>. This means we need the <div> to have an APZC (bug 912657) and have input events wired up to be delivered to that APZC (this bug). Currently the input events on the content process area in B2G go like so: gonk/nsWindow -> gecko -> hit testing -> TabParent -> APZC -> child process Input events on the root process area in B2G go through the first steps in that list, but then get sent directly to the frame/content element that the hit test finds. What I would like to do is change the flow to this instead: gonk/nsWindow -> APZC -> gecko -> hit testing -> child process This will allow input events on the root process area to also go to the APZC so that we can do async panning for things in that process. Note that with dzbarsky's work to move the browser into a child process, this may not actually be needed. The current set up will work fine for dynamic toolbar in the browser, if we move the browser into a separate process and turn on APZC in that process. However I think that for the long term it still makes sense to do this as it will result in a simpler overall flow that is consistent for both the root and child processes, and be more in line with what Fennec and Metro will be doing. Jim has already started the work to line up the Metro input events to the new model as part of bug 915213.

Whoops, uploaded the wrong patch there.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3) > Created attachment 809408 [details] [diff] [review] > (WIP) Part 3 - Remove the two-parameter ReceiveInputEvent function now that > it is redundant This great, although we'll have to come up with a better way to split off those mouse events based on widget backend. Here's the one param method in my landing - https://hg.mozilla.org/integration/mozilla-inbound/rev/401f078bbfe8 (I didn't include the special case since only metro was calling this.) What we can do here I think is something along the lines of: #ifdef XP_WIN default: if (XRE_GetWindowsEnvironment() == WindowsEnvironmentType_Metro) { ProcessEvent() } else { if (MouseEvent) ProcessMouseEvent() else ProcessEvent() } #else if (MouseEvent) ProcessMouseEvent() else ProcessEvent() #endif I can put together a patch for this on top of this work if you like.

That seems pretty complicated. I think a better approach would be to update the ReceiveInputEvent(InputData&) method to also untransform the passed-in InputData object, similar to your one-param version. And then instead of passing in an nsMouseEvent that gets converted to a MultiTouchInput, we can modify the B2G code to do the conversion at the call site and pass in a MultiTouchInput directly. I can work this into my current patchset; I'll pull and rebase on top of your changes now that they've hit central and throw in a patch that does this as well.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #7) > That seems pretty complicated. I think a better approach would be to update > the ReceiveInputEvent(InputData&) method to also untransform the passed-in > InputData object, similar to your one-param version. And then instead of > passing in an nsMouseEvent that gets converted to a MultiTouchInput, we can > modify the B2G code to do the conversion at the call site and pass in a > MultiTouchInput directly. Sounds good to me. Just to confirm though we'll keep ReceiveInputEvent(nsInputEvent& aEvent) around yes? I'm currently working with this in handling wheel and content command events sent from MetroWidget.

(In reply to Jim Mathies [:jimm] from comment #8) > Sounds good to me. Just to confirm though we'll keep > ReceiveInputEvent(nsInputEvent& aEvent) around yes? I'm currently working > with this in handling wheel and content command events sent from MetroWidget. For now (and maybe forever), yes. I would like to move all input going into APZC off the gecko thread as much as I can, which would mean transitioning to the InputData version of the function but that's a much longer-term change. Even if we do that we'll probably keep the nsInputEvent& version since it doesn't hurt to have it.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #9) > (In reply to Jim Mathies [:jimm] from comment #8) > > Sounds good to me. Just to confirm though we'll keep > > ReceiveInputEvent(nsInputEvent& aEvent) around yes? I'm currently working > > with this in handling wheel and content command events sent from MetroWidget. > > For now (and maybe forever), yes. I would like to move all input going into > APZC off the gecko thread as much as I can, which would mean transitioning > to the InputData version of the function but that's a much longer-term > change. Even if we do that we'll probably keep the nsInputEvent& version > since it doesn't hurt to have it. I can see how more generic storage would be useful particularly with the state WAITING_LISTENERS. For example wheel input has the same requirement that touch does in that content can cancel default actions. To address this I think mTouchQueue needs to become more generic and store InputData vs. MultiTouchInput. Sound reasonable?

(Just keep in mind that we should try to move to use pointer events, where touch-action let's web page to opt-in to get the events, and by default events don't need to be dispatched to DOM. Hopefully touch events will die at some point.)

Comment on attachment 809321 [details] [diff] [review] (WIP) Part 2 - Rip out capture fast-path Review of attachment 809321 [details] [diff] [review]: ----------------------------------------------------------------- Apparently this patch breaks stuff, even though it's supposed to be entirely a removable optimization. Dammit.

Note to self, look at patch 4 mostly.

Since all the work I've been doing is in bug 930939, and there we route all touch events through APZCTreeManager, marking this as a dupe.

Un-duping as I believe this will need to be fixed before we can turn on parent-process APZ, which in turn will be needed before we can move input events off the main thread.

These patches mostly do the job. However there are issues with the event regions either not being respected or not being computed properly. If you bring down the notification tray, for example, the input events will still go through it to the content underneath. In this case I think the problem is the touch-event regions are not being respected properly. There's also the issue where the touch listeners in gaia/apps/system/js/edge_swipe_detector.js don't seem to generate touch-event regions in the layer tree, meaning that the edge swipe stuff doesn't work. :dvander convinced me that conceptually it should work if we iron out all the bugs.

Comment on attachment 8536252 [details] [diff] [review] Part 1 (WIP) - Introduce InputEventContext A variant of this patch landed as part of bug 1109985.

Comment on attachment 8536253 [details] [diff] [review] Part 2 (WIP) - Add a layersId -> TabParent mapping A variant of this landed as part of bug 1111873.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #20) > If you > bring down the notification tray, for example, the input events will still > go through it to the content underneath. In this case I think the problem is > the touch-event regions are not being respected properly. This is a bug in the APZC code, which attachment 8540882 [details] [diff] [review] fixes.

Not sure who all needs to review this so flagging everybody who might be interested. Feel free to unflag yourself.

This code is #if 0 because it doesn't work yet. Conceptually it should, I think, but I need to iron out other bugs first. Just stashing it as a WIP here for now.

Comment on attachment 8542239 [details] [diff] [review] Part 3 (WIP) - Direct-dispatch events to child process (optimization) Just making it clear that this part 3 is meant to be an optimization and we should be fine without it as well. Landing parts 1 and 2 is dependent on turning on event-regions on B2G (bug 922833) as well as some APZ fixes (bug 1109873 and bug 1115111).

Comment on attachment 8542239 [details] [diff] [review] Part 3 (WIP) - Direct-dispatch events to child process (optimization) Moving this patch to bug 1116579 since there's other pieces that are blocking it, and we don't need it to complete this bug.

Comment on attachment 8542238 [details] [diff] [review] Part 2 - Send touch inputs through APZ before sending them through gecko Review of attachment 8542238 [details] [diff] [review]: ----------------------------------------------------------------- lgtm

Comment on attachment 8542238 [details] [diff] [review] Part 2 - Send touch inputs through APZ before sending them through gecko Review of attachment 8542238 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/gonk/nsWindow.cpp @@ +259,5 @@ > + > +void > +nsWindow::DispatchTouchInputViaAPZ(MultiTouchInput& aInput) > +{ > + if (!mAPZC) { Hmmm... is this allowed to be null? If not, should be an assert. If it can be, it seems like it can do better than dropping the event. Or maybe this is just in case we receive events while the window is initializing?

(In reply to Michael Wu [:mwu] from comment #30) > Hmmm... is this allowed to be null? If not, should be an assert. If it can > be, it seems like it can do better than dropping the event. Or maybe this is > just in case we receive events while the window is initializing? In general it shouldn't be null, but you're right, it might be null during initial setup (before the compositor is up). Do we care about that case? I can add a comment there if that helps.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #31) > (In reply to Michael Wu [:mwu] from comment #30) > > Hmmm... is this allowed to be null? If not, should be an assert. If it can > > be, it seems like it can do better than dropping the event. Or maybe this is > > just in case we receive events while the window is initializing? > > In general it shouldn't be null, but you're right, it might be null during > initial setup (before the compositor is up). Do we care about that case? I > can add a comment there if that helps. Sure, a comment would be fine.

Comment on attachment 8542238 [details] [diff] [review] Part 2 - Send touch inputs through APZ before sending them through gecko Review of attachment 8542238 [details] [diff] [review]: ----------------------------------------------------------------- r=me for the widget/gonk parts.

Comment on attachment 8542238 [details] [diff] [review] Part 2 - Send touch inputs through APZ before sending them through gecko >- nsEventStatus status; >- gFocusedWindow->DispatchEvent(&aEvent, status); >- return status; >+ // If it didn't get captured, dispatch the event into the gecko root process >+ // for "normal" flow. The event might get sent to the child process still, >+ // but if it doesn't we need to notify the APZ of various things. All of >+ // that happens in DispatchEventForAPZ >+ rv = DispatchEventForAPZ(&event, guid, inputBlockId); Odd name for the method. It is DispatchEvent + something else... but ok, that is what we have already.

Try looks pretty green at https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=535ba77e5bb9 Landed on b2g-inbound: remote: https://hg.mozilla.org/integration/b2g-inbound/rev/22f8aee2e0e0 remote: https://hg.mozilla.org/integration/b2g-inbound/rev/8cae7aeb3a74

https://hg.mozilla.org/mozilla-central/rev/22f8aee2e0e0 https://hg.mozilla.org/mozilla-central/rev/8cae7aeb3a74

Bug 1119497 caused the smoketest regression in bug 1121033. This bug depended on bug 1119497. So I backed out cset 8cae7aeb3a74 from this bug along with bug 1119497 to fix the regression. https://hg.mozilla.org/integration/b2g-inbound/rev/86bcc35413b4

Re-landed on inbound along with a proper fix for bug 1121033. https://hg.mozilla.org/integration/mozilla-inbound/rev/380b784bf8ec

Looks like the last patch on this bug breaks the screen reader. We do preventDefault() from the top level window with the intention of capturing and redirecting all touch input. Unfortunately, after this patch landed, child remote iframes get the events nonetheless. Don't know enough about this code to know why this is.

(In reply to Eitan Isaacson [:eeejay] from comment #40) > Looks like the last patch on this bug breaks the screen reader. > We do preventDefault() from the top level window with the intention of > capturing and redirecting all touch input. Unfortunately, after this patch > landed, child remote iframes get the events nonetheless. Don't know enough > about this code to know why this is. Let's take the discussion to bug 1125422.