[macOS, Keyboard] Derive keyboard layout using printable information from system #32152

dkwingsmt · 2022-03-21T09:26:27Z

This PR changes how Flutter derives logical keys on macOS. By gathering the keyboard layout information and analyzing them as a whole, Flutter can build the key map and fix problems that can not be overcome by calculating on a per-key basis.

This addresses the first phase of flutter/flutter#100456 on macOS, but requires flutter/flutter#100803 to apply to RawKeyboard and Shortcut.

Effect

With this PR, it's guaranteed that there are always keys that are mapped to each of the 10 digit keys and 26 alphabet keys. Additionally, Shift-Digit keys now produces digits just like when they're unmodified, instead of symbols. Examples:

When pressing physical keys...	on layout	Previous logical keys	New logical keys
1	US	`digit1`	`digit1` (unchanged)
Shift-1	US	`shiftLeft + exclamation`	`shiftLeft + digit1`
1	French	`ampersand`	`digit1`
Shift-1	French	`shiftLeft + digit1`	`shiftLeft + digit1` (unchanged)
a	Russian	`ф`	`keyA`
]	Russian	`ъ`	`bracketRight`

Other keys that are not related to alnums are not changed.

Resulting French layout:

Resulting Russian layout:

There is a duplicate key, which we should resolve in the future.

How?

Mac actually has a function that tells you "if you press this key with this modifier state and dead key state right now, you will get this character". By traversing through this function on all key codes, Flutter can get a full picture without key events, which is important because Flutter need to know "are there any keys that produces character 'a'?" before deciding which key to map to KeyA.

The tricky part is that this requires functions only available in Carbon, which Apple has been announced as deprecated, and even removed from documentation since Catalina. However, Chromium and Electron still uses it (here), as well as all code examples (as far as I can see) regarding this feature. I've discussed with @chinmaygarde, who approved it since it's not a private function.

Detail logic

For each key code, derives its "layout clue", which is the printable character when it's pressed, and when it's pressed with Shift.

If a key has any clue that is an alnum, map this key to that alnum.
- This makes sure that French keyboard's alnum keys are property assigned.
If the key's both clues are non-ASCII, map this key to US layout.
- Non-latin keyboards often maps symbol keys to their non-latin keys. For example, Russian layout maps [ and ] to х and ъ respectively. To avoid returning any non-latin keys (because no one will use them), we map them back to US layout.
If there are remaining alnum keys that are not mapped, map them according to US layout.
- This maps all alnum keys, whether the layout is latin or not.
All unmapped keys are derived using the old algorithm.

Beautiful?

The way to pass data around is kind of ugly. This PR adds a new property to PrimaryKeyResponder to notify responders of the map, and also a new field specifiedLogicalKey in RawKeyEventMacos's message, making it less "raw". Partly it's because we have too little time to migrate unit tests for a "beautiful" design, but also I can't think of other ways for the specifiedLogicalKey field except for a different name.

Breaking?

This change can be considered breaking, because it changes what keys are dispatched under certain conditions. But people have been complaining about unintuitive logical key behavior and broken shortcuts. Additionally, this PR aligns the behavior of macOS with that on Windows. So, we can also consider this a fix.

Future work

This changes have not changed the mapping strategy for symbol keys. For example, = is mapped to equal, but Shift + = is mapped to plus, which can be confusing. Also, some keyboards have dead keys for both clues.

Finding a strategy for these keys is harder and might be more controversial. We'll leave it for the future.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Remove log add sync; clean code Format

flutter-dashboard · 2022-03-25T22:23:56Z

This pull request is not mergeable in its current state, likely because of a merge conflict. Pre-submit CI jobs were not triggered. Pushing a new commit to this branch that resolves the issue will result in pre-submit jobs being scheduled.

zanderso · 2022-03-31T20:18:35Z

From PR review triage: Ping @gspencergoog

zanderso · 2022-03-31T20:19:42Z

The lint failures in presubmit look related to this PR:

Failures for clang-tidy on /opt/s/w/ir/cache/builder/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterKeyboardManagerUnittests.mm:
/opt/s/w/ir/cache/builder/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterKeyboardManagerUnittests.mm:18:1: warning: do not use namespace using-directives; use using-declarations instead [google-build-using-namespace]
using namespace flutter::testing::keycodes;
^
/opt/s/w/ir/cache/builder/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterKeyboardManagerUnittests.mm:334:7: error: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119 [clang-analyzer-security.insecureAPI.strcpy,-warnings-as-errors]
      strcpy(newCharacter, event->character);
      ^~~~~~
/opt/s/w/ir/cache/builder/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterKeyboardManagerUnittests.mm:334:7: note: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119
      strcpy(newCharacter, event->character);
      ^~~~~~

gspencergoog

gspencergoog · 2022-03-25T23:10:30Z

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

+
+// Someohow this pointer type must be defined as a single type for the compiler
+// to compile the function pointer type (due to _Nullable).
+typedef NSResponder* _NSResponderPtr;


This appears to be duplicated from above, and the _NSResponderPtr too.

gspencergoog · 2022-03-25T23:12:08Z

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

+#endif
+    // The logical key should be the first available clue from below:
+    //
+    //  - Mandatory goal, if matches any clue. This ensures that all alnum keys


Suggested change

// - Mandatory goal, if matches any clue. This ensures that all alnum keys

// - Mandatory goal, if it matches any clue. This ensures that all alnum keys

gspencergoog · 2022-03-25T23:15:20Z

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

 // Record embedder calls to the given storage.
 //
-// They are not responded to until the stored callbacks are manually called.
+// They are not to responded until the stored callbacks are manually called.


Actually, "responded to" was correct.

gspencergoog · 2022-03-31T20:27:02Z

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

 namespace flutter::testing {

 namespace {
+using namespace flutter::testing::keycodes;


Can't use using namespace, it's against the style guide:

https://google.github.io/styleguide/cppguide.html#Namespaces

It's okay to use using for a particular symbol or symbols.

gspencergoog · 2022-03-31T20:29:37Z

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

 };

+/**
+ * Returns the current unicode layout data (kTISPropertyUnicodeKeyLayoutData).


Suggested change

* Returns the current unicode layout data (kTISPropertyUnicodeKeyLayoutData).

* Returns the current Unicode layout data (kTISPropertyUnicodeKeyLayoutData).

…rmation from system (flutter/engine#32152)

dkwingsmt added 2 commits March 19, 2022 23:23

Impl

ad8b644

Remove log add sync; clean code Format

Impl

3efa8eb

flutter-dashboard bot added the platform-macos label Mar 21, 2022

dkwingsmt added 17 commits March 22, 2022 22:07

New Impl

356742a

Fix dead key

565ca20

Rename to goals and clues

5b1a7d3

CFRelease source

4b31a2f

Move to view controller

60a65a9

No global

36e249d

First-phase

2c2f2dc

No weak in dealloc

ad195e0

Tests

e444702

One more test

860a731

Format

432098f

Format

3b62474

Docs

5f09bcb

Tests

5949a20

Merge branch 'mac-key-full-queue' into macos-keyboard-detection

8b1d4e9

Docs

a60da0c

Format

f02c9fc

dkwingsmt changed the title ~~[macOS, Keyboard] Detect keyboard layout and enforce mandatory logical keys~~ [macOS, Keyboard] Derive keyboard layout using printable information from system Mar 25, 2022

dkwingsmt requested a review from gspencergoog March 25, 2022 22:22

dkwingsmt marked this pull request as ready for review March 25, 2022 22:23

dkwingsmt added 5 commits March 25, 2022 15:32

Merge remote-tracking branch 'origin/main' into macos-keyboard-detection

c002a5e

Fix compile

6d07a04

Compile

c69729e

Add channel test

c512ae0

Format

04249c7

dkwingsmt mentioned this pull request Mar 25, 2022

RawKeyboardMacos accepts a new field "specifiedLogicalKey" flutter/flutter#100803

Merged

8 tasks

gspencergoog approved these changes Mar 31, 2022

View reviewed changes

dkwingsmt added 7 commits March 31, 2022 17:40

Merge remote-tracking branch 'origin/main' into macos-keyboard-detection

9d6897d

Fix comments

ffa110d

Merge remote-tracking branch 'origin/main' into macos-keyboard-detection

ed81035

Fix keycodes

1ee95e3

Fix namespaces

1aa0aec

Format

4bd44e1

strlcpy

984d7e0

dkwingsmt merged commit 2cddca6 into main Apr 1, 2022

dkwingsmt deleted the macos-keyboard-detection branch April 1, 2022 18:57

engine-flutter-autoroll mentioned this pull request Apr 1, 2022

Roll Engine from 5f2b73e49eca to 444cf45fd2e8 (2 revisions) flutter/flutter#101207

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 1, 2022

2cddca6 [macOS, Keyboard] Derive keyboard layout using printable info…

a54db98

…rmation from system (flutter/engine#32152)

engine-flutter-autoroll mentioned this pull request Apr 1, 2022

Roll Engine from 5f2b73e49eca to 69138cca1354 (4 revisions) flutter/flutter#101213

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 1, 2022

2cddca6 [macOS, Keyboard] Derive keyboard layout using printable info…

713b24a

…rmation from system (flutter/engine#32152)

dkwingsmt mentioned this pull request Apr 14, 2022

[Linux, Keyboard] Derive keyboard layout using printable information from system #32665

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[macOS, Keyboard] Derive keyboard layout using printable information from system #32152

[macOS, Keyboard] Derive keyboard layout using printable information from system #32152

Uh oh!

dkwingsmt commented Mar 21, 2022 •

edited

Loading

Uh oh!

flutter-dashboard bot commented Mar 25, 2022

Uh oh!

zanderso commented Mar 31, 2022

Uh oh!

zanderso commented Mar 31, 2022

Uh oh!

gspencergoog left a comment

Uh oh!

gspencergoog Mar 25, 2022

Uh oh!

gspencergoog Mar 25, 2022

Uh oh!

gspencergoog Mar 25, 2022

Uh oh!

gspencergoog Mar 31, 2022

Uh oh!

gspencergoog Mar 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	// - Mandatory goal, if matches any clue. This ensures that all alnum keys
	// - Mandatory goal, if it matches any clue. This ensures that all alnum keys

	* Returns the current unicode layout data (kTISPropertyUnicodeKeyLayoutData).
	* Returns the current Unicode layout data (kTISPropertyUnicodeKeyLayoutData).

[macOS, Keyboard] Derive keyboard layout using printable information from system #32152

[macOS, Keyboard] Derive keyboard layout using printable information from system #32152

Uh oh!

Conversation

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

Uh oh!

Effect

How?

Detail logic

Beautiful?

Breaking?

Future work

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Mar 25, 2022

Uh oh!

zanderso commented Mar 31, 2022

Uh oh!

zanderso commented Mar 31, 2022

Uh oh!

gspencergoog left a comment

Choose a reason for hiding this comment

Uh oh!

gspencergoog Mar 25, 2022

Choose a reason for hiding this comment

Uh oh!

gspencergoog Mar 25, 2022

Choose a reason for hiding this comment

Uh oh!

gspencergoog Mar 25, 2022

Choose a reason for hiding this comment

Uh oh!

gspencergoog Mar 31, 2022

Choose a reason for hiding this comment

Uh oh!

gspencergoog Mar 31, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dkwingsmt commented Mar 21, 2022 •

edited

Loading