920036 - Route all events through the APZCTreeManager on B2G

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Gonk, defect)

Description

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

+++ 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.

Comment 1

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

Attachment: (WIP) Part 1 - Update method signature

Comment 2

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

Attachment: (WIP) Part 2 - Rip out capture fast-path

Comment 3

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

Attachment: (WIP) Part 3 - Remove the two-parameter ReceiveInputEvent function now that it is redundant

Comment 4

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

Attachment: (WIP) Part 4 - Move the APZC event routing to widget code

Comment 5

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

Attachment: (WIP) Part 1 - Update method signature

Whoops, uploaded the wrong patch there.

Comment 6

[Jim Mathies [:jimm]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 7

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

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.

Comment 8

[Jim Mathies [:jimm]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 9

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

(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.

Comment 10

[Jim Mathies [:jimm]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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?

Comment 11

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

(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 12

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

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.

Comment 13

[Mason Chang [Inactive] [:mchang]]

Note to self, look at patch 4 mostly.

Comment 14

[Mason Chang [Inactive] [:mchang]]

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.

Comment 15

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

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.

Comment 16

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

Attachment: Part 1 (WIP) - Introduce InputEventContext

Comment 17

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

Attachment: Part 2 (WIP) - Add a layersId -> TabParent mapping

Comment 18

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

Attachment: Part 3 (WIP) - Split SendRealTouchEvent into two pieces

Comment 19

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

Attachment: Part 4 (WIP) - Move ToWidgetMouseEvent into MultiTouchInput

Comment 20

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

Attachment: Part 5 (WIP) - Reroute input events through APZ in widget

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 21

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

Comment on attachment 8536252 [details] [diff] [review]
Part 1 (WIP) - Introduce InputEventContext

A variant of this patch landed as part of bug 1109985.

Comment 22

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

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.

Comment 23

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

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 24

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

Attachment: Part 1 - Move ToWidgetMouseEvent into MultiTouchInput

Comment 25

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

Attachment: Part 2 - Send touch inputs through APZ before sending them through gecko

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

Comment 26

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

Attachment: Part 3 (WIP) - Direct-dispatch events to child process (optimization)

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 27

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

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 28

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

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 29

[David Anderson [:dvander] - inactive, e-mail if emergency]

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 30

[Michael Wu [:mwu]]

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?

Comment 31

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

(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.

Comment 32

[Michael Wu [:mwu]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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 33

[Michael Wu [:mwu]]

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 34

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

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.

Comment 35

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

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

Comment 36

[Carsten Book [:Tomcat]]

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

Comment 37

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

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

Comment 38

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

Re-landed on inbound along with a proper fix for bug 1121033.

https://hg.mozilla.org/integration/mozilla-inbound/rev/380b784bf8ec

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/380b784bf8ec

Comment 40

[Eitan Isaacson [:eeejay]]

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.

Comment 41

[Botond Ballo [:botond]]

(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.