After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 791383 - [Wayland] Discrepancy between layout indicator and actual layout
[Wayland] Discrepancy between layout indicator and actual layout
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: wayland
3.26.x
Other Linux
: Normal major
: ---
Assigned To: mutter-maint
mutter-maint
Depends on:
Blocks:
 
 
Reported: 2017-12-08 09:28 UTC by Olivier Fourdan
Modified: 2017-12-20 09:09 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Video demonstrating the issue with the wrong layout indicator. (1.95 MB, video/webm)
2017-12-08 09:32 UTC, Olivier Fourdan
  Details
Video demonstrating the issue with the layout indicator doing nothing (1.25 MB, video/webm)
2017-12-08 09:34 UTC, Olivier Fourdan
  Details
[PATCH 1/2] clutter/evdev: Save effective xkb layout with seat (3.16 KB, patch)
2017-12-19 15:44 UTC, Olivier Fourdan
none Details | Review
[PATCH 2/2] wayland/keyboard: restore effective layout (1.60 KB, patch)
2017-12-19 15:45 UTC, Olivier Fourdan
none Details | Review
[PATCH 1/2] clutter/evdev: Save effective xkb layout with seat (3.12 KB, patch)
2017-12-20 08:31 UTC, Olivier Fourdan
committed Details | Review
[PATCH 2/2] wayland/keyboard: restore effective layout (1.56 KB, patch)
2017-12-20 08:32 UTC, Olivier Fourdan
committed Details | Review

Description Olivier Fourdan 2017-12-08 09:28:25 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.
Comment 1 Olivier Fourdan 2017-12-08 09:32:24 UTC
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”)
Comment 2 Olivier Fourdan 2017-12-08 09:34:10 UTC
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.
Comment 3 Olivier Fourdan 2017-12-08 10:45:15 UTC
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.
Comment 4 Olivier Fourdan 2017-12-11 16:58:34 UTC
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).
Comment 5 Olivier Fourdan 2017-12-12 09:17:07 UTC
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]
Comment 6 Olivier Fourdan 2017-12-15 15:45:38 UTC
(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/
Comment 7 Olivier Fourdan 2017-12-19 15:44:08 UTC
(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.
Comment 8 Olivier Fourdan 2017-12-19 15:44:46 UTC
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.
Comment 9 Olivier Fourdan 2017-12-19 15:45:19 UTC
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.
Comment 10 Jonas Ådahl 2017-12-20 07:11:09 UTC
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
Comment 11 Jonas Ådahl 2017-12-20 07:11:17 UTC
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 ?
Comment 12 Olivier Fourdan 2017-12-20 07:59:26 UTC
(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);
Comment 13 Olivier Fourdan 2017-12-20 08:06:48 UTC
(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... :)
Comment 14 Olivier Fourdan 2017-12-20 08:09:24 UTC
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.
Comment 15 Olivier Fourdan 2017-12-20 08:31:13 UTC
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.
Comment 16 Olivier Fourdan 2017-12-20 08:32:09 UTC
Created attachment 365782 [details] [review]
[PATCH 2/2] wayland/keyboard: restore effective layout

Updated patch after review in comment 11
Comment 17 Jonas Ådahl 2017-12-20 08:33:55 UTC
(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?
Comment 18 Jonas Ådahl 2017-12-20 08:34:38 UTC
Review of attachment 365781 [details] [review]:

lgtm
Comment 19 Jonas Ådahl 2017-12-20 08:35:17 UTC
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.
Comment 20 Olivier Fourdan 2017-12-20 08:55:20 UTC
(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 21 Olivier Fourdan 2017-12-20 09:08:33 UTC
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 22 Olivier Fourdan 2017-12-20 09:08:59 UTC
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
Comment 23 Olivier Fourdan 2017-12-20 09:09:54 UTC
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.