Add focus stealing prevention to `MinimalWindowManager` #3693

tarek-y-ismail · 2024-12-04T11:28:48Z

Closes #2586.

Adds a new constructor to FloatingWindowManagerPolicy to control focus stealing prevention. This in a nutshell stops new windows from being focused and raised. When used with xdg-activation-v1, this improves security as external actors can't just steal focus by opening a new window (in addition to the niceties xdg-activation-v1 adds to usability).

TODO:

~~Fix window offsetting. This is built off the position of the focused window, so all windows after the second one open in the same (x,y) position.~~
Split off to Window offsetting shouldn't assume new windows are automatically focused #3695
~~Fix windows closing in the order of opening instead of front to back. see Add focus stealing prevention to MinimalWindowManager #3693 (comment)~~
Moved to Refocus on application close is broken with focus stealing prevention #3694
~~Make sure Xwayland applications work properly. My focus has been on Wayland applications so far.~~
Xwayland doesn't seem to support xdg-activation at the moment?
Alt + tab predictably broken :/
Have to focus other applications before they work with alt + tab
Decorations are not pushed behind the focused window

AlanGriffiths

This is not what I expected. I expected a miral API to "opt in" to "focus stealing prevention" [FSP].

This demonstrates that there's already a way to "opt in" to FSP by writing a customise window management policy, but isn't what I expected (a new constructor to MinimalWindowManager)

examples/example-server-lib/floating_window_manager.h

examples/example-server-lib/floating_window_manager.cpp

AlanGriffiths

That's more like what I was expecting. One question and several nits.

On a API design note: bool parameters tend to make code hard to read:

    // unreadable
    foo(..., false, ...);

Consider using an enum class:

    // readable
    foo(..., FocusStealing::allow, ...);

That is written assuming something like

    enum class FocusStealing
    {
        allow,
        prevent
    };

src/miral/minimal_window_manager.cpp

include/miral/miral/minimal_window_manager.h

examples/miral-shell/shell_main.cpp

src/miral/minimal_window_manager.cpp

src/miral/basic_window_manager.cpp

AlanGriffiths

The API looks good. But I'm not sure the implementation is quite there

AlanGriffiths · 2024-12-05T11:44:42Z

src/miral/minimal_window_manager.cpp

+    // If focus stealing prevention is on, swap the old focused window (now in
+    // the back) with the new window in the front.
+    // If it's a legitimate window, it'll be focused and raised via
+    // xdg-activation.
+    if ((self->focus_stealing == FocusStealing::prevent) && tools.active_window())
+        tools.swap_tree_order(tools.active_window(), window_info.window());


I'm not convinced that this is exactly right.

Consider there being a fullscreen window with focus when a notification window is created. I don't expect the notification to be activated (as it shouldn't get focus), but it should be above the fullscreen window. Trying to push it "below" seems wrong (even if it ultimately fails because of an implementation detail: the windows are not in the same layer);

Consider an application that creates a new window, e.g. a menu. This should go on top without needing activation. Here the call fails with log_error("Unable to swap tree order due to overlap"); because BasicWindowManager checks for an overlap between the trees. Spamming the log with errors is a bad idea;

In addition to the SurfaceStack, application_selector is maintaining a stack of windows. I don't see this swap being notified (it will still have the new window on top). Having inconsistencies between the two stacks will surely lead to problems.

On 1., what is "a notification window"? How would we distinguish it from any other surface?

My original bug report about stealing did mention notifications, because that's how it affected me. But notifications should never be toplevels, and we can't be expected to treat them differently to any other toplevel.

we can't be expected to treat them differently to any other toplevel

This code is handling ALL windows, not just toplevels

For 1., looking at window types:

mir_window_type_normal, /**< AKA "regular" */ mir_window_type_utility, /**< AKA "floating" */ mir_window_type_dialog, mir_window_type_gloss, mir_window_type_freestyle, mir_window_type_menu, mir_window_type_inputmethod, /**< AKA "OSK" or handwriting etc. */ mir_window_type_satellite, /**< AKA "toolbox"/"toolbar" */ mir_window_type_tip, /**< AKA "tooltip" */ mir_window_type_decoration,

Maybe we can only swap mir_window_type_{normal,utility}? This would also address point 2 since menus won't get swapped automatically then.

For 3., the first thing that comes to mind is overloading advise_new_window to sync the two stacks together

I proposes that the windows that should be swapped are mir_window_type_normal, in the "application" layer, and parentless; anything else can be left on top

Seems like some wayland applications use mir_window_type_freestyle as well.

Edit: A lot of wayland applications. Including firefox, gnome-terminal, weston-terminal, qterminal, vlc

OK: in the "application" layer, and parentless

I relaxed the condition and tried around with different applications and things seem to work as expected.

examples/miral-shell/shell_main.cpp

tarek-y-ismail · 2024-12-05T17:16:14Z

@Saviq @AlanGriffiths Window offsetting might be a bit more involved?

The simplest thing I have in mind is making sure that when a window is placed, it shouldn't be completely occluding the windows beneath it in the surface stack. In the case of too many windows underneath, or just a really big window being opened, we can give up and occlude them. This would require some changes to SurfaceStack though.

What do you think? Should we spin it off into its own thing?

Saviq · 2024-12-05T17:39:20Z

What do you think? Should we spin it off into its own thing?

Yes.

There's something that dawned on me… does e.g. Firefox activate its own new windows on e.g. Ctrl+N, or do we need an exception here for new windows of the currently active client? Which would also address any children opening?

examples/miral-shell/shell_main.cpp

Add constructor to `FloatingWindowManagerPolicy` and `MinimalWindowManager` to control focus stealing prevention. For properly behaving applications, xdg-activation-v1 should take care of focusing and raising any subsequent children.

unfocused windows

Only applies to windows in the application layer that don't have a parent.

src/miral/minimal_window_manager.cpp

tarek-y-ismail · 2024-12-06T08:59:57Z

Firefox activate its own new windows on e.g. Ctrl+N

Nope. Only the first (mother?) instance activates. Ctrl+N opens a new window, but in the background.

AlanGriffiths

This looks reasonable. Not done much experimenting with it as I'm not entirely sure what the expected behaviour is. Will leave merging to @Saviq

Saviq · 2024-12-06T11:03:50Z

Nope. Only the first (mother?) instance activates. Ctrl+N opens a new window, but in the background.

That makes sense. And means the stealing prevention logic should only act when the new surface is from a client different than the active one.

AlanGriffiths · 2024-12-06T11:27:25Z

the stealing prevention logic should only act when the new surface is from a client different than the active one

+1

active application.

tarek-y-ismail · 2024-12-06T16:43:02Z

Some bugs I found while trying to fix alt+tab/grave:

qterminal (or Mir) seems to have a bug where Window::application is not the same between two different instances? This isn't the case with other terminals/applications. So it's likely to be a qterminal bug.
There's a bug where if you spawn an app from a terminal, switch to the newly spawned app, and press alt+grave, instead of doing nothing because you only have one instance of the app, you'll switch to the terminal again. This happens with (gnome-terminal + xeyes, gnome-terminal+weston-terminal, qterminal+vlc)

Debugging showed that it->application() and (*originally_selected_it).application() both have the same value, even with one instance of two different apps.

mir/src/miral/application_selector.cpp

Line 240 in 56714dc

if (it->application() == (*originally_selected_it).application() && tools.can_select_window(*it))

As for the alt+tab hackaround, this was just to validate my hypothesis of what caused the bug (FSP'd surfaces not being pushed onto mru_active_windows because they're not immediately activated), I'm not sure how to improve on it but I'll let it simmer in the background during the weekend

AlanGriffiths

qterminal (or Mir) seems to have a bug where Window::application is not the same between two different instances? This isn't the case with other terminals/applications. So it's likely to be a qterminal bug.

I don't think it is a bug: if you start two qterminal instances, you get two programs running each of which independently connects to the server, so the server sees two different applications.

If you start two gnome-terminal instances you get one /usr/bin/gnome-terminal.real program running which connect to the server once and opens two windows. So the server sees one application. (cat $(which gnome-terminal) for details)

src/miral/application_selector.cpp

src/miral/mru_window_list.h

examples/miral-shell/shell_main.cpp

tarek-y-ismail · 2024-12-09T10:01:51Z

src/miral/application_selector.cpp

@@ -204,6 +213,7 @@ auto ApplicationSelector::advance(bool reverse, bool within_app) -> Window
    auto it = find(selected);

    std::optional<Window> next_window = std::nullopt;
+    auto should_continue = true;


Pretty much the same logic, but I think this is a bit clearer. Not sure if I should revert or keep these changes..

tarek-y-ismail · 2024-12-09T10:16:11Z

src/miral/basic_window_manager.cpp

+
+    // Need to push onto the MRU list for alt+tab to work correctly.
+    //
+    // Originally, applications were pushed only when selected, but due to
+    // focus stealing prevention, they were not activated and thus weren't
+    // selected nor added to mru_active_windows, breaking alt+tab
+    auto window = info_for(surface).window();
+    auto const top = mru_active_windows.top();
+    if(top && top != window)
+        mru_active_windows.push_unfocused(window);


Pretty sure this is a tad too hacky. Suggestions welcome :)

src/miral/minimal_window_manager.cpp

tarek-y-ismail requested review from Saviq and AlanGriffiths December 4, 2024 11:28

tarek-y-ismail self-assigned this Dec 4, 2024

AlanGriffiths requested changes Dec 4, 2024

View reviewed changes

examples/example-server-lib/floating_window_manager.h Outdated Show resolved Hide resolved

examples/example-server-lib/floating_window_manager.cpp Outdated Show resolved Hide resolved

tarek-y-ismail requested a review from AlanGriffiths December 4, 2024 14:52

AlanGriffiths requested changes Dec 4, 2024

View reviewed changes

tarek-y-ismail requested a review from AlanGriffiths December 5, 2024 09:57

tarek-y-ismail changed the title ~~Add focus stealing prevention to FloatingWindowManagerPolicy~~ Add focus stealing prevention to MinimalWindowManager Dec 5, 2024

AlanGriffiths requested changes Dec 5, 2024

View reviewed changes

AlanGriffiths reviewed Dec 5, 2024

View reviewed changes

examples/miral-shell/shell_main.cpp Outdated Show resolved Hide resolved

tarek-y-ismail mentioned this pull request Dec 6, 2024

Window offsetting shouldn't assume new windows are automatically focused #3695

Open

tarek-y-ismail added 4 commits December 6, 2024 10:35

Only focus and raise the first open window.

1dbb0cb

Add constructor to `FloatingWindowManagerPolicy` and `MinimalWindowManager` to control focus stealing prevention. For properly behaving applications, xdg-activation-v1 should take care of focusing and raising any subsequent children.

Expose focus stealing as a configurable option

8284f3e

Overload ApplicationSelector::advise_new_window to account for

c71ec3a

unfocused windows

Restrict the types of windows that can be prevented from stealing focus

9002657

Only applies to windows in the application layer that don't have a parent.

tarek-y-ismail force-pushed the control-new-window-placement-in-policies branch from 2f48609 to 9002657 Compare December 6, 2024 08:50

tarek-y-ismail commented Dec 6, 2024

View reviewed changes

src/miral/minimal_window_manager.cpp Outdated Show resolved Hide resolved

AlanGriffiths approved these changes Dec 6, 2024

View reviewed changes

tarek-y-ismail added 3 commits December 6, 2024 14:08

Move FSP logic to prevent_focus_stealing. Allow new windows from the

fe15087

active application.

Hack around alt+tab for focus stealing prevented apps.

28250c4

Account for decorations when FSP is active

56714dc

tarek-y-ismail requested a review from AlanGriffiths December 6, 2024 16:46

AlanGriffiths requested changes Dec 6, 2024

View reviewed changes

src/miral/application_selector.cpp Outdated Show resolved Hide resolved

src/miral/mru_window_list.h Outdated Show resolved Hide resolved

tarek-y-ismail commented Dec 9, 2024

View reviewed changes

tarek-y-ismail added 2 commits December 9, 2024 12:40

PR feedback changes.

072b1a3

Update Debian symbols

2e0efa0

tarek-y-ismail marked this pull request as ready for review December 11, 2024 17:04

tarek-y-ismail requested a review from a team as a code owner December 11, 2024 17:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add focus stealing prevention to `MinimalWindowManager` #3693

Add focus stealing prevention to `MinimalWindowManager` #3693

tarek-y-ismail commented Dec 4, 2024 •

edited

Loading

AlanGriffiths left a comment

AlanGriffiths left a comment

AlanGriffiths left a comment

AlanGriffiths Dec 5, 2024

Saviq Dec 5, 2024

AlanGriffiths Dec 5, 2024

tarek-y-ismail Dec 5, 2024 •

edited

Loading

tarek-y-ismail Dec 5, 2024

AlanGriffiths Dec 5, 2024

tarek-y-ismail Dec 5, 2024 •

edited

Loading

AlanGriffiths Dec 5, 2024

tarek-y-ismail Dec 5, 2024

tarek-y-ismail commented Dec 5, 2024 •

edited

Loading

Saviq commented Dec 5, 2024

tarek-y-ismail commented Dec 6, 2024

AlanGriffiths left a comment •

edited

Loading

Saviq commented Dec 6, 2024

AlanGriffiths commented Dec 6, 2024

tarek-y-ismail commented Dec 6, 2024 •

edited

Loading

AlanGriffiths left a comment

tarek-y-ismail Dec 9, 2024

tarek-y-ismail Dec 9, 2024

Add focus stealing prevention to MinimalWindowManager #3693

Are you sure you want to change the base?

Add focus stealing prevention to MinimalWindowManager #3693

Conversation

tarek-y-ismail commented Dec 4, 2024 • edited Loading

AlanGriffiths left a comment

Choose a reason for hiding this comment

AlanGriffiths left a comment

Choose a reason for hiding this comment

AlanGriffiths left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarek-y-ismail Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarek-y-ismail Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarek-y-ismail commented Dec 5, 2024 • edited Loading

Saviq commented Dec 5, 2024

tarek-y-ismail commented Dec 6, 2024

AlanGriffiths left a comment • edited Loading

Choose a reason for hiding this comment

Saviq commented Dec 6, 2024

AlanGriffiths commented Dec 6, 2024

tarek-y-ismail commented Dec 6, 2024 • edited Loading

AlanGriffiths left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add focus stealing prevention to `MinimalWindowManager` #3693

Add focus stealing prevention to `MinimalWindowManager` #3693

tarek-y-ismail commented Dec 4, 2024 •

edited

Loading

tarek-y-ismail Dec 5, 2024 •

edited

Loading

tarek-y-ismail Dec 5, 2024 •

edited

Loading

tarek-y-ismail commented Dec 5, 2024 •

edited

Loading

AlanGriffiths left a comment •

edited

Loading

tarek-y-ismail commented Dec 6, 2024 •

edited

Loading