Support all keyboard actions. (#11344) by matthew-carroll · Pull Request #18855 · flutter/flutter

matthew-carroll · 2018-06-26T22:40:24Z

Support all keyboard actions for iOS and Android. (#11344)

There is a corresponding PR for the engine:
flutter/engine#5620

chinmaygarde · 2018-06-26T22:45:39Z

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

+  previous,
+
+  /// Complete the text input operation. iOS only, corresponds to "continue."
+  continue_action,


I think this needs to be camelcase instead of underscore case (like the emergencyCall member below).

Hixie · 2018-06-26T22:48:19Z

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

+
+  /// iOS only.
+  emergencyCall,
+


done, join, route, and emergencyCall need more documentation.

The ones that say "Android only" or "iOS only" should say what they do on the other platforms.

@Hixie you mentioned they should "say what they do" - do you mean semantically? Because these buttons don't do anything unless the developer implements something. I think they're all presentation options, perhaps with intended results.

Well for example for the others you say what they correspond to on Android.

For all of the values in the enum it'd be good to say what to expect the keyboard to look like on each platform.

Hixie · 2018-06-26T22:51:18Z

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

+        'The requested TextInputAction "$inputAction" is not supported on Android.',
+      );
+    }
+  }


If this is debug-only behaviour, then the method should be called "_debugEnsure..." (starting with "debug") and the entire body should be in an assert. And it should be called from an assert. Also this should be very clearly documented; my initial guess based on the docs earlier was that unsupported values would just do the same as "unspecified" on unsupported platforms, the same way that the value that only works on recent iOS versions works. (speaking of which, that should also be documented)

@Hixie Can you explain the difference between debug and not debug?

And you're saying a method with assertions should be called from an assert? Or you're saying this method should throw exceptions and then the method should be called from an assert?

Do you think the documentation should go at the Widget level or within text_input.dart or both?

Lastly, I think the default that we currently have for an unrecognized action is actually "done", not "unspecified". Do you think the default for a bad input should be "unspecified"? Also, do you think that passing in the wrong platforms action should be a silent failure to "unspecified" instead of doing assertions?

This is what I asked about previously with regard to debug logging vs crashing and I went with the judgement call to crash.

Flutter has several modes, of which "debug" and "release" are the prominent ones. In "debug" mode (what you use during development), asserts run. In release mode they do not.

Some methods only do things in debug mode, because in release mode they have no side-effects. The method you wrote here is an example of such a method. We call these debug-only methods. Our style guide says that debug-only identifiers should be prefixed with "debug".

The style guide further says that debug methods should only be called from debug code, i.e. from within an assert.

Do you think the documentation should go at the Widget level or within text_input.dart or both?

Probably both. They're different layers -- someone could write their own framework on top of text_input.dart, so we need to document what it does at that level. But similarly, most people will use the widget layer, so the widgets should document what they do too.

Lastly, I think the default that we currently have for an unrecognized action is actually "done", not "unspecified". Do you think the default for a bad input should be "unspecified"?

I don't have a strong opinion. I can see the logic in using the platform default, since it's presumably the default for a reason. But also "done" seems to make sense as a default. So... your call.

Also, do you think that passing in the wrong platforms action should be a silent failure to "unspecified" instead of doing assertions?

I don't have a strong opinion. In release builds the assertions don't do anything, so by using assertions we're saying there's one behaviour in release builds and one in debug builds. So we should think about what we want to have happen in release builds regardless. We should also consider how much we want to support people developing on Android from iOS and vice versa. The gallery has a toggle to switch platforms; how would this work with this feature? Do we expect people to use the platform-specific options? Do we expect them to only use them on platform-specific apps? All these questions are some we should consider here.

I renamed the debug method and moved it into an assert. I added docs to both the TextInputConfiguration and EditableText that explain platform differences for input types. I also updated the engine behavior so that on both iOS and Android if an unrecognized input type value is sent, the OS will go with "default" or "unspecified" depending on which platform you're talking about.

Hixie · 2018-06-26T22:51:44Z

packages/flutter/lib/src/widgets/editable_text.dart

+  /// behaviors based on the situation:
+  ///
+  ///  - When a completion action is pressed, such as "done," "go," "send," or
+  ///    "search," the user's content is submitted to the [controller] and then


"search",, with the comma outside the quotes, since the action isn't called search, with a comma.

Hixie · 2018-06-26T22:51:49Z

packages/flutter/lib/src/widgets/editable_text.dart

+  ///    "search," the user's content is submitted to the [controller] and then
+  ///    focus is given up.
+  ///
+  ///  - When a non-completion action is pressed, such as "next" or "previous,"


Hixie · 2018-06-26T22:52:16Z

packages/flutter/lib/src/widgets/editable_text.dart

  /// Called when the text being edited changes.
  final ValueChanged<String> onChanged;

+  /// Called when the user submits editable content (ex: user presses the "done"


can you elaborate on what you're requesting here?

the syntax s/pattern/replacement/ is used in various unix programs (sed, vim, perl, etc) to mean "replace the 'pattern' part with 'replacement'". So in this case, I meant "replace the "ex" (in the parenthetical) with "e.g."".

Hixie · 2018-06-26T22:54:02Z

packages/flutter/lib/src/widgets/editable_text.dart

@@ -280,9 +281,30 @@ class EditableText extends StatefulWidget {
  /// The type of keyboard to use for editing the text.
  final TextInputType keyboardType;


somewhere (maybe the class documentation) we should have a section that talks about the event lifecycle (e.g. that onEditingComplete happens before onSubmitted but after onChanged, or whatever)

I added a few lines to the class docs. Let me know if you have any comments.

Hixie · 2018-06-26T22:54:25Z

packages/flutter/lib/src/widgets/editable_text.dart

+  ///    another input widget within [onSubmitted].
+  ///
+  /// Providing [onEditingComplete] prevents the aforementioned default behavior.
+  final VoidCallback onEditingComplete;


this seems to be missing the constructor argument

Hixie · 2018-06-26T22:54:37Z

tests?

matthew-carroll · 2018-06-26T22:59:12Z

@Hixie Yes, tests are needed. Do we have any good examples of writing effective tests across the framework/engine boundary?

…t EditableText submission lifecycle.

Hixie · 2018-06-27T03:12:18Z

The main thing I think needs testing here is the event handling in the text field, which you should be able to do entirely on the framework side.

For the channels, there's three kinds of tests you can write that I can think of:

integration tests using devicelab (tests that communication across framework and engine works, and that it works on devices)
framework-side tests, with a mocked engine (unit test in flutter repo)
engine-side tests, with a mocked framework (unit test in engine repo)

…ds exceptions during execution.

matthew-carroll · 2018-06-28T23:46:24Z

@Hixie I've added tests for this PR and also a test for the fix to receiveAction(). If things look good to you then I'll merge in, along with the engine PR.

Hixie · 2018-06-28T23:56:58Z

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

+      );
+    }
+    return true;
+  }


we should wrap all of this in an assert to make it clear that this does nothing in release builds

The invocation is already wrapped in an assert. You want a 3rd one? An assert calling an assert comprised of 2 asserts?

Hixie · 2018-06-28T23:57:38Z

packages/flutter/lib/src/widgets/editable_text.dart

+/// receives focus. For example, providing iOS's "emergencyCall" action when
+/// running on an Android device will result in an error. Appropriate
+/// [inputAction]s can be chosen by checking the current platform and then
+/// selecting the appropriate action.


the error's only thrown in debug mode, right? the docs are unclear about this.

Hixie · 2018-06-28T23:59:00Z

packages/flutter/test/widgets/editable_text_test.dart


+  // Tests that the desired keyboard action button is requested.
+  //
+  // I.E., when an EditableText is given a particular [action], Flutter


i.e. is lowercase. To avoid starting with a lowercase letter consider "In other words" or some other phrase. (I'm not sure "i.e." or "in other words" really works here though?)

Hixie · 2018-06-28T23:59:09Z

packages/flutter/test/widgets/editable_text_test.dart

+              textInputAction: action,
+              style: textStyle,
+              cursorColor: cursorColor,
+            ))));


this is not flutter style

That actually isn't new code, but I'll fix it.

In fact there's a lot of it so I'll correct it everywhere.

Somehow my comments here disappeared. This isn't actually new code, but this format issue is in this file a number of times so I'll correct them all.

Hixie · 2018-06-29T00:00:24Z

packages/flutter_test/lib/src/test_text_input.dart

-          'TextInputClient.performAction',
-          <dynamic>[_client, action.toString()],
+  Future<Null> receiveAction(TextInputAction action) async {
+    return TestAsyncUtils.guard(() async {


why is the inner one async? i don't see any awaits and you return a Future, so you're not relying on the async wrapping behaviour.

Hixie · 2018-06-29T00:01:16Z

packages/flutter_test/lib/src/test_text_input.dart

+
+            // No error was found. Complete without issue.
+            completer.complete();
+          } catch (e) {


style guide says to avoid abbreviations; we usually call this error or exception.

Hixie · 2018-06-29T00:03:13Z

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

  /// The action to take when the enter button is pressed in a multi-line
  /// text field (which is typically to do nothing).
  newline,
 }


the docs for this enum still seem like they could be more detailed, for example they could say which will cause the focus to be lost and which will not, those that assert on unsupported platforms could mention the assertion you get in debug mode and what happens in release mode, and it's not entirely clear what it means for one of these to "correspond" to something (e.g. what does it mean for "emergencyCall" to correspond to "Emergency Call"?).

I don't think documenting behavior here makes sense. No selection of a given value implies a reliable behavior. The definition of the behavior that we choose to use by default is found in 'performAction()' within editable_text.dart. But even those behaviors can be easily overridden to do what you want.

The fact that the button says "Emergency Call," for example, says nothing about what it does. Tapping that button on iOS does nothing unless you tell it to.

I'm also concerned about replicating debug vs released mode error reporting here. This would be the 3rd place we're stating that same thing, and I'm concerned that if something changes, we're making it very easy to forget to update one of those docs...

So I'm really not sure how much information we want here. For the most part these enums are simply direct replications of other enums on the platform so that developers can select what they want (supposedly based on their existing knowledge of those platforms). And to be clear, the only situation out of all of these enums where we are generalizing terms is the "unspecified" term. We send that term directly to Android, but for iOS we translate it to "default". But that's the only genericizing we do when compared to the actual platform names for the same thing.

Hixie · 2018-06-29T23:02:06Z

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

+/// For example: If the user presses the keyboard action button on iOS when it
+/// reads "Emergency Call," the result should not be a focus change to the next
+/// TextField. This behavior is not logically appropriate for a button that says
+/// "Emergency Call."


nit: we use the "programming" style of quotation in the docs, so the commas and periods should be after the quote marks.

(this also applies in other parts of these docs)

Yeah...gotta break this habit.

Hixie · 2018-06-29T23:02:56Z

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

+/// thrown. If the same thing is done in release mode, then instead of sending
+/// the inappropriate value, Android will use "unspecified" on the platform
+/// side and iOS will use "default" on the platform side.
+///


these docs are fantastic. thanks!

Hixie · 2018-06-29T23:14:00Z

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

+  /// iOS: Corresponds to iOS's "UIReturnKeyContinue". The title displayed in the
+  /// action button is "Continue". This action is only available on iOS 9.0+.
+  ///
+  /// Note: the reason that this value has "Action" post-fixed to it is because


Drop the "Note:", it's cleaner. :-)

Hixie · 2018-06-29T23:16:18Z

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

+  /// iOS: Corresponds to iOS's "UIReturnKeyDefault". The title displayed in the
+  /// action button is "return".
+  ///
+  /// Notice that [TextInputAction.newline] is a term that exists in Flutter


rather than "notice that .. is a term that exists ..", consider just "The term .. exists .."

Hixie · 2018-06-29T23:17:15Z

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

+        assert(
+        _androidSupportedInputActions.contains(inputAction),
+        'The requested TextInputAction "$inputAction" is not supported on Android.',
+        );


nit: the insides of the inner asserts are now underindented

Hixie · 2018-06-29T23:17:31Z

modulo minor nits

Support all keyboard actions. (flutter#11344)

fe7772d

matthew-carroll requested review from Hixie, abarth, chinmaygarde, gspencergoog and xster June 26, 2018 22:40

googlebot added the cla: yes label Jun 26, 2018

This was referenced Jun 26, 2018

Support all keyboard actions. (#11344) flutter/engine#5620

Merged

Would like "next" form behavior in Keyboard #11344

Closed

chinmaygarde reviewed Jun 26, 2018

View reviewed changes

Hixie reviewed Jun 26, 2018

View reviewed changes

Matt Carroll added 2 commits June 26, 2018 17:43

PR Updates.

dab39b8

Extended documentation for input actions. Extended documentation abou…

60837c9

…t EditableText submission lifecycle.

Matt Carroll added 8 commits June 27, 2018 15:05

Documentation updates, debug method renamed and moved into assertion.

d3f5440

Added tests.

3766588

Changed some assertions to expect()s

ca69706

Merge branch 'master' into 11344_support_all_keyboard_actions

7d3297c

Removed unused import.

35b4c65

Added test for TestTextInput.receiveAction() to ensure that it forwar…

3560378

…ds exceptions during execution.

Fixed tests.

a82ca14

Fixed more tests.

da78044

Hixie reviewed Jun 28, 2018

View reviewed changes

Hixie reviewed Jun 29, 2018

View reviewed changes

Matt Carroll added 4 commits June 28, 2018 17:28

PR Updates

27091fe

Added an assertion to text_input

7fe7a4d

Fixed assertion.

7d570de

Updated TextInputAction docs.

f867cd3

Hixie reviewed Jun 29, 2018

View reviewed changes

Doc adjustments.

c2cb7bb

matthew-carroll merged commit 2a65505 into flutter:master Jul 2, 2018

matthew-carroll mentioned this pull request Jul 2, 2018

Support for customizing return key #17525

Closed

zoechi mentioned this pull request Jul 3, 2018

onSubmitted with TextField when pressing Enter key #19027

Closed

sir-boformer mentioned this pull request Jul 16, 2018

Add textInputAction constructor paramater to TextFormField #19397

Closed

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

		@@ -280,9 +281,30 @@ class EditableText extends StatefulWidget {
		/// The type of keyboard to use for editing the text.
		final TextInputType keyboardType;

Conversation

matthew-carroll commented Jun 26, 2018

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!

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!

Hixie commented Jun 26, 2018

Uh oh!

matthew-carroll commented Jun 26, 2018

Uh oh!

Hixie commented Jun 27, 2018

Uh oh!

matthew-carroll commented Jun 28, 2018

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!

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