Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement focus stealing prevention #2586

Open
Saviq opened this issue Aug 25, 2022 · 18 comments · May be fixed by #3693
Open

Implement focus stealing prevention #2586

Saviq opened this issue Aug 25, 2022 · 18 comments · May be fixed by #3693
Labels
enhancement triaged Triage into JIRA to plan it in

Comments

@Saviq
Copy link
Collaborator

Saviq commented Aug 25, 2022

I'm running Thunderbird under Miriway (same issue with notifications from websites in Firefox). The notification surfaces (part of the problem is lack of a notification manager) show in random locations on the screen, incorrectly cropped and steal focus.

There may be nothing that Mir can do visually, if those are simply top-level surfaces - but focus stealing prevention is something we need to do. May need a separate issue?

Using this Miriway config:

x11-window-title=Miriway
idle-timeout=180
ctrl-alt=t:miriway-terminal # Default "terminal emulator finder"

enable-x11=true

shell-component=swaybg -i /usr/share/backgrounds/warty-final-ubuntu.png
shell-component=mate-panel
meta=a:mate-panel --run-dialog
ctrl-alt=t:mate-terminal
@Saviq Saviq added the bug label Aug 25, 2022
@wmww
Copy link
Contributor

wmww commented Aug 25, 2022

We might be able to do something to prevent focus stealing, but fwiw the problem exists in sway as well. Easiest solution is probably to run mate-notification-daemon, which I ported to layer shell a while back.

@Saviq
Copy link
Collaborator Author

Saviq commented Aug 26, 2022

Easiest solution is probably to run mate-notification-daemon, which I ported to layer shell a while back.

Looks like it wants to be DBus-activated, as it silently exits a few seconds after starting...

@Saviq
Copy link
Collaborator Author

Saviq commented Aug 26, 2022

Looks like it wants to be DBus-activated, as it silently exits a few seconds after starting...

Hmm, it even is DBus-activated I think. Still shows up in the middle of the screen, though.
frame_2022-08-26T10:51+02:00

@AlanGriffiths
Copy link
Collaborator

AlanGriffiths commented Aug 26, 2022

Hmm, it even is DBus-activated I think. Still shows up in the middle of the screen, though.

It won't have access to zwlr_layer_shell unless started from miriway-shell, so try enabling add-wayland-extensions=all and see if that works

@Saviq Saviq changed the title Notification surfaces show up cropped, randomly placed and steal input Implement focus stealing prevention Aug 26, 2022
@Saviq Saviq added enhancement and removed bug labels Aug 26, 2022
@Saviq
Copy link
Collaborator Author

Saviq commented Aug 26, 2022

It won't have access to zwlr_layer_shell unless started from miriway-shell, so try enabling add-wayland-extensions=all and see if that works

Bingo!

I've modified the bug to talk about focus stealing prevention only.

@AlanGriffiths
Copy link
Collaborator

Bingo!

Well, assuming we don't want to allow zwlr_layer_shell et alias to all applications Miriway needs a way to "whitelist" such services. (Might nor might not be a Mir issue too)

@tarek-y-ismail
Copy link
Contributor

tarek-y-ismail commented Nov 28, 2024

Quick reformulation of the issue: As we have xdg-activation implemented now, there's no need to automatically focus and bring new windows to the front. xdg-activation can then raise and focus windows when it's appropriate.

I've tracked what's needed down to two places, these may not be the proper places to modify, but are a start:

  1. MinimalWindowManager::handle_window_ready. This part is responsible for focusing new windows.

    void miral::MinimalWindowManager::handle_window_ready(WindowInfo& window_info)
    {
    if (window_info.can_be_active())
    {
    tools.select_active_window(window_info.window());
    }
    }

    Should never enter this if condition unless there are no other windows.

  2. SurfaceStack::insert_surface_at_top_of_depth_layer. This is responsible for the placement of a window within a depth layer.

    void ms::SurfaceStack::insert_surface_at_top_of_depth_layer(std::shared_ptr<Surface> const& surface)
    {
    unsigned int depth_index = mir_depth_layer_get_index(surface->depth_layer());
    if (surface_layers.size() <= depth_index)
    surface_layers.resize(depth_index + 1);
    surface_layers[depth_index].push_back(surface);
    }

    Should only insert at the end for the first window, all subsequent surfaces should be behind it.

Modifying these two pieces of code to not automatically focus and push new windows to the front should allow xdg-activation to do so when it's appropriate.

What's missing: Creating and passing some sort of strategy down to control the behavior depending on whether xdg-activation is available or not. One approach could be using mir::scene::Surface to carry this information since it can be accessed through (1), and can be exposed via WindowInfo -> WindowInfo::Self -> Window -> Window::operator std::shared_ptr<mir::scene::Surface>() in (2) (though I think we don't have direct access to the surface in this case for a reason)

As for X11 applications, those are another can of worms. Now we need the logic to also depend on the app type (wayland vs X), and figure out how XWayland handles this.

@tarek-y-ismail
Copy link
Contributor

One thing I'm realizing re-reading xdg-activation-v1 is that it only applies to applications launched from other applications. So at the moment with the changes mentioned applying to all applications, applications launched through shortcuts will not start focused (can be worked around)

@Saviq
Copy link
Collaborator Author

Saviq commented Nov 29, 2024

Opening links is launching (or activating, if already running) applications from other applications. That just requires things to work together and pass the token further.

This was actually the use case Matt implemented activation for.

@tarek-y-ismail
Copy link
Contributor

Oh I think I should've elaborated more. I didn't mean URLs when I mentioned shortcuts. I meant Mir's shortcuts (CTRL+ALT+T for example)

@AlanGriffiths
Copy link
Collaborator

Oh I think I should've elaborated more. I didn't mean URLs when I mentioned shortcuts. I meant Mir's shortcuts (CTRL+ALT+T for example)

Yes, we need to update our launch code to supply a token we respect

@tarek-y-ismail
Copy link
Contributor

Some things that broke with the changes above:

  1. Since window offsets are based off the currently open window, only the second window is offset, and all windows launched after the second window are positioned at the exact same position.
  2. Xterm (probably all server-decorated apps) are borked
    20241129_14h43m57s_grim

@tarek-y-ismail
Copy link
Contributor

tarek-y-ismail commented Nov 29, 2024

2. SurfaceStack::insert_surface_at_top_of_depth_layer. This is responsible for the placement of a window within a depth layer.

void ms::SurfaceStack::insert_surface_at_top_of_depth_layer(std::shared_ptr<Surface> const& surface)
{
unsigned int depth_index = mir_depth_layer_get_index(surface->depth_layer());
if (surface_layers.size() <= depth_index)
surface_layers.resize(depth_index + 1);
surface_layers[depth_index].push_back(surface);
}

Played around with it for a bit, this is not the way.

Edit: Crisis averted. This modification doesn't break literally everything using the stack now

@@ -106,7 +106,7 @@ auto ms::ApplicationSession::create_surface(
     auto surface = surface_factory->create_surface(session, wayland_surface, streams, params);
 
     auto const input_mode = params.input_mode.is_set() ? params.input_mode.value() : input::InputReceptionMode::normal;
-    surface_stack->add_surface(surface, input_mode);
+    surface_stack->add_surface_below_top(surface, input_mode);
 
     if (observer && observer_executor)
     {

@tarek-y-ismail
Copy link
Contributor

tarek-y-ismail commented Dec 2, 2024

Spent a fair bit of time trying to figure out a way to propagate the information that xdg_activation_v1 (or really, whatever extension needs to be checked) down to BasicWindow so it can be used in handle_window_ready. This was done through exposing Server::is_wayland_extension_enabled, and adding miral::ExtensionLookup so it can be used in miral.

At the moment, I'm a bit lost how to do this with ApplicationSession::create_surface (check previous comment). The most obvious way after a quick glance of the stack trace and code would be to pass the info through SessionManager's constructor, then through ApplicationSession's constructor, then using it in create_surface.

Anyway, once this is done, we'll need to cover two more things (in addition to the stuff mentioned a couple of comments ago):

  1. Automatically focusing applications launched through Mir. I took a quick look at this and the only piece of information we get regarding those is the PID of the application. Not sure if this is very useful.
  2. Xwayland applications: No clear idea what needs to be done.

@AlanGriffiths
Copy link
Collaborator

Spent a fair bit of time trying to figure out a way to propagate the information that xdg_activation_v1 (or really, whatever extension needs to be checked) down to BasicWindow so it can be used in handle_window_ready. This was done through exposing Server::is_wayland_extension_enabled, and adding miral::ExtensionLookup so it can be used in miral.

I think this approach is problematic: We have APIs that allow shells fine grained control over enabling extensions (c.f. miral::WaylandExtensions::conditionally_enable(), so there is no way to implement Server::is_wayland_extension_enabled et alia to produce reliable results. Also, as you have noted, it requires significant work to propagate the results to where they would be useful.

I would reverse the dependency: if "focus stealing prevention" is enabled in miral, then force xdg_activation_v1to be enabled.

@AlanGriffiths
Copy link
Collaborator

I would reverse the dependency: if "focus stealing prevention" is enabled in miral, then force xdg_activation_v1to be enabled.

Or even, just allow the options to be independent: even if not the desired end state, that makes experimentation easier

@Saviq
Copy link
Collaborator Author

Saviq commented Dec 3, 2024

allow the options to be independent

That may well be the best — --require-activation or so, and no connection to the activation protocol?

@tarek-y-ismail
Copy link
Contributor

Spent some time passing things (ExtensionLookup) to ApplicationSession so surfaces can be inserted at the proper place in the stack. I also spent some time trying out different applications just to have an idea which use xdg-activation-v1.

After that, Alan, Sawicz, and I spent some time discussing whether I was going the right way and using Mir's existing customization options properly or not (hint: I was not). So, seeing how alt+tab did something similar to what we did, we agreed that this was a good place to start and figure out if FloatingWindowManagerPolicy needed any changes to expose that so it can be customized in it. The next step would've been to then add a new constructor for FloatingWindowManagerPolicy to toggle between the old and new logic.

I spent some time reading through c2553c5 to get a basic idea how alt + tab was implemented, figured that WindowManagerTools::swap_tree_order was the missing piece, and figured out what to do to disable auto raising (just swap the new window on top with the already existing one) + focusing (at least for all applications after the first, makes it easier to test :)). Was trolled a bit by kgx actually raising windows because I forgot to disable xdg-activation :P

As of right now, this is done with a few lines in FloatingWindowManager, but I believe that I discovered a bug? where the next window in the stack is not focused whenever we close the one above it. I'm not sure of the root cause at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement triaged Triage into JIRA to plan it in
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants