Implement delayed key event synthesis for Windows #23524

gspencergoog · 2021-01-08T01:51:29Z

Description

This changes the Windows text handling so that keyboard events are sent to the framework first for handling, and then passed to the text input plugin, so that the framework has a chance to handle keys before they get given to the text field.

This is complicated by the async nature of the interaction with the framework, since Windows wants a synchronous response. So, in this change, I always tell Windows that the event was handled, and if the framework (eventually) responds that it wasn't, then I synthesize a new event and send it with SendEvent.

I also added support for detecting "extended" keys, since that was missing, and converted the OnKey handlers in the API to return a bool to indicate whether or not they have handled the event.

Related Issues

Keys (Tab, arrows) should not be handled by RenderEditable when also processed as an action. flutter#47156

Tests

Added tests to make sure that events are handled properly when printable/modifier/non-printable key events are sent, and the framework handles/does not handle them, including checking to make sure that the text plugin gets invoked on them.
Added tests to make sure that the extended bit is propagated properly.

clarkezone

This generally LGM both for the current Win32 backend as well as the impending UWP/CoreWindow impl. One request would be to take a look at #23573 and LMK if you have any concerns with this working with CoreWindow. I don't think there will be but would be good to have your ACK.

dkwingsmt · 2021-01-12T21:49:45Z

The code looks fine, but can you add some unit tests for it? I see at least 3 cases we need to add:

Handled/unhandled events
Handled text events
Extended bit

Especially considering I'm definitely touching this part in my keyboard PR.

dkwingsmt

See comment above

gspencergoog · 2021-01-13T18:37:25Z

OK, I'll add some more tests.

gspencergoog · 2021-01-20T01:43:11Z

OK, @dkwingsmt I've added a bunch more testing of the event handling from the framework. The test simulates the framework response, and tests for printable, non-printable, and modifier key events (since they have slightly different code paths in Windows).

dkwingsmt · 2021-01-21T06:32:26Z

shell/platform/windows/key_event_handler.cc

  return mods;
 }
+
+uint64_t CalculateEventId(int scancode, int action, bool extended) {


Why use this complicated calculation instead of self-incrementing ID?

Because I can't attach any information to the redispatched events, so I have to be able to compute an ID from the identifying data in the event when I receive it again in order to differentiate between events that are new, and events that have been redispatched.

Another alternative would be to compute a checksum from all the data in the event (just compute it over the bytes in the struct, skipping timestamps), but I think that these fields are enough to differentiate them, and since Windows does some processing on the events, coming up with virtual key codes, setting timestamps, etc., so I'm not sure that the redispatched events would have the same checksums.

Great explanation. Can you add it to the code?

Good idea, done!

dkwingsmt

Tests take some effort to understand, among other small change requests. Otherwise looking good.

dkwingsmt · 2021-01-21T06:35:45Z

shell/platform/windows/key_event_handler.cc

+    std::cerr
+        << "There are " << pending_events_.size()
+        << " keyboard events that have not yet received a response from the "
+        << "framework. Are responses being sent?" << std::endl;


Exit with FD_ASSERT? I think it's safe we're asserting this won't happen.

I wasn't sure it merited crashing the app though. But I guess it's pretty unlikely, and getting crash reports will be better than failing silently.

I don't see any macro called FD_ASSERT, though. Do you mean just plain assert()? Or FML_LOG(FATAL)?

Sorry, it's actually FML_DCHECK

dkwingsmt · 2021-01-21T06:37:46Z

shell/platform/windows/key_event_handler.h

+
+  // The queue of key events that have been sent to the framework but have not
+  // yet received a response.
+  std::deque<std::pair<uint64_t, KEYBDINPUT>> pending_events_;


Why not unordered_map?

Because 99% of the time the event we want to remove will be the first one in the queue, so it's not necessary to go through the cost of generating a hash to look it up.

dkwingsmt · 2021-01-21T07:39:28Z

shell/platform/windows/win32_flutter_window_unittests.cc

+      std::make_unique<::testing::NiceMock<MockWindowBindingHandler>>();
+  TestFlutterWindowsView flutter_windows_view(
+      std::move(window_binding_handler),
+      [&pending_events](UINT cInputs, LPINPUT pInputs, int cbSize) -> UINT {


This logic seems quite complicated, and it's not clear whether the two tests uses the same one. Can you inherit TestFlutterWindowsView and extract this logic out?

Similarly we can move pending_events to TestFlutterWindowsView and leave out a clearPendingEvent method for clarity.

Sure, done.

dkwingsmt · 2021-01-21T07:57:06Z

shell/platform/windows/win32_flutter_window_unittests.cc

+  // Test an event not handled by the framework
+  test_response = false;
+  flutter_windows_view.SetEngine(std::move(GetTestEngine()));
+  EXPECT_CALL(


This EXPECT_CALL convention is rather confusing, since you declare "what should happen" first then make pull the trigger. It's also unclear when these calls actually happened. They can happen at any time and this test will pass, especially when you mix two tests in one block.

Can we use plain blocks ({ }) to wrap the two sub-tests so that EXPECT_CALL is more reliable?

Added {} blocks. I also added .RetiresOnSaturation() to the ones that were missing it to make sure that they aren't in force after they are called the right number of times.

I agree that it's a weird way to define things, but that's how mocks work in C++.

dkwingsmt · 2021-01-21T07:58:36Z

shell/platform/windows/win32_flutter_window_unittests.cc

+    win32window.InjectWindowMessage(event.message, event.wparam, event.lparam);
+    pending_events.pop_front();
+  }
+  pending_events.clear();


I don't understand this clear. Shouldn't it be empty after everything?

Yes, I guess that is redundant. Removed.

dkwingsmt · 2021-01-21T08:08:48Z

shell/platform/windows/win32_flutter_window_unittests.cc

+  }
+}
+
+// Tests key event propagation of modifier key down events.


Can you tell me, or point me to, why they should behave differently? Is it because SendEvent doesn't simulate WM_CHAR events for them?

The non-printable and modifier events behave differently because of the missing WM_CHAR events, as you said, so it follows a different path in the HandleMessage function.

Modifier events are largely the same as non-printable, but because they call MapVirtualKey could have different results.

Added some comments to that effect.

dkwingsmt

LGTM except for 2 nits

gspencergoog · 2021-01-22T21:06:32Z

shell/platform/windows/key_event_handler.cc

  return mods;
 }
+
+uint64_t CalculateEventId(int scancode, int action, bool extended) {


Good idea, done!

gspencergoog · 2021-01-22T21:06:40Z

shell/platform/windows/key_event_handler.cc

+    std::cerr
+        << "There are " << pending_events_.size()
+        << " keyboard events that have not yet received a response from the "
+        << "framework. Are responses being sent?" << std::endl;


gspencergoog · 2021-01-22T22:59:51Z

shell/platform/windows/key_event_handler.cc

+                                      int scancode,
+                                      int action,
+                                      bool extended) {
+  FML_DCHECK(pending_events_.size() < kMaxPendingEvents)


Actually, I can't use this here because including FML is a layer violation. I'm going to go back to the cerr output...

…gine#23524)

* 63b3440 Roll Fuchsia Linux SDK from mODEe2CNk... to edqShE0QE... (flutter/engine#23873) * f0e25c5 Roll Skia from 3193ff271628 to 2a4c0fbdca1a (3 revisions) (flutter/engine#23875) * 492759e Roll Dart SDK from 82b4c77fb17f to 748993c3997a (1 revision) (flutter/engine#23874) * 8671aef Notify Win32FlutterWindow of cursor updates (flutter/engine#23795) * c8620c3 Implement delayed key event synthesis for Windows (flutter/engine#23524) * ebbf0df Roll Skia from 2a4c0fbdca1a to 8a42b09c162e (9 revisions) (flutter/engine#23878) * bb00cb6 Roll Fuchsia Linux Toolchain from IJxh_9dNS... to 8LaTdqf7w... (flutter/engine#23876) * f77fea2 Roll Dart SDK from 748993c3997a to 2ddf810f71f6 (1 revision) (flutter/engine#23881) * dc22ede Roll Skia from 8a42b09c162e to 9702fc6f3852 (1 revision) (flutter/engine#23882) * 1f30e56 Roll Fuchsia Mac SDK from tuJCioUf3... to 9Lh_vPIXU... (flutter/engine#23883) * 443bf5c Roll Fuchsia Linux SDK from edqShE0QE... to uMOnDLfvl... (flutter/engine#23886) * a152470 Roll Fuchsia Mac SDK from 9Lh_vPIXU... to PsYsfVNbW... (flutter/engine#23888) * 221259b Roll Skia from 9702fc6f3852 to 07c5f52c947d (2 revisions) (flutter/engine#23890) * 381d8bd Roll Skia from 07c5f52c947d to 8d29ab630996 (1 revision) (flutter/engine#23892) * 397274f Roll Skia from 8d29ab630996 to d396cd50ff15 (1 revision) (flutter/engine#23893) * a5c305e push methods return layers with correct class names (flutter/engine#23542) * 4e87f60 Read loading unit mapping from AndroidManifest instead of strings (flutter/engine#23868) * d3a1acb Roll Fuchsia Linux SDK from uMOnDLfvl... to VYUnZ3Tbh... (flutter/engine#23894) * 3d966fa Roll Fuchsia Mac SDK from PsYsfVNbW... to 6swTf93jz... (flutter/engine#23897) * cae9130 Roll Skia from d396cd50ff15 to 5bbf72757349 (2 revisions) (flutter/engine#23898) * f3c5687 Roll Skia from 5bbf72757349 to 069e484cc3b9 (2 revisions) (flutter/engine#23900) * 9365230 Add support for IME-based text input on Windows (flutter/engine#23853) * cad597f Roll Fuchsia Linux SDK from VYUnZ3Tbh... to mrFdelzNr... (flutter/engine#23903) * 4557989 Roll Fuchsia Mac SDK from 6swTf93jz... to 7LGbVIHUD... (flutter/engine#23904)

This changes the Windows text handling so that keyboard events are sent to the framework first for handling, and then passed to the text input plugin, so that the framework has a chance to handle keys before they get given to the text field. This is complicated by the async nature of the interaction with the framework, since Windows wants a synchronous response. So, in this change, I always tell Windows that the event was handled, and if the framework (eventually) responds that it wasn't, then I synthesize a new event and send it with SendEvent. I also added support for detecting "extended" keys, since that was missing, and converted the OnKey handlers in the API to return a bool to indicate whether or not they have handled the event.

gspencergoog requested a review from clarkezone January 8, 2021 01:51

googlebot added the cla: yes label Jan 8, 2021

gspencergoog requested a review from dkwingsmt January 8, 2021 01:51

clarkezone reviewed Jan 10, 2021

View reviewed changes

dkwingsmt suggested changes Jan 12, 2021

View reviewed changes

gspencergoog force-pushed the windows_key_synth branch 11 times, most recently from 400fcda to 92a2591 Compare January 20, 2021 01:25

dkwingsmt reviewed Jan 21, 2021

View reviewed changes

dkwingsmt suggested changes Jan 21, 2021

View reviewed changes

gspencergoog force-pushed the windows_key_synth branch 3 times, most recently from a143027 to 5fe8b38 Compare January 22, 2021 01:35

dkwingsmt approved these changes Jan 22, 2021

View reviewed changes

gspencergoog force-pushed the windows_key_synth branch 4 times, most recently from d6aab74 to 5e48066 Compare January 22, 2021 22:51

gspencergoog commented Jan 22, 2021

View reviewed changes

gspencergoog force-pushed the windows_key_synth branch 2 times, most recently from d501314 to 682ec99 Compare January 22, 2021 23:02

gspencergoog added 6 commits January 22, 2021 16:38

Implement delayed key event synthesis for Windows

15639b8

Adding some tests

25c6337

Tests working

995c65f

Add missing file

bbb0253

Some cleanup

b22d189

Review Changes

ee4ab04

gspencergoog force-pushed the windows_key_synth branch 3 times, most recently from 08035f6 to 4bafe35 Compare January 23, 2021 01:08

Review Changes 2

76ce7c4

gspencergoog force-pushed the windows_key_synth branch from 4bafe35 to 76ce7c4 Compare January 23, 2021 01:09

gspencergoog merged commit c8620c3 into flutter:master Jan 23, 2021

gspencergoog deleted the windows_key_synth branch January 23, 2021 02:14

engine-flutter-autoroll mentioned this pull request Jan 25, 2021

Roll Engine from 05b4bec8b3c1 to 45579893b117 (24 revisions) flutter/flutter#74666

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 25, 2021

c8620c3 Implement delayed key event synthesis for Windows (flutter/en…

b90415a

…gine#23524)

Implement delayed key event synthesis for Windows #23524

Implement delayed key event synthesis for Windows #23524

Uh oh!

Conversation

gspencergoog commented Jan 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Uh oh!

clarkezone left a comment

Choose a reason for hiding this comment

Uh oh!

dkwingsmt commented Jan 12, 2021

Uh oh!

dkwingsmt left a comment

Choose a reason for hiding this comment

Uh oh!

gspencergoog commented Jan 13, 2021

Uh oh!

gspencergoog commented Jan 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkwingsmt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkwingsmt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

gspencergoog commented Jan 8, 2021 •

edited

Loading