GNOME Bugzilla – Bug 780820
[Wayland] Focused window under application first window
Last modified: 2018-01-11 10:07:45 UTC
When running Evolution with GNOME Shell under Wayland on Fedora 25, if you press "Enter" (or double-click) on a message to open it in a standalone reading window, then hit ctrl+R (or click the Reply button), the reading window will close and you won't see the newly created composer window... because it will be shown behind the main Evolution window. Filing it here because I'm not sure if it's a duplicate of bug #756202 or not, or maybe something else going on...
Thanks for a bug report. There is used gtk_window_present() in the background, but it's possible it's partly evolution issue too, due to destroying the underlying window. I'll check it.
I can reproduce this only under Wayland, but also with plain gtk+, thus I'm moving this there for further investigation.
Created attachment 349291 [details] wl.c Test application. Compile & run as shown on the first line of the file. Then follow steps shown in the window body. The window itself is shown maximized, to illustrate the issue. To have steps also here: Steps: 1) click 'Create new window' above - The newly created window is shown above this 2) click 'Create new and close this' in the new window - It will show a new window just with a label and will close the window from step 1) The window from step 2) has focus, but it's under this window when run in Wayland.
thanks for the testcase
I think this is a mutter issue, clients cannot manipulate the stack in Wayland so gtk+ is unlikely the culprit. Moreover, the same works fine under weston :)
MUTTER_VERBOSE stack ops at the time the new sub-sub-window is mapped and the sub-window unmapped: STACK: Adding window W3 to the stack STACK: Window W3 has stack_position initialized to 2 STACK: Syncing window stack to server STACK: Adding 1 windows to sorted list STACK: Recomputing layers STACK: Window W3 on layer 2 type = 0 has_focus = 0 STACK: Window W3 moved from layer 8 to 2 STACK: Window W2 (sub-window) on layer 2 type = 0 has_focus = 1 STACK: Window W1 (main windo) on layer 2 type = 0 has_focus = 0 STACK: Reapplying constraints STACK: Sorting stack list STACK: Bottom to top: 2:0 - W1 (main windo) 2:1 - W2 (sub-window) 2:2 - W3 STACK: Restacking 3 windows STACK: Syncing window stack to server STACK: Reapplying constraints STACK: Bottom to top: 2:0 - W1 (main windo) 2:1 - W2 (sub-window) 2:2 - W3 STACK: Restacking 3 windows VERBOSE: Unmanaging W2 (sub-window) FOCUS: Focusing default window since we're unmanaging W2 (sub-window) FOCUS: Focusing MRU window FOCUS: Focusing workspace MRU window W1 (main windo) FOCUS: Setting input focus to window W1 (main windo), input: 1 take_focus: 0 FOCUS: W2 (sub-window) is now the previous focus window due to being focused out or unmapped VERBOSE: Grabbing unfocused window buttons for W2 (sub-window) FOCUS: * Focus --> W1 (main windo) with serial 492 VERBOSE: Ungrabbing unfocused window buttons for W1 (main windo) VERBOSE: Grabbing window buttons for 0x0 WINDOW_OPS: Raising window W1 (main windo), ancestor of W1 (main windo) STACK: Window W1 (main windo) had stack_position set to 2 STACK: Syncing window stack to server STACK: Reapplying constraints STACK: Sorting stack list STACK: Bottom to top: 2:1 - W3 (sub-sub-wi) 2:2 - W1 (main windo) => Looks like W1 gets re-focused once W2 is unmapped (next in the MRU list) and therefore raised above the newly mapped window W3
Moving to mutter. The newly mapped window is not considered in get_default_focus_window() as called from meta_stack_get_default_focus_window() because meta_window_should_be_showing() return FALSE, and this is specific to Wayland backend because the window has no buffer attached *yet*: 1598 gboolean 1599 meta_window_should_be_showing (MetaWindow *window) 1600 { 1601 #ifdef HAVE_WAYLAND 1602 if (window->client_type == META_WINDOW_CLIENT_TYPE_WAYLAND && 1603 !meta_wayland_surface_get_buffer (window->surface)) 1604 return FALSE; 1605 #endif 1606 1607 /* Windows should be showing if they're located on the 1608 * active workspace and they're showing on their own workspace. */ 1609 return (meta_window_located_on_workspace (window, window->screen->active_workspace) && 1610 meta_window_showing_on_its_workspace (window)); 1611 } As a result, the old window is focused and raised instead, and when the new window is eventually mapped, it is focused and not raised in meta_focus_window(). So we end up with the focused window being placed underneath the non-focused window.
This is originally from bug 750552 / commit 0912013
Created attachment 350983 [details] [review] [PATCH] wayland: Raise when queuing a "calc showing" operation When showing closing a window and showing a new one, the new one may not be granted input focus until it gets a buffer. If another window is chosen to receive focus and raised on top of stack, the newly mapped window is focused but placed underneath that other window. Make sure we also raise a window when queuing a "calc showing" operation in apply_pending_state() so that we don't end up with a window focused placed underneath an unfocused one.
Review of attachment 350983 [details] [review]: ::: src/wayland/meta-wayland-surface.c @@ +733,3 @@ + meta_window_queue (surface->window, META_QUEUE_CALC_SHOWING); + if (meta_prefs_get_focus_mode () == G_DESKTOP_FOCUS_MODE_CLICK) + meta_window_raise (surface->window); This doesn't look right, it'll bypass any focus stealing prevention things
Hmm. I think the issue is that we add the Wayland client window to the stack too early. The issue seems to be that we 1. Create a new window that cant be shown 2. Adds this new window on top of the stack 3. Closes the currently focused window 4. Finds a good candidate for new focused window 5. Raises that window 6. Show the window created in 1. If step 6. would happen before 3. we're fine, but not when after. I see two possible solutions to this: 1) Delay adding the window to the stack until it is "ready" (i.e. has a buffer attached) Not sure what funny side effects this may have; does anything assume a window is always in the stack? 2) Delay creation of the MetaWindow until it's ready (i.e. has a buffer attached) Having a content-less MetaWindow has bitten us in the ass plenty of times, so maybe we should stop having that. It'd require that we duplicate the relevant state in meta-wayland-xdg-shell.c. meta-wayland-wl-shell.c already does this as it needs to create/destroy the MetaWindow like crazy because wl_shell is crazy.
(In reply to Jonas Ådahl from comment #11) ... > 2) Delay creation of the MetaWindow until it's ready (i.e. has a buffer > attached) > > Having a content-less MetaWindow has bitten us in the ass plenty of times, > so maybe we should stop having that. It'd require that we duplicate the > relevant state in meta-wayland-xdg-shell.c. meta-wayland-wl-shell.c already > does this as it needs to create/destroy the MetaWindow like crazy because > wl_shell is crazy. No, we can't really do that because we rely on it being placed and constrained properly by constrain.c.
(In reply to Jonas Ådahl from comment #11) > Hmm. I think the issue is that we add the Wayland client window to the stack > too early. The issue seems to be that we > > 1. Create a new window that cant be shown > 2. Adds this new window on top of the stack > 3. Closes the currently focused window > 4. Finds a good candidate for new focused window > 5. Raises that window > 6. Show the window created in 1. > > If step 6. would happen before 3. we're fine, but not when after. This is exactly it! > I see two possible solutions to this: > > 1) Delay adding the window to the stack until it is "ready" (i.e. has a > buffer attached) There are comments in _meta_window_shared_new() that seem to imply that some apps would misbehave otherwise, but I reckon these are X11 clients, so if we implement this for Wayland only (as commit cdac4d0 and 0912013 did before), we might be fine.
Created attachment 351545 [details] [review] [PATCH RFC] wayland: Differ stack placement without a buffer When closing a window and showing a new one, the new one may not be granted input focus until it gets a buffer. If another window is chosen to receive focus and raised on top of stack, the newly mapped window is focused but placed underneath that other window. For Wayland surfaces, differ adding the window to the stack until we actually get to show it, when we have a buffer attached.
Review of attachment 351545 [details] [review]: While this would avoid the issue, it breaks for some other apps and crashes mutter when trying to remove a window from the stack if it hasn't be put in the stack yet: FOCUS: Unmanaging window W1 () which doesn't currently have focus WINDOW_STATE: Removing W1 () from the calc_showing queue WINDOW_STATE: Removing W1 () from the move_resize queue STACK: Removing window W1 () from the stack Bug in window manager: Window W1 () removed from stack but had no stack position
Created attachment 351957 [details] [review] [PATCH RFC v2] wayland: Differ stack placement without a buffer When closing a window and showing a new one, the new one may not be granted input focus until it gets a buffer. If another window is chosen to receive focus and raised on top of stack, the newly mapped window is focused but placed underneath that other window. For Wayland surfaces, differ adding the window to the stack until we actually get to show it, when we have a buffer attached.
Created attachment 351962 [details] [review] [PATCH v3] wayland: Defer stack placement without a buffer v3: Forgot to amend the patch <facepalm> s/differ/defer/ add debug message when deferring stack placement
*** Bug 784417 has been marked as a duplicate of this bug. ***
*** Bug 792184 has been marked as a duplicate of this bug. ***
this is still annoying and would be nice to have fixed, eventually
Review of attachment 351962 [details] [review]: The solution looks sane to me, but I have some comments on the implementation. ::: src/core/stack.c @@ +113,3 @@ + window->desc); + return; + } I don't think we should have a function called "meta_stack_add()" that might not add anything. We should also try to avoid any per-winsys code anywhere outside of the MetaWindow(X11|Wayland|Xwayland) files. An idea: - Add MetaWindowClass::is_stackable() - MetaWindowWayland implements this as "has buffer", MetaWindowX11 as !override_redirect - Add a meta_stack_is_window_added() helper - In both _new_shared() and meta_window_show() do "if (!meta_stack_is_window_added (stack, window) && meta_window_is_stackable (window) meta_stack_add (stack, window)" and in unmanage do if (meta_stack_is_window_added (stack, window)) meta_stack_remove (stack, window); (alternatively add a meta_stack_maybe_add() that hides parts of the logic) - We could remove the meta_bug() thing below, which would be dead code.
Thanks a bunch Jonas for the review, much appreciated! I like your idea of MetaWindowClass::is_stackable()
Created attachment 366553 [details] [review] [PATCH v4] window: Defer stack placement without a buffer When closing a window and showing a new one, the new one may not be granted input focus until it gets a buffer on Wayland. If another window is chosen to receive focus and raised on top of stack, the newly mapped window is focused but placed underneath that other window. Meaning that for Wayland surfaces, we need to defer adding the window to the stack until we actually get to show it, once we have a buffer attached. Rather that checking the windowing backend prior to decide if a window is stackable or not, introduce a new vfunc is_stackable() which tells if a window should be added to the stack regardless of the underlying windowing system. Also add meta_window_in_stack() API rather than checking the stack position directly (replacing the define WINDOW_IN_STACK only available in stack.c) and remove a window from the stack only if it is present in the stack, so that the test in meta_stack_remote() becomes irrelevant.
Review of attachment 366553 [details] [review]: ::: src/core/window-private.h @@ +701,3 @@ void meta_window_frame_size_changed (MetaWindow *window); +gboolean meta_window_in_stack (MetaWindow *window); better name (follows convention): meta_window_is_in_stack(). Alternatively meta_window_is_stacked(). ::: src/core/window.c @@ +1701,3 @@ + /* If showing a window that wasn't added to the stack yet, make sure we + * add it to the stack now. + * See #780820 This comment just spells out what the statement below does. Suggestion: /* Some windows are not stackable until being showed, so add those now. */ (the bug link (number will be ambiguous with the move to gitlab (even though we hopefully won't have 780k bugs just on mutter!) will be easily discoverable using git annotate) @@ +6908,3 @@ +meta_window_in_stack (MetaWindow *window) +{ + return (window->stack_position >= 0); why the ( and )? ::: src/wayland/meta-window-wayland.c @@ +547,3 @@ +meta_window_wayland_is_stackable (MetaWindow *window) +{ + /* Surface without a buffer attached yet should not be added to the stack */ This comment spells out what is done below so no need for it. @@ +548,3 @@ +{ + /* Surface without a buffer attached yet should not be added to the stack */ + return (meta_wayland_surface_get_buffer (window->surface) != NULL); Why the ( and )? ::: src/x11/window-x11.c @@ +1554,3 @@ +{ + /* For X11, do not add OR windows to the stack */ + return (!window->override_redirect); Same here about the comment being the line of code but in english, and about the ( and ).
Created attachment 366607 [details] [review] [PATCH v5] window: Defer stack placement without a buffer Updated patch as per review. (The parenthesis around the return statement is an old habit, as those are logical values, I reckon it makes it more readable this way - Now removed)
Humm, looks like this patch is breaking stacking of g-s dialogs...
Review of attachment 366607 [details] [review]: lgtm!
Created attachment 366637 [details] [review] [PATCH v6] window: Defer stack placement without a buffer (In reply to Olivier Fourdan from comment #26) > Humm, looks like this patch is breaking stacking of g-s dialogs... Ah, found the issue, sorry... Had to update the patch, we still need to take int oaccount O-R windows to set the layer as before (otherwise on Wayland the gnome-shell's St dialogs end up underneath the client windows)
Review of attachment 366637 [details] [review]: sure
Comment on attachment 366637 [details] [review] [PATCH v6] window: Defer stack placement without a buffer attachment 366637 [details] [review] pushed to git master as commit bd9a300 - window: Defer stack placement without a buffer