Add autofill save for iOS and Android #18643

LongCatIsLooong · 2020-05-28T06:14:45Z

part of flutter/flutter#55613
framework PR: flutter/flutter#58731

Android

Android saves the autofillable input fields by value under the hood. We just have to call AutofillManager#commit or AutofillManager#commit at the right time.

iOS

iOS does some accounting to keep track of possible password autofill fields (but it does not save the input by value, instead it keeps track of the references to the input views).
When an input view is being removed from the view hierarchy, iOS uses heuristics to determine if the view is password related and will try to extract & save the username/password.

For iOS, I'm keeping the password-related input fields in the view hierarchy until we're told to start a new autofill context (TextInput.AutofillContext.{commit, cancel}).

iOS demo

The username & password fields (under the "login" section) initially didn't have any autofill candidates. After using automatic strong password to generate a new password (the "sign up" section) and tapping submit to save the new username/password pairs, a new autofill entry is added to the quick type bar when revisiting the username/password fields.

LongCatIsLooong · 2020-05-28T06:20:53Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

+// that adopts the UITextInput protocol, automatic strong password seems to
+// currently only support UITextFields, and password saving only supports
+// UITextFields and UITextViews, as of iOS 13.5.
+@interface FlutterSecureTextInputView : FlutterTextInputView


Changing FlutterTextInputView to a UITextField seems to require a lot of code changes so I chose this hack instead. From the documentation I think not supporting custom UITextInput is a bug in UIKit (unfortunately it happens on both iOS 12 & iOS 13), once it's fixed we can get rid of this class.

Unrecognized selectors need to be forwarded to the shared text field instance because UIKit's keyboard implementation calls private UITextField methods (we tricked it into thinking this is a UITextField instance).

justinmc

LGTM from my point of view, but definitely wait for the other reviewers to take a look as well. The FlutterSecureTextInputView hack seems reasonable to me for now.

nturgut · 2020-05-28T20:24:16Z

I'll test this locally with web engine changes and let you know. It might take a day, sorry for the inconvenience.

gaaclarke

Reviewed iOS portion. It's an intense PR but great work. I just had one legitimate concern regarding the static input field.

gaaclarke · 2020-07-09T17:08:57Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

 }

-static NSString* uniqueIdFromDictionary(NSDictionary* dictionary) {
+static NSString* autofillIdFromDictionary(NSDictionary* dictionary) {


One sentence docstring would be nice.

gaaclarke · 2020-07-09T17:13:32Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

+ (UITextField*)textField {
+  static UITextField* _textField = nil;
+  if (_textField == nil) {
+    _textField = [[UITextField alloc] init];


Why not have a UITextField per FlutterSecureTextInputView? This seems a bit risky considering we are forwarding unknown messages all to a singular instance.

The unrecognized selectors sent to FlutterSecureTextInputView that I'm seeing in my demo app:

rightView leftView textAlignment font _setContentCoverViewMode: _setContentCoverView: _setBackgroundCoverViewMode: _setBackgroundCoverView: _layoutManager

I think the assumption here is that the UITextField instance is just a stateless (for the states that matter to us) selector sponge. For the selectors I'm seeing the hack seems to work fine. If this hack doesn't work then we should probably switch to a completely different approach (e.g., subclassing UITextField).
If there are other selectors that may set the UITextField to a broken state, then having separate instances probably won't prevent that from happening? If we make each FlutterSecureTextInputView use its own UITextField, we are going to reuse the UITextField anyway when we reuse the FlutterSecureTextInputView.

It isn't stateless though since we are forwarding messages to it that could be setting all kinds of state.

[inputView1 setAccessibilityHint:@"hey"]; print(inputView2.accessibilityHint);

for example

Updated to create a UITextField per instance. What I meant was if any of the states those unrecognized selectors can change matter to us or iOS itself, then using this hack is pretty much a lost cause.

gaaclarke · 2020-07-09T17:18:44Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

+    if (!autofillIdFromDictionary(configuration)) {
+      return NO;
+    }
+    NSDictionary* autofill = configuration[@"autofill"];


For all these string literals that are shared across the code it would be nice to have const variables for them to avoid errors.
static NSString *const APIPathFoo = APIBasePath @"foo";

gaaclarke · 2020-07-09T17:21:58Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

+}
+
+- (void)setIsVisibleToAutofill:(BOOL)isVisibleToAutofill {
+  self.frame = isVisibleToAutofill ? CGRectMake(0, 0, 1, 1) : CGRectZero;


This might merit a comment since it seems a bit hacky to change the frame based on something like if it is visible to autofill.

gaaclarke · 2020-07-09T17:22:40Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

-@property(nonatomic, retain) FlutterTextInputView* nonAutofillInputView;
-@property(nonatomic, retain) FlutterTextInputView* nonAutofillSecureInputView;
-@property(nonatomic, retain) NSMutableArray<FlutterTextInputView*>* inputViews;
+@property(nonatomic, retain) FlutterTextInputView* reusableInputView;


nit: @property(nonatomic, strong)

gaaclarke · 2020-07-09T17:29:44Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

-  NSDictionary* autofill = configuration[@"autofill"];
+  inputView.textInputDelegate = _textInputDelegate;
+  [inputView configureWithDictionary:field];
+  return inputView;


super nit: move the autorelease to the return statement to make auditing a bit easier

the inputView is not necessarily a new object, putting autorelease at the end would make it a zombie when it's reused.

gaaclarke · 2020-07-09T20:31:39Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

 static const char _kTextAffinityUpstream[] = "TextAffinity.upstream";

+#pragma mark - TextInputConfiguration Field Names
+static NSString* const _kSecureTextEntry = @"obscureText";


You shouldn't have the _ prefix here: https://google.github.io/styleguide/objcguide.html#constants

LongCatIsLooong · 2020-07-09T20:56:32Z

There's a crash in objc_msgSend (related to the UITextField I guess). Fixing. Will keep you posted.

LongCatIsLooong · 2020-07-09T22:40:17Z

The crash should be fixed now.

gaaclarke

LGTM

gaaclarke · 2020-07-20T23:36:57Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

+@end
+
+@implementation FlutterSecureTextInputView {
+  UITextField* _textField;


nit: This isn't necessary since the property will declare it.

I think this is required because the property is readOnly and I overrode its getter so it will not get synthesized?

LongCatIsLooong added 3 commits May 27, 2020 17:03

WIP

5184d7f

android implementation

8f9233e

formatting

a4473ad

LongCatIsLooong requested review from justinmc, nturgut and xster May 28, 2020 06:14

googlebot added the cla: yes label May 28, 2020

LongCatIsLooong commented May 28, 2020

View reviewed changes

fix api availability

652ef5e

justinmc approved these changes May 28, 2020

View reviewed changes

LongCatIsLooong force-pushed the autofill-save branch from 7cac527 to c2960bb Compare May 28, 2020 17:40

position(0) -> rewind

6499cf5

LongCatIsLooong force-pushed the autofill-save branch from c2960bb to 6499cf5 Compare May 28, 2020 17:48

LongCatIsLooong mentioned this pull request Jun 4, 2020

Autofill save flutter/flutter#58731

Merged

13 tasks

LongCatIsLooong force-pushed the autofill-save branch from c97555c to 5d09ab5 Compare July 7, 2020 20:21

merge

fd2560f

LongCatIsLooong force-pushed the autofill-save branch from 5d09ab5 to fd2560f Compare July 7, 2020 20:22

LongCatIsLooong added 3 commits July 8, 2020 02:16

update

465cc6b

update

3eb250b

autofill visibility

2127bc5

fluttergithubbot added platform-android platform-ios labels Jul 9, 2020

LongCatIsLooong added 2 commits July 8, 2020 18:45

fix android tests

d3494c9

update

d0dd8e6

LongCatIsLooong requested review from gaaclarke and goderbauer July 9, 2020 16:01

gaaclarke reviewed Jul 9, 2020

View reviewed changes

review

17e11f5

gaaclarke reviewed Jul 9, 2020

View reviewed changes

review

93e5daf

LongCatIsLooong added 3 commits July 20, 2020 13:53

Merge remote-tracking branch 'upstream/master' into autofill-save

c635532

formatting

c5d3e04

textfield to be per instance

10c7bb4

gaaclarke approved these changes Jul 20, 2020

View reviewed changes

LongCatIsLooong merged commit d96371e into flutter:master Jul 28, 2020

LongCatIsLooong deleted the autofill-save branch July 28, 2020 17:24

engine-flutter-autoroll mentioned this pull request Jul 28, 2020

Roll Engine from f27729e97b5a to d96371e03ffc (7 revisions) flutter/flutter#62434

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 28, 2020

d96371e Add autofill save for iOS and Android (flutter/engine#18643)

7c22c66

engine-flutter-autoroll mentioned this pull request Jul 28, 2020

Roll Engine from f27729e97b5a to 2d8a00e21d8c (9 revisions) flutter/flutter#62438

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 28, 2020

d96371e Add autofill save for iOS and Android (flutter/engine#18643)

bc062dc

This was referenced Jul 31, 2020

iOS keyboard is no longer presenting the @ or numbers when 123 is pressed flutter/flutter#62659

Closed

Fix iOS Keyboard stuck as UIKeyboardTypeNamePhonePad #20181

Merged

jmagman mentioned this pull request Jan 20, 2023

iOS obscureText input security features flutter/flutter#118725

Open

Add autofill save for iOS and Android #18643

Add autofill save for iOS and Android #18643

Uh oh!

Conversation

LongCatIsLooong commented May 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Android

iOS

iOS demo

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong May 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

nturgut commented May 28, 2020

Uh oh!

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

LongCatIsLooong commented Jul 9, 2020

Uh oh!

LongCatIsLooong commented Jul 9, 2020

Uh oh!

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

LongCatIsLooong commented May 28, 2020 •

edited

Loading

LongCatIsLooong May 28, 2020 •

edited

Loading