Fix stale keysym entries when modifier changes while key is held (#4747)

`KeyboardStateTracker` used the keysym from the key-up event to erase
from `pressed_keysyms`, but that keysym can differ from what was stored
at key-down when a modifier changes mid-hold (e.g. `1` down → Shift down
→ key-up reports `XKB_KEY_exclam`, leaving `XKB_KEY_1` stranded).

Related: #4746

## What's new?

- **`scancode_to_keysym` map replaces the two separate sets**
(`pressed_keysyms` / `pressed_scancodes`). Scancode is the stable
identity of a physical key; the map stores the keysym recorded at
key-down against it.
- **Key-up erases by scancode** (`scancode_to_keysym.erase(scancode)`),
so mismatched key-up keysyms caused by held modifiers never leave stale
entries.
- **Shift transitions mutate map values in-place** via a range loop
applying `xkb_keysym_to_upper`/`xkb_keysym_to_lower`, replacing the
previous set-rebuild approach.
- **`keysym_is_pressed`** uses `std::ranges::any_of` over map values;
`scancode_is_pressed` uses `map.contains`. The linear scan for keysyms
is inconsequential given the physical bound on simultaneously held keys
(≤ ~20).
- **New test** `key_up_clears_key_when_modifier_changed_while_held`
exercises the exact failure scenario: `1` down → Shift down → `!` up →
neither `XKB_KEY_1` nor `XKB_KEY_exclam` remains pressed.

## How to test

Run the `KeyboardStateTrackerTest` suite:

```
./bin/mir_unit_tests.bin --gtest_filter="KeyboardStateTrackerTest.*"
```

All 17 tests should pass, including the new regression test.

## Checklist

- [x] Tests added and pass
- [x] Adequate documentation added
- [ ] ~(optional) Added Screenshots or videos~

Author: tarek-y-ismail

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