[iOS] Fix view removal process for `AutofillContextAction.cancel` #57209

koji-1009 · 2024-12-14T10:08:04Z

The removeFromSuperview in the hideTextInput method was triggering the save password, so I changed it to cleanUpViewHierarchy.

fix flutter/flutter#145681

sample app code

https://github.com/koji-1009/autofill_cancel_issue

before

before.mp4

after

after.mp4

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 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.

cbracken · 2024-12-20T22:57:33Z

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

@@ -2675,8 +2675,7 @@ - (void)hideTextInput {
  [self removeEnableFlutterTextInputViewAccessibilityTimer];
  _activeView.accessibilityEnabled = NO;
  [_activeView resignFirstResponder];
-  [_activeView removeFromSuperview];
-  [_inputHider removeFromSuperview];
+  [self cleanUpViewHierarchy:YES clearText:NO delayRemoval:NO];


Based on the comment here my assumption is that we should be setting clearText:YES in this case.

@LongCatIsLooong can you confirm?

It's been a while since I looked at this. Is there a quick explanation of what is supposed to happen here? I think this will be called when the framework dismisses the software keyboard?

It's not making a lot sense to me right now that cleanUpViewHierarchy needs to be called when the framework dismisses the keyboard. A bit explanation as to why the bug is happening (is the framework dismissing the keyboard before the autofill group cleans up the view hierarchy?)

In the apps I put in the description,

push from screen1 to screen2

focus on TextField (password)

pop from screen2 to screen1

then MethodChannel is called as follows.

TextInput.setClient TextInput.setEditableSizeAndTransform TextInput.setMarkedTextRect TextInput.setStyle TextInput.setEditingState TextInput.show TextInput.requestAutofill TextInput.setCaretRect TextInput.clearClient TextInput.hide TextInput.finishAutofillContext

I noticed that in TextInput.finishAutofillContext the auto fill process is expected to be performed, but is actually performed in TextInput.hide. When either _activeView or _inputHider was removeFromSuperview, the process would run.

removeFromSuperView has been added at #23776, but if it is not needed I will add a commit to remove it.

Thanks for the explanation!

@chunhtai do you remember why the active field is removed from the view hierarchy when the keyboard is dismissed? iOS autofill will try to save the password if the active field is a password field so we should probably keep the field in the view hierarchy if possible so iOS doesn't think it's the best time to show the "save password" prompt.

Sorry for the late reply, still this fix doesn't feel right. IIRC hideTextInput is called when a framework input field tries to dismiss the software keyboard. I don't think this should be tied to autofill at all. With the current implementation in the PR, dismissing the input field will destroy the current autofill context, so if the developer wants to show the "save password" prompt after the keyboard is dismissed, they wouldn't be able to because the autofill context is already destroyed at that point.

Is the case @LongCatIsLooong are pointing out the behavior of this video?

request_after_hide.mp4

It works on the simulator, but it looks like even if I hide the keyboard or move to the next screen, I can still call the password manager by requesting finishAutofillContext. Could you please let me know if I may have missed any edge cases?

Thanks for the video, sorry i wasn't clear, by "dismissing the input field" I meant the framework sending a TextInput.hide message to the text input plugin (users dismissing the keyboard doesn't trigger hideTextInput). I looked into the EditableText implementation it seems TextInput.hide is sent to the text input plugin in EditableTextState.dispose, and in clearConnection (which means the field loses focus, and the new focus is not an input field).

So hypothetically, if the user enters their credentials, and moves the focus to something that is not a text field (to reproduce this you can add a button that calls FocusNode.unfocus() on the password field's focus node), they won't be able to save passwords after that. If someone implements their own TextInputClient, then their users won't be able to save password after the custom TextInputClient implementation calls TextInput.hide.

I updated sample app and tested.
https://github.com/koji-1009/autofill_cancel_issue/blob/d0537895cea743cc7544083690e2497b8e007e3b/lib/main.dart

Movie "primaryFocus" is called primaryFocus?.unfocus() with StatelessWidget, "focusNode" is called FocusNode.unfocus() with StatefulWidget. Please let me know if there is a different implementation that should be tested.

primaryFocus.mp4

focusNode.mp4

In the master branch, calling primaryFocus?.unfocus() invokes password manager. So perhaps there is a problem with the current implementation, but I believe this fix will solve the problem described in the issue.

master.mp4

I created a PR on flutter/flutter. flutter/flutter#160653

cbracken

Hi @koji-1009 thanks for your contribution. To avoid a future regression, please add a test. If you need suggestions on how to write it or where it should be wired up, please let us know and we'll be glad to point you in the right direction! Take a look at shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.mm for examples.

koji-1009 · 2024-12-21T00:16:12Z

@cbracken
Thanks for the review. I'm just going on vacation and will try to add the test code.

koji-1009 · 2025-01-05T23:04:10Z

The results of the test run are shown here.
flutter/flutter#160653 (comment)

cbracken

Thanks for adding the test!

cbracken

cbracken · 2025-01-10T18:50:59Z

Oh wait. Just noticed this is open against the flutter/engine repo.

@koji-1009 in late December, we merged the engine repo into the flutter/flutter repo. Would you mind re-opening this patch against that repo, link to this one in the description, and I can re-approve over there.

Apologies for the confusion!

koji-1009 · 2025-01-11T00:42:03Z

@cbracken
I have updated flutter/flutter#160653 from draft to open! Thanks for the review!

jtmcdole · 2025-01-14T17:43:29Z

Monorepo Migration Completed

TL;DR: Please migrate your PR to flutter/flutter

The flutter/engine repository has been migrated to the flutter/flutter repository. This PR will no longer be landed here and must be migrated. To migrate your PR to the flutter repository, please follow these steps:

Create a patch for this PR:
- Generate a patch set that represents the changes in this PR. The exact method will vary depending on your PR's history. If your PR includes merges, it is highly recommended that you rebase it onto main first.
```
git format-patch $START_COMMIT..$END_COMMIT --stdout > combined_patch.patch
```

Update the patch for the new engine location:

The engine source code now resides in the engine/ subdirectory within the flutter/flutter repository. You'll need to update the file paths in your patch accordingly.

# Insert the `engine/` prefix to all paths. Note that this usage works on macOS and
# linux versions of sed.
sed -i.bak \
-e 's|^\(diff --git a/\)\(.*\) b/\(.*\)|\1engine/\2 b/engine/src/flutter/\3|' \
-e 's|^\(--- a/\)\(.*\)|\1engine/src/flutter/\2|' \
-e 's|^\(\+\+\+ b/\)\(.*\)|\1engine/src/flutter/\2|' \
combined_patch.patch

Checkout and set up your Flutter development environment:
- Follow the instructions in Setting up the Framework development environment to check out the flutter/flutter repository.
- Remember to rename the origin remote to upstream (e.g., git remote rename origin upstream).
Set up the engine development environment within the monorepo:
- Follow the updated instructions in Setting up the engine environment, paying close attention to the changes in gclient setup and location.
Apply the patch to a new branch:
- Create a new branch in your flutter/flutter repository.
- Apply the updated patch to this branch:
```
git apply combined_patch.patch
```
Open a new PR:
- Open a new PR in the flutter/flutter repository with your changes.
- Reference this original PR in the new PR's description using flutter/engine#<PR_NUMBER>.

This is a canned message

fix: Fix view removal process for AutofillContextAction.cancel

2b08036

github-actions bot added the platform-ios label Dec 14, 2024

koji-1009 marked this pull request as ready for review December 14, 2024 10:43

koji-1009 mentioned this pull request Dec 20, 2024

[iOS][Engine] Fix view removal process for AutofillContextAction.cancel flutter/flutter#160653

Open

9 tasks

cbracken reviewed Dec 20, 2024

View reviewed changes

cbracken requested changes Dec 20, 2024

View reviewed changes

koji-1009 added 2 commits January 5, 2025 21:33

test: Confirm hide doesn’t but commit does trigger autofill

a8ef388

fix: Set clearText:YES

436ae8e

koji-1009 requested a review from cbracken January 5, 2025 13:32

cbracken reviewed Jan 7, 2025

View reviewed changes

refactor: Add comment

8f1fe67

cbracken approved these changes Jan 10, 2025

View reviewed changes

Merge branch 'main' into fix/ios_autofill_password

cb31da3

cbracken mentioned this pull request Jan 14, 2025

[IOS] Inconsistent Behavior of Password Save Prompt in Flutter Autofill using TextInput.finishAutofillContext(shouldSave: true) flutter/flutter#161576

Open

jtmcdole closed this Jan 14, 2025

koji-1009 deleted the fix/ios_autofill_password branch January 16, 2025 09:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[iOS] Fix view removal process for `AutofillContextAction.cancel` #57209

[iOS] Fix view removal process for `AutofillContextAction.cancel` #57209

koji-1009 commented Dec 14, 2024 •

edited

Loading

cbracken Dec 20, 2024

LongCatIsLooong Dec 21, 2024

LongCatIsLooong Dec 21, 2024

koji-1009 Jan 5, 2025

LongCatIsLooong Jan 6, 2025

LongCatIsLooong Jan 12, 2025 •

edited

Loading

koji-1009 Jan 12, 2025

LongCatIsLooong Jan 13, 2025 •

edited

Loading

koji-1009 Jan 13, 2025 •

edited

Loading

koji-1009 Jan 21, 2025

cbracken left a comment

koji-1009 commented Dec 21, 2024

koji-1009 commented Jan 5, 2025

cbracken left a comment

cbracken left a comment

cbracken commented Jan 10, 2025

koji-1009 commented Jan 11, 2025

jtmcdole commented Jan 14, 2025

[iOS] Fix view removal process for AutofillContextAction.cancel #57209

[iOS] Fix view removal process for AutofillContextAction.cancel #57209

Conversation

koji-1009 commented Dec 14, 2024 • edited Loading

Pre-launch Checklist

cbracken Dec 20, 2024

Choose a reason for hiding this comment

LongCatIsLooong Dec 21, 2024

Choose a reason for hiding this comment

LongCatIsLooong Dec 21, 2024

Choose a reason for hiding this comment

koji-1009 Jan 5, 2025

Choose a reason for hiding this comment

LongCatIsLooong Jan 6, 2025

Choose a reason for hiding this comment

LongCatIsLooong Jan 12, 2025 • edited Loading

Choose a reason for hiding this comment

koji-1009 Jan 12, 2025

Choose a reason for hiding this comment

LongCatIsLooong Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

koji-1009 Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

koji-1009 Jan 21, 2025

Choose a reason for hiding this comment

cbracken left a comment

Choose a reason for hiding this comment

koji-1009 commented Dec 21, 2024

koji-1009 commented Jan 5, 2025

cbracken left a comment

Choose a reason for hiding this comment

cbracken left a comment

Choose a reason for hiding this comment

cbracken commented Jan 10, 2025

koji-1009 commented Jan 11, 2025

jtmcdole commented Jan 14, 2025

Monorepo Migration Completed

[iOS] Fix view removal process for `AutofillContextAction.cancel` #57209

[iOS] Fix view removal process for `AutofillContextAction.cancel` #57209

koji-1009 commented Dec 14, 2024 •

edited

Loading

LongCatIsLooong Jan 12, 2025 •

edited

Loading

LongCatIsLooong Jan 13, 2025 •

edited

Loading

koji-1009 Jan 13, 2025 •

edited

Loading