1155186 - Clean up event dispatching code in android widget

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

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

The Android widget code has some rather convoluted code to dispatch events. In current master, the nsWindow instances that get created are the same as what is described at [1]. In bug 681733, the hidden window stuff is removed, so we are left with three windows that get created initially, and two windows in the steady state. Note that of these two windows, the root-level window is returned by nsWindow::TopWindow(), and the child window is always the "target" or "focus" window for the various events in nsWindow::OnGlobalAndroidEvent.

In fact, as Danilo discovered while trying to implement bug 1146024, the DispatchEvent function on the root-level window is a no-op because the widget listener for that window is a nsWebShellWindow which doesn't implement the HandleEvent function.

So just dispatching all the events on the child window would be fine, except that it doesn't play well with APZ because it's the root window that has the APZCTreeManager and compositor and all that nice stuff. So really what we should do is have the root window manage all the state related to input events, but then when dispatching events, dispatch to the child window. Since we know we have only two windows, this also lets us get rid of a bunch of cruft like FindWindowForPoint which isn't really needed any more (and perhaps never was).

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=785597#c12

Comment 1

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

Attachment: WIP

This is what I had in mind, although it seems to screw up the keyboard events somehow, and when typing in text fields the letters get prepended rather than appended. Jim, do you know what I might have missed here?

Danilo, if you apply this patch, does it help with the problems you were seeing with event dispatch and clicking?

Comment 2

[Danilo Cesar Lemes de Paula]

It does help, but clicking positioning looks to be misbehaving if there's any zoom factor applied, I'm checking it.

Comment 3

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

Attachment: Fix for keyboard wonkiness

For some reason this change applied on top of the WIP fixes the typing wonkiness I was seeing. However I think this is a case of "two wrongs cancelling each other out" because this pushes back some of IME state into the child window when really I want everything to be in the root window. I must have missed some state in the first patch but for the life of me I can't figure out what.

Comment 4

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

(In reply to Danilo Cesar Lemes de Paula from comment #2)
> It does help, but clicking positioning looks to be misbehaving if there's
> any zoom factor applied, I'm checking it.

Note that I don't really expect zooming to be working properly yet. I think if panning and clicking works for now that's pretty good.

Comment 5

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

Attachment: Keep all state in the root window, and only use the child window for event dispatching

Ok, figured out what was happening - NotifyIMEInternal was getting called on the child window from nsBaseWidget, so I bumped that call over to the root window and then everything works great.

Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac33099173d4

Comment 6

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

Comment on attachment 8593515 [details] [diff] [review]
Keep all state in the root window, and only use the child window for event dispatching

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

::: widget/android/nsWindow.cpp
@@ +1124,5 @@
>  {
> +    LayoutDeviceIntPoint pt(ae->Points()[0].x,
> +                            ae->Points()[0].y);
> +    double delta = ae->X();
> +    int msg = 0;

This bit is mostly just fixing indentation and inlining the DispatchGestureEvent function. No functional change in this hunk.

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8593515 [details] [diff] [review]
Keep all state in the root window, and only use the child window for event dispatching

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

Heh I was just going to mention NotifyIMEInternal. The patch looks okay, but can you explain more why we need to keep states in the root window vs the focused window? For example, why does the APZ care about where we keep IME states? As you found out, IMEStateManager expects to be working with the focused window, and I'm worried that we will end up with more subtle bugs, where we get other calls on the focused window but need to forward them to the top window.

Comment 8

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

I guess technically we don't need to move the IME state, but I feel it makes things more consistent. My chain of reasoning was as follows:

- On Fennec, touch/mouse events need to be dispatched to the child window for them to work
- When APZ is enabled, the code that does the dispatching is shared with other platforms and lives at nsBaseWidget::ProcessUntransformedAPZEvent [1].
- ProcessUntransformedAPZEvent uses |this| (i.e. the widget it's called on) for other APZ-related things, and that widget must be the same widget that has the compositor/APZCTreeManager. Therefore, ProcessUntransformedAPZEvent must be called on the root window
- Since ProcessUntransformedAPZEvent has to be called on the root window, but DispatchEvent needs to dispatch to the child window, I modified the DispatchEvent to shunt the event to the child window. (The other alternative here would be to override ProcessUntransformedAPZEvent and modify it so that it uses one window for some things and the other window for other things, but that seems less clean.)
- Now that we are shunting events to the child window in DispatchEvent, all the code that does FindWindowForPoint is redundant and can be removed, and we can send those events directly to the root window's DispatchEvent.
- For consistency we can do the same for the key/IME events.

There are a few other alternatives we can do:
1) Keep all the events going to the child window, except for the APZ_INPUT_EVENT type.
2) Keep only key/IME events going to the child window, but move all the FindWindowForPoint events over to the root window
3) Just override ProcessUntransformedAPZEvent as mentioned above

I think that the current approach is more straightforward because it is more consistent - all the "state" is stored in the root window, and all the events are dispatched from the child window. It's not a per-event decision. It also means that going forward it will be easier to send all the events through APZ which we will want to do eventually anyway (e.g. to support keyboard-input scrolling), and it also means it's easier to get rid of the second (child) window if we want.

I agree about the subtle bugs - I just don't know how tight that relationship is of the IME working with the focused window. If it's a strong assumption baked into the code then maybe it's not worth it to do this and we can go with one of the other alternatives.

[1] http://mxr.mozilla.org/mozilla-central/source/widget/nsBaseWidget.cpp?rev=879e74f2e74c#997

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8593515 [details] [diff] [review]
Keep all state in the root window, and only use the child window for event dispatching

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

Looks ok to me, but I still don't fully understand why we need two windows in the first place. We can discuss that elsewhere, though. Jim's point about IME is interesting, though, and we could maybe eliminate a whole class of bugs if the event dispatching can go wrong like this.

Comment 10

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

(In reply to James Willcox (:snorp) ([e-mail]) from comment #9)
> Looks ok to me, but I still don't fully understand why we need two windows
> in the first place. We can discuss that elsewhere, though.

Agreed. blassey or roc might know more about this, it's been this way for a long time.

> Jim's point about
> IME is interesting, though, and we could maybe eliminate a whole class of
> bugs if the event dispatching can go wrong like this.

Would you guys prefer if I went with one of the other approaches I described above? If so let me know, otherwise I'm assuming this patch is ok to land.

Comment 11

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

(Alternatively we can just get QA to hammer on IME stuff after I land this).

Comment 12

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8593515 [details] [diff] [review]
Keep all state in the root window, and only use the child window for event dispatching

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

Okay let's go with your current patch. I think it covers all the current IME usages so shouldn't need much work from QA. Down the road, we can probably get the top window and focused window to share the same set of IME states (or just use one window like Snorp said).

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/be0b67543856

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/be0b67543856