Skip to content

Conversation

@KammererTob
Copy link
Contributor

@KammererTob KammererTob commented Sep 3, 2020

This is based on the UIKey introduction in 13.4 and this PR: flutter/engine#20972

Description

This PR generates RawKeyEventData for iOS and in turn RawKeyDownEvent and RawKeyUpEvent appropriately.
The mapping for the keyCodes to PhysicalKeyboardKeys was done based on this documentation: https://developer.apple.com/documentation/uikit/uikeyboardhidusage?language=objc, which seems to be a complete copy of the usage table found here: https://www.usb.org/sites/default/files/documents/hut1_12v2.pdf
I have not included every key listed there since they seemed to be very hardware specific, but i can add them if desired.

I also noticed that the RawKeyDownEvent has a character property which is filled with message['character']. This does not work for MacOs and iOS since it is called "characters" for both of them. I was not sure if this was intentional or an oversight. Should this be changed to "character" for iOS and MacOS?

This PR depends on an engine PR: flutter/engine#20972, although it should not be breaking even without this.

Related Issues

Tests

I added iOS as a platform to the already excellent tests for the other platforms. I also added specific tests for iOS in the style of other tests already existing for other platforms.

Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I signed the [CLA].
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

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

@flutter-dashboard flutter-dashboard bot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. labels Sep 3, 2020
@KammererTob KammererTob marked this pull request as draft September 3, 2020 20:49
@KammererTob
Copy link
Contributor Author

I just realized that i did not use gen_keycodes.dart to generate the key map. Will work on that.

@flutter-dashboard
Copy link

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

@KammererTob KammererTob marked this pull request as ready for review September 5, 2020 13:00
@flutter-dashboard flutter-dashboard bot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Sep 5, 2020
@KammererTob
Copy link
Contributor Author

KammererTob commented Sep 5, 2020

I have now made the iOS key code mapping generatable and collectable from the chromium files. This works since the key codes used for iOS are bascially the usbHidCodes (with only very few missing).
Few notes:

  • The gen_keycodes.dart also generates header (.h) files inside the engine/src/shell/platform/.../keycodes folder. I did not find any usage for those files and they also do not appear to be checked in. I am mentioning this since i had to split the darwin file into two: keyboard_map_ios.h and keyboard_map_macos.h
  • I removed the // @dart = 2.8 comment on top of the keyboard_maps.tmpl file since this lead to errors when importing the generated file
  • The raw_keyboard_ios.dart file contains a _kiOSToLogicalMap map, which defines a mapping for special non-visible keys in iOS: https://developer.apple.com/documentation/uikit/uikeycommand/input_strings_for_special_keys?language=objc. I was unsure if this is the right place for the mapping, but i felt like it would be wrong to put this very specific use-case in the keyboard_maps.dart file.

@gspencergoog
Copy link
Contributor

@KammererTob Thanks SO MUCH for this PR. This is clearly a lot of work, and we have needed keyboard support on iOS for quite a while now.

I'll be looking over this PR and sending you some feedback soon, but I just wanted to say "thank you" first.

Copy link
Contributor

@gspencergoog gspencergoog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this looks quite good, just some naming issues.

@gspencergoog
Copy link
Contributor

  • The gen_keycodes.dart also generates header (.h) files inside the engine/src/shell/platform/.../keycodes folder. I did not find any usage for those files and they also do not appear to be checked in. I am mentioning this since i had to split the darwin file into two: keyboard_map_ios.h and keyboard_map_macos.h

They aren't actually used yet, but are part of our (currently underway) effort to migrate all of this platform specific code into the engine. (see https://flutter.dev/go/platform-based-key-events)

  • I removed the // @dart = 2.8 comment on top of the keyboard_maps.tmpl file since this lead to errors when importing the generated file

Thanks, this should have been removed as part of our non-nullable-by-default migration which is currently in progress.

That seems like the right place to put it to me.

@gspencergoog
Copy link
Contributor

FYI @dkwingsmt, who is doing the platform based key event migration.

@dkwingsmt
Copy link
Contributor

@gspencergoog Thanks! I can't find any issues that gspencergoog hasn't. (There is yet another case of IOS that probably should be changed to Ios).
Also great PR!

@KammererTob
Copy link
Contributor Author

Thanks for the review and explanations! I have made the requested changes, and also renamed some more variables to be more in line with the requested naming scheme (if this went too far, then i will revert it if needed).

Copy link
Contributor

@gspencergoog gspencergoog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

32384589-a60f0e74-c078-11e7-9bc1-e5b5287aea9d

I could merge this, but until you land the engine change, it obviously won't do anything, so I'd like to wait until you're close to landing the engine change, and land them about the same time so that people don't get confused as to why it looks like we have iOS key support, but it doesn't work.

@gspencergoog
Copy link
Contributor

@KammererTob looks like this needs to have a conflict resolved, but once that happens, I can land it, since the corresponding engine change has landed now.

@KammererTob
Copy link
Contributor Author

@gspencergoog I resolved the merge conflict so it should be g2g

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: tests "flutter test", flutter_test, or one of our tests c: contributor-productivity Team-specific productivity, code health, technical debt. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Framework does not handle key events sent by iOS simulator ( Xcode 12.2 ) Keyboard events should be implemented on iOS

5 participants