Fix keysym tracking: use scancode-to-keysym map to avoid stale entries on key-up

Co-authored-by: tarek-y-ismail <171774743+tarek-y-ismail@users.noreply.github.com>

Author: Copilot

src/server/frontend_wayland/keyboard_state_tracker.cpp

@@ -20,7 +20,7 @@
 
 #include <xkbcommon/xkbcommon-keysyms.h>
 
-#include <ranges>
+#include <algorithm>
 
 namespace mf = mir::frontend;
 
@@ -40,7 +40,7 @@ bool mf::KeyboardStateTracker::process(MirEvent const& event)
     auto const action = key_event->action();
     auto const modifiers = key_event->modifiers();
 
-    auto& [pressed_keysyms, pressed_scancodes, shift_state] = device_states[input_event->device_id()];
+    auto& [scancode_to_keysym, shift_state] = device_states[input_event->device_id()];
 
     auto const prev_shift_state = shift_state;
     shift_state = modifiers & (mir_input_event_modifier_shift | mir_input_event_modifier_shift_left |
@@ -49,47 +49,46 @@ bool mf::KeyboardStateTracker::process(MirEvent const& event)
     auto processed = false;
     if (action == mir_keyboard_action_down)
     {
-        pressed_keysyms.insert(keysym);
-        pressed_scancodes.insert(scancode);
+        scancode_to_keysym[scancode] = keysym;
         processed = true;
     }
     else if (action == mir_keyboard_action_up)
     {
-        pressed_keysyms.erase(keysym);
-        pressed_scancodes.erase(scancode);
+        // Remove by scancode so that a mismatched key-up keysym (caused by a
+        // modifier change while the key was held) does not leave stale entries.
+        scancode_to_keysym.erase(scancode);
         processed = true;
     }
 
     // Transitioned from no shift to at least one shift
     if (prev_shift_state == 0 && shift_state != 0)
     {
-        auto const uppercase =
-            std::ranges::views::transform(pressed_keysyms, [](auto key) { return xkb_keysym_to_upper(key); });
-        pressed_keysyms = std::unordered_set<xkb_keysym_t>(uppercase.begin(), uppercase.end());
+        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
-        auto const lowercase =
-            std::ranges::views::transform(pressed_keysyms, [](auto key) { return xkb_keysym_to_lower(key); });
-        pressed_keysyms = std::unordered_set<xkb_keysym_t>(lowercase.begin(), lowercase.end());
+        for (auto& [sc, ks] : scancode_to_keysym)
+            ks = xkb_keysym_to_lower(ks);
     }
 
     return processed;
 }
 
 auto mf::KeyboardStateTracker::keysym_is_pressed(MirInputDeviceId device, xkb_keysym_t keysym) const -> bool
 {
-    if(!device_states.contains(device)) return false;
+    if (!device_states.contains(device))
+        return false;
 
-    auto const& [pressed_keysyms, _, __] = device_states.at(device);
-    return pressed_keysyms.contains(keysym);
+    return std::ranges::any_of(
+        device_states.at(device).scancode_to_keysym, [keysym](auto const& pair) { return pair.second == keysym; });
 }
 
 auto mf::KeyboardStateTracker::scancode_is_pressed(MirInputDeviceId device, int32_t scancode) const -> bool
 {
-    if(!device_states.contains(device)) return false;
+    if (!device_states.contains(device))
+        return false;
 
-    auto const& [_, pressed_scancodes, __] = device_states.at(device);
-    return pressed_scancodes.contains(scancode);
+    return device_states.at(device).scancode_to_keysym.contains(scancode);
 }

src/server/frontend_wayland/keyboard_state_tracker.h

@@ -19,7 +19,7 @@
 
 #include <mir/events/event.h>
 
-#include <unordered_set>
+#include <unordered_map>
 
 namespace mir
 {
@@ -40,6 +40,11 @@ namespace frontend
 /// 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.
+///
+/// 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
+/// that occurred while the key was held (e.g. pressing Shift while holding
+/// a digit key).
 class KeyboardStateTracker
 {
 public:
@@ -56,8 +61,9 @@ class KeyboardStateTracker
 private:
     struct DeviceState
     {
-        std::unordered_set<xkb_keysym_t> pressed_keysyms;
-        std::unordered_set<int32_t> pressed_scancodes;
+        /// 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;
         MirInputEventModifiers shift_state{0};
     };
 

tests/unit-tests/frontend_wayland/test_keyboard_state_tracker.cpp

@@ -33,6 +33,7 @@ namespace
 
 /// Based on standard US QWERTY layout (PC keyboard / evdev scancodes)
 
+constexpr uint32_t key_1_scancode = 2;
 constexpr uint32_t a_scancode = 30;
 constexpr uint32_t b_scancode = 48;
 
@@ -306,4 +307,28 @@ TEST_F(KeyboardStateTrackerTest, shift_release_on_one_device_does_not_demote_key
     EXPECT_TRUE(tracker.keysym_is_pressed(other_device_id, XKB_KEY_A));
     EXPECT_FALSE(tracker.keysym_is_pressed(other_device_id, XKB_KEY_a));
 }
+
+TEST_F(KeyboardStateTrackerTest, key_up_clears_key_when_modifier_changed_while_held)
+{
+    // Simulate: '1' pressed (keysym = XKB_KEY_1), then Shift pressed, then '1'
+    // released while Shift is held. The key-up event reports XKB_KEY_exclam
+    // ('!') because Shift is active. The tracker must still clear the pressed
+    // state using the stored scancode rather than the key-up keysym.
+    tracker.process(*key_down(XKB_KEY_1, key_1_scancode));
+    EXPECT_TRUE(tracker.keysym_is_pressed(device_id, XKB_KEY_1));
+    EXPECT_TRUE(tracker.scancode_is_pressed(device_id, key_1_scancode));
+
+    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));
+    EXPECT_TRUE(tracker.scancode_is_pressed(device_id, key_1_scancode));
+
+    // Key-up event reports XKB_KEY_exclam because Shift is still held
+    tracker.process(*key_up(XKB_KEY_exclam, key_1_scancode));
+
+    EXPECT_FALSE(tracker.keysym_is_pressed(device_id, XKB_KEY_1));
+    EXPECT_FALSE(tracker.keysym_is_pressed(device_id, XKB_KEY_exclam));
+    EXPECT_FALSE(tracker.scancode_is_pressed(device_id, key_1_scancode));
+}
 } // namespace