Closed Bug 1155186 Opened 5 years ago Closed 5 years ago

Clean up event dispatching code in android widget

Categories

(Core :: Widget: Android, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: kats, Assigned: kats)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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
Attached patch WIP (obsolete) — Splinter Review
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?
Attachment #8593365 - Flags: feedback?(nchen)
Attachment #8593365 - Flags: feedback?(danilo.eu)
It does help, but clicking positioning looks to be misbehaving if there's any zoom factor applied, I'm checking it.
Attached patch Fix for keyboard wonkiness (obsolete) — Splinter Review
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.
(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.
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
Attachment #8593365 - Attachment is obsolete: true
Attachment #8593434 - Attachment is obsolete: true
Attachment #8593365 - Flags: feedback?(nchen)
Attachment #8593365 - Flags: feedback?(danilo.eu)
Attachment #8593515 - Flags: review?(snorp)
Attachment #8593515 - Flags: review?(nchen)
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.
Assignee: nobody → bugmail.mozilla
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.
Attachment #8593515 - Flags: review?(nchen) → feedback+
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 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.
Attachment #8593515 - Flags: review?(snorp) → review+
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
Flags: needinfo?(nchen)
(Alternatively we can just get QA to hammer on IME stuff after I land this).
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).
Attachment #8593515 - Flags: review+
Flags: needinfo?(nchen)
https://hg.mozilla.org/mozilla-central/rev/be0b67543856
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.