Enable delayed event delivery for macOS #21231

gspencergoog · 2020-09-17T01:20:11Z

Description

This enables delayed event delivery for macOS, so that shortcuts can handle keys that are headed for a text field and intercept them. This fixes the problem where pressing TAB (or other shortcuts) in a text field also inserts a tab character into the text field.

Related Issues

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

Tests

I added tests to make sure that the events were sent to the framework, that they were propagated if the framework didn't handle them, and that they weren't propagated if the framework did handle them.

Writing the tests was a little weird, since I needed to use OCMock to mock things, but calling OCMVerify required that it be run inside of an NSObject method (it wanted self to be available). To do that, I created Flutter TEST() methods that verify that the NSObject method succeeded.

One note: The linter doesn't like that the OCM methods throw exceptions, so I had to mark each call to the OCM macros with // NOLINT(google-objc-avoid-throwing-exceptions).

Breaking Change

No, no existing tests failed, so this is not a breaking change.

gspencergoog · 2020-09-25T22:16:48Z

Yep, looks like I hit the same problem that Justin did in #20531 (comment)

[ERROR:flutter/runtime/dart_vm_data.cc(18)] VM snapshot invalid and could not be inferred from settings.
[ERROR:flutter/runtime/dart_vm.cc(249)] Could not setup VM data to bootstrap the VM from.
[ERROR:flutter/runtime/dart_vm_lifecycle.cc(84)] Could not create Dart VM instance.
[FATAL:flutter/shell/common/shell.cc(265)] Check failed: vm. Must be able to initialize the VM.
[ERROR:flutter/fml/backtrace.cc(110)] Caught signal SIGABRT during program execution.

I'll need to keep from running the tests in run_tests.py if I want to land this, and re-enable it when flutter/flutter#66664 is fixed.

The tests do succeed when run locally.

cc @chinmaygarde @stuartmorgan

stuartmorgan-g

It seems odd to describe this in the PR as "event synthesis" since all it's doing is delaying the actual events, not synthesizing new ones.

shell/platform/darwin/macos/framework/Headers/FlutterViewController.h

shell/platform/darwin/macos/framework/Source/FlutterViewController.mm

gspencergoog · 2020-09-28T21:21:50Z

It seems odd to describe this in the PR as "event synthesis" since all it's doing is delaying the actual events, not synthesizing new ones.

Good point. I had to synthesize events on Android, so I guess the name stuck. Updated the description.

dkwingsmt · 2020-12-02T23:16:22Z

shell/platform/darwin/macos/framework/Source/FlutterViewController.mm

+- (void)propagateKeyEvent:(NSEvent*)event ofType:(NSString*)type {
+  if ([type isEqual:@"keydown"]) {
+    for (FlutterIntermediateKeyResponder* responder in self.additionalKeyResponders) {
+      if ([responder respondsToSelector:@selector(handleKeyDown:)]) {


Do we still need to check respondsToSelector? Since we're already limiting the elements to be FlutterIntermediateKeyResponder they should be required to implement handleKeyDown and handleKeyUp.

Or we can keep this check but remove the implementation of FlutterIntermediateKeyResponder. According to what I read this is allowed.

OK. Actually, I already tried to remove the implementation, and it causes compile time errors. I'll just remove the respondsToSelector checks.

shell/platform/darwin/macos/framework/Headers/FlutterViewController.h

shell/platform/darwin/macos/framework/Source/FlutterIntermediateKeyResponder.h

shell/platform/darwin/macos/framework/Source/FlutterViewController.mm

stuartmorgan-g

LGTM with nits

shell/platform/darwin/macos/framework/Source/FlutterIntermediateKeyResponder.h

stuartmorgan-g · 2020-12-08T13:28:18Z

shell/platform/darwin/macos/framework/Source/FlutterViewController.mm

-  controller->_engine = [[FlutterEngine alloc] initWithName:@"io.flutter"
-                                                    project:controller->_project
-                                     allowHeadlessExecution:NO];
+  if (controller->_engine == nullptr) {


Nit: I believe the prevailing style in the macOS code is !foo rather than foo == nullptr for pointer checks.

I see about 38 comparisons to nullptr in the .mm files in the repo. It's harder to search for references to using the bang operator, so I'm not sure how many of those there are, but I was able to find >100, so I definitely agree. I'll switch this instance (I didn't make any others), and maybe we can make the others conform in another PR.

shell/platform/darwin/macos/framework/Source/FlutterViewController.mm

shell/platform/darwin/macos/framework/Source/FlutterViewControllerTest.mm

chinmaygarde · 2020-12-10T22:46:17Z

@gspencergoog is OOO today. Should be back by next weeks PR triage.

tvolkert · 2020-12-19T04:49:55Z

This has been bisected to have introduced a crash in flutter/flutter#72634 (by trying to CMD-TAB away from the app). Sending a revert (assuming this reverts cleanly)

tvolkert · 2020-12-19T04:50:35Z

Alas, it does not revert cleanly.

gspencergoog · 2020-12-19T05:45:34Z

Yes, @tvolkert this problem is fixed in #23154

tvolkert · 2020-12-19T07:47:24Z

That'll learn me to sync to ToT first 😄

gspencergoog added affects: text input Work in progress (WIP) Not ready (yet) for review! labels Sep 17, 2020

googlebot added the cla: yes label Sep 17, 2020

gspencergoog marked this pull request as draft September 17, 2020 01:24

gspencergoog force-pushed the macos_key_synthesis branch 8 times, most recently from c609986 to e3f2404 Compare September 25, 2020 17:40

gspencergoog force-pushed the macos_key_synthesis branch 4 times, most recently from cd5dfe4 to b565965 Compare September 25, 2020 21:40

gspencergoog marked this pull request as ready for review September 25, 2020 21:42

gspencergoog removed the Work in progress (WIP) Not ready (yet) for review! label Sep 25, 2020

auto-assign bot requested a review from gw280 September 25, 2020 21:42

gspencergoog requested review from stuartmorgan-g and removed request for gw280 September 25, 2020 21:42

gspencergoog force-pushed the macos_key_synthesis branch from b565965 to 43b38a2 Compare September 25, 2020 21:45

flutter deleted a comment from flutter-dashboard bot Sep 25, 2020

gspencergoog mentioned this pull request Sep 25, 2020

flutter_desktop_darwin_unittests fails in AOT modes (profile and release) flutter/flutter#66664

Closed

stuartmorgan-g suggested changes Sep 28, 2020

View reviewed changes

gspencergoog changed the title ~~Enable delayed event synthesis for macOS~~ Enable delayed event delivery for macOS Sep 28, 2020

gspencergoog force-pushed the macos_key_synthesis branch 2 times, most recently from 799dd88 to 25db203 Compare September 28, 2020 23:26

gspencergoog force-pushed the macos_key_synthesis branch from c1b1184 to 7a4c145 Compare December 1, 2020 23:55

dkwingsmt suggested changes Dec 3, 2020

View reviewed changes

gspencergoog force-pushed the macos_key_synthesis branch 4 times, most recently from 45886b7 to c44e86b Compare December 3, 2020 23:40

stuartmorgan-g approved these changes Dec 8, 2020

View reviewed changes

gspencergoog added 4 commits December 11, 2020 10:50

Add delayed key event propagation to macOS

01de84b

Replace NSResponder for custom class that can handle return type

272d33d

Review Changes

b83595e

Review Changes

96035f5

gspencergoog force-pushed the macos_key_synthesis branch from c44e86b to 96035f5 Compare December 11, 2020 20:07

dkwingsmt approved these changes Dec 11, 2020

View reviewed changes

gspencergoog merged commit 21691f1 into flutter:master Dec 11, 2020

gspencergoog deleted the macos_key_synthesis branch December 11, 2020 23:21

fanchou mentioned this pull request Dec 12, 2020

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

Closed

engine-flutter-autoroll mentioned this pull request Dec 14, 2020

Roll Engine from 4797b0665242 to 3ef9b93657cb (34 revisions) flutter/flutter#72283

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 14, 2020

21691f1 Enable delayed event delivery for macOS (flutter/engine#21231)

560ed4d

engine-flutter-autoroll mentioned this pull request Dec 14, 2020

Roll Engine from 4797b0665242 to 46d7bf9d5aff (39 revisions) flutter/flutter#72296

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 14, 2020

21691f1 Enable delayed event delivery for macOS (flutter/engine#21231)

67ee0f9

engine-flutter-autoroll mentioned this pull request Dec 14, 2020

Roll Engine from 4797b0665242 to 360a16ad7542 (40 revisions) flutter/flutter#72304

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 14, 2020

21691f1 Enable delayed event delivery for macOS (flutter/engine#21231)

a843c78

engine-flutter-autoroll mentioned this pull request Dec 15, 2020

Roll Engine from 4797b0665242 to 1749dbcc1b09 (49 revisions) flutter/flutter#72318

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 15, 2020

21691f1 Enable delayed event delivery for macOS (flutter/engine#21231)

04b1f5c

tvolkert mentioned this pull request Dec 19, 2020

Crash on MacOS: NSInternalInconsistencyException (Invalid message sent to event) flutter/flutter#72634

Closed

Enable delayed event delivery for macOS #21231

Enable delayed event delivery for macOS #21231

Uh oh!

Conversation

gspencergoog commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Breaking Change

Uh oh!

gspencergoog commented Sep 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gspencergoog commented Sep 28, 2020

Uh oh!

dkwingsmt Dec 2, 2020

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Dec 3, 2020

Choose a reason for hiding this comment

Uh oh!

gspencergoog Dec 3, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stuartmorgan-g Dec 8, 2020

Choose a reason for hiding this comment

Uh oh!

gspencergoog Dec 11, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chinmaygarde commented Dec 10, 2020

Uh oh!

tvolkert commented Dec 19, 2020

Uh oh!

tvolkert commented Dec 19, 2020

Uh oh!

gspencergoog commented Dec 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tvolkert commented Dec 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

gspencergoog commented Sep 17, 2020 •

edited

Loading

gspencergoog commented Sep 25, 2020 •

edited

Loading

gspencergoog commented Dec 19, 2020 •

edited

Loading