After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 773210 - [WAYLAND] random loss of focus on toplevel when mapping/unmapping grab-less xdg_popup
[WAYLAND] random loss of focus on toplevel when mapping/unmapping grab-less x...
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: wayland
3.22.x
Other Linux
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
Depends on: 771694
Blocks: WaylandRelated 772360 773000 773136
 
 
Reported: 2016-10-19 09:24 UTC by Olivier Fourdan
Modified: 2016-10-27 15:30 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
MUTTER_VERBOSE log when the issue occurs (146.38 KB, text/plain)
2016-10-19 09:24 UTC, Olivier Fourdan
  Details
[PATCH] wayland: Force grab-less xdg_popup on top (2.52 KB, patch)
2016-10-19 12:27 UTC, Olivier Fourdan
none Details | Review
[PATCH] wayland: Force grab-less xdg_popup on top (2.52 KB, patch)
2016-10-19 12:28 UTC, Olivier Fourdan
none Details | Review
[PATCH v3] wayland: Force grab-less xdg_popup on top (3.10 KB, patch)
2016-10-20 09:19 UTC, Olivier Fourdan
none Details | Review
PATCH 1/2] wayland: Do not unfocus on new window (2.20 KB, patch)
2016-10-25 07:22 UTC, Olivier Fourdan
committed Details | Review
[PATCH 2/2] window: do not focus on ungrab for o-r like windows (2.12 KB, patch)
2016-10-25 07:24 UTC, Olivier Fourdan
none Details | Review
[PATCH 2/2] wayland: do not focus o-r like windows (1.75 KB, patch)
2016-10-25 12:44 UTC, Olivier Fourdan
none Details | Review
[PATCH 2/2] wayland: do not explicitly focus xdg_popup (1.63 KB, patch)
2016-10-25 15:00 UTC, Olivier Fourdan
committed Details | Review

Description Olivier Fourdan 2016-10-19 09:24:19 UTC
Created attachment 338001 [details]
MUTTER_VERBOSE log when the issue occurs

+++ This bug was initially created as a clone of Bug #771694 +++

Description:

Using the patch from bug 771694 to allow grab-less xdg_popup in gtk+/gdk Wayland backend sometimes leads to a loss of focus on the toplevel window.

Steps to reproduce:

1. Apply attachment 337961 [details] [review] to gtk+-3.22.1, rebuild, install
2. Run gtksourceview test-completion
3. Type some text quickly, dismiss the popup, re-type, etc.

Actual result:

Sometimes the toplevel window loses its focus

Expected result:

The toplevel remain focused as the popup should not receive focus anyway.
Comment 1 Olivier Fourdan 2016-10-19 09:45:28 UTC
  "STARTUP: The focus window W1 (lt-test-co) is an ancestor of the newly mapped
   window W5 which isn't being focused.  Unfocusing the ancestor."

This comes from meta_window_show() in src/core/window.c when showing the popup window:

2230 static void
2231 meta_window_show (MetaWindow *window)
2232 {
       [...]
2255   /* Now, in some rare cases we should *not* put a new window on top.
2256    * These cases include certain types of windows showing for the first
2257    * time, and any window which would be covered because of another window
2258    * being set "above" ("always on top").
2259    *
2260    * FIXME: Although "place_on_top_on_map" and "takes_focus_on_map" are
2261    * generally based on the window type, there is a special case when the
2262    * focus window is a terminal for them both to be false; this should
2263    * probably rather be a term in the "if" condition below.
2264    */
2265 
2266   if ( focus_window != NULL && window->showing_for_first_time &&
2267       ( (!place_on_top_on_map && !takes_focus_on_map) ||
2268       window_would_be_covered (window) )
2269     ) {
2270       if (meta_window_is_ancestor_of_transient (focus_window, window))
2271         {
2272           guint32     timestamp;
2273 
2274           timestamp = meta_display_get_current_time_roundtrip (window->display);
2275 
2276           /* This happens for error dialogs or alerts; these need to remain on
2277            * top, but it would be confusing to have its ancestor remain
2278            * focused.
2279            */
2280           meta_topic (META_DEBUG_STARTUP,
2281                       "The focus window %s is an ancestor of the newly mapped "
2282                       "window %s which isn't being focused.  Unfocusing the "
2283                       "ancestor.\n",
2284                       focus_window->desc, window->desc);
2285 
2286           meta_display_focus_the_no_focus_window (window->display,
2287                                                   window->screen,
2288                                                   timestamp);
2289         }

The xdg_popup window is indeed on top, doesn;t take focus (as it's without grab), it;s showing for the first time does cover the toplevel window, which explains why we get into this code path.

But there is this other message as well:

  "STARTUP: window W5 focus prevented by other activity; 76886753 < 76886786"

Which comes from intervening_user_event_occurred() in src/core/window.c called from window_state_on_map(), which is precisely what sets takes_focus to FALSE, ths trigging the condition.

The problem is the xdg_popup should not take focus, which means we /should/ hit this condition every time, i.e. focus should be lost every time...
Comment 2 Olivier Fourdan 2016-10-19 09:51:52 UTC
When it works (ie when focus is not lost) we have:

  WINDOW_STATE: Window W4 does not focus on map, and does place on top on map.

So the key to success is the value of "place_on_top_on_map"
Comment 3 Olivier Fourdan 2016-10-19 10:02:35 UTC
So what happens here is, if an event occurred after the one that triggered the popup, "places_on_top" is false (as it takes by default the same value as "takes_focus" which is false because intervening_user_event_occurred() returned true), and in that case, the toplevel which popup is transient for loses focus... 

That logic sounds pretty broken to me.
Comment 4 Carlos Garnacho 2016-10-19 11:24:33 UTC
Ugh... first though crossing my mind: should we trigger focus stealing prevention on xdg_popups? we certainly don't for override-redirect windows, and there's no ongoing focus change anyway...
Comment 5 Olivier Fourdan 2016-10-19 11:31:57 UTC
yeah, the idea I'm trying now is to set the type to META_WINDOW_OVERRIDE_OTHER for grab-less xdg_popup (leaving the current value of META_WINDOW_DROPDOWN_MENU for xdg_popup with a grab) and then set places_on_top to true for META_WINDOW_OVERRIDE_OTHER.
Comment 6 Olivier Fourdan 2016-10-19 12:27:10 UTC
Created attachment 338014 [details] [review]
[PATCH] wayland: Force grab-less xdg_popup on top

mutter would remove focus from a toplevel when showing one of its
transient window which is not on top and not focused.

When using xdg_popup without grab as allowed in xdg_shell v6, the popup
wouldn't be focused, and if an intermediate event occurs before the
popup is shown, it's not placed on top either, which could randomly
trigger a loss of focus in the corresponding toplevel window.

For grab-less xdg_popup set the type to META_WINDOW_OVERRIDE_OTHER and
make sure such windows are placed on top by mutter.
Comment 7 Olivier Fourdan 2016-10-19 12:28:54 UTC
Created attachment 338015 [details] [review]
[PATCH] wayland: Force grab-less xdg_popup on top

(Remove an additional empty line I added by mistake in the middle of the switch/case statement)
Comment 8 Olivier Fourdan 2016-10-20 09:19:05 UTC
Created attachment 338086 [details] [review]
[PATCH v3] wayland: Force grab-less xdg_popup on top

v3: Includes Carlos' addition not to focus META_WINDOW_OVERRIDE_OTHER

=> That fixes the "Details" button not working in gtksourceview's test-completion popup.
Comment 9 Rui Matos 2016-10-21 13:47:11 UTC
Review of attachment 338086 [details] [review]:

Honestly, I'd prefer to just delete the code in meta_window_show() that calls meta_display_focus_the_no_focus_window() instead of using a weakly defined window type for these popups.

I just can't think of any case where it makes sense for a new window to cause us to globally unset focus. Anyone can?

::: src/core/window.c
@@ +7807,3 @@
    */
+  if (window->override_redirect ||
+      window->type == META_WINDOW_OVERRIDE_OTHER)

This should in any case be a separate patch since it's a different issue, and check for any of the OR window types, not just this one.
Comment 10 Olivier Fourdan 2016-10-25 07:22:48 UTC
Created attachment 338385 [details] [review]
PATCH 1/2] wayland: Do not unfocus on new window

Remove the code in meta_window_show() as per review in comment 9
Comment 11 Olivier Fourdan 2016-10-25 07:24:02 UTC
Created attachment 338386 [details] [review]
[PATCH 2/2] window: do not focus on ungrab for o-r like windows

Separate patch for the ungrab issue as per comment 9
Comment 12 Rui Matos 2016-10-25 11:28:36 UTC
Review of attachment 338385 [details] [review]:

The commit subject should start with window:..., with that fixed, lgtm
Comment 13 Rui Matos 2016-10-25 12:21:43 UTC
Review of attachment 338386 [details] [review]:

There is no "ungrab" event, this function is handling "ungrabbed" events, i.e. events for which there's no explicit grab (display->grab_op == META_GRAB_OP_NONE) but still come through mutter because either we're a wayland compositor or mutter has a passive button grab on the window because it was unfocused (we need that grab to be able to get an event on X to focus the window if users click it).

So, thinking a bit more about this, it seems that we should instead be handling these wayland windows that don't want keyboard focus in the wayland vfunc meta_window_wayland_focus() and leave the generic code here as is.
Comment 14 Olivier Fourdan 2016-10-25 12:44:01 UTC
Created attachment 338400 [details] [review]
[PATCH 2/2] wayland: do not focus o-r like windows

You mean something like that?

The override-redirect concept is X11, other windowing systems such as
Wayland do not have override-redirect.

On Wayland, do not focus windows for which the type would match an
override redirect window in X11.
Comment 15 Rui Matos 2016-10-25 13:41:04 UTC
Review of attachment 338400 [details] [review]:

ok, but IMO the commit message should be less about X11 and OR window semantics and more like what Jonas says in https://bugzilla.gnome.org/show_bug.cgi?id=771694#c24

::: src/wayland/meta-window-wayland.c
@@ +139,3 @@
                            guint32     timestamp)
 {
+  if (is_like_override_redirect (window))

maybe rename this to something like should_get_input_focus() ?
Comment 16 Olivier Fourdan 2016-10-25 15:00:48 UTC
Created attachment 338419 [details] [review]
[PATCH 2/2] wayland: do not explicitly focus xdg_popup

    
    The keyboard focus semantics for non-grabbing xdg_shell v6 popups is
    pretty undefined.
    
    Same applies for subsurfaces, but in practice, subsurfaces never receive
    keyboard focus, so it makes sense to do the same for non-grabbing
    popups.
Comment 17 Rui Matos 2016-10-27 15:20:28 UTC
Review of attachment 338419 [details] [review]:

looks fine
Comment 18 Olivier Fourdan 2016-10-27 15:29:02 UTC
Comment on attachment 338385 [details] [review]
PATCH 1/2] wayland: Do not unfocus on new window

attachment 338385 [details] [review] pushed in git master as commit 998d921 - window: Do not unfocus on new window

attachment 338385 [details] [review] pushed in branch "gnome-3-22" as commit 5919a4a - window: Do not unfocus on new window
Comment 19 Olivier Fourdan 2016-10-27 15:30:16 UTC
Comment on attachment 338419 [details] [review]
[PATCH 2/2] wayland: do not explicitly focus xdg_popup

attachment 338419 [details] [review] pushed to git master as commit 4295fdb - wayland: do not explicitly focus xdg_popup

attachment 338419 [details] [review] pushed to branch gnome-3-22 as commit c52808b - wayland: do not explicitly focus xdg_popup