[TextInput] enroll in autofill by default #28333

LongCatIsLooong · 2021-08-26T23:31:34Z

Related: flutter/flutter#85554
Framework PR: https://github.com/flutter/flutter/pull/86312/files

The framework will be setting the autofill field to null to indicate that autofill is explicitly disabled on the text field (flutter/flutter#86312).
This PR allows the platform to make autofill attempts using heuristics when the framework sends a TextInputConfiguration that has 0 autofill hints.

@yjbanov I'll try to wrap each input field in a Form (and potentially make the autofill implementation a mixin) in a separate PR.

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.

…ong/engine into autofill-enabled-by-default

…by-default

…ong/engine into autofill-enabled-by-default

LongCatIsLooong · 2021-08-31T01:14:53Z

lib/web_ui/test/text_editing_test.dart

@@ -462,6 +479,11 @@ void testMain() {
      const MethodCall show = MethodCall('TextInput.show');
      sendFrameworkMessage(codec.encodeMethodCall(show));

+      final MethodCall setSizeAndTransform =


The "setSizeAndTransform" message has to be there since on some platforms (e.g. Desktop Safari) we don't put the input element on DOM until we get its correct dimensions from the framework.

Please put this in a comment in the code so people later can easily see why setSizeAndTransform is necessary.

mdebbar

I only reviewed the web part and it looks good overall with a couple comments.

mdebbar · 2021-08-31T19:00:31Z

lib/web_ui/test/text_editing_test.dart

@@ -462,6 +479,11 @@ void testMain() {
      const MethodCall show = MethodCall('TextInput.show');
      sendFrameworkMessage(codec.encodeMethodCall(show));

+      final MethodCall setSizeAndTransform =


Please put this in a comment in the code so people later can easily see why setSizeAndTransform is necessary.

mdebbar · 2021-09-01T18:06:04Z

lib/web_ui/lib/src/engine/text_editing/text_editing.dart

+      element.name = autofillHint;
+      if (placeholder != null) {
+        element.placeholder = placeholder;
+      }
+      element.autocomplete = autofillHint ?? 'on';
+      if (autofillHint == null) {
+        return;
+      }
+      element.id = autofillHint;
+      if (autofillHint.contains('password')) {
        element.type = 'password';
      } else {
        element.type = 'text';
      }
    } else if (domElement is html.TextAreaElement) {
-      final html.TextAreaElement element = domElement;
-      element.name = hint;
-      element.id = hint;
-      element.setAttribute('autocomplete', hint);
+      if (placeholder != null) {
+        domElement.placeholder = placeholder;
+      }
+      if (autofillHint != null) {
+        domElement.name = autofillHint;
+        domElement.id = autofillHint;
+      }
+      domElement.setAttribute('autocomplete', autofillHint ?? 'on');


Is there a reason these two are different? The implementation for input and textarea should remain mostly identical.

justinmc

LGTM 👍

justinmc · 2021-09-02T22:28:44Z

lib/web_ui/lib/src/engine/text_editing/text_editing.dart

+  /// The `fields` argument corresponds to the "fields" field in a
+  /// `TextInputConfiguration`.
+  ///
+  /// Returns null if the input field is not within an autofill group.


I appreciate the added docs here. We need an analyzer rule to require this like in the framework.

justinmc · 2021-09-02T22:39:12Z

lib/web_ui/lib/src/engine/text_editing/text_editing.dart

    } else if (domElement is html.TextAreaElement) {
-      final html.TextAreaElement element = domElement;


Not sure why this variable was created in the first place, good idea to remove it.

justinmc · 2021-09-02T22:40:59Z

lib/web_ui/lib/src/engine/text_editing/text_editing.dart


-  late TextEditingChannel channel;
+  late final TextEditingChannel channel = TextEditingChannel(this);


Does this still need late?

I feel like I'm wrong and I got tripped up by this recently...

I think late is still required because the right-hand-side expression refers to this.

Yes. This needs lazy initialization because this has to be initialized first, to be passed to the TextEditingChannel constructor, otherwise we'll have a circular dependency.

justinmc · 2021-09-02T22:42:10Z

lib/web_ui/test/text_editing_test.dart

@@ -462,6 +479,15 @@ void testMain() {
      const MethodCall show = MethodCall('TextInput.show');
      sendFrameworkMessage(codec.encodeMethodCall(show));

+      // The "setSizeAndTransform" message has to be here before we call
+      // checkInputEditingState, since on some platforms (e.g. Desktop Safari)
+      // we don't put the input element on DOM until we get its correct


Nit: "on DOM" => "into the DOM"

If that's correct and I'm not misunderstanding, I think it's more explicit this way.

Same for the duplicate comments below.

yjbanov

LGTM if LGT @mdebbar

yjbanov · 2021-09-02T23:30:55Z

lib/web_ui/lib/src/engine/text_editing/text_editing.dart

@@ -129,8 +129,9 @@ void _hideAutofillElements(html.HtmlElement domElement,

 /// Form that contains all the fields in the same AutofillGroup.
 ///
-/// These values are to be used when autofill is enabled and there is a group of
-/// text fields with more than one text field.
+/// These values are to be used when autofill is enabled (the default) on the


What does "These values" refer to? Is there a better way to phrase it?

yjbanov · 2021-09-02T23:31:24Z

lib/web_ui/lib/src/engine/text_editing/text_editing.dart

@@ -153,12 +154,23 @@ class EngineAutofillForm {
  /// See [formsOnTheDom].
  final String formIdentifier;

+  /// Creates an [EngineAutofillFrom] from the JSON representation of a flutter


nit: s/flutter/Flutter/

yjbanov · 2021-09-02T23:35:20Z

lib/web_ui/lib/src/engine/text_editing/text_editing.dart

@@ -287,7 +299,7 @@ class EngineAutofillForm {
            element.onInput.listen((html.Event e) {
              if (items![key] == null) {
                throw StateError(
-                    'Autofill would not work withuot Autofill value set');
+                    'Autofill would not work without Autofill value set');


Is this an error that could happen due to a framework-side bug? If so, can the error message be more specific? For example, would it help if it mentioned the key that was missing and why it was expected?

yjbanov · 2021-09-02T23:36:47Z

lib/web_ui/lib/src/engine/text_editing/text_editing.dart

-      required this.textCapitalization});
+      required this.autofillHint,
+      required this.textCapitalization,
+      this.placeholder});


nit: code style could be improved here, e.g.:

AutofillInfo({ required this.editingState, required this.uniqueIdentifier, required this.hint, required this.textCapitalization}); required this.autofillHint, required this.textCapitalization, this.placeholder, });

yjbanov · 2021-09-02T23:37:58Z

lib/web_ui/lib/src/engine/text_editing/text_editing.dart

@@ -364,42 +377,64 @@ class AutofillInfo {
  /// Used as a guidance to the browser as to the type of information expected
  /// in the field.
  /// See: https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/autocomplete
-  final String hint;
+  final String? autofillHint;


nit: is the first sentence in the dartdoc still useful? Seems obvious now that this is used for autofill.

yjbanov · 2021-09-02T23:41:47Z

lib/web_ui/lib/src/engine/text_editing/text_editing.dart


-  late TextEditingChannel channel;
+  late final TextEditingChannel channel = TextEditingChannel(this);


Does this still need to be late?

I think late is still required because the right-hand-side expression refers to this.

Yes: #28333 (comment)

mdebbar · 2021-09-04T00:58:29Z

lib/web_ui/lib/src/engine/text_editing/text_editing.dart

+          element.type = 'text';
+        }
+      }
+      element.autocomplete = autofillHint ?? 'on';


Do we want to enable autocomplete when autofill hint is null?

Yeah that's what the pull request trying to do: flutter/flutter#85554. If we want to do it on Android then the default behavior should probably be consistent across platforms (or does it make sense on the web?).

https://github.com/flutter/flutter/pull/86312/files tries to make so that even if the developer leaves the hint list empty (the default), EditableTextState will still generate an autofill JSON object for the text field when it's focused, and the autofill service can try to determine the type of the text field (or whether the field should be autofilled at all) using heuristics.
if the developer explicitly sets EditableText.autofillHints to null, then autofill will be disabled.

mdebbar · 2021-09-04T00:59:54Z

lib/web_ui/lib/src/engine/text_editing/text_editing.dart

+    final String? autofillHint = this.autofillHint;
+    final String? placeholder = this.placeholder;
+    if (autofillHint != null) {
+      domElement.id = autofillHint;


We are setting the id again below. Is this still necessary?

…) (#89463)

enable autofill by default

846462d

flutter-dashboard bot added platform-android platform-ios platform-web Code specifically for the web engine labels Aug 26, 2021

google-cla bot added the cla: yes label Aug 26, 2021

add tests

151023a

LongCatIsLooong force-pushed the autofill-enabled-by-default branch 3 times, most recently from 7f47689 to a39c769 Compare August 27, 2021 06:41

add tests

1d23024

LongCatIsLooong force-pushed the autofill-enabled-by-default branch from a39c769 to 1d23024 Compare August 27, 2021 07:02

LongCatIsLooong added 2 commits August 27, 2021 17:05

Merge branch 'autofill-enabled-by-default' of github.com:LongCatIsLoo…

1153f5f

…ong/engine into autofill-enabled-by-default

add tests

4b8e30d

LongCatIsLooong force-pushed the autofill-enabled-by-default branch from 1153f5f to 4b8e30d Compare August 29, 2021 04:51

LongCatIsLooong added 4 commits August 29, 2021 14:48

add tests

cc3d028

Merge remote-tracking branch 'upstream/master' into autofill-enabled-…

318e941

…by-default

Merge branch 'autofill-enabled-by-default' of github.com:LongCatIsLoo…

d76e1b9

…ong/engine into autofill-enabled-by-default

Merge branch 'autofill-enabled-by-default' of github.com:LongCatIsLoo…

6c6aa9e

…ong/engine into autofill-enabled-by-default

LongCatIsLooong force-pushed the autofill-enabled-by-default branch from dc94599 to 6b37ca5 Compare August 30, 2021 20:07

LongCatIsLooong requested review from yjbanov, mdebbar and justinmc August 30, 2021 20:57

LongCatIsLooong marked this pull request as ready for review August 30, 2021 20:58

LongCatIsLooong commented Aug 31, 2021

View reviewed changes

LongCatIsLooong force-pushed the autofill-enabled-by-default branch from 6b37ca5 to 87e1927 Compare August 31, 2021 18:12

mdebbar reviewed Sep 1, 2021

View reviewed changes

LongCatIsLooong force-pushed the autofill-enabled-by-default branch from 87e1927 to ea2b425 Compare September 1, 2021 21:41

fix tests

040581d

LongCatIsLooong force-pushed the autofill-enabled-by-default branch from ea2b425 to 040581d Compare September 1, 2021 21:43

justinmc approved these changes Sep 2, 2021

View reviewed changes

yjbanov approved these changes Sep 2, 2021

View reviewed changes

mdebbar reviewed Sep 4, 2021

View reviewed changes

review

a2c62b4

LongCatIsLooong force-pushed the autofill-enabled-by-default branch from f85e4d3 to a2c62b4 Compare September 4, 2021 01:12

mdebbar approved these changes Sep 4, 2021

View reviewed changes

LongCatIsLooong added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Sep 4, 2021

fluttergithubbot merged commit 1c4e42e into flutter:master Sep 4, 2021

LongCatIsLooong deleted the autofill-enabled-by-default branch September 4, 2021 03:27

engine-flutter-autoroll mentioned this pull request Sep 4, 2021

Roll Engine from cc6074f95938 to 1c4e42e395d7 (1 revision) flutter/flutter#89463

Merged

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

1c4e42e [TextInput] enroll in autofill by default (flutter/engine#28333)

c9ee49f

fluttergithubbot pushed a commit to flutter/flutter that referenced this pull request Sep 4, 2021

1c4e42e [TextInput] enroll in autofill by default (flutter/engine#28333…

574539b

…) (#89463)

		} else if (domElement is html.TextAreaElement) {
		final html.TextAreaElement element = domElement;


		late TextEditingChannel channel;
		late final TextEditingChannel channel = TextEditingChannel(this);

[TextInput] enroll in autofill by default #28333

[TextInput] enroll in autofill by default #28333

Uh oh!

Conversation

LongCatIsLooong commented Aug 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LongCatIsLooong commented Aug 26, 2021 •

edited

Loading