riverwm
Xwayland: keep previous view activated when focusing Xwayland OR surfaces
I have looked into https://github.com/riverwm/river/issues/620 again and more deeply, and I believe I have found a final solution. I feel, however, that it can be improved because it is not in keeping with river's normal behaviour. Specifically, two views can be activated at the same time: If an Xwayland override redirect surface takes focus and a normal view was in focus previously, the previous view's focus will not be removed; the focus for this view is only reset if a new view takes focus over the override redirect surface. This ensures that when an Xwayland popup menu (or similar) is created, its parent Xwayland view is still activated, allowing the menu to continue its operation. (Perhaps deactivating the parent view implies that the view's popup menu should no longer be shown?) Please note that this applies to any previously focused view, not only to an override redirect surface's parent view. For example, if dmenu is run while a foot window is currently focused, the foot window will still be activated (with keyboard input given to dmenu) until focus is set to another view (possibly the original foot window). I implemented this generalised case for simplicity, as opposed to checking if the currently focused view is the parent of the newly focused override redirect surface.
My solution was inspired by sway's handling of focus, where the previous focus is not unset if the seat's focus is set directly to a surface (see https://github.com/swaywm/sway/blob/master/sway/input/seat.c#L1300). This was the main difference in logic I found between sway and river which seemed to explain the disparity in Xwayland popup menu behaviour.
From my testing, it should also no longer be necessary to check for an override redirect surface's ICCCM input model, but I have left the checks as they do not appear to impact the required popup menu functionality. (Some popup menus also passed the checks when they likely should not have, such as those of qt6ct---mentioned by @Bonicgamer in the linked issue---which passed the overrideRedirectWantsFocus() heuristic and has a local input model; and of winecfg, which also passed the heuristic but has a global input model. This inconsistency in advertised window type and input model properties made me look more closely at sway's codebase for a possible solution.)
If this patch could be tested to make sure no further bugs are introduced (such as from having two views activated simultaneously) and Xwayland menus are working appropriately, that would be much appreciated.
I really don't want to maintain Xwayland specific hacks in non-Xwayland-specific codepaths like this. Is there any way to keep this fix in Xwayland-specific codepaths, even if it's even hackier?
It almost seems like things were more reliable before we started using the overrideRedirectWantsFocus() heuristic in the first place. I wonder if we could stop using that and rely on the ICCCM input model?
Is there any way to keep this fix in Xwayland-specific codepaths, even if it's even hackier?
I think it might be possible. This is my patch for the change: or_focus_patch.txt. I have attempted to move the functionality into XwaylandOverrideRedirect, but because management of the previously focused view is not in Seat anymore, the view is unfocused when the OR surface is unmapped instead of when there is any change in focus. I feel that this patch still introduces a maintenance burden. Do you think it is better at all?
I also noticed another unrelated issue while working on this (not in the patch), but I am not sure if it is intended:
- Passing an Xwayland OR surface into
View.fromWlrSurface()crashes river because the surface has no associated view and the@intToPtr(*Self, xwayland_surface.data)cast evaluates to null.
It almost seems like things were more reliable before we started using the
overrideRedirectWantsFocus()heuristic in the first place. I wonder if we could stop using that and rely on the ICCCM input model?
I am not sure if this is possible. Because OR clients set their window type and input model so inconsistently, it is difficult for river to reliably determine whether they should actually receive focus.
To my knowledge, there are two ways to solve the original issue of disappearing menus:
- Do not set focus on the menu. This was done prior to my implementing OR surface focus handling.
- Keep the previously focused view activated while the menu is mapped. (Keeping only the parent view focused did not fix the menus in VS Code and winecfg, but implementing the more general case with any previous view did. I am currently unsure as to why.)
Regarding the ICCCM input model, IDEA's menus are the only ones I know that have a 'none' input model. Regarding the heuristic, the issue appears to be with clients, not river or the heuristic itself. Clients should set their window type appropriately (see the X11 EWMH specification here) and based on the heuristic, surfaces with a menu window type should not be given focus (see the NET_WM_WINDOW_TYPE_*MENU atoms used here). So using the heuristic alone should work for handling menu focus, but it appears that some clients do not set their window type correctly.
For reference, a table overview of the (Xwayland) applications I have tested is shown below. Qt5 and GTK applications are the most consistent in that their menus do not 'want focus' (by the heuristic) and have a local input model. The issue is primarily with qt6ct and wine applications, which both 'want focus' but have local and global input models, respectively.
| Application (menus) | Wants focus? | ICCCM input model | Works in river |
|---|---|---|---|
| IntelliJ IDEA | T | None | Y |
| Firefox | F | Local | Y |
| VS Code | F | Passive | Y |
| qt6ct | T | Local | N |
| qt5ct | F | Local | Y |
| qpwgraph | F | Local | Y |
| KWrite | F | Local | Y |
| dolphin-emu | F | Local | Y |
| fcitx5-config-qt | F | Local | Y |
| Inkscape | F | Local | Y |
| Utsushi | F | Local | Y |
| pavucontrol | F | Local | Y |
| winecfg | T | Global | N |
| Kero Blaster (wine) | T | Global | N |
I am not sure how focus can be handled for OR surfaces when a menu could have any of the four input models (also, dmenu is passive and rofi is local and both of these should be given focus), which is why I looked into keeping the previously focused view activated instead. What are your thoughts on this?
I think it might be possible. This is my patch for the change: or_focus_patch.txt. I have attempted to move the functionality into XwaylandOverrideRedirect, but because management of the previously focused view is not in Seat anymore, the view is unfocused when the OR surface is unmapped instead of when there is any change in focus. I feel that this patch still introduces a maintenance burden. Do you think it is better at all?
I think this approach does have the potential to be better, though I'm still not a fan of the fact that native Wayland views would remain activated despite not receiving keyboard input. Perhaps we could do this only for Xwayland views?
- Passing an Xwayland OR surface into
View.fromWlrSurface()crashes river because the surface has no associated view and the@intToPtr(*Self, xwayland_surface.data)cast evaluates to null.
Great catch! Just pushed db366e9455d73a026e321966c07beeb525c4584a to fix that, thanks.
I am not sure if this is possible. Because OR clients set their window type and input model so inconsistently, it is difficult for river to reliably determine whether they should actually receive focus.
Man, this issue really does a great job of demonstrating what's wrong with X11 :/ EMWH support is not actually required of clients, it's just something they SHOULD support.
- Keep the previously focused view activated while the menu is mapped. (Keeping only the parent view focused did not fix the menus in VS Code and winecfg, but implementing the more general case with any previous view did. I am currently unsure as to why.)
Hmm, how were you determining the parent view here? wlr.XwaylandSurface.parent? I'm guessing that X11 clients probably aren't terribly consistent at setting that either :/
I am not sure how focus can be handled for OR surfaces when a menu could have any of the four input models (also, dmenu is passive and rofi is local and both of these should be given focus), which is why I looked into keeping the previously focused view activated instead. What are your thoughts on this?
Yeah, that table makes me think that a purely heuristics based approach is sadly not possible because X11 clients are too inconsistent with how they do things. (This is why Wayland is about policy over mechanism.)
I'm happy to go forward with a solution based on keeping the previously focused view activated, thanks for taking the time to make that table!
I think this approach does have the potential to be better, though I'm still not a fan of the fact that native Wayland views would remain activated despite not receiving keyboard input. Perhaps we could do this only for Xwayland views?
Apologies for the long wait. I have modified the patch, attached here. It now only maintains focus on a previously focused Xwayland view instead of any previous view. Additionally, to resolve the issue where an Xwayland view with a menu is still activated after focusing a normal Wayland view, I introduced a function call to XwaylandOverrideRedirect.handleClearFocus() in Seat. Is this solution feasible, or is it too intrusive on Seat's functionality?
Despite the patch, there is at least one OR menu to my knowledge which remains mapped after its parent view is deactivated: the right-click menu in winecfg. I am unsure as to why. As a side note, winecfg's dropdown menus in contrast do not appear to be OR surfaces at all (this may have been the issue with my wlr.XwaylandSurface.parent check, which was only in an OR code path), despite being affected by my changes in XwaylandOverrideRedirect's focus behaviour. The application's functionality does work, however, and the winecfg window itself is deactivated when focusing a normal Wayland view (or an Xwayland one), so I do not think this is an issue in terms of usability.
Do you think there are any further improvements I can make with regard to the patch? Also, should I push the patch onto this PR (and overwrite the old one), or should I open a new PR for it?
Apologies for the long wait.
No worries at all! This is free and open source software, you've got no obligation to do this work in the first place though I certainly appreciate it.
I have modified the patch, attached here. It now only maintains focus on a previously focused Xwayland view instead of any previous view. Additionally, to resolve the issue where an Xwayland view with a menu is still activated after focusing a normal Wayland view, I introduced a function call to
XwaylandOverrideRedirect.handleClearFocus()inSeat. Is this solution feasible, or is it too intrusive onSeat's functionality?
That patch looks quite clean, I'd be fine merging that or something similar.
Despite the patch, there is at least one OR menu to my knowledge which remains mapped after its parent view is deactivated: the right-click menu in winecfg. I am unsure as to why. As a side note, winecfg's dropdown menus in contrast do not appear to be OR surfaces at all (this may have been the issue with my wlr.XwaylandSurface.parent check, which was only in an OR code path), despite being affected by my changes in XwaylandOverrideRedirect's focus behaviour. The application's functionality does work, however, and the winecfg window itself is deactivated when focusing a normal Wayland view (or an Xwayland one), so I do not think this is an issue in terms of usability.
Interesting, that still sounds like a large improvement over the status quo in general though. I think there will likely always be some strangely behaved enough X11 client that will fail to work portably across X11 server/window manger implementations, this was already an issue before throwing Xwayland into the mix.
Do you think there are any further improvements I can make with regard to the patch? Also, should I push the patch onto this PR (and overwrite the old one), or should I open a new PR for it?
I'll give it a more in depth read and testing when I find time, force pushing it to this PR branch is fine.
I have found that nested menus are still broken with this approach. This is because, upon mapping a submenu, the previous menu loses focus and deactivates the parent Xwayland view, which results in the menus being unmapped. If this is avoided (by ensuring the submenu increments the Xwayland view's pending focus), then the submenu functions correctly.
However, issues occur when the submenu is closed. Either the entire hierarchy of menus is also closed (as a result of the submenu deactivating the parent Xwayland view), or the previous menus no longer take focus and the parent Xwayland view's pending focus is not reset. These issues stem from the need to deactivate the Xwayland view and its menus when transferring focus to another application (see the handleClearFocus() function used before).
The latest commit on this branch has working nested menus (tested in Reaper DAW and Kero Blaster via Wine). To do this, it omits the deactivation of the parent Xwayland view until the menus themselves are unmapped (previously, this would occur on each menu surface losing focus). This introduces the issue of the Xwayland view and its menus remaining activated and mapped when focus has moved to another application, which I am currently unsure how to solve (barring implementing this logic in Seat).
If you need any help with testing patch(es) for this, feel free to ping me. This has been bugging me for a long time in my browser (Vivaldi).
This introduces the issue of the Xwayland view and its menus remaining activated and mapped when focus has moved to another application, which I am currently unsure how to solve (barring implementing this logic in Seat).
I'm OK with adding some more Xwayland-specific code to Seat if necessary, I only ask that it be cleanly separated from the rest of the code and not be run when the xwayland build option is disabled.
Thanks for your continued work on this issue and sorry I can't be of more help on the X11 side of things.
I'm OK with adding some more Xwayland-specific code to
Seatif necessary, I only ask that it be cleanly separated from the rest of the code and not be run when the xwayland build option is disabled.
This patch is my best attempt thus far; menus function correctly in my testing, and the special case behaviour is restricted to Xwayland, although I think the code could be organised more cleanly.
@icp1994 Thank you very much for the offer! Could you check the patch I linked above and see if it fixes your issue?
Unfortunately, it's still happening. Here's a minimal river.log
Thank you for testing! It is much appreciated. However, I am unable to determine the issue from that log file. Could you try this patch instead, please? (It includes some additional logging.)
Could you also test this patch? It should have the same result, but it would be helpful to see if it works as a point of reference.
Still can repro, hope these are helpful: river_patch_v2.log river_patch_v3.log
I exited river as soon as I could get the drop-downs/right-clicks to misbehave so anything "bad" should be towards the end of the logs.
Still can repro, hope these are helpful: river_patch_v2.log river_patch_v3.log
I exited river as soon as I could get the drop-downs/right-clicks to misbehave so anything "bad" should be towards the end of the logs.
As far as I can tell, focus is not given to any override redirect windows based on those log files; the involved menus already appear to be filtered out by the heuristic or ICCCM input model that were implemented before. I have also downloaded Vivaldi locally, and the browser's right click and drop-down menus (specifically, the menus from right clicking the page and clicking the Vivaldi logo in the tab bar) function correctly on my end. Yours may be a separate issue. Does the issue occur only with specific menus in Vivaldi or all menus?
It doesn't happen immediately when I start the browser. It starts to happen after some browsing/scrolling around - the waiting time varies. Then I have to restart the browser and it goes away temporarily for a while and the cycle repeats.
It doesn't happen immediately when I start the browser. It starts to happen after some browsing/scrolling around - the waiting time varies. Then I have to restart the browser and it goes away temporarily for a while and the cycle repeats.
I have reproduced your issue consistently with the following actions:
- Send Vivaldi to fullscreen.
- Send Vivaldi out of fullscreen.
- Open a menu. (Note that giving it cursor focus will close it.)
- Close the menu.
- Any menus opened hereafter will disappear immediately, unless steps 1 and 2 are repeated.
Nevertheless, I am convinced that this is a separate problem. With some limited debugging, I found that Vivaldi's menus do not want focus (based on the heuristic used), and they advertise a passive ICCCM input model, akin to VS Code's menus in my table at the top of this PR. As such, Vivaldi's menus do not explicitly set focus, so I am unsure of the cause of this problem. It may be useful to open a new issue for this. Do the steps outlined above also reproduce the error on your end?
Note: I can also reproduce it in sway, so it is possible that it is an issue with Vivaldi and/or Xwayland as opposed to river.
@zakvf I think I like the look of the 3rd patch linked which adds the activated_xwayland_view field to Seat the most. Putting the field there rather than in XwaylandOverrideRedirect makes more sense even if it is a bit ugly to add another bit of Xwayland-specific state.
Do you feel like that patch is near ready to merge? I have few small code suggestions that would be easier to make if it was pushed to this branch so I could use the Github code review features.
As for the Vivaldi issue, I'd be happy to leave that unsolved in this PR. This work has been sitting here unmerged long enough and looks to be a substantial improvement over what we have on master branch.
@zakvf I think I like the look of the 3rd patch linked which adds the
activated_xwayland_viewfield toSeatthe most. [...] Do you feel like that patch is near ready to merge?
The branch is now updated with the patch (and an additional comment) and is ready for review.
Thanks for making those changes. While testing this I noticed that riverctl focus-view next/previous didn't work as expected while a dropdown menu using these new semantics was open. While investigating a fix for that, I tried out a new approach which seems a bit cleaner to me. What do you think of this commit? https://github.com/riverwm/river/commit/250e496274c9757bf60809477cc859bea4e6c28e.
Pushed on top of your commit to this branch https://github.com/riverwm/river/tree/xwayland-or-focus, diffing against master may be cleaner to see what's going on.
That looks to be a better solution than mine, and everything works on my end. It also retains the application's title when queried via focused_view in the river-status protocol, which would otherwise become null when focusing an OR menu. Thank you for having a look!