From 74ec47aa104682fc0cb895866d38dd8bec3d33db Mon Sep 17 00:00:00 2001 From: yaakovschectman <109111084+yaakovschectman@users.noreply.github.com> Date: Tue, 20 Sep 2022 17:00:55 -0400 Subject: [PATCH] Modify assert keyboard condition on Windows to account for violation of invariant (flutter/engine#36129) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Skip assert on critical keys * Comment motivation * Update shell/platform/windows/keyboard_key_embedder_handler.cc Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com> * Unit test keyboard change * Format * Ditto * Update shell/platform/windows/keyboard_unittests.cc Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com> * Update shell/platform/windows/keyboard_key_embedder_handler.cc Co-authored-by: Tong Mu * Discard repeat keyup messages * Initial * Post synchronization * Explain assertion * Reassess pressing records * Formatting * Update shell/platform/windows/keyboard_key_embedder_handler.cc Co-authored-by: Tong Mu * One space * Update shell/platform/windows/keyboard_key_embedder_handler.cc Co-authored-by: Tong Mu * Comment spacing Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com> Co-authored-by: Tong Mu --- .../windows/keyboard_key_embedder_handler.cc | 28 ++++++++++++++++--- ...keyboard_key_embedder_handler_unittests.cc | 5 +++- .../platform/windows/keyboard_unittests.cc | 13 +++++++++ 3 files changed, 41 insertions(+), 5 deletions(-) diff --git a/engine/src/flutter/shell/platform/windows/keyboard_key_embedder_handler.cc b/engine/src/flutter/shell/platform/windows/keyboard_key_embedder_handler.cc index 0a072e80551..68288b3469b 100644 --- a/engine/src/flutter/shell/platform/windows/keyboard_key_embedder_handler.cc +++ b/engine/src/flutter/shell/platform/windows/keyboard_key_embedder_handler.cc @@ -164,8 +164,8 @@ void KeyboardKeyEmbedderHandler::KeyboardHookImpl( action == WM_SYSKEYDOWN || action == WM_SYSKEYUP); 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 = + bool had_record = last_logical_record_iter != pressingRecords_.end(); + uint64_t last_logical_record = had_record ? last_logical_record_iter->second : 0; // The logical key for the current "tap sequence". @@ -208,6 +208,12 @@ void KeyboardKeyEmbedderHandler::KeyboardHookImpl( SynchronizeCritialPressedStates(key, physical_key, is_event_down, event_key_can_be_repeat); + // Reassess the last logical record in case pressingRecords_ was modified + // by the above synchronization methods. + last_logical_record_iter = pressingRecords_.find(physical_key); + had_record = last_logical_record_iter != pressingRecords_.end(); + last_logical_record = had_record ? last_logical_record_iter->second : 0; + // The resulting event's `type`. FlutterKeyEventType type; character = UndeadChar(character); @@ -264,8 +270,14 @@ void KeyboardKeyEmbedderHandler::KeyboardHookImpl( pressingRecords_[physical_key] = eventual_logical_record; } else { auto record_iter = pressingRecords_.find(physical_key); - assert(record_iter != pressingRecords_.end()); - pressingRecords_.erase(record_iter); + // Assert this in debug mode. But in cases that it doesn't satisfy + // (such as due to a bug), make sure the `erase` is only called + // with a valid value to avoid crashing. + if (record_iter != pressingRecords_.end()) { + pressingRecords_.erase(record_iter); + } else { + assert(false); + } } FlutterKeyEvent key_data{ @@ -299,6 +311,14 @@ void KeyboardKeyEmbedderHandler::KeyboardHookImpl( pending_responses_[response_id] = std::move(pending_ptr); SendEvent(key_data, KeyboardKeyEmbedderHandler::HandleResponse, reinterpret_cast(pending_responses_[response_id].get())); + + // Post-event synchronization. It is useful in cases where the true pressing + // state does not match the event type. For example, a CapsLock down event is + // received despite that GetKeyState says that CapsLock is not pressed. In + // such case, post-event synchronization will synthesize a CapsLock up event + // after the main event. + SynchronizeCritialPressedStates(key, physical_key, is_event_down, + event_key_can_be_repeat); } void KeyboardKeyEmbedderHandler::KeyboardHook( diff --git a/engine/src/flutter/shell/platform/windows/keyboard_key_embedder_handler_unittests.cc b/engine/src/flutter/shell/platform/windows/keyboard_key_embedder_handler_unittests.cc index 3c526d9ba65..cf614379c5d 100644 --- a/engine/src/flutter/shell/platform/windows/keyboard_key_embedder_handler_unittests.cc +++ b/engine/src/flutter/shell/platform/windows/keyboard_key_embedder_handler_unittests.cc @@ -942,7 +942,10 @@ TEST(KeyboardKeyEmbedderHandlerTest, VK_NUMLOCK, kScanCodeNumLock, WM_KEYDOWN, 0, true, false, [&last_handled](bool handled) { last_handled = handled; }); EXPECT_EQ(last_handled, false); - EXPECT_EQ(results.size(), 3); + // 4 total events should be fired: + // Pre-synchronization toggle, pre-sync press, + // main event, and post-sync press. + EXPECT_EQ(results.size(), 4); event = &results[0]; EXPECT_EQ(event->type, kFlutterKeyEventTypeDown); EXPECT_EQ(event->physical, kPhysicalNumLock); diff --git a/engine/src/flutter/shell/platform/windows/keyboard_unittests.cc b/engine/src/flutter/shell/platform/windows/keyboard_unittests.cc index 4ce5ba4724e..1d088b9226a 100644 --- a/engine/src/flutter/shell/platform/windows/keyboard_unittests.cc +++ b/engine/src/flutter/shell/platform/windows/keyboard_unittests.cc @@ -2374,5 +2374,18 @@ TEST(KeyboardTest, VietnameseTelexAddDiacriticWithSlowTrueResponse) { VietnameseTelexAddDiacriticWithSlowResponse(true); } +// Ensure that the scancode-less key events issued by Narrator +// when toggling caps lock don't violate assert statements. +TEST(KeyboardTest, DoubleCapsLock) { + KeyboardTester tester; + tester.Responding(false); + + tester.InjectKeyboardChanges(std::vector{ + WmKeyDownInfo{VK_CAPITAL, 0, kNotExtended}.Build(), + WmKeyUpInfo{VK_CAPITAL, 0, kNotExtended}.Build()}); + + clear_key_calls(); +} + } // namespace testing } // namespace flutter