Re-implement hardware keyboard text selection. #42879

gspencergoog · 2019-10-17T00:18:18Z

Description

This re-implements keyboard text selection so that it will work on platforms other than Android (e.g. macOS, Linux, etc.).

Also, fixed a number of bugs in editing selection via a hardware keyboard (unable to select backwards, incorrect conversion to ASCII when cutting to clipboard, lack of support for CTRL-SHIFT-ARROW word selection, etc.).

Did not address the keyboard locale issues that remain, or add platform specific switches for the bindings. All that will need some more design work before implementing them.

Related Issues

Fixes #31951

Tests

Added a large number of test scenarios for text selection, and cut/copy/paste.

Breaking Change

No, this is not a breaking change.

justinmc

LGTM 👍

There is an existing issue about Gboard's directional soft keyboard not working: #9419. It looks like this PR will make it easier to get that sort of thing working, would you agree? Just making sure you're aware that we'll have to support that at some point.

justinmc · 2019-10-17T14:48:37Z

packages/flutter/lib/src/rendering/editable.dart

    onSelectionChanged(nextSelection, this, cause);
  }

-  // TODO(goderbauer): doesn't handle extended grapheme clusters with more than one Unicode scalar value (https://github.com/flutter/flutter/issues/13404).


Is this TODO obsolete now or should we keep it?

Actually, you're right, this does still apply, but only because some parts of this code depend upon the length of a string in code points instead of extended grapheme clusters. I'll restore it and add more info.

justinmc · 2019-10-17T15:06:13Z

packages/flutter/lib/src/services/keyboard_key.dart

+  /// It is used, for example, to make sets of keys with members like
+  /// [controlRight] and [controlLeft] and convert that set to contain just
+  /// [control], so that the question "is any control key down?" can be asked.
+  static Set<LogicalKeyboardKey> collapseSynonyms(Set<LogicalKeyboardKey> input) {


I love static methods. This could even be directly unit tested if you want.

LOL! Yeah, ok, added a test. Very subtle. :-)

justinmc · 2019-10-17T15:08:01Z

packages/flutter/lib/src/rendering/editable.dart

-        ),
-        cause,
-      );
+    if (onSelectionChanged == null) {


Why do all of these checks if onSelectionChanged exist here instead of one check inside of _handleSelectionChanged?

I think the idea was to avoid doing the work needed to get to _handleSelectionChanged, but I think I'll move this up to _handleKeyEvent, and also a check in _handleSelectionChanged, since there are other call sites for it that aren't protected.

Sounds good.

justinmc · 2019-10-17T15:09:10Z

packages/flutter/test/widgets/editable_text_test.dart

+
+    await tester.pump(); // Wait for autofocus to take effect.
+
+    Future<void> sendKeys(List<LogicalKeyboardKey> keys, {bool shift = false, bool control = false}) async {


I was wondering if we had a good way to test hardware keyboard keys 👍

Yes! I added that stuff last month because it was missing. It's turning out to be very useful.

gspencergoog · 2019-10-17T19:09:26Z

Yes, it could make it easier to implement a fix for #9419, but we'd have to convert those events into calls to a method here. Probably we'd have to subscribe to some listener for those events, and refactor this class to handle either those or key events. In any case, it shouldn't be too bad.

justinmc · 2019-10-17T20:50:20Z

Sounds good about #9419. I'm going to add some notes in there about this for when I get to it.

pchampio · 2019-10-28T10:13:29Z

Just to be sure.

If I understand correctly, this code handles text selection at the flutter framework level using RawKeyEvent.

Embedders have control over the text input (and text selection/position). And existing embedders are making use of the flutter/textinput method channel to send text, cursor position, and in our case text selection & copy/paste.

On go-flutter, work has been done to implement keyboard selection/copy/paste at the embedder level. textinput.go.

My question is, should go-flutter stop using the flutter/textinput channel for text selection/copy/paste? (implies delete of textinput_{darwin.,linux,windows}.go that were used to distinguish platform specific bindings)

The linux shell also uses flutter/textinput method channel for cursor position text_input_plugin.cc.

This PR isn't aligned with embedders, and handles are duplicate.
Other than #31951, there seems to be another related issue: #36316

My very personal opinion, I think putting these handles in the flutter framework makes embedders lighter which is a good thing. Waiting for #31366 to be merged so that platform specific bindings can be addressed.
I feel like the flutter/keyevent method channel is doing a lot, maybe add documentation on https://api.flutter.dev/flutter/services/SystemChannels/keyEvent-constant.html ?

…4130) This adds support for the command key for text selection/editing on macOS. I had ported the text editing code (in #42879), but forgot to add support for the command key itself. This also adds a test that tests the text editing on multiple platforms instead of just testing Android. There appears to still be a bug (filed #44135) where we're losing key events sometimes on macOS, leaving some keys "stuck" on, but this PR at least allows the right key combinations to be used.

This re-implements keyboard text selection so that it will work on platforms other than Android (e.g. macOS, Linux, etc.). Also, fixed a number of bugs in editing selection via a hardware keyboard (unable to select backwards, incorrect conversion to ASCII when cutting to clipboard, lack of support for CTRL-SHIFT-ARROW word selection, etc.). Did not address the keyboard locale issues that remain, or add platform specific switches for the bindings. All that will need some more design work before implementing them. Related Issues Fixes flutter#31951

fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Oct 17, 2019

googlebot added the cla: yes label Oct 17, 2019

gspencergoog added 4 commits October 16, 2019 17:21

Convert selection to work on more platforms than Android

5bc8708

Checkpoint

57d6e5d

Fixed bugs

29d7aa2

Added lots of tests

d4c5f3c

gspencergoog force-pushed the keyboard_text_selection branch from 665a38d to d4c5f3c Compare October 17, 2019 00:21

gspencergoog requested a review from justinmc October 17, 2019 00:31

justinmc approved these changes Oct 17, 2019

View reviewed changes

Review Changes

4175d5a

gspencergoog force-pushed the keyboard_text_selection branch from 012c244 to 739b5c8 Compare October 17, 2019 20:16

justinmc mentioned this pull request Oct 17, 2019

Selection controls in GBoard for Android fail to work #9419

Closed

Fix Test

38c1485

gspencergoog force-pushed the keyboard_text_selection branch from 739b5c8 to 38c1485 Compare October 17, 2019 21:27

gspencergoog merged commit a7aa661 into flutter:master Oct 17, 2019

gspencergoog deleted the keyboard_text_selection branch October 18, 2019 00:57

hpoul mentioned this pull request Oct 18, 2019

[macos] Modifier keys are not detected correctly (isShiftPressed, etc) #43007

Closed

pchampio mentioned this pull request Oct 28, 2019

Support multi-line text editing on desktop platforms #36316

Closed

5 tasks

gspencergoog mentioned this pull request Nov 4, 2019

Add command key bindings to macOS text editing and fix selection. #44130

Merged

1 task

pchampio mentioned this pull request Nov 18, 2019

hardware keyboard text selection go-flutter-desktop/go-flutter#314

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2021


		await tester.pump(); // Wait for autofocus to take effect.

		Future<void> sendKeys(List<LogicalKeyboardKey> keys, {bool shift = false, bool control = false}) async {

Re-implement hardware keyboard text selection. #42879

Re-implement hardware keyboard text selection. #42879

Uh oh!

Conversation

gspencergoog commented Oct 17, 2019

Description

Related Issues

Tests

Breaking Change

Uh oh!

justinmc 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!

gspencergoog commented Oct 17, 2019

Uh oh!

justinmc commented Oct 17, 2019

Uh oh!

pchampio commented Oct 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pchampio commented Oct 28, 2019 •

edited

Loading