diff --git a/shell/platform/windows/keyboard_key_embedder_handler.cc b/shell/platform/windows/keyboard_key_embedder_handler.cc index 47ac20ecb1d22..762051e723884 100644 --- a/shell/platform/windows/keyboard_key_embedder_handler.cc +++ b/shell/platform/windows/keyboard_key_embedder_handler.cc @@ -162,25 +162,60 @@ void KeyboardKeyEmbedderHandler::KeyboardHookImpl( const uint64_t logical_key = GetLogicalKey(key, extended, scancode); assert(action == WM_KEYDOWN || action == WM_KEYUP || action == WM_SYSKEYDOWN || action == WM_SYSKEYUP); - const bool is_physical_down = action == WM_KEYDOWN || action == WM_SYSKEYDOWN; auto last_logical_record_iter = pressingRecords_.find(physical_key); const bool had_record = last_logical_record_iter != pressingRecords_.end(); const uint64_t last_logical_record = had_record ? last_logical_record_iter->second : 0; + // The logical key for the current "tap sequence". + // + // Key events are formed in tap sequences: down, repeats, up. The logical key + // stays consistent throughout a tap sequence, which is this value. + uint64_t sequence_logical_key = + had_record ? last_logical_record : logical_key; + + if (sequence_logical_key == VK_PROCESSKEY) { + // VK_PROCESSKEY means that the key press is used by an IME. These key + // presses are considered handled and not sent to Flutter. These events must + // be filtered by result_logical_key because the key up event of such + // presses uses the "original" logical key. + callback(true); + return; + } + + const bool is_event_down = action == WM_KEYDOWN || action == WM_SYSKEYDOWN; + + UpdateLastSeenCritialKey(key, physical_key, sequence_logical_key); + // Synchronize the toggled states of critical keys (such as whether CapsLocks + // is enabled). Toggled states can only be changed upon a down event, so if + // the recorded toggled state does not match the true state, this function + // will synthesize (an up event if the key is recorded pressed, then) a down + // event. + // + // After this function, all critical keys will have their toggled state + // updated to the true state, while the critical keys whose toggled state have + // been changed will be pressed regardless of their true pressed state. + // Updating the pressed state will be done by SynchronizeCritialPressedStates. + SynchronizeCritialToggledStates(key, is_event_down); + // Synchronize the pressed states of critical keys (such as whether CapsLocks + // is pressed). + // + // After this function, all critical keys except for the target key will have + // their toggled state and pressed state matched with their true states. The + // target key's pressed state will be updated immediately after this. + SynchronizeCritialPressedStates(key, physical_key, is_event_down); + // The resulting event's `type`. FlutterKeyEventType type; - // The resulting event's `logical_key`. - uint64_t result_logical_key; + character = UndeadChar(character); + char character_bytes[kCharacterCacheSize]; // What pressingRecords_[physical_key] should be after the KeyboardHookImpl // returns (0 if the entry should be removed). uint64_t eventual_logical_record; - char character_bytes[kCharacterCacheSize]; - - character = UndeadChar(character); + uint64_t result_logical_key; - if (is_physical_down) { + if (is_event_down) { if (had_record) { if (was_down) { // A normal repeated key. @@ -223,35 +258,6 @@ void KeyboardKeyEmbedderHandler::KeyboardHookImpl( } } - if (result_logical_key == VK_PROCESSKEY) { - // VK_PROCESSKEY means that the key press is used by an IME. These key - // presses are considered handled and not sent to Flutter. These events must - // be filtered by result_logical_key because the key up event of such - // presses uses the "original" logical key. - callback(true); - return; - } - - UpdateLastSeenCritialKey(key, physical_key, result_logical_key); - // Synchronize the toggled states of critical keys (such as whether CapsLocks - // is enabled). Toggled states can only be changed upon a down event, so if - // the recorded toggled state does not match the true state, this function - // will synthesize (an up event if the key is recorded pressed, then) a down - // event. - // - // After this function, all critical keys will have their toggled state - // updated to the true state, while the critical keys whose toggled state have - // been changed will be pressed regardless of their true pressed state. - // Updating the pressed state will be done by SynchronizeCritialPressedStates. - SynchronizeCritialToggledStates(key, type == kFlutterKeyEventTypeDown); - // Synchronize the pressed states of critical keys (such as whether CapsLocks - // is pressed). - // - // After this function, all critical keys except for the target key will have - // their toggled state and pressed state matched with their true states. The - // target key's pressed state will be updated immediately after this. - SynchronizeCritialPressedStates(key, type != kFlutterKeyEventTypeRepeat); - if (eventual_logical_record != 0) { pressingRecords_[physical_key] = eventual_logical_record; } else { @@ -333,8 +339,10 @@ void KeyboardKeyEmbedderHandler::UpdateLastSeenCritialKey( } void KeyboardKeyEmbedderHandler::SynchronizeCritialToggledStates( - int this_virtual_key, - bool is_down_event) { + int event_virtual_key, + bool is_event_down) { + // NowState ----------------> PreEventState --------------> TrueState + // Synchronization Event for (auto& kv : critical_keys_) { UINT virtual_key = kv.first; CriticalKey& key_info = kv.second; @@ -346,21 +354,26 @@ void KeyboardKeyEmbedderHandler::SynchronizeCritialToggledStates( // Check toggling state first, because it might alter pressing state. if (key_info.check_toggled) { + const bool target_is_pressed = + pressingRecords_.find(key_info.physical_key) != + pressingRecords_.end(); // The togglable keys observe a 4-phase cycle: // // Phase# 0 1 2 3 // Event Down Up Down Up // Pressed 0 1 0 1 // Toggled 0 1 1 0 - SHORT state = get_key_state_(virtual_key); - bool should_toggled = state & kStateMaskToggled; - if (virtual_key == this_virtual_key && is_down_event) { - key_info.toggled_on = !key_info.toggled_on; + const bool true_toggled = get_key_state_(virtual_key) & kStateMaskToggled; + bool pre_event_toggled = true_toggled; + // Check if the main event's key is the key being checked. If it's the + // non-repeat down event, toggle the state. + if (virtual_key == event_virtual_key && !target_is_pressed && + is_event_down) { + pre_event_toggled = !pre_event_toggled; } - if (key_info.toggled_on != should_toggled) { + if (key_info.toggled_on != pre_event_toggled) { // If the key is pressed, release it first. - if (pressingRecords_.find(key_info.physical_key) != - pressingRecords_.end()) { + if (target_is_pressed) { SendEvent(SynthesizeSimpleEvent( kFlutterKeyEventTypeUp, key_info.physical_key, key_info.logical_key, empty_character), @@ -373,14 +386,26 @@ void KeyboardKeyEmbedderHandler::SynchronizeCritialToggledStates( key_info.logical_key, empty_character), nullptr, nullptr); } - key_info.toggled_on = should_toggled; + key_info.toggled_on = true_toggled; } } } void KeyboardKeyEmbedderHandler::SynchronizeCritialPressedStates( - int this_virtual_key, - bool pressed_state_will_change) { + int event_virtual_key, + int event_physical_key, + bool is_event_down) { + // During an incoming event, there might be a synthesized Flutter event for + // each key of each pressing goal, followed by an eventual main Flutter + // event. + // + // NowState ----------------> PreEventState --------------> TrueState + // Synchronization Event + // + // The goal of the synchronization algorithm is to derive a pre-event state + // that can satisfy the true state (`true_pressed`) after the event, and that + // requires as few synthesized events based on the current state + // (`now_pressed`) as possible. for (auto& kv : critical_keys_) { UINT virtual_key = kv.first; CriticalKey& key_info = kv.second; @@ -390,25 +415,42 @@ void KeyboardKeyEmbedderHandler::SynchronizeCritialPressedStates( } assert(key_info.logical_key != 0); if (key_info.check_pressed) { - SHORT state = get_key_state_(virtual_key); - auto recorded_pressed_iter = pressingRecords_.find(key_info.physical_key); - bool recorded_pressed = recorded_pressed_iter != pressingRecords_.end(); - bool should_pressed = state & kStateMaskPressed; - if (virtual_key == this_virtual_key && pressed_state_will_change) { - should_pressed = !should_pressed; + SHORT true_pressed = get_key_state_(virtual_key) & kStateMaskPressed; + auto pressing_record_iter = pressingRecords_.find(key_info.physical_key); + bool now_pressed = pressing_record_iter != pressingRecords_.end(); + bool pre_event_pressed = true_pressed; + // Check if the main event is the key being checked to get the correct + // target state. + if (is_event_down) { + // For down events, this key is the event key if they have the same + // virtual key, because virtual key represents "functionality." + if (virtual_key == event_virtual_key) { + pre_event_pressed = false; + } + } else { + // For up events, this key is the event key if they have the same + // physical key, because it is necessary to ensure that the physical + // key is correctly released. + // + // In that case, although the previous state should be pressed, don't + // synthesize a down event even if it's not. The later code will handle + // such cases by skipping abrupt up events. Obviously don't synthesize + // up events either. + if (event_physical_key == key_info.physical_key) { + continue; + } } - if (recorded_pressed != should_pressed) { - if (recorded_pressed) { - pressingRecords_.erase(recorded_pressed_iter); + if (now_pressed != pre_event_pressed) { + if (now_pressed) { + pressingRecords_.erase(pressing_record_iter); } else { pressingRecords_[key_info.physical_key] = key_info.logical_key; } const char* empty_character = ""; SendEvent( - SynthesizeSimpleEvent(recorded_pressed ? kFlutterKeyEventTypeUp - : kFlutterKeyEventTypeDown, - key_info.physical_key, key_info.logical_key, - empty_character), + SynthesizeSimpleEvent( + now_pressed ? kFlutterKeyEventTypeUp : kFlutterKeyEventTypeDown, + key_info.physical_key, key_info.logical_key, empty_character), nullptr, nullptr); } } diff --git a/shell/platform/windows/keyboard_key_embedder_handler.h b/shell/platform/windows/keyboard_key_embedder_handler.h index 7add4247ac774..426a38d42deab 100644 --- a/shell/platform/windows/keyboard_key_embedder_handler.h +++ b/shell/platform/windows/keyboard_key_embedder_handler.h @@ -114,10 +114,13 @@ class KeyboardKeyEmbedderHandler uint64_t logical_key); // Check each key's state from |get_key_state_| and synthesize events // if their toggling states have been desynchronized. - void SynchronizeCritialToggledStates(int virtual_key, bool is_down); + void SynchronizeCritialToggledStates(int event_virtual_key, + bool is_event_down); // Check each key's state from |get_key_state_| and synthesize events // if their pressing states have been desynchronized. - void SynchronizeCritialPressedStates(int virtual_key, bool was_down); + void SynchronizeCritialPressedStates(int event_virtual_key, + int event_physical_key, + bool is_event_down); // Wraps perform_send_event_ with state tracking. Use this instead of // |perform_send_event_| to send events to the framework. diff --git a/shell/platform/windows/keyboard_win32_unittests.cc b/shell/platform/windows/keyboard_win32_unittests.cc index b90f274285f6c..f1b395cf797ac 100644 --- a/shell/platform/windows/keyboard_win32_unittests.cc +++ b/shell/platform/windows/keyboard_win32_unittests.cc @@ -46,6 +46,7 @@ constexpr uint64_t kScanCodeKeyO = 0x18; constexpr uint64_t kScanCodeKeyQ = 0x10; constexpr uint64_t kScanCodeKeyW = 0x11; constexpr uint64_t kScanCodeDigit1 = 0x02; +constexpr uint64_t kScanCodeDigit2 = 0x03; constexpr uint64_t kScanCodeDigit6 = 0x07; // constexpr uint64_t kScanCodeNumpad1 = 0x4f; // constexpr uint64_t kScanCodeNumLock = 0x45; @@ -1915,6 +1916,44 @@ TEST(KeyboardTest, ImeExtendedEventsAreIgnored) { EXPECT_EQ(tester.RedispatchedMessageCountAndClear(), 0); } +// Ensures that synthesization works correctly when a Shift key is pressed and +// (only) its up event is labeled as an IME event (VK_PROCESSKEY). +// +// Regression test for https://github.com/flutter/flutter/issues/104169. These +// are real messages recorded when pressing Shift-2 using Microsoft Pinyin IME +// on Win 10 Enterprise, which crashed the app before the fix. +TEST(KeyboardTest, UpOnlyImeEventsAreCorrectlyHandled) { + KeyboardTester tester; + tester.Responding(true); + + // US Keyboard layout. + + // Press CtrlRight in IME mode. + tester.InjectKeyboardChanges(std::vector{ + KeyStateChange{VK_LSHIFT, true, false}, + WmKeyDownInfo{VK_SHIFT, kScanCodeShiftLeft, kNotExtended, kWasUp}.Build( + kWmResultZero), + WmKeyDownInfo{VK_PROCESSKEY, kScanCodeDigit2, kNotExtended, kWasUp}.Build( + kWmResultZero), + KeyStateChange{VK_LSHIFT, false, true}, + WmKeyUpInfo{VK_PROCESSKEY, kScanCodeShiftLeft, kNotExtended}.Build( + kWmResultZero), + WmKeyUpInfo{'2', kScanCodeDigit2, kNotExtended, kWasUp}.Build( + kWmResultZero)}); + + EXPECT_EQ(key_calls.size(), 4); + EXPECT_CALL_IS_EVENT(key_calls[0], kFlutterKeyEventTypeDown, + kPhysicalShiftLeft, kLogicalShiftLeft, "", + kNotSynthesized); + EXPECT_CALL_IS_EVENT(key_calls[1], kFlutterKeyEventTypeDown, 0, 0, "", + kNotSynthesized); + EXPECT_CALL_IS_EVENT(key_calls[2], kFlutterKeyEventTypeUp, kPhysicalShiftLeft, + kLogicalShiftLeft, "", kNotSynthesized); + EXPECT_CALL_IS_EVENT(key_calls[3], kFlutterKeyEventTypeDown, 0, 0, "", + kNotSynthesized); + clear_key_calls(); +} + TEST(KeyboardTest, DisorderlyRespondedEvents) { KeyboardTester tester;