From 95d46e2aa217e4977529a7cfbe1e3ef94f525a78 Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Mon, 1 Aug 2022 05:29:47 -0700 Subject: [PATCH 1/5] Impl & test --- .../windows/keyboard_key_embedder_handler.cc | 24 +++++++++++++++---- .../windows/keyboard_key_embedder_handler.h | 6 +++-- shell/platform/windows/keyboard_unittests.cc | 12 ++++++++++ 3 files changed, 35 insertions(+), 7 deletions(-) diff --git a/shell/platform/windows/keyboard_key_embedder_handler.cc b/shell/platform/windows/keyboard_key_embedder_handler.cc index 5d596091a4053..25da2d39dc78c 100644 --- a/shell/platform/windows/keyboard_key_embedder_handler.cc +++ b/shell/platform/windows/keyboard_key_embedder_handler.cc @@ -186,6 +186,7 @@ void KeyboardKeyEmbedderHandler::KeyboardHookImpl( const bool is_event_down = action == WM_KEYDOWN || action == WM_SYSKEYDOWN; + bool event_key_can_be_repeat = true; 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 @@ -197,14 +198,14 @@ void KeyboardKeyEmbedderHandler::KeyboardHookImpl( // 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); + SynchronizeCritialToggledStates(key, is_event_down, &event_key_can_be_repeat); // 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); + SynchronizeCritialPressedStates(key, physical_key, is_event_down, event_key_can_be_repeat); // The resulting event's `type`. FlutterKeyEventType type; @@ -340,7 +341,8 @@ void KeyboardKeyEmbedderHandler::UpdateLastSeenCritialKey( void KeyboardKeyEmbedderHandler::SynchronizeCritialToggledStates( int event_virtual_key, - bool is_event_down) { + bool is_event_down, + bool* event_key_can_be_repeat) { // NowState ----------------> PreEventState --------------> TrueState // Synchronization Event for (auto& kv : critical_keys_) { @@ -364,6 +366,7 @@ void KeyboardKeyEmbedderHandler::SynchronizeCritialToggledStates( // Pressed 0 1 0 1 // Toggled 0 1 1 0 const bool true_toggled = get_key_state_(virtual_key) & kStateMaskToggled; + const bool true_pressed = get_key_state_(virtual_key) & kStateMaskPressed; 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. @@ -385,6 +388,7 @@ void KeyboardKeyEmbedderHandler::SynchronizeCritialToggledStates( key_info.physical_key, key_info.logical_key, empty_character), nullptr, nullptr); + *event_key_can_be_repeat = false; } key_info.toggled_on = true_toggled; } @@ -394,7 +398,8 @@ void KeyboardKeyEmbedderHandler::SynchronizeCritialToggledStates( void KeyboardKeyEmbedderHandler::SynchronizeCritialPressedStates( int event_virtual_key, int event_physical_key, - bool is_event_down) { + bool is_event_down, + bool event_key_can_be_repeat) { // 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. @@ -424,8 +429,17 @@ void KeyboardKeyEmbedderHandler::SynchronizeCritialPressedStates( 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." + // + // In that case, normally Flutter should synthesize nothing since the + // resulting event can be a down or a repeat depending on the current + // state. However, in certain cases (when Flutter has just synchronized + // the key's toggling state) the event must not be a repeat event. if (virtual_key == event_virtual_key) { - pre_event_pressed = false; + if (event_key_can_be_repeat) { + continue; + } else { + pre_event_pressed = false; + } } } else { // For up events, this key is the event key if they have the same diff --git a/shell/platform/windows/keyboard_key_embedder_handler.h b/shell/platform/windows/keyboard_key_embedder_handler.h index 426a38d42deab..67ff6490e5fc4 100644 --- a/shell/platform/windows/keyboard_key_embedder_handler.h +++ b/shell/platform/windows/keyboard_key_embedder_handler.h @@ -115,12 +115,14 @@ class KeyboardKeyEmbedderHandler // Check each key's state from |get_key_state_| and synthesize events // if their toggling states have been desynchronized. void SynchronizeCritialToggledStates(int event_virtual_key, - bool is_event_down); + bool is_event_down, + bool* event_key_can_be_repeat); // Check each key's state from |get_key_state_| and synthesize events // if their pressing states have been desynchronized. void SynchronizeCritialPressedStates(int event_virtual_key, int event_physical_key, - bool is_event_down); + bool is_event_down, + bool event_key_can_be_repeat); // 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_unittests.cc b/shell/platform/windows/keyboard_unittests.cc index 0bea6b97167d8..f532c1a92af85 100644 --- a/shell/platform/windows/keyboard_unittests.cc +++ b/shell/platform/windows/keyboard_unittests.cc @@ -696,6 +696,18 @@ TEST(KeyboardTest, ShiftLeftUnhandled) { clear_key_calls(); EXPECT_EQ(tester.RedispatchedMessageCountAndClear(), 1); + // Hold ShiftLeft + tester.InjectKeyboardChanges(std::vector{ + WmKeyDownInfo{VK_SHIFT, kScanCodeShiftLeft, kNotExtended, kWasDown}.Build( + kWmResultZero)}); + + EXPECT_EQ(key_calls.size(), 1); + EXPECT_CALL_IS_EVENT(key_calls[0], kFlutterKeyEventTypeRepeat, + kPhysicalShiftLeft, kLogicalShiftLeft, "", + kNotSynthesized); + clear_key_calls(); + EXPECT_EQ(tester.RedispatchedMessageCountAndClear(), 1); + // Release ShiftLeft tester.InjectKeyboardChanges(std::vector{ KeyStateChange{VK_LSHIFT, false, true}, From ae9615071c672467095febc283e6f8711c1091d9 Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Mon, 1 Aug 2022 05:38:38 -0700 Subject: [PATCH 2/5] Remove unused --- shell/platform/windows/keyboard_key_embedder_handler.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/shell/platform/windows/keyboard_key_embedder_handler.cc b/shell/platform/windows/keyboard_key_embedder_handler.cc index 25da2d39dc78c..8fd6f38f0b0a0 100644 --- a/shell/platform/windows/keyboard_key_embedder_handler.cc +++ b/shell/platform/windows/keyboard_key_embedder_handler.cc @@ -366,7 +366,6 @@ void KeyboardKeyEmbedderHandler::SynchronizeCritialToggledStates( // Pressed 0 1 0 1 // Toggled 0 1 1 0 const bool true_toggled = get_key_state_(virtual_key) & kStateMaskToggled; - const bool true_pressed = get_key_state_(virtual_key) & kStateMaskPressed; 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. From 9a6751717f17dcf458bd05c246ddf0e9b7b33aac Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Mon, 1 Aug 2022 05:39:50 -0700 Subject: [PATCH 3/5] Better doc --- shell/platform/windows/keyboard_key_embedder_handler.cc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/shell/platform/windows/keyboard_key_embedder_handler.cc b/shell/platform/windows/keyboard_key_embedder_handler.cc index 8fd6f38f0b0a0..b8af3428fb799 100644 --- a/shell/platform/windows/keyboard_key_embedder_handler.cc +++ b/shell/platform/windows/keyboard_key_embedder_handler.cc @@ -430,9 +430,10 @@ void KeyboardKeyEmbedderHandler::SynchronizeCritialPressedStates( // virtual key, because virtual key represents "functionality." // // In that case, normally Flutter should synthesize nothing since the - // resulting event can be a down or a repeat depending on the current - // state. However, in certain cases (when Flutter has just synchronized - // the key's toggling state) the event must not be a repeat event. + // resulting event can adapt to the current state by flexing between a + // down and a repeat event. However, in certain cases (when Flutter has + // just synchronized the key's toggling state) the event must not be a + // repeat event. if (virtual_key == event_virtual_key) { if (event_key_can_be_repeat) { continue; From cad6162945ec2f55beaac77a85f8d65de8fa503f Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Mon, 1 Aug 2022 05:41:02 -0700 Subject: [PATCH 4/5] format --- shell/platform/windows/keyboard_key_embedder_handler.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/shell/platform/windows/keyboard_key_embedder_handler.cc b/shell/platform/windows/keyboard_key_embedder_handler.cc index b8af3428fb799..550d6d6be4158 100644 --- a/shell/platform/windows/keyboard_key_embedder_handler.cc +++ b/shell/platform/windows/keyboard_key_embedder_handler.cc @@ -205,7 +205,8 @@ void KeyboardKeyEmbedderHandler::KeyboardHookImpl( // 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, event_key_can_be_repeat); + SynchronizeCritialPressedStates(key, physical_key, is_event_down, + event_key_can_be_repeat); // The resulting event's `type`. FlutterKeyEventType type; From 18d815a32d6b2c5e3857876bbacd0cf1837431dc Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Mon, 1 Aug 2022 13:27:22 -0700 Subject: [PATCH 5/5] Fix comment --- shell/platform/windows/keyboard_key_embedder_handler.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/shell/platform/windows/keyboard_key_embedder_handler.cc b/shell/platform/windows/keyboard_key_embedder_handler.cc index 550d6d6be4158..0a072e80551f7 100644 --- a/shell/platform/windows/keyboard_key_embedder_handler.cc +++ b/shell/platform/windows/keyboard_key_embedder_handler.cc @@ -431,8 +431,8 @@ void KeyboardKeyEmbedderHandler::SynchronizeCritialPressedStates( // virtual key, because virtual key represents "functionality." // // In that case, normally Flutter should synthesize nothing since the - // resulting event can adapt to the current state by flexing between a - // down and a repeat event. However, in certain cases (when Flutter has + // resulting event can adapt to the current state by dispatching either + // a down or a repeat event. However, in certain cases (when Flutter has // just synchronized the key's toggling state) the event must not be a // repeat event. if (virtual_key == event_virtual_key) {