fix: SelectableText should handle focus changes #155771

Renzo-Olivares · 2024-09-26T16:55:25Z

This change updates the behavior of SelectableText, to clear its selection when it loses focus and the application is currently running. This fixes the behavior where you may have multiple active highlights if you have SelectableText along with other "selectable" widgets such as TextField, or Text widgets under a SelectionArea.

If the application is in the background, for example when another window is focused, the selection should be retained so when a user returns to the application it is still there.

This change also updates the behavior of selection on macOS, single tap up, previously it was selecting the word edge closest to the tapped position, the correct behavior on native is to select the precise position. This was causing onSelectionChanged to be called twice, once for tap down (sets the precise tapped position, handled by logic in TextSelectionGestureDetector), and a second time for single tap up (moves the cursor to closest word edge, handled by logic in _SelectableTextSelectionGestureDetectorBuilder). This type of selection inconsistency is related to this issue #129726, I plan to look into this further in a separate PR.

Fixes #117573
Fixes #103725

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 this PR is [test-exempt].
I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
All existing and new tests are passing.

justinmc

LGTM 👍

It seems a little weird that we have to check the lifecycle state when the focus changes, but if that's how Flutter's focus works then I guess that's what you have to do.

justinmc · 2024-09-26T20:10:41Z

packages/flutter/lib/src/material/selectable_text.dart

+      // We should only clear the selection when this SelectableText loses
+      // focus while the application is currently running. It is possible
+      // that the application is not currently running, for example on desktop
+      // platforms, clicking on a different window switches the focus to
+      // the new window causing the Flutter application to go inactive. In this
+      // case we want to retain the selection so it remains when we return to
+      // the Flutter application.


So @gspencergoog when the application is backgrounded, any focused FocusNode loses focus? Just making sure that's how it works and is intended.

For more context on this, as of #142930, the FocusManager responds to app lifecycle changes.

This looks correct to me then, thanks for the context 👍

justinmc · 2024-09-26T20:14:25Z

packages/flutter/test/widgets/selectable_text_test.dart

+    await tester.pumpAndSettle();
+    expect(controllerA.selection, TextRange.empty);
+    expect(controllerB.selection, const TextSelection.collapsed(offset: 0));
+  }, variant: TargetPlatformVariant.all());


Is it worth adding .all here instead of just omitting it? You do have a switch on the platform above, but maybe I think it doesn't affect this particular test.

I'm okay with either, but I usually lean towards .all if possible, and not platform specific, for more test coverage. What do you think?

justinmc · 2024-09-26T20:15:09Z

packages/flutter/test/widgets/selectable_text_test.dart

-    variant: const TargetPlatformVariant(<TargetPlatform>{ TargetPlatform.iOS,  TargetPlatform.macOS }),
+    variant: const TargetPlatformVariant(<TargetPlatform>{ TargetPlatform.iOS, TargetPlatform.macOS }),


justinmc · 2024-09-26T20:17:39Z

I think you need to merge master into this branch to fix the Google test failure.

auto-submit · 2024-09-26T22:17:25Z

auto label is removed for flutter/flutter/155771, due to - The status or check suite Windows build_tests_2_7 has failed. Please fix the issues identified (or deflake) before re-applying this label.

…tion

…ter#155771)

Manual roll requested by [email protected] flutter/flutter@fa402c8...ead6b0d 2024-09-27 [email protected] Remove left-over traces of "link-dry-run" - which isn't used anywhere in flutter (flutter/flutter#155820) 2024-09-27 [email protected] Roll Flutter Engine from e57b440ec4ee to 7c603de2dca7 (5 revisions) (flutter/flutter#155811) 2024-09-27 [email protected] Fix DropdownMenu rendered behind AppBar (flutter/flutter#155539) 2024-09-27 [email protected] Roll Flutter Engine from 53517772a5b0 to e57b440ec4ee (8 revisions) (flutter/flutter#155799) 2024-09-27 [email protected] Throw StateError when implicitView is null on `wrapWithDefaultView`. (flutter/flutter#155734) 2024-09-27 [email protected] Roll packages manually (flutter/flutter#155786) 2024-09-27 [email protected] fix: SelectableText should handle focus changes (flutter/flutter#155771) 2024-09-27 [email protected] Use flutter from in same repo (not path) in `generate_gradle_lockfiles.dart` (again) (flutter/flutter#155794) 2024-09-26 [email protected] Use flutter from in same repo (not path) in `generate_gradle_lockfiles.dart` (flutter/flutter#155790) 2024-09-26 [email protected] `RenderParagraph` should invalidate its `_SelectableFragment`s cached rects on window size updates (flutter/flutter#155719) 2024-09-26 [email protected] Fix broken text field with set hint and min and max lines(#153183) (flutter/flutter#153235) 2024-09-26 [email protected] Roll Flutter Engine from 9e6133e8d906 to 53517772a5b0 (1 revision) (flutter/flutter#155772) 2024-09-26 [email protected] Fix line-wrapping in `flutter create` error message. (flutter/flutter#150325) 2024-09-26 [email protected] remove fujino from CODEOWNERS (flutter/flutter#155369) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…ter#155771)

github-actions bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Sep 26, 2024

Renzo-Olivares requested a review from justinmc September 26, 2024 17:34

Renzo-Olivares force-pushed the selectabletext-focus-fix branch from aa001c5 to a6dc254 Compare September 26, 2024 18:27

justinmc approved these changes Sep 26, 2024

View reviewed changes

Renzo-Olivares force-pushed the selectabletext-focus-fix branch from a6dc254 to 6ea29fa Compare September 26, 2024 20:50

Renzo-Olivares added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 26, 2024

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 26, 2024

Renzo-Olivares added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 26, 2024

Renzo Olivares added 4 commits September 26, 2024 16:16

SelectableText should not retain selection on focus changes

cddf2a4

updates

9d61e67

Add tests and update macOS single tap behavior to select precise posi…

eaa0fe9

…tion

address review comments

be2d9d3

Renzo-Olivares force-pushed the selectabletext-focus-fix branch from 020592c to be2d9d3 Compare September 26, 2024 23:16

auto-submit bot merged commit 799cf16 into flutter:master Sep 27, 2024

engine-flutter-autoroll mentioned this pull request Sep 27, 2024

Roll Flutter from fa402c8057a1 to ead6b0d17c89 (14 revisions) flutter/packages#7718

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 27, 2024

799cf16 fix: SelectableText should handle focus changes (flutter/flut…

90aa8b3

…ter#155771)

engine-flutter-autoroll mentioned this pull request Sep 27, 2024

Roll Flutter from fa402c8057a1 to ead6b0d17c89 (14 revisions) flutter/packages#7719

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 27, 2024

799cf16 fix: SelectableText should handle focus changes (flutter/flut…

2cd6c26

…ter#155771)

engine-flutter-autoroll mentioned this pull request Sep 27, 2024

Roll Flutter from fa402c8057a1 to 180f40108bcc (15 revisions) flutter/packages#7720

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 27, 2024

799cf16 fix: SelectableText should handle focus changes (flutter/flut…

e10350c

…ter#155771)

Piinks mentioned this pull request Sep 27, 2024

Flutter Framework Team: Notable Commits #121415

Closed

engine-flutter-autoroll mentioned this pull request Sep 28, 2024

Roll Flutter from fa402c8057a1 to 9f88de930bb5 (24 revisions) flutter/packages#7726

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 28, 2024

799cf16 fix: SelectableText should handle focus changes (flutter/flut…

2bfcadb

…ter#155771)

engine-flutter-autoroll mentioned this pull request Sep 28, 2024

Roll Flutter from fa402c8057a1 to 9f88de930bb5 (24 revisions) flutter/packages#7727

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 28, 2024

799cf16 fix: SelectableText should handle focus changes (flutter/flut…

08ba331

…ter#155771)

engine-flutter-autoroll mentioned this pull request Sep 29, 2024

Roll Flutter from fa402c8057a1 to 4fdfbd4dde8a (28 revisions) flutter/packages#7728

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 1, 2024

799cf16 fix: SelectableText should handle focus changes (flutter/flut…

9ab73d8

…ter#155771)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 1, 2024

799cf16 fix: SelectableText should handle focus changes (flutter/flut…

51f15f9

…ter#155771)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 1, 2024

799cf16 fix: SelectableText should handle focus changes (flutter/flut…

f7b1080

…ter#155771)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 1, 2024

799cf16 fix: SelectableText should handle focus changes (flutter/flut…

d45a90d

…ter#155771)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 1, 2024

799cf16 fix: SelectableText should handle focus changes (flutter/flut…

018de33

…ter#155771)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 1, 2024

799cf16 fix: SelectableText should handle focus changes (flutter/flut…

28808e2

…ter#155771)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 2, 2024

799cf16 fix: SelectableText should handle focus changes (flutter/flut…

c3760e5

…ter#155771)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 2, 2024

799cf16 fix: SelectableText should handle focus changes (flutter/flut…

85ff747

…ter#155771)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 2, 2024

799cf16 fix: SelectableText should handle focus changes (flutter/flut…

f9c3668

…ter#155771)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 2, 2024

799cf16 fix: SelectableText should handle focus changes (flutter/flut…

f873138

…ter#155771)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 2, 2024

799cf16 fix: SelectableText should handle focus changes (flutter/flut…

ced18d5

…ter#155771)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 2, 2024

799cf16 fix: SelectableText should handle focus changes (flutter/flut…

43f851d

…ter#155771)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 3, 2024

799cf16 fix: SelectableText should handle focus changes (flutter/flut…

508502f

…ter#155771)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 3, 2024

799cf16 fix: SelectableText should handle focus changes (flutter/flut…

7567434

…ter#155771)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 3, 2024

799cf16 fix: SelectableText should handle focus changes (flutter/flut…

32b23b3

…ter#155771)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 3, 2024

799cf16 fix: SelectableText should handle focus changes (flutter/flut…

09c51f0

…ter#155771)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 3, 2024

799cf16 fix: SelectableText should handle focus changes (flutter/flut…

86f9f53

…ter#155771)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 3, 2024

799cf16 fix: SelectableText should handle focus changes (flutter/flut…

3be73ec

…ter#155771)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 4, 2024

799cf16 fix: SelectableText should handle focus changes (flutter/flut…

0e57831

…ter#155771)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 4, 2024

799cf16 fix: SelectableText should handle focus changes (flutter/flut…

3062490

…ter#155771)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 4, 2024

799cf16 fix: SelectableText should handle focus changes (flutter/flut…

e12ca30

…ter#155771)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 5, 2024

799cf16 fix: SelectableText should handle focus changes (flutter/flut…

83bd13a

…ter#155771)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 5, 2024

799cf16 fix: SelectableText should handle focus changes (flutter/flut…

55874ee

…ter#155771)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 6, 2024

799cf16 fix: SelectableText should handle focus changes (flutter/flut…

a1a513c

…ter#155771)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 6, 2024

799cf16 fix: SelectableText should handle focus changes (flutter/flut…

6abc979

…ter#155771)

engine-flutter-autoroll mentioned this pull request Oct 7, 2024

Manual roll Flutter from fa402c8057a1 to ead6b0d17c89 (14 revisions) flutter/packages#7806

Merged

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

799cf16 fix: SelectableText should handle focus changes (flutter/flut…

4048ac3

…ter#155771)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 11, 2024

799cf16 fix: SelectableText should handle focus changes (flutter/flut…

86d641f

…ter#155771)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 12, 2024

799cf16 fix: SelectableText should handle focus changes (flutter/flut…

d86d050

…ter#155771)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: SelectableText should handle focus changes #155771

fix: SelectableText should handle focus changes #155771

Uh oh!

Renzo-Olivares commented Sep 26, 2024

Uh oh!

justinmc left a comment

Uh oh!

justinmc Sep 26, 2024

Uh oh!

Renzo-Olivares Sep 26, 2024

Uh oh!

justinmc Sep 26, 2024

Uh oh!

justinmc Sep 26, 2024

Uh oh!

Renzo-Olivares Sep 26, 2024

Uh oh!

justinmc Sep 26, 2024

Uh oh!

justinmc commented Sep 26, 2024

Uh oh!

auto-submit bot commented Sep 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		variant: const TargetPlatformVariant(<TargetPlatform>{ TargetPlatform.iOS, TargetPlatform.macOS }),
		variant: const TargetPlatformVariant(<TargetPlatform>{ TargetPlatform.iOS, TargetPlatform.macOS }),

fix: SelectableText should handle focus changes #155771

fix: SelectableText should handle focus changes #155771

Uh oh!

Conversation

Renzo-Olivares commented Sep 26, 2024

Pre-launch Checklist

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc Sep 26, 2024

Choose a reason for hiding this comment

Uh oh!

Renzo-Olivares Sep 26, 2024

Choose a reason for hiding this comment

Uh oh!

justinmc Sep 26, 2024

Choose a reason for hiding this comment

Uh oh!

justinmc Sep 26, 2024

Choose a reason for hiding this comment

Uh oh!

Renzo-Olivares Sep 26, 2024

Choose a reason for hiding this comment

Uh oh!

justinmc Sep 26, 2024

Choose a reason for hiding this comment

Uh oh!

justinmc commented Sep 26, 2024

Uh oh!

auto-submit bot commented Sep 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants