[iOS] option to enable suggestions with autocorrect off #56859

jan-swiezynski · 2024-11-28T09:33:20Z

There is no possibility on iOS to leave the auto-suggest bar visible and have the autocorrection turned off when showing TextField. Tapping outside TextField with autocorrection on fills the TextField with suggestion and it's not always an intended behaviour.

Fixes flutter/flutter#159584

Removed 1 Test.

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.

chinmaygarde · 2024-12-02T19:03:33Z

cc @cbracken

cbracken

I've left style/testing comments inline, but at a much higher level, I'm not entirely whether enableSuggestions is the right approach here.

EditableText widgets have a spellCheckConfiguration property that holds a SpellCheckConfiguration. I would have assumed that passing SpellCheckConfiguration.disabled would be the way to do this.

@justinmc will know the answer to that question.

cbracken · 2024-12-04T16:09:28Z

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

  self.autocorrectionType =
      autocorrectIsDisabled ? UITextAutocorrectionTypeNo : UITextAutocorrectionTypeDefault;
+  NSString* enableSuggestions = configuration[kEnableSuggestions];
+  bool disableSuggestions = enableSuggestions && ![enableSuggestions boolValue];


A few notes:

In Objective-C we should be using BOOL, not bool. In this case, the surrounding code is unfortunately already using C++ bool. I'll send a separate cleanup patch.

The Dart side API for this is enableSuggestions, for readability, please deal with this settings in terms of enableSuggestions in the Obj-C code. See the style guide rationale for why. Again, the surrounding code violates this and I'll send a separate patch to clean it up.

Something like this works:

NSString* enableSuggestionsValue = configuration[kEnableSuggestions]; BOOL enableSuggestions = enabledSuggestionsValue ? enableSuggestionsValue.boolValue : YES;

cbracken · 2024-12-04T16:20:58Z

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

+  bool disableSuggestions = enableSuggestions && ![enableSuggestions boolValue];
  self.spellCheckingType =
-      autocorrectIsDisabled ? UITextSpellCheckingTypeNo : UITextSpellCheckingTypeDefault;
+      disableSuggestions ? UITextSpellCheckingTypeNo : UITextSpellCheckingTypeDefault;


The existing code was landed by @bleroux in #46144 to fix flutter/flutter#134881. Updating this behaviour will be a minor regression to users who were relying on disabling autocorrect to disable spelling suggestions, but that seems reasonable since this adds a new finer-grained mechanism to opt out.

The enableSuggestions API docs currently state that this flag only affects Android. @bleroux do you know the background on why we don't apply it to iOS as well?

If we change this behaviour, we will definitely need to update the framework side API documentation to reflect this and point to Apple's UITextSpellCheckingType docs.

The enableSuggestions API docs currently state that this flag only affects Android. @bleroux do you know the background on why we don't apply it to iOS as well?

I don't know the background. The documentation landed in flutter/flutter#42550. It seems that this was implemented to match an Android feature. Maybe something similar did not exist on the iOS side at that time?

My guess is that it wasn't possible to control these independently at the time that Flutter's enableSuggestions was added. Are we sure that's not still true? +1 to updating the docs if this change lands.

cbracken · 2024-12-04T16:24:29Z

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

-  XCTAssertEqual(inputView.autocorrectionType, UITextAutocorrectionTypeNo);
-  XCTAssertEqual(inputView.spellCheckingType, UITextSpellCheckingTypeNo);
-}
-


Please don't delete tests for existing behaviour; they were added for a reason.

You'll need to update the existing test to reflect the new expected behaviour and add additional testing to cover all four combinations of autocorrect/enableSuggestions.

justinmc

To be clear on iOS these are the three features we're talking about here:

Spell check	Suggestions	Autocorrect
Red underline, red highlight, and suggestion dialog for misspelled words.	Completion suggestions that appear above the software keyboard as the user types. If a hardware keyboard is used instead, there is a blue highlight and a suggestion dialog.	Automatic replacement of common typos.
Disabled by default, enable with `spellCheckConfiguration: SpellCheckConfiguration()`.	Enabled by default, disable with `autocorrect: false`. After this PR, would instead be disabled with `enableSuggestions: false`.	Enabled by default, disable with `autocorrect: false`.

So suggestions and autocorrect are controlled by the same flag, and this PR wants to be able to control them independently? @jan-swiezynski Can you confirm that your understanding is the same as mine and that this PR has the desired effect?

justinmc · 2024-12-04T21:28:28Z

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

+  bool disableSuggestions = enableSuggestions && ![enableSuggestions boolValue];
  self.spellCheckingType =
-      autocorrectIsDisabled ? UITextSpellCheckingTypeNo : UITextSpellCheckingTypeDefault;
+      disableSuggestions ? UITextSpellCheckingTypeNo : UITextSpellCheckingTypeDefault;


My guess is that it wasn't possible to control these independently at the time that Flutter's enableSuggestions was added. Are we sure that's not still true? +1 to updating the docs if this change lands.

jan-swiezynski · 2024-12-04T21:44:44Z

@justinmc Yes, exactly. The change separates autocorrect from suggestions displays and allows presenting suggestions but with autocorrect feature disabled.

justinmc

I checked out this branch locally and it fixes the intended bug, but it's not possible to hide suggestions while enabling autocorrect! Is it possible to fix that as well?

autocorrect	enableSuggestions	spellCheckConfiguration	This PR
false	false	SpellCheckConfiguration.disabled()	✅
false	false	SpellCheckConfiguration()	✅
false	true	SpellCheckConfiguration.disabled()	✅
false	true	SpellCheckConfiguration()	✅
true	false	SpellCheckConfiguration.disabled()	❌ Suggestions appear but should not.
true	false	SpellCheckConfiguration()	❌ Suggestions appear but should not.
true	true	SpellCheckConfiguration.disabled()	✅
true	true	SpellCheckConfiguration()	✅

Edit: This is broken on the main branch as well, so I'm ok with considering this a separate issue if it's not a trivial fix.

cbracken · 2024-12-20T23:00:19Z

Hi @jan-swiezynski is this still on your radar?

jan-swiezynski · 2024-12-28T20:04:49Z

Hi @jan-swiezynski is this still on your radar?

Yes, I'll get back to it in few days.

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

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

[iOS] option to enable suggestions with autocorrect off

b654815

github-actions bot added the platform-ios label Nov 28, 2024

jan-swiezynski mentioned this pull request Nov 28, 2024

No way to disable autocorrection and leave the auto-suggest bar visible on iOS with TextField flutter/flutter#159584

Closed

cbracken self-requested a review December 4, 2024 15:33

cbracken suggested changes Dec 4, 2024

View reviewed changes

cbracken requested review from bleroux and justinmc December 4, 2024 16:33

justinmc reviewed Dec 4, 2024

View reviewed changes

justinmc reviewed Dec 6, 2024

View reviewed changes

jtmcdole closed this Jan 14, 2025

[iOS] option to enable suggestions with autocorrect off #56859

[iOS] option to enable suggestions with autocorrect off #56859

Uh oh!

Conversation

jan-swiezynski commented Nov 28, 2024

Pre-launch Checklist

Uh oh!

chinmaygarde commented Dec 2, 2024

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

cbracken Dec 4, 2024 • 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!

cbracken Dec 4, 2024

Choose a reason for hiding this comment

Uh oh!

bleroux Dec 4, 2024

Choose a reason for hiding this comment

Uh oh!

justinmc Dec 4, 2024

Choose a reason for hiding this comment

Uh oh!

cbracken Dec 4, 2024

Choose a reason for hiding this comment

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc Dec 4, 2024

Choose a reason for hiding this comment

Uh oh!

jan-swiezynski commented Dec 4, 2024

Uh oh!

justinmc left a comment • 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!

cbracken commented Dec 20, 2024

Uh oh!

jan-swiezynski commented Dec 28, 2024

Uh oh!

jtmcdole commented Jan 14, 2025

Monorepo Migration Completed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cbracken Dec 4, 2024 •

edited

Loading

justinmc left a comment •

edited

Loading