More EditableText docs #66864

LongCatIsLooong · 2020-09-28T23:26:37Z

Description

Historically in text input plugins we send the TextEditingValues we received from the framework back to the framework, in order to trigger user input callbacks for text selection menu operations (flutter/engine@a90d17b). This has caused a few issues including infinite loops.

Related Issues

Fixes #66144

Tests

I added the following tests:

EditableText does not send editing values more than once

Checklist

Before you create this PR, confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.

…IsLooong/flutter into editable-text-updateEditingValue

…ateEditingValue

LongCatIsLooong · 2020-09-29T00:04:53Z

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

This is a bit questionable. For example if inputFormatters is updated and the same TextEditingValue is received a second time it should probably not shortcircuit.

It was recently added in #65754. I'm not sure what's up with the numeric keyboard problem.

This is a bit questionable. For example if inputFormatters is updated and the same TextEditingValue is received a second time it should probably not shortcircuit.

Yes, the situation you said should not be short-circuited. Maybe we should go through _formatAndSetValue if we receive duplicate values at this situation. Can we add a test case to test the situation you said?
There is also a scenario where duplicate values will be received. When the keyboard is hidden, the engine will send the current value again. #66036

LongCatIsLooong · 2020-09-29T00:05:48Z

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

Do we need to update the formatter (and reapply it) when _textDirection changes?

It's possible to write a formatter that does something different depending on the text direction, right? If so I would say yes we need to reapply it. But I can't think of a real world example. Maybe open an issue for it for another PR.

I didn't take a very careful look but _WhitespaceDirectionalityFormatter maintains states and may need to be re-instantiated and re-applied.

LongCatIsLooong · 2020-09-29T00:06:51Z

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

The logic here is a bit different. We used to skip the _whitespaceFormatter when there's no other formatters.

LongCatIsLooong · 2020-09-29T19:22:20Z

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

I think flutter/engine#20160 prevents the repeat on iOS. The android text input plugin still does that.

Doing this because this seems to be the path of least resistance.

That sounds good to me. At some point did you say that you plan to implement the same kind of idea as flutter/engine#20160 in Android? Or is the plan to leave it as-is?

LongCatIsLooong · 2020-09-29T19:42:36Z

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

Removing the formatter caching altogether. I think a TextInputFormatter is allowed to have states? If that's the case we shouldn't cache the formatter outputs.

I could imagine a formatter that has state. Not sure what Flutter's opinion on that is, though.

LongCatIsLooong · 2020-09-29T19:51:54Z

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

Should inputFormatters work retroactively?

Good question... I'm not sure. I guess it would be easy for the user to apply their input formatter to the existing text if they want to. Maybe some users are doing that now. I guess I don't have a strong reason to change it.

justinmc

This is a great clean up, we really needed this. The docs are much more useful now. I was constantly confused by how all of this engine/framework communication happens.

Most of my comments are just nits or attempts to answer some of your questions.

justinmc · 2020-09-29T23:57:55Z

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

+  ///
+  /// See also:
+  ///
+  /// * [EditableTextState.textEditingValue]: an implementation that appleis


Typo on "applies".

justinmc · 2020-09-29T23:58:39Z

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

 /// See also:
 ///
 ///  * [TextInput.attach]
+///


No empty line needed here I think?

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

justinmc · 2020-09-30T00:18:56Z

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

That sounds good to me. At some point did you say that you plan to implement the same kind of idea as flutter/engine#20160 in Android? Or is the plan to leave it as-is?

justinmc · 2020-09-30T00:19:10Z

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

+///   [TextInputClient]'s [AutofillScope], and the [AutofillScope] will further
+///   relay the value to the correct [TextInputClient].
+///
+/// When synchronizing the [TextEditingValue]s, the communication may stuck in


"may stuck" => "may get stuck"

justinmc · 2020-09-30T00:26:58Z

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

+  ///
+  /// Used in [_updateRemoteEditingValueIfNeeded] to determine whether the
+  /// remote value is outdated and needs updating.
+  TextEditingValue? _lastKnownRemoteTextEditingValue;


👍 for the name improvement.

It's a great job to remove those caches.

justinmc · 2020-09-30T00:31:38Z

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

It was recently added in #65754. I'm not sure what's up with the numeric keyboard problem.

justinmc · 2020-09-30T00:36:40Z

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

It's possible to write a formatter that does something different depending on the text direction, right? If so I would say yes we need to reapply it. But I can't think of a real world example. Maybe open an issue for it for another PR.

justinmc · 2020-09-30T00:37:32Z

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

I could imagine a formatter that has state. Not sure what Flutter's opinion on that is, though.

justinmc · 2020-09-30T00:43:35Z

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

+    // Before triggering the [_value] change notifier, we want to make sure
+    // [_updateRemoteEditingValueIfNeeded] is muted, so we can consolidate the
+    // changes and send them to the engine in one batch.
+    _muteRemoteUpdate = true;


I think the mute idea might have the potential to create confusing behavior if future editors aren't careful. I can't think of a better architecture though. I see why we need to batch these.

I am sorry that I did not understand the purpose of this code, can you guys explain what were batched? thanks :)

Because Is possible to modify this value on widget.onChanged?

Yeah onChanged and the listeners registered to TextEditingController both may update the editing value.

Should I call it beginBatchEdit & endBatchEdit, and allow nesting?

xu-baolin

    // For consistency with Android behavior, send an update to the framework.
    [self updateEditingState];

The comments say consistency with Android, but sometimes the Android does not do this, is that right? Is it possible that we make modify it here?

Should we determine a data synchronization principle? For example, both sides do not respond to the changes notified by the other side, but only notify the local changes to remote. Otherwise, our scene will become complicated and difficult to maintain.

xu-baolin · 2020-09-30T02:43:01Z

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

This is a bit questionable. For example if inputFormatters is updated and the same TextEditingValue is received a second time it should probably not shortcircuit.

Yes, the situation you said should not be short-circuited. Maybe we should go through _formatAndSetValue if we receive duplicate values at this situation. Can we add a test case to test the situation you said?
There is also a scenario where duplicate values will be received. When the keyboard is hidden, the engine will send the current value again. #66036

xu-baolin · 2020-09-30T06:10:36Z

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

 ///
+/// ## Handling User Input
+///
+/// Currently the user may change the text this widget contains via keyboard or


The user also can change the text by the controller directly. If change the text by controller [inpitFormatters] will not be applied. Should [inpitFormatters] work here?

The "user" here refers to end-users (that use the app). I think inputFormatters is meant for sanitizing user input. If the developer decides to change the text programmatically then they should be responsible for making sure the new text is valid.

Oh in the documentation it only says "when the text input changes": https://api.flutter.dev/flutter/material/TextField/inputFormatters.html. I think we should make that clear.

xu-baolin · 2020-09-30T06:29:41Z

packages/flutter/test/widgets/editable_text_test.dart

+
+      // If by coincidence the text input plugin sends the same value back,
+      // do nothing.
+      state.updateEditingValue(const TextEditingValue(text: 'remote listener onChanged listener'));


Should the inputFormatters be applied?
This happens to be the scene we discussed above
This is a bit questionable. For example if inputFormatters is updated and the same TextEditingValue is received a second time it should probably not shortcircuit.

If the answer is yes and the iOS platform always returns this value, is it possible to cause an endless loop?

I think the answer depends on the situation. If it is the value sent back immediately (like iOS), it should not be applied, but if it is the user input later, it should be applied.

We really can’t let the plug-in send back the value to the framework, which will make the problem very tricky.

With flutter/engine#20160 the ios input plugin should never send values back if the change is initiated by the framework.

I see.
In addition, should the inputFormatters be applied here?

I'm not sure. In theory it should. I'll go find out if there's practically reasons about the current input formatter implementation, and open a new PR if there's anything needs changing.

xu-baolin · 2020-09-30T06:36:41Z

packages/flutter/test/widgets/editable_text_test.dart

+    });
+
+    testWidgets('input from text selection menu', (WidgetTester tester) async {
+      await tester.pumpWidget(widget);


This appears to just be the same test case of the previous, although I'll be pleased to hear that I'm missing something :-).

It's not exactly the same as the previous "input from text input plugin" test, as it tests the textEditingValue setter call path instead of updateEditingValue

xu-baolin · 2020-10-09T01:58:31Z

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

    }

-    // Setting _value here ensures the selection and composing region info is passed.
+    // Before triggering the [_value] change notifier, we want to make sure


Could you comment here why batch processing is needed?

Changed wording.

…ateEditingValue

LongCatIsLooong · 2020-10-09T04:51:52Z

@justinmc @xu-baolin @GaryQian
Ready for review again. Updates and notes:

It seems the input formatters should trigger a reformat when they change, but currently we're not doing so. I'll try to figure out if there's any practical reasons behind that.
Update: I'm going to open a sepaprate PR for the text input formatter stuff.
"platform text input plugin" vs "system text input". Still not quite sure about the nomenclature. I'd like to think these 2 are different entities, the former is the engine code that implements input handling, and the latter refers to the OS part of text input (i.e., IME/software keyboard, InputMethodManager). ~~But I don't think developers need to be aware of the difference. Leaning towards system text input as it's more self-explanatory.~~ That API doc section targets TextInputClient and platform text input plugin authors I think it's ok to distinguish the two.
Added beginBatchEdit and endBatchEdit.

justinmc

LGTM but FYI there is a merge conflict.

Linking @xu-baolin's PR #67892, which is related and may affect the text input formatter stuff you mentioned.

I'm on board with the platform/system nomenclature and will try to use it consistently!

lzhuor · 2020-10-13T02:53:36Z

Hi @justinmc , may I ask will this be patched to 1.22 or it will be in the next minor release (e.g. 1.23)?

This reverts commit daa6b2c.

justinmc · 2020-10-13T16:42:06Z

It looks like this has been reverted unfortunately, but we'll try to get it back in soon. Watch for another PR linked here.

This reverts commit a57f45e.

lzhuor · 2020-10-18T16:31:39Z

Hi @justinmc , thanks for your help. May I ask is there any update on this issue? I understand it's reverted. Could you please share the new issue / PR related to this fix?

justinmc · 2020-10-19T14:38:02Z

Sure, this was relanded in #68043 and is currently in master.

LongCatIsLooong added 7 commits September 23, 2020 17:01

update

0bbaf3b

remove text formatter caching

852dcae

update tests

d2169f1

update tests

4ced40d

add tests

edddcba

Merge branch 'editable-text-updateEditingValue' of github.com:LongCat…

59a0101

…IsLooong/flutter into editable-text-updateEditingValue

Merge remote-tracking branch 'upstream/master' into editable-text-upd…

9ced998

…ateEditingValue

flutter-dashboard bot added framework flutter/packages/flutter repository. See also f: labels. work in progress; do not review labels Sep 28, 2020

googlebot added the cla: yes label Sep 28, 2020

LongCatIsLooong force-pushed the editable-text-updateEditingValue branch from 8cb420f to c3fc5cc Compare September 28, 2020 23:27

docs

21cc3c1

LongCatIsLooong force-pushed the editable-text-updateEditingValue branch from c3fc5cc to 21cc3c1 Compare September 29, 2020 17:00

new test

38d6e99

LongCatIsLooong force-pushed the editable-text-updateEditingValue branch 5 times, most recently from cbda64f to 60b7a93 Compare September 29, 2020 19:51

LongCatIsLooong commented Sep 29, 2020

View reviewed changes

LongCatIsLooong requested review from GaryQian and justinmc September 29, 2020 19:56

LongCatIsLooong marked this pull request as ready for review September 29, 2020 19:56

LongCatIsLooong removed the work in progress; do not review label Sep 29, 2020

docs

d5f5bd8

LongCatIsLooong force-pushed the editable-text-updateEditingValue branch from 60b7a93 to d5f5bd8 Compare September 29, 2020 20:13

justinmc reviewed Sep 30, 2020

View reviewed changes

xu-baolin reviewed Sep 30, 2020

View reviewed changes

LongCatIsLooong mentioned this pull request Oct 1, 2020

[Android Text Input] Make the editing state listenable and allow batch edits flutter/engine#21534

Merged

8 tasks

justinmc mentioned this pull request Oct 1, 2020

iOS Text Editing Infinite Loop flutter/engine#20160

Merged

LongCatIsLooong added 2 commits October 4, 2020 23:26

review

6aeb71e

batch editing and tests

181a34c

xu-baolin reviewed Oct 9, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into editable-text-upd…

ac6090e

…ateEditingValue

justinmc approved these changes Oct 12, 2020

View reviewed changes

Merge branch 'master' into editable-text-updateEditingValue

306832f

LongCatIsLooong added the waiting for tree to go green label Oct 12, 2020

fluttergithubbot merged commit daa6b2c into flutter:master Oct 12, 2020

LongCatIsLooong deleted the editable-text-updateEditingValue branch October 12, 2020 21:21

renyou added a commit that referenced this pull request Oct 13, 2020

Revert "More EditableText docs (#66864)"

978d073

This reverts commit daa6b2c.

renyou mentioned this pull request Oct 13, 2020

Revert "More EditableText docs" #68025

Merged

renyou added a commit that referenced this pull request Oct 13, 2020

Revert "More EditableText docs (#66864)" (#68025)

a57f45e

This reverts commit daa6b2c.

LongCatIsLooong added a commit to LongCatIsLooong/flutter that referenced this pull request Oct 13, 2020

Reland "More EditableText docs (flutter#66864)"

6b2d818

This reverts commit a57f45e.

This was referenced Oct 14, 2020

TextField doesn't respect maxLength limit #67898

Closed

Rerun text formatters in didUpdateWidget if needed #68166

Merged

fluttergithubbot pushed a commit that referenced this pull request Oct 16, 2020

Reland "More EditableText docs (#66864)" (#68043)

438c4ff

LongCatIsLooong mentioned this pull request Nov 19, 2020

Skip reformatting and calling onChanged for composing region only changes. #70883

Merged

9 tasks

Renzo-Olivares mentioned this pull request Jan 26, 2023

[macOS] Avoid using TextEditingDeltaNonTextUpdate for syncing state. #118759

Closed

More EditableText docs #66864

More EditableText docs #66864

Uh oh!

Conversation

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

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

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 Oct 3, 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!

xu-baolin 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong commented Sep 28, 2020 •

edited

Loading

LongCatIsLooong Oct 3, 2020 •

edited

Loading

xu-baolin left a comment •

edited

Loading

xu-baolin Sep 30, 2020 •

edited

Loading

LongCatIsLooong Oct 9, 2020 •

edited

Loading

LongCatIsLooong commented Oct 9, 2020 •

edited

Loading