[EditableText] preserve selection/composition range on unfocus by LongCatIsLooong · Pull Request #86796 · flutter/flutter

LongCatIsLooong · 2021-07-21T18:01:28Z

Related Issue: #79982
Fixes #86752

Let the selection/composition range of an EditableText survive through
unfocus, and allows the original selection/composition range to be
restored when the field regains focus.

Demo

Unfortunately the selection handle (and the context menu) is gone when a text field regains focus. This probably needs a new issue since the current implementation of _handleSelectionChanged (and the corresponding logic in the gesture detector callbacks) is really convoluted.

ezgif.com-gif-maker.mp4

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.

Related Issue: flutter#79982 Let the selection/composition range of an `EditableText` survive through unfocus, and allows the original selection/composition range to be restored when the field regains focus.

…on-on-unfocus

LongCatIsLooong · 2021-07-21T18:02:53Z

@chunhtai I think you mentioned the desktop accessibility implementation specifically use (-1, -1) for something. Is that going to be a problem if we don't remove the selection and the composing range upon unfocus?

LongCatIsLooong · 2021-07-21T18:06:28Z

The buildTextSpan method has something similar (the composing region is not painted if the widget is readOnly). But this is a little bit too magical for a low level widget.

Is it ever possible for a SelectableText to show its selection while another EditableText has focus?

Doesn't seem possible, when a SelectableText loses focus the selection is set to TextRange.empty. Unless there's a SelectableText that initially has a non-empty selection and is not focused.

LongCatIsLooong · 2021-07-21T18:11:47Z

Before the change the underline was shown even when the text field is not focused. That's why this test was passing without the additional pump.

chunhtai · 2021-07-21T18:18:26Z

@chunhtai I think you mentioned the desktop accessibility implementation specifically use (-1, -1) for something. Is that going to be a problem if we don't move the selection and the composing range upon unfocus?

That should be fine.

justinmc

LGTM 👍 Just one question and some nits.

justinmc · 2021-08-02T22:03:36Z

I guess the caret was blinking when the field had a selection even when it wasn't focused.

Any suggestions on process of recovering data with this issue

EditableTextState does call _stopCursorTimer when the widget is unfocused. Reverting this change does not seem to break any tests so I undid the change.

justinmc · 2021-08-02T22:10:20Z

Is it ever possible for a SelectableText to show its selection while another EditableText has focus?

justinmc · 2021-08-02T22:12:31Z

Nit: Extra newline here.

justinmc · 2021-08-02T22:13:07Z

Nit: "an" => "a"

…on-on-unfocus

flutter-dashboard · 2021-08-03T01:05:46Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

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

Changes reported for pull request #86796 at sha dfdf9fd

…on-on-unfocus

LongCatIsLooong

@justinmc I moved the focus selection highlight logic to high-level text fields. Could you take a look again?

LongCatIsLooong · 2021-08-02T22:41:29Z

Doesn't seem possible, when a SelectableText loses focus the selection is set to TextRange.empty. Unless there's a SelectableText that initially has a non-empty selection and is not focused.

LongCatIsLooong · 2021-08-03T19:07:08Z

EditableTextState does call _stopCursorTimer when the widget is unfocused. Reverting this change does not seem to break any tests so I undid the change.

justinmc

Changes LGTM minus a potential scuba failure, happy to look again when that's fixed.

justinmc · 2021-08-04T23:37:19Z

+    if (widget.focusNode != oldWidget.focusNode) {
+      (oldWidget.focusNode ?? _focusNode)?.removeListener(_handleFocusChanged);
+      (widget.focusNode ?? _focusNode)?.addListener(_handleFocusChanged);


I guess we weren't swapping these properly before 😁

The _handleFocusChange listener is added in this PR. We didn't have to remove/add listeners before.

justinmc · 2021-08-04T23:41:53Z

          minLines: widget.minLines,
          expands: widget.expands,
-          selectionColor: selectionColor,
+          // Only show the selection highlight when the text field is focused.


Super nit: There was no corresponding comment in CupertinoTextField here.

LongCatIsLooong added 3 commits July 20, 2021 15:30

[EditableText] preserve selection/composition range on unfocus

f0caf8b

Related Issue: flutter#79982 Let the selection/composition range of an `EditableText` survive through unfocus, and allows the original selection/composition range to be restored when the field regains focus.

Merge remote-tracking branch 'upstream/master' into dont-drop-selecti…

70a3869

…on-on-unfocus

test

35ec1ac

LongCatIsLooong requested review from chunhtai and justinmc July 21, 2021 18:01

flutter-dashboard Bot added p: material_ui material_ui package in flutter/packages framework flutter/packages/flutter repository. See also f: labels. labels Jul 21, 2021

google-cla Bot added the cla: yes label Jul 21, 2021

LongCatIsLooong commented Jul 21, 2021

View reviewed changes

LongCatIsLooong force-pushed the dont-drop-selection-on-unfocus branch from 8f7fa0b to 03aede3 Compare July 21, 2021 18:08

LongCatIsLooong commented Jul 21, 2021

View reviewed changes

LongCatIsLooong force-pushed the dont-drop-selection-on-unfocus branch from 03aede3 to 7303e34 Compare July 21, 2021 18:17

justinmc approved these changes Aug 2, 2021

View reviewed changes

LongCatIsLooong added 2 commits August 2, 2021 17:37

fix tests

c5e70f0

Merge remote-tracking branch 'upstream/master' into dont-drop-selecti…

dfdf9fd

…on-on-unfocus

LongCatIsLooong force-pushed the dont-drop-selection-on-unfocus branch from 7303e34 to dfdf9fd Compare August 3, 2021 00:37

flutter-dashboard Bot added the will affect goldens Changes to golden files label Aug 3, 2021

LongCatIsLooong added 5 commits August 3, 2021 09:44

Merge remote-tracking branch 'upstream/master' into dont-drop-selecti…

aee07f4

…on-on-unfocus

fix tests

f61a98d

move focus selection highlight changes to high-level text field widgets

ccd1905

Merge remote-tracking branch 'upstream/master' into dont-drop-selecti…

1a814aa

…on-on-unfocus

remove unnecessary changes

6dc779a

LongCatIsLooong force-pushed the dont-drop-selection-on-unfocus branch from 13c421d to 6dc779a Compare August 3, 2021 19:08

LongCatIsLooong commented Aug 3, 2021

View reviewed changes

LongCatIsLooong requested a review from justinmc August 3, 2021 21:04

justinmc approved these changes Aug 4, 2021

View reviewed changes

review

f6a0c44

LongCatIsLooong added waiting for tree to go green and removed will affect goldens Changes to golden files labels Aug 7, 2021

fluttergithubbot merged commit d3bdbed into flutter:master Aug 7, 2021

LongCatIsLooong deleted the dont-drop-selection-on-unfocus branch September 11, 2021 21:12

TahaTesser mentioned this pull request Nov 18, 2021

[CupertinoTextField] Add missing focus listener #93875

Merged

7 of 8 tasks

justinmc mentioned this pull request Jan 10, 2022

Programmatically select text in unfocused textfield not working #96339

Closed

markusaksli-nc mentioned this pull request Apr 29, 2022

TextField context menus do not show after focus switch #99707

Closed

Conversation

LongCatIsLooong commented Jul 21, 2021

Demo

Pre-launch Checklist

Uh oh!

LongCatIsLooong commented Jul 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

chunhtai commented Jul 21, 2021

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!

flutter-dashboard Bot commented Aug 3, 2021

Uh oh!

LongCatIsLooong 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

LongCatIsLooong commented Jul 21, 2021 •

edited

Loading