[EditableText] call `onSelectionChanged` only when there are actual selection/cause changes #87971

LongCatIsLooong · 2021-08-09T23:23:58Z

Also cleaned up the updateEditingValue/userUpdateEditingValue
paths a bit. Hopefully we will be able to merge the two.

Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.

List which issues are fixed by this PR. You must list at least one issue.

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

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 feature I am adding, or Hixie said the PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

selection/cause changes Fixes flutter#76349. Also cleaned up the `updateEditingValue`/`userUpdateEditingValue` paths a bit. Hopefully we will be able to merge the two.

chunhtai · 2021-08-11T20:09:15Z

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

  bool _wasSelectingVerticallyWithKeyboard = false;

  void _setTextEditingValue(TextEditingValue newValue, SelectionChangedCause cause) {
-    textSelectionDelegate.textEditingValue = newValue;


I kept this for backward compatibility, we should only remove it once we remove the deprecated setter textEditingValue

Could you elaborate? People are required to implement userUpdateTextEditingValue right? It's part of the TextSelectionDelegate interface.

I didn't notice userUpdateTextEditingValue is abstract method. yeah this should be fine.

chunhtai · 2021-08-11T20:11:44Z

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

  ScrollController? _scrollController;

-  late AnimationController _cursorBlinkOpacityController;
+  late final AnimationController _cursorBlinkOpacityController = AnimationController(vsync: this, duration: _fadeDuration);


I think we missed this but it should be disposed in the dispose method.
same for _floatingCursorResetController. I think the existing format is cleaner, it is weird the we give a default value in here and then attach listener in initState

Whoops good catch about the dispose.

These should be fixed in #87973

moved the addListener to the lazy initializer. My initial goal was to mark the property as final. I'll do the same for other animation controllers in #87973.

chunhtai · 2021-08-11T20:20:05Z

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

+      ? _value.selection != value.selection
+      : _value != value;
+    if (shouldShowCaret) {
+      _scheduleShowCaretOnScreen();


I didnt get the logic, the _scheduleShowCaretOnScreen will be called in line 1770 anyway?

Ah I forgot to remove the existing _sheduleShowCaretOnScreen when copied the other one over.

chunhtai · 2021-08-11T20:29:04Z

packages/flutter/test/cupertino/text_field_test.dart

    await gesture.moveBy(const Offset(600, 0));
    // To the edge of the screen basically.
    await tester.pump();
+    await tester.pumpAndSettle();


where do we introduce frame delays?

There's no need for these changes. Removed.

justinmc

LGTM 👍 . This was badly needed, thanks for cleaning this up.

justinmc · 2021-08-12T03:30:29Z

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

  void onDoubleTapDown(TapDownDetails details) {
    if (delegate.selectionEnabled) {
-      renderEditable.selectWord(cause: SelectionChangedCause.tap);
+      renderEditable.selectWord(cause: SelectionChangedCause.doubleTap);


justinmc · 2021-08-12T03:31:38Z

packages/flutter/test/widgets/editable_text_test.dart


    await testTextEditing(tester, targetPlatform: defaultTargetPlatform);
-
    debugKeyEventSimulatorTransitModeOverride = null;


Can you remove this line now that you have the teardown?

Same with the next test below.

This doesn't seem to work, _verifyInvariants is called before the the tear down callbacks are invoked.

Ah ok interesting, best to keep both then 👍

justinmc · 2021-08-12T03:32:54Z

packages/flutter/test/widgets/editable_text_test.dart

-      index++;
-    }
+
+    expect(


justinmc · 2021-08-12T03:35:14Z

packages/flutter/test/widgets/editable_text_test.dart

+      expect(selection, const TextSelection(baseOffset: 1, extentOffset: 2));
+      expect(cause, SelectionChangedCause.keyboard);
+
+      // Selection and cause both changed


Nit: period here.

justinmc · 2021-08-12T03:42:46Z

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

-        _handleSelectionChanged(TextSelection.collapsed(offset: _lastTextPosition!.offset), SelectionChangedCause.forcePress);
+        final TextEditingValue newValue = _value.copyWith(
+          selection: TextSelection.fromPosition(_lastTextPosition!),
+        );


The new code preserves the TextAffinity of _lastTextPosition, but the old code didn't. Probably better this way but something to be aware of.

The floating cursor stuff currently doesn't care much about affinity (e.g., final TextPosition currentTextPosition = TextPosition(offset: renderEditable.selection!.baseOffset);). I'm hoping we can take cursor handling out of EditableTextState and maybe clean floating cursor a bit in the process.

justinmc · 2021-08-12T03:57:43Z

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

      Rect? composingRect = renderEditable.getRectForComposingRange(composingRange);
      // Send the caret location instead if there's no marked text yet.
      if (composingRect == null) {
-        assert(!composingRange.isValid || composingRange.isCollapsed);


Why was this assert removed?

I must have thought getRectForComposingRange only returns null only when !composingRange.isValid || composingRange.isCollapsed is true but if getBoxesForSelection returns an empty list that method returns null too. It's triggered in one of the tests so I removed it.

justinmc · 2021-08-12T03:58:24Z

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

+  // This method is similar to updateEditingValue, but is used to handle user
+  // input caused by hardware keyboard events and gesture events, while
+  // updateEditingValue handles IME/software keyboard input.


Thank you for adding the comment here!

justinmc · 2021-08-12T04:18:07Z

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

+      _currentPromptRectRange = null;
+
+      if (_hasInputConnection) {
+        if (widget.obscureText && value.text.length == _value.text.length + 1) {


I got really worried about extended grapheme clusters and obscured fields when I saw this + 1 here (even though I know it has nothing to do with this PR). Indeed they show up as multiple characters when entered into an obscured field and don't have a delay, but it turns out that's the behavior on native too 🤷 . All is well.

justinmc · 2021-08-12T04:19:54Z

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

  ScrollController? _scrollController;

-  late AnimationController _cursorBlinkOpacityController;
+  late final AnimationController _cursorBlinkOpacityController = AnimationController(vsync: this, duration: _fadeDuration);


Whoops good catch about the dispose.

…ctual selection/cause changes (#87971)" This reverts commit 6e224ee.

…ctual selection/cause changes (#87971)" (#88183)

…lection/cause changes (flutter#87971)

…ctual selection/cause changes (flutter#87971)" (flutter#88183)

[EditableText] call onSelectionChanged when there's actual

b04b362

selection/cause changes Fixes flutter#76349. Also cleaned up the `updateEditingValue`/`userUpdateEditingValue` paths a bit. Hopefully we will be able to merge the two.

LongCatIsLooong requested review from chunhtai and justinmc August 9, 2021 23:23

flutter-dashboard bot added f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Aug 9, 2021

google-cla bot added the cla: yes label Aug 9, 2021

LongCatIsLooong changed the title ~~[EditableText] call onSelectionChanged when there's actual selection/cause changes~~ [EditableText] call onSelectionChanged only when there're actual selection/cause changes Aug 9, 2021

chunhtai reviewed Aug 11, 2021

View reviewed changes

justinmc approved these changes Aug 12, 2021

View reviewed changes

LongCatIsLooong force-pushed the remove-excessive-onSelectionChanged-calls branch from b04b362 to d751b73 Compare August 12, 2021 07:16

review

f674e17

LongCatIsLooong force-pushed the remove-excessive-onSelectionChanged-calls branch from d751b73 to f674e17 Compare August 12, 2021 21:29

LongCatIsLooong added the waiting for tree to go green label Aug 13, 2021

fluttergithubbot merged commit 6e224ee into flutter:master Aug 13, 2021

LongCatIsLooong deleted the remove-excessive-onSelectionChanged-calls branch August 13, 2021 00:34

LongCatIsLooong added a commit that referenced this pull request Aug 13, 2021

Revert "[EditableText] call onSelectionChanged only when there're a…

fa3a8f8

…ctual selection/cause changes (#87971)" This reverts commit 6e224ee.

LongCatIsLooong mentioned this pull request Aug 13, 2021

Revert "[EditableText] call onSelectionChanged only when there're actual selection/cause changes" #88183

Merged

fluttergithubbot pushed a commit that referenced this pull request Aug 14, 2021

Revert "[EditableText] call onSelectionChanged only when there're a…

b3d12eb

…ctual selection/cause changes (#87971)" (#88183)

blasten pushed a commit to blasten/flutter that referenced this pull request Aug 19, 2021

[EditableText] call onSelectionChanged only when there're actual se…

f0df4c9

…lection/cause changes (flutter#87971)

blasten pushed a commit to blasten/flutter that referenced this pull request Aug 19, 2021

Revert "[EditableText] call onSelectionChanged only when there're a…

040de19

…ctual selection/cause changes (flutter#87971)" (flutter#88183)

HansMuller changed the title ~~[EditableText] call onSelectionChanged only when there're actual selection/cause changes~~ [EditableText] call onSelectionChanged only when there are actual selection/cause changes Aug 20, 2021

Renzo-Olivares mentioned this pull request May 14, 2025

EditableText calls onSelectionChanged even if the selection does not change. #76349

Open


		await testTextEditing(tester, targetPlatform: defaultTargetPlatform);

		debugKeyEventSimulatorTransitModeOverride = null;

[EditableText] call onSelectionChanged only when there are actual selection/cause changes #87971

[EditableText] call onSelectionChanged only when there are actual selection/cause changes #87971

Uh oh!

Conversation

LongCatIsLooong commented Aug 9, 2021

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[EditableText] call `onSelectionChanged` only when there are actual selection/cause changes #87971

[EditableText] call `onSelectionChanged` only when there are actual selection/cause changes #87971