-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Fix EditableText _justResumed is not accurate #177658
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 _justResumed is not accurate #177658
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request addresses an issue where text selection behavior is incorrect after an app resumes. The fix involves resetting a _justResumed flag upon the first focus change after a resume, which is a sound approach. The new test case effectively covers the scenario. However, I've identified a potential memory leak in the implementation where a listener might be added multiple times to the global FocusManager, and I've provided a suggestion to fix it.
3320a51 to
d13b7e9
Compare
justinmc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
| // Reset _justResumed as soon as there is a focus change. | ||
| FocusManager.instance.addListener(_resetJustResumed); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good call. Those should be all the situations where we want to reset _justResumed that I can think of...
| await setAppLifeCycleState(AppLifecycleState.resumed); | ||
| await setAppLifecycleState(AppLifecycleState.inactive); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I agree with you that it should be Lifecycle and not LifeCycle, but it looks like we have both all over the code base 🤷
flutter/flutter@7cf0dc1...df72035 2025-10-29 [email protected] Roll Packages from c8ba0cc to 41c6b3d (3 revisions) (flutter/flutter#177725) 2025-10-29 [email protected] [Android] Remove unecessary `spy` in `FlutterActivityAndFragmentDelegateTest` (flutter/flutter#177120) 2025-10-29 [email protected] Add discussion of federated plugin documentation (flutter/flutter#177659) 2025-10-29 [email protected] [web] Deprecate --pwa-strategy (flutter/flutter#177613) 2025-10-29 [email protected] Roll Skia from 53b8b802bbc4 to 0a0c9f8c704f (1 revision) (flutter/flutter#177701) 2025-10-29 [email protected] Add `Navigator.popUntilWithResult` (flutter/flutter#169341) 2025-10-29 [email protected] Replace deprecated `withOpacity` with `withValues` in `text_style.dart` (flutter/flutter#177537) 2025-10-29 [email protected] Replace deprecated withOpacity in `radio.1.dart` example (flutter/flutter#177606) 2025-10-29 [email protected] Roll Skia from 4408d8ea88b0 to 53b8b802bbc4 (2 revisions) (flutter/flutter#177698) 2025-10-29 [email protected] Remove generated file from template manifest (flutter/flutter#177034) 2025-10-29 [email protected] Roll Skia from e582a5594b6f to 4408d8ea88b0 (5 revisions) (flutter/flutter#177691) 2025-10-28 [email protected] Roll Fuchsia Linux SDK from ir6J2isKAYa1jNLyJ... to 3EF6k6lqXPWDwrdyj... (flutter/flutter#177682) 2025-10-28 [email protected] [Gradle 9] Fix Engine Deps (flutter/flutter#177623) 2025-10-28 [email protected] Replace deprecated `withOpacity` in `focus_scope.0.dart` example (flutter/flutter#177542) 2025-10-28 [email protected] Fix EditableText _justResumed is not accurate (flutter/flutter#177658) 2025-10-28 [email protected] Replace deprecated `withOpacity` in `interactive_viewer.builder.0.dart` (flutter/flutter#177541) 2025-10-28 [email protected] Fix TextButton.icon breaks focus traversal and ink effect when toggling icon (flutter/flutter#176579) 2025-10-28 [email protected] [Android 16] Update Engine `ci.yaml` to test against Java 21 (flutter/flutter#177677) 2025-10-28 [email protected] Replace deprecated `withOpacity` in `interactive_viewer.constrained.0.dart` (flutter/flutter#177540) 2025-10-28 [email protected] Replace opacity from random color in navigation bar test (flutter/flutter#177490) 2025-10-28 [email protected] Roll Skia from e4d3d8f31aef to e582a5594b6f (6 revisions) (flutter/flutter#177679) 2025-10-28 [email protected] Workaround for lag when dragging window titlebar on Windows (flutter/flutter#177597) 2025-10-28 [email protected] Roll Packages from bbf96a0 to c8ba0cc (2 revisions) (flutter/flutter#177672) 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
## Description This PR tweaks the selection logic added in flutter#157399. Before this PR (and since flutter#157399) when the app resumed while a TextField was selected the selection of the TextField is maintained. This is the right behavior for the currently focused TextField. But when there are several TextFields, after the app resumed and the user move the focus to a another TextField, the behavior should be to select all the content of the newly focused TextField. To achieve this the `_justResumed`flag added in flutter#157399 should be reset as soon as the focus move as it is needed only for the current focused TextField to restore its selection just after the app resumed. ## Related Issue Fixes [Pressing tab does select all content when app is resumed for TextFields which were not focused](flutter#177650) ## Tests - Adds 1 test
## Description This PR tweaks the selection logic added in flutter#157399. Before this PR (and since flutter#157399) when the app resumed while a TextField was selected the selection of the TextField is maintained. This is the right behavior for the currently focused TextField. But when there are several TextFields, after the app resumed and the user move the focus to a another TextField, the behavior should be to select all the content of the newly focused TextField. To achieve this the `_justResumed`flag added in flutter#157399 should be reset as soon as the focus move as it is needed only for the current focused TextField to restore its selection just after the app resumed. ## Related Issue Fixes [Pressing tab does select all content when app is resumed for TextFields which were not focused](flutter#177650) ## Tests - Adds 1 test
Description
This PR tweaks the selection logic added in #157399.
Before this PR (and since #157399) when the app resumed while a TextField was selected the selection of the TextField is maintained. This is the right behavior for the currently focused TextField. But when there are several TextFields, after the app resumed and the user move the focus to a another TextField, the behavior should be to select all the content of the newly focused TextField. To achieve this the
_justResumedflag added in #157399 should be reset as soon as the focus move as it is needed only for the current focused TextField to restore its selection just after the app resumed.Related Issue
Fixes Pressing tab does select all content when app is resumed for TextFields which were not focused
Tests