GNOME Bugzilla – Bug 791383
[Wayland] Discrepancy between layout indicator and actual layout
Last modified: 2017-12-20 09:09:54 UTC
Description: gnome-shell layout indicator shows the wrong keymap at startup and fails to switch keyboard layouts in gnome-shell UI. How reproducible: Always Steps to reproduce: 1. Configure a user account with a locale and a keyboard layout different from the default locale layout (like locale English and layout French) - Set "Language" to "English UK" - Set "Input source "to "French, English (UK)" 2. log out, log back (Wayland) 3. Notice the layout indication states "en" 4. Switch to the "overview" view in gnome-shell, type some text in the search field, notice the "en" layout is used (“qwerty”) <- expected 5. Open an xterm (Xwayland application), type some text, the French layout is used (“azerty”)even though the indicator states "en" <- broken 6. Move the xterm window, type again, now the layout has switch to the "en" layout (“qwerty”) <- unexpected 7. Switch to the "overview" again, type some text in the search bar, type some text, now the French layout is used (“azerty”) even though the indicator still states "en" <- broken 8. Try to switch layouts usign the indicator menu, no effect, the layout remains on the French layout (“azerty”) no matter what. <- broken Additional info: This is a serious issue because all passwords typed in gnopme-shell are impacted by this, like typing the VPN password or even unlocking the screen (and because passwords are hidden, it's impossible to tell whether or not the layout matches the indicator!) Additional info: Looks like there are a lot of those issues being reported for quite some time, not sure if those are exactly the same, possibly related... https://bugzilla.gnome.org/show_bug.cgi?id=789794, https://bugzilla.gnome.org/show_bug.cgi?id=789761, https://bugzilla.gnome.org/show_bug.cgi?id=789300#c11 https://bugzilla.gnome.org/show_bug.cgi?id=780039 https://bugzilla.gnome.org/show_bug.cgi?id=770529 Maybe steps #5 and '6 (moving the Xwayland window to switch layouts) is an Xwayland issue, I need to check, but the rest really looks like a gnome-shell issue.
Created attachment 365231 [details] Video demonstrating the issue with the wrong layout indicator. I understand the description in comment #0 might sound confusing, a screencast of the issue might help here; In this video, I always press the key “q” on my UK keyboard (“qwerty”) which maps to “a” on a French keyboard (“azerty”)
Created attachment 365232 [details] Video demonstrating the issue with the layout indicator doing nothing Again, here, I always press the same key “q” on the “qwerty” keyboard, and it always maps to “a” no matter what is set with the layout indicator.
Adding traces in mutter and gnome-shell shows that gnome-shell does invoke the correct group index changes and mutter's meta_backend_native_lock_layout_group() in invoked. Also, considering this issue does not happen in Xorg, I reckon the issue is rather in mutter's Wayland backend.
Small update, some headache... - keymap layouts is “fr,gb,dk,gb” - gnome-shell keyboard layout indicator shows "en". - switch VT, and back - gnome-shell keyboard layout indicator shows "en". - layout is actually "fr" -> I suspect because the group is set back to 0 (group1, i.e. “fr”) when updating the state (after taking keymap, on keyboard enable).
Actually, I see 3 bugs: - gnome-shell UI doesn't reflect changes in xkb_state resulting from VT switches (which recreate resources and set the layout index to 0, if I am not mistaken) - That could be fixed by updating the UI on the appropriate signal ("keymap-changed" on the backend?) - Changing layouts in gnome-shell UI (either using the panel indicator or the keyuboard shortcut) has no effect whatsoever on gnome-shell UI itself (which is very problematic with passwords!) - That one is puzzling me because I fail to root cause it - attachment 365232 [details] - At first start, first Xwayland window mapped have the wrong layout, until the window is moved! - attachment 365231 [details]
(In reply to Olivier Fourdan from comment #5) > - At first start, first Xwayland window mapped have the wrong layout, until > the window is moved! - attachment 365231 [details] The following patch sent to xorg-devel mailing list fixes that particular issue: https://patchwork.freedesktop.org/series/35418/
(In reply to Olivier Fourdan from comment #6) > The following patch sent to xorg-devel mailing list fixes that particular > issue: > > https://patchwork.freedesktop.org/series/35418/ The fix has landed in Xwayland, so the Xwayland issue is now sorted. For the mutter/gnome-shell xkb layout discrepancies and layout reset on VT switch, I'll attach a couple of patches for mutter that fix the issue for me.
Created attachment 365748 [details] [review] [PATCH 1/2] clutter/evdev: Save effective xkb layout with seat On VT switch, the xkd state is lost and reset to the first group, so if the first layout is not the one being used, the xkb state used in both meta-wayland-keyboard.c and clutter/evdev will be desynchronized with the keyboard source indicator in the UI. Keep the effective layout choosen along with the seat so it can be restored when reclaiming devices.
Created attachment 365749 [details] [review] [PATCH 2/2] wayland/keyboard: restore effective layout Use the effective layout from the clutter/evdev's seat to restore the layout in meta-wayland-keyboard, so that switching VTs doesn't reset the layout and causes further discrepancies with the layout indicator in the gnome-shell UI.
Review of attachment 365748 [details] [review]: Shouldn't you also restore the index in clutter_evdev_update_xkb_state()? It still resets it to 0 now. ::: clutter/clutter/evdev/clutter-device-manager-evdev.c @@ +2466,3 @@ xkb_mod_mask_t locked_mods; struct xkb_state *state; + GSList *iter; s/iter/l/ @@ +2480,3 @@ + for (iter = manager_evdev->priv->seats; iter; iter = iter->next) + { + ClutterSeatEvdev *seat = iter->data; style nit: empty line after declarations
Review of attachment 365749 [details] [review]: ::: src/wayland/meta-wayland-keyboard.c @@ +509,3 @@ MetaWaylandXkbInfo *xkb_info = &keyboard->xkb_info; xkb_mod_mask_t latched, locked; + xkb_layout_index_t layout_idx = 0; nit: it's unconditionally initialized later so need to set it to something arbitrary here @@ +527,2 @@ xkb_info->state = xkb_state_new (xkb_info->keymap); + xkb_state_update_mask (xkb_info->state, 0, latched, locked, 0, 0, layout_idx); Have you checked whether this reintroduces the bug fixed in 08e6aaa95382c0b9c99e3916c67ca5b20b42def6 ?
(In reply to Jonas Ådahl from comment #10) > Review of attachment 365748 [details] [review] [review]: > > Shouldn't you also restore the index in clutter_evdev_update_xkb_state()? It > still resets it to 0 now. But it does restore the index in clutter_evdev_update_xkb_state(), that's the first line in the patch: --- a/clutter/clutter/evdev/clutter-device-manager-evdev.c +++ a/clutter/clutter/evdev/clutter-device-manager-evdev.c @@ -2317,7 +2317,7 @@ clutter_evdev_update_xkb_state (ClutterDeviceManagerEvdev *manager_evdev) 0, /* depressed */ latched_mods, locked_mods, - 0, 0, 0); + 0, 0, seat->effective_layout);
(In reply to Jonas Ådahl from comment #11) > Have you checked whether this reintroduces the bug fixed in > 08e6aaa95382c0b9c99e3916c67ca5b20b42def6 ? I don't know how to reproduce bug 789300 fixed with commit 08e6aaa9, but from what I understand this is resulting from a discrepancy of xkb_states between the Wayland and the clutter/evdev backend. commit 08e6aaa9 fixed that discrepancy by using an index of 0 to mimic what clutter/evedv does, but while this works for bug 789300, it doesn;t solve the problem of this bug here where switching VT changes the layout index to 0 in the back of the UI. All this is "as far as I understand" and might be complete bollocks, of course... :)
I mean, these two patches here /should/ not reintroduces bug 789300 because the layout index of the xkb_state for both the Wayland and clutter/evedv backends are changed the same, so the discrepancy should not be reintroduced.
Created attachment 365781 [details] [review] [PATCH 1/2] clutter/evdev: Save effective xkb layout with seat Updated patch after review in comment 10 Additional change: renamed “effective_layout” to “layout_idx” for clarity.
Created attachment 365782 [details] [review] [PATCH 2/2] wayland/keyboard: restore effective layout Updated patch after review in comment 11
(In reply to Olivier Fourdan from comment #12) > (In reply to Jonas Ådahl from comment #10) > > Review of attachment 365748 [details] [review] [review] [review]: > > > > Shouldn't you also restore the index in clutter_evdev_update_xkb_state()? It > > still resets it to 0 now. > > But it does restore the index in clutter_evdev_update_xkb_state(), that's > the first line in the patch: Not sure how I managed to miss that :P (In reply to Olivier Fourdan from comment #13) > (In reply to Jonas Ådahl from comment #11) > > Have you checked whether this reintroduces the bug fixed in > > 08e6aaa95382c0b9c99e3916c67ca5b20b42def6 ? > > I don't know how to reproduce bug 789300 fixed with commit 08e6aaa9, but > from what I understand this is resulting from a discrepancy of xkb_states > between the Wayland and the clutter/evdev backend. > > commit 08e6aaa9 fixed that discrepancy by using an index of 0 to mimic what > clutter/evedv does, but while this works for bug 789300, it doesn;t solve > the problem of this bug here where switching VT changes the layout index to > 0 in the back of the UI. > > All this is "as far as I understand" and might be complete bollocks, of > course... :) Makes sense though. FWIW, I think (why didn't I write it down in the bug?!?) I reproduced the issue by having multiple latin based layouts and switching to the index != 0 one, then VT switching back and forth.. or something. It seems like the first patch might cause a regression there then, as the, states may get desynchronized again, and the second patch will fix the regression. Maybe these two patches should be squashed so that no one accidentally applies only one of them?
Review of attachment 365781 [details] [review]: lgtm
Review of attachment 365782 [details] [review]: lgtm. As mentioned, should consider squashing these two so we don't potentially regress if only one is applied.
(In reply to Jonas Ådahl from comment #19) > lgtm. As mentioned, should consider squashing these two so we don't > potentially regress if only one is applied. Yeap, agreed, the only reason I kept those two separate is because one touched clutter and the other one wayland. I'll squash them before pushing then.
Comment on attachment 365781 [details] [review] [PATCH 1/2] clutter/evdev: Save effective xkb layout with seat Attachment 365781 [details] squashed with attachment 365782 [details] [review] and pushed to git master as commit 7f5f5eb - wayland/keyboard: preserve layout index
Comment on attachment 365782 [details] [review] [PATCH 2/2] wayland/keyboard: restore effective layout Attachment 365781 [details] squashed with attachment 365782 [details] [review] and pushed to git master as commit 7f5f5eb - wayland/keyboard: preserve layout index
Closing this bug for now, Xwayland part has landed and I can't reproduce the discrepancy on VT switch with the patch(es) for mutter.