Hardware Keyboard: macOS #23469

dkwingsmt · 2021-01-06T23:09:19Z

Description

This PR contains the macOS changes for the Hardware Keyboard project.

Classes has been adjusted as follows:

Before	After	Explain
FlutterViewController	FlutterViewController	(Unchanged)
	FlutterKeyChannelHandler	Handles events by sending raw information to the framework, corresponding to RawKeyboard API.
	FlutterKeyboardManager	Handles how events are dispatched between handlers and whether to propagate them to the next responder.
FlutterIntermediateKeyResponder	FlutterKeyFinalResponder	An interface for additional key handlers.
-	FlutterKeyHandler	An interface for key handlers.
-	FlutterKeyEmbedderHandler	Handles events by sending converted events to the framework, corresponding to HardwardKeyboard API.

Related Issues

Umbrella: ☂️ Device-specific keyboard events should happen in the embedder flutter#44918

Tests

I added the following tests:

FlutterKeyChannelHandlerUnittests
- BasicKeyEvent
- EmptyResponseIsTakenAsHandled
FlutterKeyEmbedderHandlerUnittests
- BasicKeyEvent
- NonAsciiCharacters
- IgnoreDuplicateDownEvent
- ToggleModifiersDuringKeyTap
- SpecialModiferFlags
- IdentifyLeftAndRightModifiers
- SynthesizeMissedModifierEvents
- SynthesizeMissedModifierEventsInNormalEvents
- ConvertCapsLockEvents
- SynchronizeCapsLockStateOnCapsLock
- SynchronizeCapsLockStateOnNormalKey
FlutterKeyboardManagerUnittests
- NextResponderShouldThrowOnKeyUp
- SingleAsyncHandler
- DoubleAsyncHandlers
- SingleFinalResponder
- EmptyNextResponder

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the contributor guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the C++, Objective-C, Java style guides for the engine.
I read the tree hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation.
All existing and new tests are passing.
I am willing to follow-up on review comments in a timely manner.

Reviewer Checklist

I have submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

Up to 201118

dkwingsmt · 2021-03-12T03:23:53Z

@gspencergoog I have finished the fixes. Here are some non-trivial changes:

Renamed handler and additionalHandler to primaryResponder and secondaryResponder
Added a FlutterKeyCallbackGuard to ensure the callback is handled exactly once
Added logic and test for ignoring duplicate Up events

among other mostly doc changes.

Can you take a look? Thanks!

gspencergoog · 2021-03-12T19:15:24Z

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

+    // 0xF747 is actually used. Here we choose to filter out 0xF700-0xF7FF
+    // section. The reason for keeping the 0xF800-0xF8FF section is because
+    // 0xF8FF is used for the "Apple logo" character (Option-Shift-K on US
+    // keyboard.)


Can you document here why you're including the 0xF800-0xF8FE region? e.g. "The 0xF800-0xF8FE region is included in anticipation of any new private values that are printable."

Do you have any documentation that this is how they split it up, or is that just a feeling? If it's not just a feeling, including a link to that information would be good.

I'm still a little leery of making the assumption that the region will include printable characters, but the consequences aren't huge, so I guess it's OK.

I understand. It is indeed only a feeling, and a result out of experimentation. The only documentation I can find is the one I included that reserves 0xF700-0xF8FF. Since we're trusting the official documentation, the doc only declared a limited number of values (https://developer.apple.com/documentation/appkit/1535851-function-key_unicodes?language=objc), all of which are within 0xF700-0xF747. I'll add a little more to the doc. We can always come back and adjust this range anyway.

How does this look?

// Some function keys are assigned characters with codepoints from the // private use area. These characters are filtered out since they're // unprintable. // // The official documentation reserves 0xF700-0xF8FF as private use area // (https://developer.apple.com/documentation/appkit/1535851-function-key_unicodes?language=objc). // But macOS seems to only use a reduced range of it. The official doc // defines a few constants, all of which are within 0xF700-0xF747. // (https://developer.apple.com/documentation/appkit/1535851-function-key_unicodes?language=objc). // This mostly aligns with the experimentation result, except for 0xF8FF, // which is used for the "Apple logo" character (Option-Shift-K on a US // keyboard.) // // We hereby assume that non-printable function keys are defined from // 0xF700 upwards, and printable private keys are defined from 0xF8FF // downwards. We want to keep the printable private keys, therefore // we only filter out 0xF700-0xF7FF.

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

gspencergoog · 2021-03-12T19:34:29Z

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

+      continue;
+    }
+    BOOL shouldDown = (currentFlagsOfInterest & currentFlag) != 0;
+    [self sendModifierEventOfType:shouldDown


Couldn't it just be a bitmask of the current modifier state? You could convert the bitmask to a platform-independent version here.

Actually, I guess it doesn't really matter much. It's not going to be THAT many events (like maybe four max per event).

dkwingsmt · 2021-03-16T00:37:15Z

@gspencergoog The reason why lastModifierFlags is not simply event.modifierFlags does involve a bit of taste, and is explained as follows:

At the end of synchronizeModifiers:, instead of simply assigning _lastModifierFlags = event.modifierFlags, only the affected bits are updated, in order to avoid covering potential synchronization issues.
In this way, the uninteresting bits of _lastModifierFlags will never be updated.
Therefore, the assertion at the end of handleFlagEvent will never be NSAssert(_lastModifierFlags == event.modifierFlags), but always NSAssert(_lastModifierFlags & modifierFlagOfInterestMask == event.modifierFlags & modifierFlagOfInterestMask).
Since changing the definition for _lastModifierFlags does not make the assertion simpler, I'd rather make lastModifierFlags easier to write logic for, by saying that it doesn't contain any uninterested bits. It actually makes some docs and logic simpler.

I'm renaming it to lastModifierFlagsOfInterest to avoid confusion.

gspencergoog

* b2d02f7 Roll Fuchsia Linux SDK from GsRYVri0-... to aRqEDMjwC... (flutter/engine#24990) * 3a1a3e6 Roll Skia from 7854da39b3c1 to 4fb13e026b6b (20 revisions) (flutter/engine#24993) * e3a84f9 Fixed issue where the gpu disable syncswitch was being overridden after init. (flutter/engine#24958) * aa83691 fix selectable text selections are not announced in voice over (flutter/engine#24933) * 04b0451 fixes reference retaining issue in flutter text input plugin (flutter/engine#24768) * 1ea7dc6 Set automatic simulator rotation in scenario test (flutter/engine#24985) * 38977a5 Implement AXPlatformNodeBase::GetInstanceCountForTesting (flutter/engine#24999) * a25b0de Roll Fuchsia Mac SDK from xOxFrRRO6... to pmsuWkRQA... (flutter/engine#24997) * 3270c87 Roll Skia from 4fb13e026b6b to 1aa25c3217b6 (25 revisions) (flutter/engine#25001) * 96d5104 [deferred components] Handle base module loading units (flutter/engine#24983) * 6a3d8c7 Roll Skia from 1aa25c3217b6 to ead52dc068d5 (1 revision) (flutter/engine#25005) * 0f52360 Hardware Keyboard: macOS (flutter/engine#23469)

The macOS changes for the Hardware Keyboard project.

chunhtai · 2021-04-13T22:19:23Z

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

 - (instancetype)initWithViewController:(FlutterViewController*)viewController {
  self = [super init];
  if (self != nil) {
-    _flutterViewController = viewController;


Why do we remove the _flutterViewController? if we do want to remove it, we should remove all the reference to it and the private property as well. cc @dkwingsmt

The reason I removed it is because it's not used after initializing _channel.

It's indeed an oversight for me not to remove the private property. I can submit a new PR if you agree with the removal.

A later commit reused flutterViewController #24867, I think we should add it back

Sure. Sorry for the confusion I caused.
Although I see #25524 is pending to fix the caret, can you just ask him to include this change?

@dkwingsmt, #25524 already does it.

The macOS changes for the Hardware Keyboard project.

dkwingsmt and others added 3 commits November 18, 2020 22:33

Merge pull request #1 from flutter/master

29bcc33

Up to 201118

Merge branch 'master' of https://github.com/flutter/engine

550c23f

Split macos changes out

011ea61

flutter-dashboard bot added the platform-ios label Jan 6, 2021

googlebot added the cla: yes label Jan 6, 2021

dkwingsmt mentioned this pull request Jan 6, 2021

☂️ Device-specific keyboard events should happen in the embedder flutter/flutter#44918

Closed

dkwingsmt added 24 commits February 22, 2021 15:59

Merge remote-tracking branch 'upstream/master' into keyboard-macos

1b5ceeb

Compile

9f17107

Refactor channel to be a handler

faf23c8

keyboard manager

ce16bc4

Merge remote-tracking branch 'upstream/master' into keyboard-macos

b043711

Basic channel test compile

6b47a0e

More tests for channel

a9728fd

Add embedder test

52aba7d

Implement responding

ce9ed0b

Resume view controller tests

ce5dae1

Change key event gen

c26c23a

Refactor: remove ofType

09adc79

Shift and A unit test

1f6f920

IdentifyLeftAndRightModifiers

963fca5

Modifier synthesizing test

de0fa2e

Refactor tests

0ec1292

Add capslock test

4d62a9d

Numpad and F1 keys

9aec952

Add character filtering

3793b0a

Simplify logic and more assertion

99b4abb

Rewrite modifier algorithm

c508b6f

Compute mask

8b051f4

Add modifier sync test and assertion

5eb0f28

Stricter state update

43a16cb

dkwingsmt added 8 commits March 11, 2021 15:50

Merge remote-tracking branch 'upstream/master' into keyboard-macos

47f6c68

Renames

e950e23

More rename fixes

6143ed9

Format

88957a7

Compile

4618dcd

FlutterKeyCallbackGuard

fb51a67

init check

8dd318e

Fix review

6c6bb8b

dkwingsmt requested a review from gspencergoog March 12, 2021 03:21

gspencergoog reviewed Mar 12, 2021

View reviewed changes

Fix docs

eabdde4

dkwingsmt added 2 commits March 15, 2021 17:40

Doc for lastModifierFlags

ad270a8

Merge remote-tracking branch 'upstream/master' into keyboard-macos

2178dc5

dkwingsmt requested a review from gspencergoog March 16, 2021 00:40

Rename

8756067

gspencergoog approved these changes Mar 16, 2021

View reviewed changes

Fix test

5959edc

dkwingsmt merged commit 0f52360 into flutter:master Mar 16, 2021

dkwingsmt deleted the keyboard-macos branch March 16, 2021 05:35

engine-flutter-autoroll mentioned this pull request Mar 16, 2021

Roll Engine from 20375cebcf35 to 0f52360d12d5 (12 revisions) flutter/flutter#78300

Merged

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

0f52360 Hardware Keyboard: macOS (flutter/engine#23469)

64ef7d7

gspencergoog mentioned this pull request Mar 17, 2021

Infinite loop in Mac text field flutter/flutter#77855

Closed

hjfreyer pushed a commit to hjfreyer/engine that referenced this pull request Mar 22, 2021

Hardware Keyboard: macOS (flutter#23469)

2c32cd5

The macOS changes for the Hardware Keyboard project.

chriscraws pushed a commit to chriscraws/engine that referenced this pull request Mar 23, 2021

Hardware Keyboard: macOS (flutter#23469)

5b1950b

The macOS changes for the Hardware Keyboard project.

chunhtai reviewed Apr 13, 2021

View reviewed changes

duanqz pushed a commit to duanqz/engine that referenced this pull request Apr 16, 2021

Hardware Keyboard: macOS (flutter#23469)

d4d9597

The macOS changes for the Hardware Keyboard project.

dkwingsmt mentioned this pull request Apr 27, 2021

Hardware keyboard: codegen flutter/flutter#73440

Merged

13 tasks

Hardware Keyboard: macOS #23469

Hardware Keyboard: macOS #23469

Uh oh!

Conversation

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

Uh oh!

Description

Related Issues

Tests

Checklist

Reviewer Checklist

Breaking Change

Uh oh!

dkwingsmt commented Mar 12, 2021

Uh oh!

gspencergoog Mar 12, 2021

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Mar 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkwingsmt commented Mar 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gspencergoog left a comment

Choose a reason for hiding this comment

Uh oh!

chunhtai Apr 13, 2021

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Apr 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai Apr 13, 2021

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Apr 13, 2021

Choose a reason for hiding this comment

Uh oh!

chunhtai Apr 13, 2021

Choose a reason for hiding this comment

Uh oh!

knopp Apr 13, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dkwingsmt commented Jan 6, 2021 •

edited

Loading

dkwingsmt Mar 16, 2021 •

edited

Loading

gspencergoog Mar 12, 2021 •

edited

Loading

dkwingsmt commented Mar 16, 2021 •

edited

Loading

dkwingsmt Apr 13, 2021 •

edited

Loading