GNOME Bugzilla – Bug 727178
wayland: Make sure we don't send compositor consumed keys on enter
Last modified: 2016-11-24 13:10:15 UTC
This seems to work. I no longer get Tabs processed in apps when Alt+Tabbing quickly (i.e. releasing Alt first - gnome-shell drops the grab - and then releasing Tab). Comments welcome.
Created attachment 273102 [details] [review] wayland-keyboard: Make update_pressed_keys() more generic It will allow us to re-use this function next. Also rename the keys array to pressed_keys since we'll need to add a different one.
Created attachment 273103 [details] [review] wayland: Make sure we don't send compositor consumed keys on enter Key presses that trigger keybindings or that happen while we have a compositor grab shouldn't be sent to clients on wl_keyboard.enter .
Created attachment 280718 [details] [review] wayland-keyboard: Make update_pressed_keys() more generic -- Rebased on master
Created attachment 280719 [details] [review] wayland: Make sure we don't send compositor consumed keys on enter -- Rebased
Review of attachment 280718 [details] [review]: OK.
Review of attachment 280719 [details] [review]: An expanded commit message would be nice, about the problem this solves and about why we prefer this solution over others (e.g. why not just pass no keys at all to "enter"). I'm not too happy with the code here. :( ::: src/wayland/meta-wayland-keyboard.c @@ +535,3 @@ + wl_array_release (&keys_tmp); + keyboard->consumed_keys.size = 0; + } This seems wrong to me if you do a sequence like: Press Alt, Press Tab, Release Tab, Press Shift, Press Tab, Release Alt That is, Alt-Tab followed by Alt-Shift-Tab, and release Alt first. Now the "Tab" is marked as consumed but we haven't switched windows. If I press Tab again and get key repeat going, then click on another window, it will ignore the Tab, since it's marked as a consumed key. Edge case, yeah, but I imagine this might come up with going to the app grid with <Super>A and then having "a" not work in an application. The only solution I can think of is to make sure that pressing a key normally wipes it out from the consumed keys. Yikes, this is a mess :(
(In reply to comment #6) > This seems wrong to me if you do a sequence like: > > Press Alt, Press Tab, Release Tab, Press Shift, Press Tab, Release Alt > > That is, Alt-Tab followed by Alt-Shift-Tab, and release Alt first. Now the > "Tab" is marked as consumed but we haven't switched windows. If I press Tab > again and get key repeat going, then click on another window, it will ignore > the Tab, since it's marked as a consumed key. > > Edge case, yeah, but I imagine this might come up with going to the app grid > with <Super>A and then having "a" not work in an application. I don't think this can ever happen. There is an implicit assumption in this code but it's one that I think holds true in every case: We only add keys to the consumed array when bypass_wayland in meta_display_handle_event() is TRUE, and for that to be true it means that we have removed keyboard focus from a wayland surface. This means that, as soon as focus a wayland surface (even if it's the same!) we'll send the enter event and at that point clear the consumed keys array. Perhaps we could instead make this more explicit (and safer?) if we only consumed keys if MetaWaylandKeyboard->focus_surface == NULL ?
Aha. Yeah, having an assert along those lines would be nice, along with a comment explaining it. It's a lot to keep in your head, as I'm sure you're well aware :) I'm still a bit worried, but as long as we document our assumptions and make sure they hold true in every case, I'll feel a lot better.
Created attachment 281104 [details] [review] wayland: Make sure we don't send compositor consumed keys on enter The wayland protocol requires us to send an array with the currently pressed keys on the wl_keyboard.enter event. One reason for that is so that clients can trigger their own key repeat loop when they're given focus while a key is already pressed. But we can't always do this, for intance, key presses that trigger keybindings or that happen while we have a compositor grab shouldn't be sent to clients on wl_keyboard.enter, otherwise clients would process events that are an implementation detail of the compositor. E.g. on a quick Alt+Tab, with Alt being released before Tab, the focused client would see Tab pressed on the enter event and thus process it. -- (In reply to comment #7) > We only add keys to the consumed array when bypass_wayland in > meta_display_handle_event() is TRUE, and for that to be true it > means that we have removed keyboard focus from a wayland > surface. This means that, as soon as focus a wayland surface (even > if it's the same!) we'll send the enter event and at that point > clear the consumed keys array. > > Perhaps we could instead make this more explicit (and safer?) if we > only consumed keys if MetaWaylandKeyboard->focus_surface == NULL ? So, I tried this and experimented several different scenarios and the above isn't totally true. bypass_wayland doesn't imply that focus_surface == NULL and we still want to consume keys even then. For instance, with two xterms open, pressing Alt+F4 results in the focused one being closed and the focus_surface immediately switching to the other one. On master, this means that the newly focused surface gets and enter event with F4 in the keys array which xwayland, in this case, translates into an F4 press event that reaches xterm. Basically, I still think that bypass_wayland gives us the right semantics here. I.e. any key presses that we process when bypass_wayland is true should not go into the next enter event. > > Edge case, yeah, but I imagine this might come up with going to > > the app grid with <Super>A and then having "a" not work in an > > application. This really can't ever happen, this patch only changes which keys are delivered in the next enter event and at that point we clear the comsumed_keys array. I added a comment to make this clear.
Comment on attachment 280718 [details] [review] wayland-keyboard: Make update_pressed_keys() more generic Just pushing this one to get it out of my local branch. I'll upload a new patch later to never send pressed keys on enter, at least for now, as discussed in GUADEC. Attachment 280718 [details] pushed as bf9fdf4 - wayland-keyboard: Make update_pressed_keys() more generic
FWIW, I've been hitting this in my wayland testing, I filed on xwayland and they sent me back to GNOME, this looks like the bug. https://bugs.freedesktop.org/show_bug.cgi?id=81769 "Running Fedora 21 with Wayland 1.5.0 and xorg-x11-server-Xwayland-1.15.99.904-4.fc21.x86_64 . If I activate a Firefox window and then alt-tab out of it, it seems like the alt keypress itself is also interpreted - Firefox shows or hides its menu bar, like it would if I just pressed <alt> (in any Firefox release with the hidden menu bar). Something also seems to be sent to the window I switched to - if it's a GNOME terminal, I sometimes see an error "bash: words: bad array subscript" to reproduce, just log into a Shell-on-Wayland session in a current Fedora 21 and try alt-tabbing between a Firefox window and a GNOME terminal window, at least that dependably reproduces the bug for me."
*** Bug 734232 has been marked as a duplicate of this bug. ***
Created attachment 287981 [details] [review] wayland-keyboard: Don't send pressed keys on enter We never want to send pressed keys to wayland clients on enter. The protocol says that we should send them, presumably so that clients can trigger their own key repeat routine in case they are given focus and a key is physically pressed. Unfortunately this causes some clients, in particular Xwayland, to register key events that they really shouldn't handle, e.g. on an Alt+Tab keybinding, where Alt is released before Tab, clients would see Tab being pressed on enter followed by a key release event for Tab, meaning that Tab would be processed by the client when it really shouldn't. Since the use case for the pressed keys array on enter seems weak to us, we'll just fake that there are no pressed keys instead which should still be spec compliant even though it's not true. -- Jasper, does that sound good?
Review of attachment 287981 [details] [review]: I'm fine with this.
Attachment 287981 [details] pushed as c39f18c - wayland-keyboard: Don't send pressed keys on enter
*** Bug 738634 has been marked as a duplicate of this bug. ***
While keys are no longer inserted when I switch between desktops (which is great) I have a new problem. If I launch gnome-terminal I used to be able to hit cntl-shift-n n n n and get a new terminal every single time I hit n (while holding cntl-alt the whole time) That no longer works. Now subsequent "n" keys are inserted as text into the new terminal window. So to launch a new terminal window I have to hit all 3 keys over and over and over.
(In reply to eparis from comment #17) > While keys are no longer inserted when I switch between desktops (which is > great) I have a new problem. If I launch gnome-terminal I used to be able > to hit cntl-shift-n n n n and get a new terminal every single time I hit n > (while holding cntl-alt the whole time) > > That no longer works. Now subsequent "n" keys are inserted as text into the > new terminal window. So to launch a new terminal window I have to hit all 3 > keys over and over and over. I believe this works correctly now.
Hi, I am still seeing the issue that Adam Williamson pointed out with Firefox showing its hidden menu every time I alt-tab to a different window. Was this fix supposed to address that? I'm running an up-to-date Arch Linux system (mutter 3.22.2+1+g5c46094-1, wayland 1.12.0-1, xorg-server-xwayland 1.18.4-1).
(In reply to Damien Grassart from comment #19) > Hi, I am still seeing the issue that Adam Williamson pointed out with > Firefox showing its hidden menu every time I alt-tab to a different window. > Was this fix supposed to address that? I'm running an up-to-date Arch Linux > system (mutter 3.22.2+1+g5c46094-1, wayland 1.12.0-1, xorg-server-xwayland > 1.18.4-1). That's a different issue than the one this bug was about. Incidentally I made a fix for that a couple of days ago but it's an xwayland fix which is still waiting for review: https://patchwork.freedesktop.org/patch/123072/