Fix EditableText misplaces caret when selection is invalid #123777

bleroux · 2023-03-30T14:47:53Z

Description

EditableTextState._handleFocusChanged adjusts the selection when a text field is focused. But if the selection becomes invalid (for instance after setting TextController.text) on an already focused field the position won’t be adjusted. The issue can be very visible when TextField.textAlign is set to TextAlign.center (video using the Android emulator):

120631_Before.mp4

Code sample for the repro

import 'package:flutter/material.dart';
import 'package:flutter/services.dart';

void main() {
  runApp(MyApp());
}

class MyApp extends StatelessWidget {
  final TextEditingController textController = TextEditingController();

  MyApp({super.key});

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      theme: ThemeData(
        primarySwatch: Colors.blue,
      ),
      home: Scaffold(
        body: Center(
          child: Row(children: [
            SizedBox(
                width: 60,
                child: TextField(
                  controller: textController,
                  textAlign: TextAlign.center,
                )),
            TextButton(
              child: const Text("Reset text", style: TextStyle(color: Colors.red)),
              onPressed: () {
                textController.text = "0";
              },
            ),
          ]),
        ),
      ),
    );
  }
}

Implementation

AFAIK, there are several ways to fix this issue:

Not showing the caret when the selection is invalid. This is described in RenderEditable should not show the caret when the selection is invalid (i.e. (-1, -1)) #79495. Unfortunately a PR for it was reverted (see [RenderEditable] Dont paint caret when selection is invalid #79607) due to a Google internal test failure.
Changing the selection to a valid one when TextController selection is invalid and the field is focused. this is the solution implemented in this PR. It breaks several tests which expect that the selection stays invalid.
Adding an assert to throw a detailed error message. The impact on existing tests will probably be worth than with solution 2 because for some of these tests It might be difficult to make them complied with this new requirement (for solution 2, tests can be updated by updating some expect calls).

Related Issue

Fixes #120631

Tests

Adds 1 test.
Will need to update several existing tests if becoming a real PR.

justinmc

Thanks for fixing this! I say do it and go ahead with updating the failing tests. It looks like there are only 2 failing in the framework tests check?

I'll also run the Google tests on this.

justinmc · 2023-03-31T22:03:13Z

packages/flutter/test/widgets/editable_text_test.dart

+            ),
+      );
+
+      // Update text with an invalid selection.


This comment isn't exactly what you're doing, is it old?

I can make this comment more self explanatory, but it was done on purpose because TextEditingController.test setter explicitly set the selection offset to -1 (which is the root of the issue addressed in this PR):

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

Lines 224 to 230 in f98499f

set text(String newText) {

value = value.copyWith(

text: newText,

selection: const TextSelection.collapsed(offset: -1),

composing: TextRange.empty,

);

}

Because TextEditingController.text is a public API, I did not focus on trying to change its implementation as it will probably be a breaking change and also because there might be no fix that would work in all cases. What can be done at the TextEditingController.text level is probably to enhance its documentation to discourage its use and to recommand using TextEditingController.value instead.

While writing this, I wonder if we can add a method to TextEditingController, with a String parameter, which will set the text using the given value AND set the selection collapsed at the end of the text?

Oh, got it: my comment is true for the moment but it would be wrong if the PR is merged.
I updated it, thanks!

Thanks 👍 . I've shied away from adding new methods to TextEditingController because it's used so widely. I think we do need a better place to put typical text editing operations, though.

justinmc · 2023-03-31T22:06:44Z

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

+      // If this field is focused and the selection is invalid, place the cursor at
+      // the end. Does not rely on _handleFocusChanged because it makes selection
+      // handles visible on Android.
+      widget.controller.selection = TextSelection.collapsed(offset: _value.text.length);


This seems reasonable to me. There have been some problems with invalid selection. The engine often will set the selection to -1, -1, to mean "no selection" (and elsewhere, sometimes a null selection is used to mean no selection). If the field is focused though, it makes sense to set the cursor to the end of the field as an improvement on those kinds of bugs.

bleroux · 2023-04-03T13:24:32Z

Thanks for fixing this! I say do it and go ahead with updating the failing tests. It looks like there are only 2 failing in the framework tests check?

I see a lot more tests failures: 22 test failures locally 😅
I will try to fix them so we can discuss in detail if it makes sense to update these tests.

bleroux · 2023-04-05T14:07:11Z

@justinmc
I updated failing tests. One choice was to replace most of TextController.text setter usage. With this PR, the selection won't be invalid but I think it is better to promote TextController.value instead.
I commented out one test in editable_text_shortcuts_test.dartbecause it would be obsoleted by this PR. I don't know if removing a test is ok?

justinmc · 2023-04-05T23:06:35Z

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

+      // handles visible on Android.
+      // Unregister as a listener to the text controller while making the change.
+      widget.controller.removeListener(_didChangeTextEditingValue);
+      widget.controller.selection = _adjustedSelectionWhenFocused()!;


Nit: I find it a little strange that _adjustedSelectionWhenFocused returns a nullable value and we use ! here. Like you have to have knowledge about the if statement above and the internals of _adjustedSelectionWhenFocused in order to use that !.

I can't think of a better way to do it though. If nothing jumps out at you as a way to improve that, I say don't worry about it and leave it as-is.

justinmc · 2023-04-05T23:06:56Z

packages/flutter/test/material/text_field_test.dart

@@ -6503,8 +6503,8 @@ void main() {
    variant: KeySimulatorTransitModeVariant.all()
  );

-  // Regressing test for https://github.com/flutter/flutter/issues/78219
-  testWidgets('Paste does not crash when the section is inValid', (WidgetTester tester) async {
+  // Regression test for https://github.com/flutter/flutter/issues/78219


Good catch 🔎

justinmc · 2023-04-05T23:08:39Z

packages/flutter/test/widgets/editable_text_shortcuts_test.dart

+  // Should we remove this test, because if this PR is approved it won't be possible
+  // to have an invalid selection for the controller if it is attached to a focused
+  // TextField.


I think it's fine to delete the whole test! Making a bug impossible is even better than having a test to catch it.

justinmc · 2023-04-05T23:14:52Z

packages/flutter/test/widgets/editable_text_test.dart

+            ),
+      );
+
+      // Update text with an invalid selection.


Thanks 👍 . I've shied away from adding new methods to TextEditingController because it's used so widely. I think we do need a better place to put typical text editing operations, though.

justinmc

LGTM after you delete that test 👍

justinmc · 2023-04-06T22:13:53Z

CC @Renzo-Olivares Can you run these Google tests locally and see if they pass?

ebadta81 · 2023-04-07T07:15:25Z

Great job guys. I tested this on my test project, it is working fine.

…lutter/flutter#123777)

…23777) Fix EditableText misplaces caret when selection is invalid

…143452) ## Description This PR adds more documentation for `TextEditingController(String text)` constructor and it adds one example. #96245 was a first improvement to the documentation. #79495 tried to hide the cursor when an invalid selection is set but it was reverted. #123777 mitigated the issue of having a default invalid selection: it takes care of setting a proper selection when a text field is focused and its controller selection is not initialized. I will try changing the initial selection in another PR, but It will probably break several existing tests. ## Related Issue Fixes #95978 ## Tests Adds 1 test for the new example.

flutter-dashboard bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. labels Mar 30, 2023

bleroux requested a review from justinmc March 30, 2023 14:48

justinmc reviewed Mar 31, 2023

View reviewed changes

bleroux force-pushed the fix_EditableText_misplaces_caret_when_selection_is_invalid branch 3 times, most recently from 5042b1c to 649709d Compare April 4, 2023 18:48

goderbauer requested a review from justinmc April 4, 2023 22:18

bleroux force-pushed the fix_EditableText_misplaces_caret_when_selection_is_invalid branch 3 times, most recently from 852a339 to 10221e8 Compare April 5, 2023 11:53

justinmc reviewed Apr 5, 2023

View reviewed changes

justinmc approved these changes Apr 5, 2023

View reviewed changes

bleroux marked this pull request as ready for review April 6, 2023 12:45

bleroux force-pushed the fix_EditableText_misplaces_caret_when_selection_is_invalid branch from f4477f7 to 7c6b71c Compare April 6, 2023 13:45

bleroux added 5 commits April 6, 2023 20:22

Fix EditableText misplaces caret when selection is invalid

fe9b260

Fix existing tests

11b05fa

Focus selection is not the same for web + avoid reentrant call

c66f1d9

Update comment in new tests

b6a480d

Remove obsoleted test

923c036

bleroux force-pushed the fix_EditableText_misplaces_caret_when_selection_is_invalid branch from e384c4b to 923c036 Compare April 6, 2023 18:22

bleroux added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 7, 2023

auto-submit bot merged commit aa6a0a9 into flutter:master Apr 7, 2023
72 checks passed

bleroux deleted the fix_EditableText_misplaces_caret_when_selection_is_invalid branch April 7, 2023 05:02

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

Roll Flutter from a086c640dcc4 to aa6a0a9dea63 (25 revisions) flutter/packages#3661

Merged

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

aa6a0a9de Fix EditableText misplaces caret when selection is invalid (f…

b19c60d

…lutter/flutter#123777)

HansMuller mentioned this pull request Apr 7, 2023

Flutter Framework Team: Notable Commits #121415

Open

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

Fix EditableText misplaces caret when selection is invalid (flutter#1…

18cd858

…23777) Fix EditableText misplaces caret when selection is invalid

bleroux mentioned this pull request Aug 30, 2023

TextFiled cannot delete text. #133636

Closed

2 tasks

This was referenced Feb 14, 2024

Add more documentation for TextEditingController default constructor #143452

Merged

TextField not receiving input on mobile keyboard #88833

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix EditableText misplaces caret when selection is invalid #123777

Fix EditableText misplaces caret when selection is invalid #123777

bleroux commented Mar 30, 2023

justinmc left a comment •

edited

justinmc Mar 31, 2023

bleroux Apr 3, 2023 •

edited

bleroux Apr 5, 2023

justinmc Apr 5, 2023

justinmc Mar 31, 2023

bleroux commented Apr 3, 2023

bleroux commented Apr 5, 2023

justinmc Apr 5, 2023 •

edited

justinmc Apr 5, 2023

justinmc Apr 5, 2023

justinmc Apr 5, 2023

justinmc left a comment

justinmc commented Apr 6, 2023

ebadta81 commented Apr 7, 2023

	set text(String newText) {
	value = value.copyWith(
	text: newText,
	selection: const TextSelection.collapsed(offset: -1),
	composing: TextRange.empty,
	);
	}

Fix EditableText misplaces caret when selection is invalid #123777

Fix EditableText misplaces caret when selection is invalid #123777

Conversation

bleroux commented Mar 30, 2023

Description

Implementation

Related Issue

Tests

justinmc left a comment • edited

Choose a reason for hiding this comment

justinmc Mar 31, 2023

Choose a reason for hiding this comment

bleroux Apr 3, 2023 • edited

Choose a reason for hiding this comment

bleroux Apr 5, 2023

Choose a reason for hiding this comment

justinmc Apr 5, 2023

Choose a reason for hiding this comment

justinmc Mar 31, 2023

Choose a reason for hiding this comment

bleroux commented Apr 3, 2023

bleroux commented Apr 5, 2023

justinmc Apr 5, 2023 • edited

Choose a reason for hiding this comment

justinmc Apr 5, 2023

Choose a reason for hiding this comment

justinmc Apr 5, 2023

Choose a reason for hiding this comment

justinmc Apr 5, 2023

Choose a reason for hiding this comment

justinmc left a comment

Choose a reason for hiding this comment

justinmc commented Apr 6, 2023

ebadta81 commented Apr 7, 2023

justinmc left a comment •

edited

bleroux Apr 3, 2023 •

edited

justinmc Apr 5, 2023 •

edited