Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix EditableText misplaces caret when selection is invalid #123777

Conversation

bleroux
Copy link
Contributor

@bleroux bleroux commented Mar 30, 2023

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:

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

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

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

@flutter-dashboard 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 bleroux requested a review from justinmc March 30, 2023 14:48
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

),
);

// Update text with an invalid selection.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@bleroux bleroux Apr 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

bleroux commented Apr 3, 2023

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 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 goderbauer requested a review from justinmc April 4, 2023 22:18
@bleroux 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
@bleroux
Copy link
Contributor Author

bleroux commented Apr 5, 2023

@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?

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

@justinmc justinmc Apr 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch 🔎

Comment on lines 93 to 95
// 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

),
);

// Update text with an invalid selection.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after you delete that test 👍

@bleroux bleroux marked this pull request as ready for review April 6, 2023 12:45
@bleroux bleroux force-pushed the fix_EditableText_misplaces_caret_when_selection_is_invalid branch from f4477f7 to 7c6b71c Compare April 6, 2023 13:45
@bleroux bleroux force-pushed the fix_EditableText_misplaces_caret_when_selection_is_invalid branch from e384c4b to 923c036 Compare April 6, 2023 18:22
@justinmc
Copy link
Contributor

justinmc commented Apr 6, 2023

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

@bleroux bleroux added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 7, 2023
@auto-submit auto-submit bot merged commit aa6a0a9 into flutter:master Apr 7, 2023
69 checks passed
@bleroux bleroux deleted the fix_EditableText_misplaces_caret_when_selection_is_invalid branch April 7, 2023 05:02
@ebadta81
Copy link

ebadta81 commented Apr 7, 2023

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

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

Fix EditableText misplaces caret when selection is invalid
@bleroux bleroux mentioned this pull request Aug 30, 2023
2 tasks
auto-submit bot pushed a commit that referenced this pull request Feb 14, 2024
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: text input Entering text in a text field or keyboard related problems autosubmit Merge PR when tree becomes green via auto submit App framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TextField ignores user input, and cursor is rendered wrong in the middle of the text
3 participants