fixes TextInputFormatter gets wrong old value of a selection #75541

chunhtai · 2021-02-05T23:30:23Z

Currently EditableText listens to text editing value changes from its RenderEditable in two ways:

The renderObject.onSelectionChange callback.
The EditableText.textEditingValue setter.

I believe that the original intent of these two methods was:

The onSelectionChange method is only called when a new selection has been created; the EditableText rebuilds the selection overlay when it is called.
EditableText.textEditingValue is called when the selection changes from an old value to a new value, and the EditableText simply updates the selection value in overlay without rebuilding it.

RenderEditable becomes confused when the value changes, sometimes, both methods may need to be called... This causes another issue because they will mess up each other.

If renderObject.onSelectionChange is called before EditableText.textEditingValue, the EditableText.textEditingValue will not work properly. The EditableText.textEditingValue will need to notify the inputformatter about value changes, it will send the value to the inputformatter before and after the update. However, the selection has already been updated by the enderObject.onSelectionChange at this point, it does not know what's the selection in the old value. This is the cause of this issue #75502.

if the EditableText.textEditingValue is called before renderObject.onSelectionChange, the renderObject.onSelectionChange will be ignored and will cause the selection overlay to not be built.

Combining all of the above reasons, I created this PR to make editableText only listen to EditableText.textEditingValue (I changes it to updateTextEditingValue because it can't no longer be just a setter), and make renderEditable always call updateTextEditingValue when the selection changes. On top of that. I want to deprecate the renderObject.onSelectionChange because it is not really useful to editableText.

There is currently no test, I will add more test once we are sure this is the path we want to proceed.

fixes #75502.

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

skia-gold · 2021-02-05T23:36:34Z

Gold has detected about 17 untriaged digest(s) on patchset 1.
View them at https://flutter-gold.skia.org/cl/github/75541

flutter-dashboard · 2021-02-06T00:35:36Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

skia-gold · 2021-02-06T00:45:35Z

Gold has detected about 3 untriaged digest(s) on patchset 2.
View them at https://flutter-gold.skia.org/cl/github/75541

skia-gold · 2021-02-06T01:09:35Z

Gold has detected about 3 untriaged digest(s) on patchset 3.
View them at https://flutter-gold.skia.org/cl/github/75541

skia-gold · 2021-02-06T01:12:35Z

Gold has detected about 6 untriaged digest(s) on patchset 4.
View them at https://flutter-gold.skia.org/cl/github/75541

justinmc

I think I'm on board with this approach assuming tests pass and everything works the same as before. The setter and onSelectionChanged do seem to be redundant and their differences are confusing.

Do we know if anyone besides Flutter itself has been using onSelectionChanged? It seems like there will no longer be a straightforward way to listen for selection changes. Users would have to use updateTextEditingValue and then check if the new value is different, or something like that.

justinmc · 2021-02-08T19:21:12Z

packages/flutter/lib/src/rendering/editable.dart

@@ -198,6 +163,10 @@ class RenderEditable extends RenderBox with RelayoutWhenSystemFontsChangeMixin {
    double textScaleFactor = 1.0,
    TextSelection? selection,
    required ViewportOffset offset,
+    @Deprecated(


I think you need another @Deprecated down on the onSelectionChanged property as well.

Will onSelectionChanged continue to function as it did before until we remove this deprecation? It looks like it may not be called anymore after this PR, or at least the behavior may change.

I think this looks good now 👍

justinmc · 2021-02-08T19:30:30Z

packages/flutter/lib/src/rendering/editable.dart

-    if (onSelectionChanged != null) {
-      onSelectionChanged!(nextSelection, this, cause);
-    }
+  void _setSelectionChange(TextSelection nextSelection, SelectionChangedCause cause) {


This slightly bumps against my TextEditingActions PR #75032, where I'm starting to move this _handelKeyEvent code into Shortcuts. It shouldn't be a problem though, I can just update my PR after this one is merged.

justinmc · 2021-02-08T19:50:45Z

packages/flutter/lib/src/rendering/editable.dart

@@ -33,37 +33,6 @@ const Radius _kFloatingCaretRadius = Radius.circular(1.0);
 /// Used by [RenderEditable.onSelectionChanged].
 typedef SelectionChangedHandler = void Function(TextSelection selection, RenderEditable renderObject, SelectionChangedCause cause);

-/// Indicates what triggered the change in selected text (including changes to
-/// the cursor location).
-enum SelectionChangedCause {


Is there a dependency loop or something if you keep this here and import it from text_input.dart?

I think that will be a layer violation, text_input.dart is in service layer

chunhtai · 2021-02-08T20:20:27Z

@justinmc unfortunately I know there are a lot of library out there just copied the entire EditableText, (sometimes also RenderEditable) and makes changes to fit their need, so the changes to the textEditingValue setter will be a big breaking change for them. I think this is something that needed to be done anyway, and i am glad you thought the same. I am curious on the text refactoring project, will it also be a big breaking change too? If so, should we coordinate the merge so we only break them once?

Thanks for the feedback, I will update the PR and add more tests

LongCatIsLooong · 2021-02-08T21:04:21Z

@justinmc Is this still needed once we move key events handling out? I'm under the impression that once we're done RenderEditable won't be able to make changes to its TextEditingValue?

skia-gold · 2021-02-10T19:30:35Z

Gold has detected about 6 untriaged digest(s) on patchset 10.
View them at https://flutter-gold.skia.org/cl/github/75541

chunhtai · 2021-02-10T20:33:37Z

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

@@ -102,6 +102,7 @@ class _CupertinoTextFieldSelectionGestureDetectorBuilder extends TextSelectionGe

  @override
  void onSingleTapUp(TapUpDetails details) {
+    editableText.hideToolbar();


We have this in Material Textfield but not cupertinotextfield, This is to deal with the case where there are toolbar open and user tap the text outside of the current selection. The expect behavior is the tool bar will be closed and the collapsed selection moved to a new place with the toolbar remaining closed.

The workflow is as the following:

onSingleTapUp(hide toolbar) -> EditableTextOnselectionChange(update toolbar) -> composing changes(hide toolbar)

In theory, if we don't add this hide toolbar, you should see the selection overlay gets moved to the new location with the toolbar open. the toolbar will then be hidden after a frame delay.

This works prior to this pr in a weird way. When user tap and the toolbar will be rebuild with the new selection, but that will only show up after one frame delay. right before the frame end, the selection change again probably due to composing changes. This will hide the toolbar before it builds.

In short, we have a selection overlay built and destroyed in the same frame.

My PR optimizes the selection change that it won't need to rebuild the selection overlay every time. This breaks because the overlay will get updated immediately before the hide toolbar remove the overlay on next frame. Visually, you will see the selection change to the new place with the toolbar still open, and the toolbar will close after one frame.

This is also part of the reason why i think we should move the selection overlay out of editable text.

I'm on board with all of this, I think we're moving in the right direction. Currently, our behavior for showing/hiding the text selection toolbar is incorrect in a bunch of cases already (see #48434 (comment)). Once we've been able to refactor more I hope we can get it all right.

chunhtai · 2021-02-10T20:40:44Z

packages/flutter/test/cupertino/text_field_test.dart

@@ -3475,7 +3475,6 @@ void main() {
        from: tester.getTopRight(find.byType(CupertinoApp)),
        cause: SelectionChangedCause.tap,
      );
-      expect(state.showToolbar(), true);


Prior to this pr the entire selection overlay will be rebuilt when the editableText rebuild with different selection.

I feel that is weird. if you have a selection with toolbar and handles on a textfield and you update the texteditingcontroller with different selection, you will see the selection gets moved to a new place with the handles but no toolbar.

After this PR, the toolbar will remain open when the selection moved to the new place.

That means we don't need to call the showtoolbar here in the test.

chunhtai · 2021-02-10T20:43:20Z

packages/flutter/test/material/text_field_focus_test.dart

@@ -119,6 +119,8 @@ void main() {
    expect(tester.testTextInput.isVisible, isTrue);

    tester.testTextInput.hide();
+    final EditableTextState state = tester.state<EditableTextState>(find.byType(EditableText));
+    state.connectionClosed();


This test is testing a non realistic situation.

In real app, closing the input connection will cause the textfield to be unfocus, tapping it again, the textfield will regain focus and open the input connection.

This works before because it relies on the bug that the selection does not update correctly in rendereditable. So everytime you tap, you receive a new selection which cause a selection changes and open the keyboard.

chunhtai · 2021-02-10T20:44:28Z

packages/flutter/test/widgets/editable_text_cursor_test.dart

@@ -49,6 +49,7 @@ void main() {
  });

  testWidgets('cursor layout has correct width', (WidgetTester tester) async {
+    EditableText.debugDeterministicCursor = true;


The pr shifted the blinking timer a bit, i figured the blinking is not necessary for this test

chunhtai · 2021-02-10T20:45:18Z

packages/flutter/test/widgets/editable_text_test.dart

@@ -5357,6 +5357,7 @@ void main() {
      'TextInput.setEditingState',
      'TextInput.setEditingState',
      'TextInput.show',
+      'TextInput.show',


the additional show is called due to selection change which was not called before, which it should.

chunhtai · 2021-02-10T20:46:56Z

packages/flutter/test/widgets/editable_text_test.dart

      text: 'a',
+      selection: controller.selection,


if the selection is not set, it will be set to collapsed at -1, which should cause a selection change. This is not needed before because it previously just ignored the selection change

chunhtai · 2021-02-10T20:48:26Z

packages/flutter/test/widgets/editable_text_test.dart

@@ -6619,6 +6629,7 @@ void main() {
    final EditableTextState state = tester.state<EditableTextState>(find.byType(EditableText));
    state.updateEditingValue(const TextEditingValue(
      text: 'foo composing bar',
+      selection: TextSelection.collapsed(offset: 4),


selection needed to be within composing range, otherwise the composing will be reseted by texteditingcontroller. It is not needed before because the it does not fire selection changes prior to this pr

chunhtai · 2021-02-10T22:52:44Z

Hi @justinmc this pr is ready, can you take another look? thanks!

chunhtai · 2021-02-17T22:36:13Z

a friendly bump

LongCatIsLooong

Left a few questions.

An alternative to this is to use EditableText's batchEdit function, storing the old value in beginBatchEdit and call the user callbacks in endBatchEdit. But the call paths look really convoluted and thanks for cleaning it up!

LongCatIsLooong · 2021-02-18T06:26:55Z

packages/flutter/lib/src/services/text_input.dart

+  /// * [EditableTextState.userUpdateTextEditingValue]: an implementation that
+  ///   applies additional pre-processing to the specified [value], before
+  ///   updating the text editing state.
+  void userUpdateTextEditingValue(TextEditingValue value, SelectionChangedCause? cause);


It's not clear to me when cause should be null. Could you add that to the documentation?

probably should be not nullable, thanks for catching

LongCatIsLooong · 2021-02-18T06:37:54Z

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

+    // if the selection offset didn't change.
+    if (selectionChanged ||
+        (userInteraction &&
+        ((value.selection.baseOffset == 0 && value.selection.extentOffset == 0 && !_hasFocus) ||


I saw a similar piece of code in RenderEditable but I'm not sure I understand what it is for.

when the widget loses focus don't we always set the selection to (-1, -1) in _handleFocusChanged?
Also _handleSelectionChanged seems to call the widget.onSelectionChanged callback which according to its documentation shouldn't be called when the selection isn't changed.

Why is it necessary to distinguish TextEditingValue changes from the virtual keyboard (i.e. userInteraction = false) and value changes from the raw keyboard events & gestures (i.e. userInteraction = true)?

this is for corner case where the textfield is active and user single tap the textfield again to show the selection overlay.
Therefore, we only want to build overlay when it is trigger by the user if the selection is still at 0

ah yea i should check for selection change before calling onselectionchange

why the !_hasFocus if the context menu should only be updated on a focused field?

ok this is a series of legacy code and bug make me think this is correct. We want to build the selection overlay when user type keyboard or long press. and we need to want to call the onselectionchange because textfield relies on it to decide whether it really want to show selection handles. (I know it is weird, but again, this why i think we should move selectionoverlay outside..). It has already like this prior to the change, that longpress on a textfield will fire selection change even if selection does not change.

I update it to check for long press instead

LongCatIsLooong · 2021-02-18T17:50:48Z

packages/flutter/lib/src/rendering/editable.dart

+    textSelectionDelegate.userUpdateTextEditingValue(newValue, cause);
+  }
+
+  void _setSelectionChange(TextSelection nextSelection, SelectionChangedCause cause) {


nit: maybe _setSelection?

LongCatIsLooong · 2021-02-18T17:56:37Z

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

-    Overlay.of(context, rootOverlay: true, debugRequiredFor: debugRequiredFor)!.insertAll(_handles!);
+    if (_handles == null) {
+      _handles = <OverlayEntry>[
+        OverlayEntry(builder: (BuildContext context) =>


nit: the style guide says => functions should be one-liners.

justinmc

LGTM

Sorry I missed this notification! Thanks for doing this clean up. I'm interested in the batch idea by @LongCatIsLooong above but it should be fine either way.

My text editing refactoring is trying to be minimally breaking, so I think we shouldn't need to coordinate. The first PR is #75032 and it doesn't contain any breaking changes. As I port some of the tap events that affect the stuff in this PR then I might want to break some of the API there, but not yet.

justinmc · 2021-02-18T18:25:09Z

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

@@ -102,6 +102,7 @@ class _CupertinoTextFieldSelectionGestureDetectorBuilder extends TextSelectionGe

  @override
  void onSingleTapUp(TapUpDetails details) {
+    editableText.hideToolbar();


I'm on board with all of this, I think we're moving in the right direction. Currently, our behavior for showing/hiding the text selection toolbar is incorrect in a bunch of cases already (see #48434 (comment)). Once we've been able to refactor more I hope we can get it all right.

justinmc · 2021-02-18T18:26:26Z

packages/flutter/lib/src/rendering/editable.dart

@@ -198,6 +163,10 @@ class RenderEditable extends RenderBox with RelayoutWhenSystemFontsChangeMixin {
    double textScaleFactor = 1.0,
    TextSelection? selection,
    required ViewportOffset offset,
+    @Deprecated(


I think this looks good now 👍

justinmc · 2021-02-18T18:43:30Z

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

-    ];
-
-    Overlay.of(context, rootOverlay: true, debugRequiredFor: debugRequiredFor)!.insertAll(_handles!);
+    if (_handles == null) {


Nit: This could be an early return if you prefer.

if (_handles != null) { return; }

chunhtai · 2021-02-18T20:13:59Z

Thanks for the review, I will proceed to write breaking change doc for the deprecated callbacks

chunhtai · 2021-02-18T20:20:17Z

oh it looks like we don't consider breaking change f no customer tests fail. I guess we should be fine then.

LongCatIsLooong · 2021-02-18T22:45:14Z

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

+        (userInteraction &&
+        (cause == SelectionChangedCause.longPress ||
+         cause == SelectionChangedCause.keyboard))) {
+      _handleSelectionChanged(value.selection, cause);


nit: could you add a TODO since ideally this shouldn't call widget.onSelectionChanged?

…lutter#75541)" This reverts commit a46139a.

…75541)" (#76627) This reverts commit a46139a.

chunhtai · 2021-03-08T17:29:55Z

The stable branch in Flutter 2 has this bug now. Could we get this fixed?

@sgehrman This change was reverted right before the stable release, but it is now relanded. I will see if we can cherry pick this.

flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Feb 5, 2021

google-cla bot added the cla: yes label Feb 5, 2021

chunhtai force-pushed the issues/75502 branch from e8a6406 to 13d05f8 Compare February 6, 2021 00:34

chunhtai marked this pull request as ready for review February 6, 2021 00:35

chunhtai requested review from LongCatIsLooong and justinmc February 6, 2021 00:35

chunhtai changed the title ~~fixes TextInputFormatter gets wrong old value of a selection~~ [Proof of Concept] fixes TextInputFormatter gets wrong old value of a selection Feb 6, 2021

justinmc reviewed Feb 8, 2021

View reviewed changes

chunhtai force-pushed the issues/75502 branch from f2bb546 to 194df08 Compare February 10, 2021 19:23

chunhtai commented Feb 10, 2021

View reviewed changes

chunhtai force-pushed the issues/75502 branch from 88cafe0 to 4ef68d4 Compare February 10, 2021 22:38

chunhtai changed the title ~~[Proof of Concept] fixes TextInputFormatter gets wrong old value of a selection~~ fixes TextInputFormatter gets wrong old value of a selection Feb 10, 2021

flutter-dashboard bot added f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. labels Feb 10, 2021

chunhtai requested a review from justinmc February 10, 2021 22:52

LongCatIsLooong reviewed Feb 18, 2021

View reviewed changes

justinmc approved these changes Feb 18, 2021

View reviewed changes

LongCatIsLooong approved these changes Feb 18, 2021

View reviewed changes

chunhtai force-pushed the issues/75502 branch from 26542bf to 661324a Compare February 18, 2021 23:46

chunhtai added 4 commits February 19, 2021 09:46

refactor text selection overlay and selection update

4913dd8

addressing comments

96283ab

update comments

b00c26c

update

8868819

chunhtai force-pushed the issues/75502 branch from c74943d to 8868819 Compare February 19, 2021 17:47

chunhtai added the waiting for tree to go green label Feb 19, 2021

fluttergithubbot merged commit a46139a into flutter:master Feb 19, 2021

chunhtai added a commit to chunhtai/flutter that referenced this pull request Feb 23, 2021

Revert "fixes TextInputFormatter gets wrong old value of a selection (f…

8f6d3c0

…lutter#75541)" This reverts commit a46139a.

chunhtai mentioned this pull request Feb 23, 2021

Revert "fixes TextInputFormatter gets wrong old value of a selection … #76627

Merged

8 tasks

chunhtai added a commit that referenced this pull request Feb 23, 2021

Revert "fixes TextInputFormatter gets wrong old value of a selection (#…

b513951

…75541)" (#76627) This reverts commit a46139a.

This comment was marked as abuse.

Sign in to view

kf6gpe mentioned this pull request Apr 13, 2021

Increased memory use in <elided use case> #80379

Closed

fixes TextInputFormatter gets wrong old value of a selection #75541

fixes TextInputFormatter gets wrong old value of a selection #75541

Conversation

chunhtai commented Feb 5, 2021 • edited by HansMuller

Pre-launch Checklist

skia-gold commented Feb 5, 2021

flutter-dashboard bot commented Feb 6, 2021

skia-gold commented Feb 6, 2021

skia-gold commented Feb 6, 2021

skia-gold commented Feb 6, 2021

justinmc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunhtai commented Feb 8, 2021

LongCatIsLooong commented Feb 8, 2021

skia-gold commented Feb 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunhtai Feb 10, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunhtai commented Feb 10, 2021

chunhtai commented Feb 17, 2021

LongCatIsLooong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunhtai commented Feb 18, 2021

chunhtai commented Feb 18, 2021

Choose a reason for hiding this comment

This comment was marked as abuse.

chunhtai commented Mar 8, 2021

chunhtai commented Feb 5, 2021 •

edited by HansMuller

chunhtai Feb 10, 2021 •

edited