Comment 1 Andrew Comminos [:acomminos] 2015-06-01 15:25:44 PDT

Created attachment 8613737 [details] [diff] [review]
Use window state signal to set focus on GTK3.

This patch works around the hanging issue by using focus updates from window state updates on GTK3 (GTK2 does not provide focus updates in the window state signal).

Thanks!

Comment 2 Andrew Comminos [:acomminos] 2015-06-01 17:04:21 PDT

Comment on attachment 8613737 [details] [diff] [review]
Use window state signal to set focus on GTK3.

Some early testing has revealed this causes other bustage on try- going to look into it first.

Comment 3 Andrew Comminos [:acomminos] 2015-06-02 10:10:12 PDT

Unfortunately, listening for focus with GDK_WINDOW_STATE_FOCUS doesn't work on WMs that don't support _NET_WM_STATE_FOCUSED (including the version of compiz on the tryserver)- we'll have to look for another workaround to get focus to work consistently.

Comment 4 Andrew Comminos [:acomminos] 2015-06-02 14:11:49 PDT

Created attachment 8614299 [details] [diff] [review]
Implement custom handling for XInput2 events.

This patch works around the issues with GDK's XInput2 event handler by translating XI2 GDK events ourselves. It fixes the issues with focus on try (https://treeherder.mozilla.org/#/jobs?repo=try&revision=0feb1d11d25b, note mochitest-1).

More info can be found on this GTK bug: https://bugzilla.gnome.org/show_bug.cgi?id=677329

Unfortunately it doesn't seem that there's much we can do without interecepting the X events themselves.

Thanks!

Comment 5 Andrew Comminos [:acomminos] 2015-06-02 14:45:33 PDT

Created attachment 8614328 [details] [diff] [review]
Implement custom handling for XInput2 events.

Updated to make translated events use the client pointer device to stop GDK complaints.

Comment 6 Andrew Comminos [:acomminos] 2015-06-05 14:15:28 PDT

I've looked into this and have some insight into why this might be happening.

When an X11 window hands off focus to an inferior window, the parent receives a FocusIn w/ NotifyAncestor event indicating that the child window will be focusing in. This is where we switch the parent's has_pointer_focus, allowing it to send the focus out event through GDK. For whatever reason, this event doesn't occur on the X11 Window's focus_window- only the toplevel. I suspect this should be propagating down to the focus window, but doesn't with XI2's focus events.

Comment 7 Karl Tomlinson (:karlt) 2015-07-09 03:30:54 PDT

(In reply to Andrew Comminos [:acomminos] from comment #6)
> I've looked into this and have some insight into why this might be happening.
> 
> When an X11 window hands off focus to an inferior window, the parent
> receives a FocusIn w/ NotifyAncestor event indicating that the child window
> will be focusing in.

Are you referring to focus_window with "inferior window"?
Doesn't its parent get a FocusOut w/ NotifyInferior,
or FocusIn with NotifyVirtual?

> This is where we switch the parent's has_pointer_focus,
> allowing it to send the focus out event through GDK. For whatever reason,
> this event doesn't occur on the X11 Window's focus_window- only the
> toplevel.

The early return in _gdk_device_manager_core_handle_focus when
(toplevel->focus_window == original) skips any handling of focus events on the
focus_window.

https://git.gnome.org/browse/gtk+/tree/gdk/x11/gdkdevicemanager-core-x11.c?id=3.16.5#n827

Comment 8 Karl Tomlinson (:karlt) 2015-07-09 03:34:06 PDT

Comment on attachment 8614328 [details] [diff] [review]
Implement custom handling for XInput2 events.

Getting this kind of approach right would take some care.

I'm concerned about changing what parts of GDK code see because that could mean that end up with unexpected consequences if/when the GDK code changes.

If we can identify the cause of the bug and fix it, then we can look at this
kind of workaround because we can limit it to past software where we know the
internals.  I'm not sure this bug is bad enough that it is worth a workaround as risky as this.

We may be able to come up with a general approach that does not depend on GDK
internals, such as consistently modifying all related events to trick GDK into thinking that the ancestor/root window never has focus.  But first I'd like to know what the bug is that we are trying to work around.

One observation of the patch here is that it still lets NotifyAncestor and
NotifyVirtual and crossing events affect GDK internal state, including the
setting of focus, but NotifyNonlinear and NotifyNonlinearVirtual no longer do,
possibly leading to inconsistencies if the focus in and out paths are not
symmetric.

It also doesn't handle grabs in the same way as GDK.

I don't think the device for focus is necessarily the client pointer.  I would
expect focus to be related to a master keyboard, which may be the master
keyboard paired with the client pointer, but I'm not sure that it necessarily
is.  gdk_device_manager_get_client_pointer requires a round trip, which I'd
like to avoid.

Not directly related to this patch, but I don't know how multiple master
keyboards each with their own focus are meant to be handled.  I would guess
that we are to either pick one master keyboards or show focus if any are
currently providing focus.

Does XSetInputFocus() select a single master device to change focus or all of
them or none of them?  If none of them, then that would explain why focus is
not moving to the focus_window, and so pointer focus would remain until
another app has focus.

Comment 9 Karl Tomlinson (:karlt) 2015-07-09 03:35:48 PDT

Focus is handled in GDK by the same functions (handle_focus_change and
_gdk_device_manager_core_handle_focus) regardless of whether the core or xi2
device manager is used, so it seems likely this is an X server bug.

[1] and [2] seem to indicate that NotifyPointer events are missing with XI2.

[3] points out that at least some NotifyPointer FocusIn events are received,
but at least some NotifyPointer FocusOut events are missing.

Are you able to reproduce the situation in [3] on a modern server including
the patch in [4]?
Use "xinput list" to identify the core keyboard.  If it says "id=3" then use
"xinput test-xi2 3" to skip being spammed with pointer events.
I can't reproduce with kwin or metacity, which I assume is because focus never
moves to the root window. 
Which window manager are you using?

If you can't reproduce that, then can you reproduce any problems with Firefox
on a modern server?  If so, can you use xtrace [5] or modify attachment 519832 [details]
to also log XI2 focus events, and record the series of events please?

[1] https://bugzilla.gnome.org/show_bug.cgi?id=677329#c20
[2] https://bugzilla.gnome.org/show_bug.cgi?id=677329#c22
[3] https://bugzilla.gnome.org/show_bug.cgi?id=677329#c27
[4] http://lists.x.org/archives/xorg-devel/2013-February/035331.html
[5] http://xtrace.alioth.debian.org/

Comment 10 Milan Sreckovic [:milan] 2015-09-22 13:09:23 PDT

I don't think Andrew updated his bugzilla e-mail, I'm going to contact him on the side.

Comment 11 Andrew Comminos [:acomminos] 2015-09-22 15:50:53 PDT

Sorry for the delay in responding!

I can reproduce the issue in [3] with gnome-shell 3.16 and Xserver 1.17.2. We do, in fact, only receive a NotifyPointer detail on the focus in event.

I can also still reproduce this issue locally on Firefox trunk with using XInput2 (by unsetting GDK_USE_CORE_DEVICE_EVENTS in runtests.py) and running the plain mochitests in docshell/test/ *with the pointer inside the test harness window*. Curiously, the focus handling appears to work fine if the pointer is outside the window.

I'll look into this some more and get some event logs.

Comment 12 Andrew Comminos [:acomminos] 2015-09-22 16:23:54 PDT

Created attachment 8664548 [details] [diff] [review]
Disable XInput2 by default on GTK3.

I spoke with :jrmuizel about this on IRC and we agreed that disabling XI2 is a good choice for now to get GTK3 to ship due to bugs like bug 1182700 and bug 1196777.

This patch uses gdk_disable_multidevice to turn off GDK's XI2 device manager entirely, falling back to the core device manager. Thanks!

Comment 13 Andrew Comminos [:acomminos] 2015-09-22 16:24:16 PDT

Created attachment 8664549 [details] [diff] [review]
Don't disable XInput2 for mochitests on GTK3, off by default now.

Remove mochitest workaround.

Comment 14 Karl Tomlinson (:karlt) 2015-09-22 19:46:19 PDT

Comment on attachment 8664548 [details] [diff] [review]
Disable XInput2 by default on GTK3.

Yes, I agree, and this is a nice way to do it, thanks!

Comment 15 Andrew Comminos [:acomminos] 2015-09-22 19:57:45 PDT

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

Comment 16 Andrew Comminos [:acomminos] 2015-09-22 21:16:25 PDT

Created attachment 8664633 [details] [diff] [review]
Disable XInput2 by default on GTK3. Carry r=karlt

Looks like GTK 3.4 on the test infrastructure expects GLib to be initialized before calling gdk_display_get_default; let's skip that display check. It's possible to have a device manager initialized before the display anyway.

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

Comment 17 Pulsebot 2015-09-22 22:30:46 PDT

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7017f284e77
https://hg.mozilla.org/integration/mozilla-inbound/rev/274691ea288e

Comment 18 Carsten Book [:Tomcat] 2015-09-23 03:33:43 PDT

https://hg.mozilla.org/mozilla-central/rev/e7017f284e77
https://hg.mozilla.org/mozilla-central/rev/274691ea288e

Comment 19 Jeff Muizelaar [:jrmuizel] 2015-09-23 06:15:46 PDT

Comment on attachment 8664633 [details] [diff] [review]
Disable XInput2 by default on GTK3. Carry r=karlt

Approval Request Comment
[Feature/regressing bug #]: GTK3
[User impact if declined]: bugs 1182700, 1170342
[Describe test coverage new/current, TreeHerder]: Testing on treeherder has already had XInput2 disabled
[Risks and why]: Very low risk

Comment 20 Andrew Comminos [:acomminos] 2015-09-23 10:33:08 PDT

*** Bug 1170783 has been marked as a duplicate of this bug. ***

Comment 21 Bill McCloskey (:billm) 2015-09-25 05:03:51 PDT

I'm having trouble following the comments here, but mozregression pointed me to this bug. Is it possible that it broke smooth trackpad scrolling in Linux? I used to be able to scroll a few pixels at a time. Since the latest nightly I can only scroll in big jumps. It's not really a functionality issue, but it feels awful if you're used to the old behavior.

Comment 22 Jeff Muizelaar [:jrmuizel] 2015-09-25 05:52:18 PDT

(In reply to Bill McCloskey (:billm) from comment #21)
> I'm having trouble following the comments here, but mozregression pointed me
> to this bug. Is it possible that it broke smooth trackpad scrolling in
> Linux? I used to be able to scroll a few pixels at a time. Since the latest
> nightly I can only scroll in big jumps. It's not really a functionality
> issue, but it feels awful if you're used to the old behavior.

Yes, breaking smooth trackpad scrolling is expected. We had some bugs with XInput2 and didn't want to block shipping GTK3 on them. Bug 1207973 adds an environment variable that will allow you to have it back until these bugs are dealt with.

Comment 23 Bill McCloskey (:billm) 2015-09-25 09:25:51 PDT

Is this something we would ship on release? It seems like a pretty major regression from GTK2. It makes Firefox feel really sluggish on laptops. I expect to be able to directly manipulate the scroll position, and when it doesn't happen, it feels like jank to me (even though it isn't).

Does the focus problem happen to just a subset of users? If so, is there a way we could detect those users and only then disable XInput2?

Comment 24 Jeff Muizelaar [:jrmuizel] 2015-09-25 10:47:18 PDT

(In reply to Bill McCloskey (:billm) from comment #23)
> Is this something we would ship on release? It seems like a pretty major
> regression from GTK2. It makes Firefox feel really sluggish on laptops. I
> expect to be able to directly manipulate the scroll position, and when it
> doesn't happen, it feels like jank to me (even though it isn't).
> 
> Does the focus problem happen to just a subset of users? If so, is there a
> way we could detect those users and only then disable XInput2?

My understanding was that this should not be a regression relative to GTK2. i.e. XInput2 is required for smooth scrolling, GTK2 doesn't support XInput2, and so I'd expect current GTK2 to not have smooth scrolling.

Comment 25 Bill McCloskey (:billm) 2015-09-25 10:58:18 PDT

Wow, you're right, sorry. I guess I got used to the new scrolling behavior really quickly.

Comment 26 Martin Stránský 2015-09-29 06:19:13 PDT

(In reply to Bill McCloskey (:billm) from comment #25)
> Wow, you're right, sorry. I guess I got used to the new scrolling behavior
> really quickly.

You can enable it, see Bug 1207973 for details. Just set MOZ_USE_XINPUT2=1 env variable.

Comment 27 Liz Henry (:lizzard) (needinfo? me) 2015-10-01 07:13:21 PDT

It might be good to add a release note for this as a known issue. Jeff, can you suggest wording for it if you think that's a good idea?

Comment 28 Liz Henry (:lizzard) (needinfo? me) 2015-10-01 07:16:42 PDT

Comment on attachment 8664633 [details] [diff] [review]
Disable XInput2 by default on GTK3. Carry r=karlt

Approved for aurora uplift; we want scrolling to work

Comment 29 Liz Henry (:lizzard) (needinfo? me) 2015-10-01 07:18:49 PDT

Comment on attachment 8664549 [details] [diff] [review]
Don't disable XInput2 for mochitests on GTK3, off by default now.

Uplift test changes for scrolling/gtk to aurora

Comment 30 Sylvestre Ledru [:sylvestre] 2015-10-05 08:10:30 PDT

GTK3 has been disabled in 42 (and never enabled in 41), not tracking

Comment 31 Carsten Book [:Tomcat] 2015-10-17 00:36:23 PDT

landed but seems there was a problem in marking the bug 
https://hg.mozilla.org/releases/mozilla-aurora/rev/e99258733708
https://hg.mozilla.org/releases/mozilla-aurora/rev/bdfe683a5e01

Comment 32 Andrew Comminos [:acomminos] 2015-11-03 03:33:08 PST

I've spent some time recently trying to dig through the X input stack; here's what I've found so far.

- I've clarified that the problem is indeed missing FocusOut NotifyPointer events on the mochitest window after dialogs have been spawned.
    - When the pointer is in an inferior of the mochitest window, the pointer window should be notified regarding focus changes in the ancestor as per https://tronche.com/gui/x/xlib/events/input-focus/normal-and-grabbed.html.
    - A NotifyPointer focus out event on a window is a precondition for losing 'GDK focus'- observe the HAS_FOCUS macro in GDK's X11 core device manager. A change in this is what gets reported to Gecko.
- The issue is most easily observable by logging GDK's events (with GDK_DEBUG=events) when running docshell/test/navigation/test_bug270414.html.
    - When running with XInput2 (MOZ_USE_XINPUT2=1), we are clearly missing three events on the mochitest window when comparing without XI2 and GDK_DEBUG=events.
    - Debugging without XInput2 reveals that these are indeed the focus out events we were missing.

Something that caught my eye is the following comment in `dix/enterleave.c`;

> NotifyPointer events may be sent if the focus changes from window A to
> B. The assumption used in this model is that NotifyPointer events are only
> sent for the pointer paired with the keyboard that is involved in the focus
> events. For example, if F(W) changes because of keyboard 2, then
> NotifyPointer events are only sent for pointer 2.

This leads me to believe that perhaps when the newly created window steals focus, it's not using the correct master keyboard such that the pointer window is notified. I need to investigate this more, however.

Comment 33 Andrew Comminos [:acomminos] 2015-12-29 22:09:22 PST

I believe I've finally discovered the root issue here; a bug in Xorg has been filed to fix NotifyPointer not sending focus out events to the pointer window. Details are on the freedesktop bugzilla;

https://bugs.freedesktop.org/show_bug.cgi?id=93539

I'll be submitting a patch upstream soon, but we should still consider working around this in Firefox by potentially blocking NotifyPointer events from reaching GTK's input handling system.

Comment 34 Karl Tomlinson (:karlt) 2016-02-18 19:28:51 PST

https://git.gnome.org/browse/gtk+/commit/?id=d55b815
In
https://git.gnome.org/browse/gtk+/log/gdk/x11/gdkeventsource.c?h=3.19.9
not in
https://git.gnome.org/browse/gtk+/log/gdk/x11/gdkeventsource.c?h=3.19.8
https://git.gnome.org/browse/gtk+/log/gdk/x11/gdkeventsource.c?h=3.18.6

Comment 35 Andrew Comminos [:acomminos] 2016-05-16 15:24:22 PDT

https://cgit.freedesktop.org/xorg/xserver/commit/?id=2fbf5c2f91d33efbda573c4be036248b1d8ed7f1 should fix this upstream. We should aim to backport this patch to the version of X running on our test machines.