[iOS] Add spell check suggestions toolbar on tap #119189

camsim99 · 2023-01-25T22:05:36Z

Add iOS spell check toolbar to show spell check suggestions, as part of #34688. Fixes #119537.

This PR will cause the following to occur on single tap (touch only) if the word tapped is misspelled:

The word will be highlighted.
The toolbar will appear once (after this, the text selection toolbar will be toggled on/off).
If a suggestion in the toolbar is clicked, the misspelled word will be replaced with that suggestion, and the word edge will be selected.

The toolbar shows on single tap up if the word tap is misspelled. Otherwise, the normal behavior takes precedent.

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, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

camsim99 · 2023-02-03T20:32:30Z

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


  /// Replace composing region with specified text.
-  void replaceComposingRegion(SelectionChangedCause cause, String text) {
+  void replaceComposingRegion({required SelectionChangedCause cause, required String text, TextRange? composingRegionRange, bool shouldSelectWordEdgeAfterReplacement = false}) {


Particularly looking for feedback here. Need a way to do this same logic without access to the composing region for iOS, and this is what I came up with. Also considered a separate method for that case.

Check out ReplaceTextIntent. I think it basically does the same thing as this and the API is nearly identical, so I think you're on the right track. I would just make these parameters match as closely with those as possible in name and order.

I wonder if this could be somehow combined with _replaceText, which uses ReplaceTextIntent in this file?

Also, maybe Android should be using this full API anyway, instead of relying on the composing region here. Especially since non-Gboard IMEs might not have a composing region, either. Maybe that's not this PR's responsibility, though.

Then I would drop the "composing region" terminology from this method.

This could be a breaking change so keep an eye out, but it's probably very unlikely since this was just released.

We will ultimately need to move away from using the composing region to fix the Samsung and iOS issue (#119534), so I took your advice on not using it for Android or iOS. This method uses _replaceText already, which is used in other ways, as well, so I'm not sure if we should combine it. Maybe I could name this something like replaceTextFromToolbar since it contains toolbar-specific behavior.

I talked to @LongCatIsLooong and @Renzo-Olivares about this. We came to some good conclusions about how to organize the code that updates TextEditingValue in EditableText, but I'm worried they won't be possible here...

We'd like to keep all mutations of TextEditingValue like this in userUpdateTextEditingValue. So in this case that would mean deleting replaceComposingRegion/replaceText and putting all of that logic inside of SpellCheckSuggestionsToolbar. The one reason I'm not sure if that will work is the call to _didChangeTextEditingValue, which is private...

Can you try this out and see if you can get it to work without being able to call _didChangeTextEditingValue directly? If not we're probably ok to turn this into a public replaceWithSuggestion method or something like that. Currently the other toolbar buttons like cut/copy/paste all have their own public methods on EditableTextState, so it wouldn't be too terrible if this has a public method too.

justinmc

Looking good, mostly small stuff besides some suggestions you asked for on replaceComposingRegion. Excited for this PR.

packages/flutter/lib/src/cupertino/spell_check_suggestions_toolbar.dart

packages/flutter/lib/src/cupertino/text_field.dart

justinmc · 2023-02-04T01:53:52Z

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

+      if (shouldSelectWordEdgeAfterReplacement) {
+        renderEditable.selectWordEdge(cause: cause);
+      }


Would it work out if it were the caller's responsibility to call selectWordEdge after calling this if they want that behavior?

I mean as long as they call it, it should be fine. Otherwise, the word would stay highlighted after the replacement was made until another tap was made.

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

packages/flutter/test/widgets/text_selection_test.dart

justinmc · 2023-02-04T01:58:41Z

packages/flutter/test/widgets/text_selection_test.dart

+    expect(state.showSpellCheckSuggestionsToolbarCalled, isFalse);
+    expect(state.toggleToolbarCalled, isTrue);
+    }, variant: const TargetPlatformVariant(<TargetPlatform>{ TargetPlatform.iOS }));
+


Is it possible to do one test that does the full cycle of 1. tap misspelled word 2. tap suggestion in the toolbar 3. expect the suggestion to have replaced the misspelled word.

This sort of behavior is tested in editable_text_test.dart, so I'll try adding a test for at least 2/3 there. And keep 1 tested here, like with the Material style toolbar.

It looks fine to me now, thanks.

camsim99 · 2023-03-16T21:03:50Z

packages/flutter/lib/src/cupertino/spell_check_suggestions_toolbar.dart

+      }
+      buttonItems.add(ContextMenuButtonItem(
+        onPressed: () {
+          if (!editableTextState.mounted) {


@LongCatIsLooong like this? I think this is causing a test to break: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8786435562172940817/+/u/run_test.dart_for_web_tests_shard_and_subshard_6/test_stdout

Yeah this is what I meant. Looks like the failure was caused by Path.combine not being implemented by the HTML renderer, but I double this mounted check is the direct cause of that failure.

justinmc · 2023-03-22T21:48:54Z

packages/flutter/test/widgets/editable_text_test.dart

+        selection: TextSelection(affinity: TextAffinity.upstream, baseOffset: 0, extentOffset: 4),
+      );
+      controller.value = value;
+        await tester.pumpWidget(


Nit: This block should be outdented here.

This still looks like it needs its indentation fixed.

justinmc · 2023-03-23T23:54:32Z

The actual setting of the updated TextEditingValue is what seems to cause this strange test failure. All the suspicious things from postFrameCallbacks etc. didn't seem to fix it though. I'll take a closer look if I have time.

justinmc

LGTM 👍

justinmc · 2023-03-28T23:37:40Z

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

+    // Spell check suggestions toolbars are intended to be shown on non-web
+    // platforms. Additionally, the Cupertino style toolbar can't be drawn on
+    // the web with the HTML renderer due to
+    // https://github.com/flutter/flutter/issues/44572.


I just created an issue specific to this, can you link to that one instead? #123560

justinmc · 2023-03-28T23:41:10Z

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

  }

-  /// Replace composing region with specified text.
-  void replaceComposingRegion(SelectionChangedCause cause, String text) {


Looks like Google tests passed 👍

justinmc · 2023-03-28T23:42:08Z

packages/flutter/test/widgets/editable_text_test.dart

+        selection: TextSelection(affinity: TextAffinity.upstream, baseOffset: 0, extentOffset: 4),
+      );
+      controller.value = value;
+        await tester.pumpWidget(


This still looks like it needs its indentation fixed.

…tter#119189)

[iOS] Add spell check suggestions toolbar on tap

…tter#119189)

camsim99 added 3 commits January 24, 2023 15:57

Add toolbar code

9511322

Start adding tests

c6b163c

Add tests for repalceComposingRegion

71cef97

camsim99 added 8 commits January 25, 2023 16:13

Fix analyze

6c76fb1

Fx comment

da29d8f

Styling

38d9f00

Clean up, add todo

8cd6118

Merge remote-tracking branch 'upstream/master' into ios_sc_1

8ee9a18

Remove unecessary test

67fb235

Fix analyze:

6c5566b

Fix tests

eb4903e

camsim99 commented Feb 3, 2023

View reviewed changes

Fix spacing

900c7ab

camsim99 marked this pull request as ready for review February 3, 2023 20:34

camsim99 requested a review from justinmc February 3, 2023 21:16

camsim99 added 2 commits February 3, 2023 13:47

Fix test

78ae3d8

Merge remote-tracking branch 'upstream/master' into ios_sc_1

8689f13

justinmc reviewed Feb 4, 2023

View reviewed changes

camsim99 added 8 commits February 7, 2023 10:44

Start addressing review

225bb47

Merge remote-tracking branch 'upstream/master' into ios_sc_1

257b944

Add required

e696aaa

Replace names, no fix

64813ee

Add range

d06d183

Fix replace text tests

dafc426

Change replaceText

4e6e2de

Debugging

0a9e96f

camsim99 requested a review from LongCatIsLooong March 16, 2023 21:03

camsim99 commented Mar 16, 2023

View reviewed changes

justinmc reviewed Mar 22, 2023

View reviewed changes

camsim99 added 2 commits March 22, 2023 14:58

testing

c1e9a7d

Merge remote-tracking branch 'upstream/master' into ios_sc_1

ef05c9e

camsim99 added 6 commits March 25, 2023 16:43

Ignore linux for now

0eb36f1

Merge remote-tracking branch 'upstream/master' into ios_sc_1

85c5994

Disable for web

0065a8c

Merge remote-tracking branch 'upstream/master' into ios_sc_1

c63e431

Fix comment

9a46ef0

Fix test

150f6cf

justinmc approved these changes Mar 28, 2023

View reviewed changes

camsim99 added 3 commits March 29, 2023 09:14

Make fixes

2dcd0a1

Merge remote-tracking branch 'upstream/master' into ios_sc_1

7066c9f

Merge remote-tracking branch 'upstream/master' into ios_sc_1

9277837

camsim99 added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 31, 2023

auto-submit bot merged commit 52cacd9 into flutter:master Mar 31, 2023

engine-flutter-autoroll mentioned this pull request Apr 1, 2023

Roll Flutter from 12b36de8f7ac to 1d90b7654afa (91 revisions) flutter/packages#3617

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 1, 2023

52cacd9 [iOS] Add spell check suggestions toolbar on tap (flutter/flu…

620747e

…tter#119189)

engine-flutter-autoroll mentioned this pull request Apr 1, 2023

Roll Flutter from 12b36de8f7ac to 1d90b7654afa (91 revisions) flutter/packages#3618

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 1, 2023

52cacd9 [iOS] Add spell check suggestions toolbar on tap (flutter/flu…

2482420

…tter#119189)

engine-flutter-autoroll mentioned this pull request Apr 1, 2023

Roll Flutter from 12b36de8f7ac to 18c9fa6cc80e (92 revisions) flutter/packages#3619

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 1, 2023

52cacd9 [iOS] Add spell check suggestions toolbar on tap (flutter/flu…

db0ef8e

…tter#119189)

reidbaker mentioned this pull request Apr 5, 2023

Update getGradleVersion to ignore commented out lines #124260

Merged

8 tasks

exaby73 pushed a commit to NevercodeHQ/flutter that referenced this pull request Apr 17, 2023

[iOS] Add spell check suggestions toolbar on tap (flutter#119189)

40545b9

[iOS] Add spell check suggestions toolbar on tap

gnprice mentioned this pull request Apr 18, 2023

Spellcheck text selection styling doesn't look right in CupertinoTextField #124404

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023

52cacd9 [iOS] Add spell check suggestions toolbar on tap (flutter/flu…

f713fd4

…tter#119189)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023

52cacd9 [iOS] Add spell check suggestions toolbar on tap (flutter/flu…

482e8c6

…tter#119189)

justinmc mentioned this pull request May 26, 2023

Autocorrect tooltips don't appear on iOS #12920

Open

[iOS] Add spell check suggestions toolbar on tap #119189

[iOS] Add spell check suggestions toolbar on tap #119189

Uh oh!

Conversation

camsim99 commented Jan 25, 2023 • 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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

justinmc commented Mar 23, 2023

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

camsim99 commented Jan 25, 2023 •

edited

Loading