Fix shift remapping in KeyboardStateTracker to use xkb_state

Author: tarek-y-ismail

src/server/frontend_wayland/keyboard_state_tracker.cpp

@@ -15,15 +15,65 @@
  */
 
 #include "keyboard_state_tracker.h"
+#include "mir_toolkit/events/enums.h"
 
 #include <mir/events/keyboard_event.h>
+#include <mir/fatal.h>
 
-#include <xkbcommon/xkbcommon-keysyms.h>
+#include <xkbcommon/xkbcommon.h>
 
 #include <algorithm>
 
 namespace mf = mir::frontend;
 
+namespace
+{
+// xkb scancodes are offset by 8 from evdev scancodes for compatibility with
+// the X protocol. This matches the same helper used in xkb_mapper.cpp.
+uint32_t constexpr to_xkb_scan_code(uint32_t evdev_scan_code)
+{
+    return evdev_scan_code + 8;
+}
+} // namespace
+
+void mf::KeyboardStateTracker::XkbKeyState::update_keymap(
+    std::shared_ptr<mir::input::Keymap> const& new_keymap, xkb_context* context)
+{
+    if (!new_keymap || (current_keymap && current_keymap->matches(*new_keymap)))
+        return;
+
+    current_keymap = new_keymap;
+    compiled_keymap = new_keymap->make_unique_xkb_keymap(context);
+    state = {xkb_state_new(compiled_keymap.get()), xkb_state_unref};
+}
+
+void mf::KeyboardStateTracker::XkbKeyState::update_key(uint32_t xkb_keycode, MirKeyboardAction action)
+{
+    if (state)
+        xkb_state_update_key(state.get(), xkb_keycode, action == mir_keyboard_action_down ? XKB_KEY_DOWN : XKB_KEY_UP);
+}
+
+void mf::KeyboardStateTracker::XkbKeyState::rederive_keysyms_from_scancodes(
+    std::unordered_map<uint32_t, xkb_keysym_t>& scancode_to_keysym) const
+{
+    if (!state)
+        return;
+
+    for (auto& [sc, ks] : scancode_to_keysym)
+    {
+        auto const derived = xkb_state_key_get_one_sym(state.get(), to_xkb_scan_code(sc));
+        if (derived != XKB_KEY_NoSymbol)
+            ks = derived;
+    }
+}
+
+mf::KeyboardStateTracker::KeyboardStateTracker()
+    : context{xkb_context_new(XKB_CONTEXT_NO_FLAGS), xkb_context_unref}
+{
+    if (!context)
+        fatal_error("KeyboardStateTracker: failed to create XKB context");
+}
+
 bool mf::KeyboardStateTracker::process(MirEvent const& event)
 {
     if (event.type() != mir_event_type_input)
@@ -40,7 +90,15 @@ bool mf::KeyboardStateTracker::process(MirEvent const& event)
     auto const action = key_event->action();
     auto const modifiers = key_event->modifiers();
 
-    auto& [scancode_to_keysym, shift_state] = device_states[input_event->device_id()];
+    auto& [scancode_to_keysym, shift_state, xkb_key_state] =
+        device_states[input_event->device_id()];
+
+    xkb_key_state.update_keymap(key_event->keymap(), context.get());
+
+    // Keep xkb_key_state in sync with every key event so that its modifier
+    // tracking stays accurate for subsequent keysym queries.
+    auto const xkb_keycode = to_xkb_scan_code(static_cast<uint32_t>(scancode));
+    xkb_key_state.update_key(xkb_keycode, action);
 
     auto const prev_shift_state = shift_state;
     shift_state = modifiers & (mir_input_event_modifier_shift | mir_input_event_modifier_shift_left |
@@ -60,18 +118,10 @@ bool mf::KeyboardStateTracker::process(MirEvent const& event)
         processed = true;
     }
 
-    // Transitioned from no shift to at least one shift
-    if (prev_shift_state == 0 && shift_state != 0)
-    {
-        for (auto& [sc, ks] : scancode_to_keysym)
-            ks = xkb_keysym_to_upper(ks);
-    }
-    else if (prev_shift_state != 0 && shift_state == 0)
-    {
-        // Transitioned from at least one shift to no shift
-        for (auto& [sc, ks] : scancode_to_keysym)
-            ks = xkb_keysym_to_lower(ks);
-    }
+    // When the shift state changes, re-derive every pressed keysym from its
+    // scancode using the layout-aware XKB state.
+    if (prev_shift_state != shift_state)
+        xkb_key_state.rederive_keysyms_from_scancodes(scancode_to_keysym);
 
     return processed;
 }

src/server/frontend_wayland/keyboard_state_tracker.h

@@ -17,8 +17,11 @@
 #ifndef MIR_FRONTEND_WAYLAND_KEYBOARD_STATE_TRACKER_H_
 #define MIR_FRONTEND_WAYLAND_KEYBOARD_STATE_TRACKER_H_
 
+#include "mir_toolkit/events/enums.h"
 #include <mir/events/event.h>
+#include <mir/input/keymap.h>
 
+#include <memory>
 #include <unordered_map>
 
 namespace mir
@@ -35,11 +38,10 @@ namespace frontend
 /// need to inspect it, so a single tracker instance can be shared across many
 /// objects without each one having to maintain its own pressed-key set.
 ///
-/// Shift-key transitions are handled specially: when a Shift key is pressed
-/// all lowercase keysyms already in the pressed set are promoted to their
-/// uppercase equivalents, and when all Shift keys are released they are
-/// demoted back. This keeps the stored keysyms consistent with what the
-/// keyboard layer reports as the logical key for subsequent events.
+/// Shift-key transitions are handled specially: when the shift state changes,
+/// all currently-pressed keysyms are re-derived from their scancodes using the
+/// layout-aware XKB state. This keeps the stored keysyms consistent with what
+/// the keyboard layer reports as the logical key for subsequent events.
 ///
 /// Keysyms are stored per scancode so that a key-up event always removes the
 /// keysym that was recorded at key-down, regardless of any modifier changes
@@ -48,7 +50,7 @@ namespace frontend
 class KeyboardStateTracker
 {
 public:
-    KeyboardStateTracker() = default;
+    KeyboardStateTracker();
 
     // Returns true if the passed in event was an up or down keyboard event and
     // was processed, false otherwise.
@@ -59,14 +61,49 @@ class KeyboardStateTracker
     auto scancode_is_pressed(MirInputDeviceId device, int32_t scancode) const -> bool;
 
 private:
+    /// Owns the per-device XKB keymap and state used to resolve keysyms from
+    /// scancodes in a layout- and modifier-aware way.
+    struct XkbKeyState
+    {
+        /// Update the compiled keymap and XKB state when a new keymap arrives.
+        /// \param new_keymap  The keymap carried on the incoming event.
+        /// \param context     The shared XKB context owned by the tracker.
+        void update_keymap(std::shared_ptr<mir::input::Keymap> const& new_keymap, xkb_context* context);
+
+        /// Feed a key press or release into the XKB state so that modifier
+        /// tracking stays accurate for subsequent keysym queries.
+        /// \param xkb_keycode  The XKB keycode for the key.
+        /// \param action       The keyboard action.
+        void update_key(uint32_t xkb_keycode, MirKeyboardAction action);
+
+        /// Re-derive every keysym in \a scancode_to_keysym from its scancode
+        /// using the current modifier state of the XKB state machine.
+        void rederive_keysyms_from_scancodes(
+            std::unordered_map<uint32_t, xkb_keysym_t>& scancode_to_keysym) const;
+
+    private:
+        /// The keymap currently in use for this device, used to detect keymap
+        /// changes and rebuild compiled_keymap/state when they occur.
+        std::shared_ptr<mir::input::Keymap> current_keymap;
+
+        /// The compiled keymap and live XKB state, kept in sync with every
+        /// key event so that modifier state is accurate for keysym queries.
+        std::unique_ptr<xkb_keymap, void(*)(xkb_keymap*)> compiled_keymap{nullptr, xkb_keymap_unref};
+        std::unique_ptr<xkb_state, void(*)(xkb_state*)> state{nullptr, xkb_state_unref};
+    };
+
     struct DeviceState
     {
         /// Maps each currently-pressed scancode to the keysym that was recorded
         /// when it was pressed (updated on shift-state transitions).
-        std::unordered_map<uint32_t, uint32_t> scancode_to_keysym;
+        std::unordered_map<uint32_t, xkb_keysym_t> scancode_to_keysym;
         MirInputEventModifiers shift_state{0};
+        XkbKeyState xkb_key_state;
     };
 
+    /// Shared XKB context — one per tracker, reused across all devices.
+    std::unique_ptr<xkb_context, void(*)(xkb_context*)> context;
+
     std::unordered_map<MirInputDeviceId, DeviceState> device_states;
 };
 }

tests/unit-tests/frontend_wayland/test_keyboard_state_tracker.cpp

@@ -17,6 +17,8 @@
 #include "src/server/frontend_wayland/keyboard_state_tracker.h"
 
 #include <mir/test/doubles/advanceable_clock.h>
+#include <mir/events/keyboard_event.h>
+#include <mir/input/parameter_keymap.h>
 
 #include <xkbcommon/xkbcommon-keysyms.h>
 
@@ -34,12 +36,13 @@ namespace
 /// Based on standard US QWERTY layout (PC keyboard / evdev scancodes)
 
 constexpr uint32_t key_1_scancode = 2;
+constexpr uint32_t key_0_scancode = 11;
 constexpr uint32_t a_scancode = 30;
 constexpr uint32_t b_scancode = 48;
 
-constexpr uint32_t ctrl_l_scancode = 37;
-constexpr uint32_t shift_l_scancode = 50;
-constexpr uint32_t shift_r_scancode = 62;
+constexpr uint32_t ctrl_l_scancode = 29;
+constexpr uint32_t shift_l_scancode = 42;
+constexpr uint32_t shift_r_scancode = 54;
 
 class KeyboardStateTrackerTest : public Test
 {
@@ -52,13 +55,15 @@ class KeyboardStateTrackerTest : public Test
         if (keysym == XKB_KEY_Shift_R)
             mod |= mir_input_event_modifier_shift_right;
 
-        return mir::events::make_key_event(
+        auto event = mir::events::make_key_event(
             id,
             clock.now().time_since_epoch(),
             mir_keyboard_action_down,
             keysym,
             scancode,
             mod);
+        event->to_input()->to_keyboard()->set_keymap(default_keymap);
+        return event;
     }
 
     auto key_up(uint32_t keysym, uint32_t scancode, MirInputDeviceId id = device_id) -> mir::EventUPtr
@@ -69,13 +74,15 @@ class KeyboardStateTrackerTest : public Test
         if (keysym == XKB_KEY_Shift_R)
             mod &= ~mir_input_event_modifier_shift_right;
 
-        return mir::events::make_key_event(
+        auto event = mir::events::make_key_event(
             id,
             clock.now().time_since_epoch(),
             mir_keyboard_action_up,
             keysym,
             scancode,
             mod);
+        event->to_input()->to_keyboard()->set_keymap(default_keymap);
+        return event;
     }
 
     auto static inline const device_id = MirInputDeviceId{0};
@@ -84,6 +91,12 @@ class KeyboardStateTrackerTest : public Test
     mtd::AdvanceableClock clock;
     mf::KeyboardStateTracker tracker;
 
+    // Default US QWERTY keymap attached to every test event so that the
+    // tracker's xkb_state is populated and shift-transition re-derivation
+    // via xkb_state_key_get_one_sym() works correctly.
+    std::shared_ptr<mir::input::Keymap> const default_keymap{
+        std::make_shared<mir::input::ParameterKeymap>()};
+
     // Track modifier state per device to emulate Mir's tracking of modifiers
     std::unordered_map<MirInputDeviceId, MirInputEventModifiers> modifier_states;
 };
@@ -308,6 +321,29 @@ TEST_F(KeyboardStateTrackerTest, shift_release_on_one_device_does_not_demote_key
     EXPECT_FALSE(tracker.keysym_is_pressed(other_device_id, XKB_KEY_a));
 }
 
+TEST_F(KeyboardStateTrackerTest, pressing_shift_after_digit_promotes_to_symbol)
+{
+   tracker.process(*key_down(XKB_KEY_0, key_0_scancode));
+    EXPECT_TRUE(tracker.keysym_is_pressed(device_id, XKB_KEY_0));
+
+    tracker.process(*key_down(XKB_KEY_Shift_L, shift_l_scancode));
+
+    EXPECT_TRUE(tracker.keysym_is_pressed(device_id, XKB_KEY_parenright));
+    EXPECT_FALSE(tracker.keysym_is_pressed(device_id, XKB_KEY_0));
+}
+
+TEST_F(KeyboardStateTrackerTest, releasing_shift_after_digit_demotes_symbol_back_to_digit)
+{
+    // Shift held, '0' pressed (which the layout reports as ')'), then Shift
+    // released: the tracker should revert to '0'.
+    tracker.process(*key_down(XKB_KEY_Shift_L, shift_l_scancode));
+    tracker.process(*key_down(XKB_KEY_parenright, key_0_scancode));
+    tracker.process(*key_up(XKB_KEY_Shift_L, shift_l_scancode));
+
+    EXPECT_TRUE(tracker.keysym_is_pressed(device_id, XKB_KEY_0));
+    EXPECT_FALSE(tracker.keysym_is_pressed(device_id, XKB_KEY_parenright));
+}
+
 TEST_F(KeyboardStateTrackerTest, key_up_clears_key_when_modifier_changed_while_held)
 {
     // Simulate: '1' pressed (keysym = XKB_KEY_1), then Shift pressed, then '1'
@@ -320,11 +356,13 @@ TEST_F(KeyboardStateTrackerTest, key_up_clears_key_when_modifier_changed_while_h
 
     tracker.process(*key_down(XKB_KEY_Shift_L, shift_l_scancode));
 
-    // XKB_KEY_1 has no uppercase equivalent so it remains as XKB_KEY_1
-    EXPECT_TRUE(tracker.keysym_is_pressed(device_id, XKB_KEY_1));
+    // With a real xkb_state, pressing Shift re-derives '1' -> '!' correctly.
+    EXPECT_TRUE(tracker.keysym_is_pressed(device_id, XKB_KEY_exclam));
+    EXPECT_FALSE(tracker.keysym_is_pressed(device_id, XKB_KEY_1));
     EXPECT_TRUE(tracker.scancode_is_pressed(device_id, key_1_scancode));
 
-    // Key-up event reports XKB_KEY_exclam because Shift is still held
+    // Key-up event reports XKB_KEY_exclam because Shift is still held.
+    // The tracker must clear the entry by scancode, not by keysym.
     tracker.process(*key_up(XKB_KEY_exclam, key_1_scancode));
 
     EXPECT_FALSE(tracker.keysym_is_pressed(device_id, XKB_KEY_1));