Fix TextField selects all content after the application is resumed #156968

bleroux · 2024-10-16T12:24:14Z

Description

This PR makes EditableText aware of the lifecycle 'resumed' state to let the current selection unchanged when the application is resumed (on web and desktop, 'resumed' means the Flutter app window regained focus).

Before this PR, on web and desktop, the whole content of a TextField was selected whenever a TextField gained focus. This is the correct behavior when tabbing between fields but it is not when a field regains focus after the application is resumed

Related Issue

Fixes When switching to another browser tab or window and then going back, all text on TextField is selected automatically.

Tests

Adds 1 test.

flutter-dashboard · 2024-10-16T12:24:18Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

justinmc

I feel like this approach LGTM... It does seem like it should be possible to know the cause of a focus change, but without that functionality, this seems to be the best way to do it. @gspencergoog is this a reliable way to do this or is there a better way?

If Greg is onboard then this just needs a test and a comment somewhere explaining what this is for, maybe on the declaration of justResumed.

I checked out the code locally and confirmed that it works in all the situations it should:

Scenario	Works?
Changing to another window and returning on desktop (Linux)	✔️
Changing tabs on Chrome	✔️
Changing to another window and returning to Chrome	✔️

gspencergoog

This seems good to me. Transitioning to resumed can occur on app startup too (resumed is just the "running while focused" state of an app), but I think that not selecting the whole text in that case is probably the right behavior anyhow.

justinmc

LGTM 👍

justinmc · 2024-10-21T17:07:51Z

packages/flutter/test/widgets/editable_text_test.dart

+
+      expect(focusNode1.hasFocus, true);
+      expect(controller1.selection, collapsedAtEnd('Flutter!').selection);
+    }, variant: TargetPlatformVariant.all());


Nit: Is TargetPlatformVariant.all() necessary? I think the behavior will be the same for all platform variants.

Good point, I think it is important to keep it, otherwise this test will not fail if the fix is removed (the 'select all' behavior is only desktop and web so the logic I added to fix this issue is not used on Android).

jacobsimionato · 2024-10-22T19:56:30Z

Reason for revert: Google3 roll failing - see b/375019489

…sumed (#156968)" This reverts commit 383a221.

…esumed (#156968)" (#157378) Reverts: #156968 Initiated by: jacobsimionato Reason for reverting: Google3 roll failing - see b/375019489 Original PR Author: bleroux Reviewed By: {justinmc, gspencergoog} This change reverts the following previous change: ## Description This PR makes `EditableText` aware of the lifecycle 'resumed' state to let the current selection unchanged when the application is resumed (on web and desktop, 'resumed' means the Flutter app window regained focus). Before this PR, on web and desktop, the whole content of a `TextField` was selected whenever a `TextField` gained focus. This is the correct behavior when tabbing between fields but it is not when a field regains focus after the application is resumed ## Related Issue Fixes [When switching to another browser tab or window and then going back, all text on TextField is selected automatically](#156078). ## Tests Adds 1 test.

jacobsimionato · 2024-10-22T20:07:40Z

I'm sorry for the revert here! So what happened is some Google integration tests failed at this line:

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

Line 253 in 4814978

    
           assert(previousState == null || previousState == AppLifecycleState.hidden, 'Invalid state transition from $previousState to $state');

'package:flutter/src/widgets/app_lifecycle_listener.dart': Failed assertion: line 253 pos 16: 'previousState == null || previousState == AppLifecycleState.hidden': Invalid state transition from AppLifecycleState.resumed to AppLifecycleState.paused

The test logic is doing something like:

// Enter some text in text fields, click some buttons

tester.binding.handleAppLifecycleStateChanged(AppLifecycleState.paused);

I imagine the test logic needs updating, but I'm not sure exactly what to do yet, hence the revert. Should I try to fix this by simulating a change to the 'hidden' state before 'paused' ?

tester.binding.handleAppLifecycleStateChanged(AppLifecycleState.hidden);

gspencergoog · 2024-10-22T20:52:28Z

@jacobsimionato See this diagram for the allowed state transitions (or this one)

I think the test is doing an invalid state transition, and one that the engine wouldn't send. The engine implementations are coded to synthesize any intermediate state transitions if the platform jumps from one state to another so that the state machine is followed, and an application can count on getting all the states in the expected order.

So, yes, you should probably add that transition, but if the test is expected to start in a particular state, then you should generate any intermediate states too, so if, for instance, the test starts at resumed, it should generate these states to get to paused:

tester.binding.handleAppLifecycleStateChanged(AppLifecycleState.inactive);
tester.binding.handleAppLifecycleStateChanged(AppLifecycleState.hidden);
tester.binding.handleAppLifecycleStateChanged(AppLifecycleState.paused);

jacobsimionato · 2024-10-23T04:07:18Z

I've now updated the internal tests to do the valid state transitions as you suggested.

@bleroux could you reland the PR? Sorry to make you do it twice!

bleroux · 2024-10-23T04:51:26Z

Could you reland the PR? Sorry to make you do it twice!

Hey @jacobsimionato 👋 !
No problem at all, especially because I just woke up (France time) and see that you already sorted everything out 😄🙏.

Here is the reland: #157399.
Can you approve it? That way I will be able to merge it as soon as the CI checks are ok.

…sumed" (#157399) ## Description Relands #156968 wich was reverted in #157378 This PR makes `EditableText` aware of the lifecycle 'resumed' state to let the current selection unchanged when the application is resumed (on web and desktop, 'resumed' means the Flutter app window regained focus). Before this PR, on web and desktop, the whole content of a `TextField` was selected whenever a `TextField` gained focus. This is the correct behavior when tabbing between fields but it is not when a field regains focus after the application is resumed ## Related Issue Fixes [When switching to another browser tab or window and then going back, all text on TextField is selected automatically](#156078). ## Tests Adds 1 test.

…esumed (flutter#156968)" (flutter#157378) Reverts: flutter#156968 Initiated by: jacobsimionato Reason for reverting: Google3 roll failing - see b/375019489 Original PR Author: bleroux Reviewed By: {justinmc, gspencergoog} This change reverts the following previous change: ## Description This PR makes `EditableText` aware of the lifecycle 'resumed' state to let the current selection unchanged when the application is resumed (on web and desktop, 'resumed' means the Flutter app window regained focus). Before this PR, on web and desktop, the whole content of a `TextField` was selected whenever a `TextField` gained focus. This is the correct behavior when tabbing between fields but it is not when a field regains focus after the application is resumed ## Related Issue Fixes [When switching to another browser tab or window and then going back, all text on TextField is selected automatically](flutter#156078). ## Tests Adds 1 test.

…sumed" (flutter#157399) ## Description Relands flutter#156968 wich was reverted in flutter#157378 This PR makes `EditableText` aware of the lifecycle 'resumed' state to let the current selection unchanged when the application is resumed (on web and desktop, 'resumed' means the Flutter app window regained focus). Before this PR, on web and desktop, the whole content of a `TextField` was selected whenever a `TextField` gained focus. This is the correct behavior when tabbing between fields but it is not when a field regains focus after the application is resumed ## Related Issue Fixes [When switching to another browser tab or window and then going back, all text on TextField is selected automatically](flutter#156078). ## Tests Adds 1 test.

…sumed (flutter/flutter#156968)

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. labels Oct 16, 2024

bleroux mentioned this pull request Oct 16, 2024

[TextField] When switching to another browser tab or window and then going back, all text on TextField is selected automatically #156078

Closed

bleroux requested a review from justinmc October 16, 2024 15:13

bleroux marked this pull request as draft October 16, 2024 15:14

justinmc reviewed Oct 16, 2024

View reviewed changes

bleroux added 2 commits October 17, 2024 09:57

Fix TextField selects all content after the application is resumed

be60de9

Add test

8c52729

bleroux force-pushed the fix_textfield_select_all_after_application_is_resumed branch from 13a94f1 to 8c52729 Compare October 17, 2024 07:58

bleroux marked this pull request as ready for review October 17, 2024 07:59

bleroux changed the title ~~[WIP] Fix TextField selects all content after the application is resumed~~ Fix TextField selects all content after the application is resumed Oct 17, 2024

bleroux requested review from gspencergoog and justinmc October 17, 2024 15:21

gspencergoog approved these changes Oct 21, 2024

View reviewed changes

justinmc approved these changes Oct 21, 2024

View reviewed changes

bleroux added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 22, 2024

auto-submit bot merged commit 383a221 into flutter:master Oct 22, 2024

bleroux deleted the fix_textfield_select_all_after_application_is_resumed branch October 22, 2024 04:58

jacobsimionato added the revert Autorevert PR (with "Reason for revert:" comment) label Oct 22, 2024

auto-submit bot pushed a commit that referenced this pull request Oct 22, 2024

Revert "Fix TextField selects all content after the application is re…

d912608

…sumed (#156968)" This reverts commit 383a221.

auto-submit bot mentioned this pull request Oct 22, 2024

Reverts "Fix TextField selects all content after the application is resumed (#156968)" #157378

Merged

auto-submit bot removed the revert Autorevert PR (with "Reason for revert:" comment) label Oct 22, 2024

bleroux mentioned this pull request Oct 23, 2024

Reland "Fix TextField selects all content after the application is resumed" #157399

Merged

Piinks mentioned this pull request Oct 25, 2024

Flutter Framework Team: Notable Commits #121415

Closed

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

383a221 Fix TextField selects all content after the application is re…

036f8b9

…sumed (flutter/flutter#156968)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025

383a221 Fix TextField selects all content after the application is re…

3887830

…sumed (flutter/flutter#156968)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025

383a221 Fix TextField selects all content after the application is re…

eb2a63d

…sumed (flutter/flutter#156968)

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

383a221 Fix TextField selects all content after the application is re…

55a162b

…sumed (flutter/flutter#156968)

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

383a221 Fix TextField selects all content after the application is re…

e4b4acb

…sumed (flutter/flutter#156968)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix TextField selects all content after the application is resumed #156968

Fix TextField selects all content after the application is resumed #156968

Uh oh!

bleroux commented Oct 16, 2024 •

edited

Loading

Uh oh!

flutter-dashboard bot commented Oct 16, 2024

Uh oh!

justinmc left a comment

Uh oh!

gspencergoog left a comment

Uh oh!

justinmc left a comment

Uh oh!

justinmc Oct 21, 2024

Uh oh!

bleroux Oct 22, 2024

Uh oh!

jacobsimionato commented Oct 22, 2024

Uh oh!

jacobsimionato commented Oct 22, 2024

Uh oh!

gspencergoog commented Oct 22, 2024 •

edited

Loading

Uh oh!

jacobsimionato commented Oct 23, 2024

Uh oh!

bleroux commented Oct 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Fix TextField selects all content after the application is resumed #156968

Fix TextField selects all content after the application is resumed #156968

Uh oh!

Conversation

bleroux commented Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Tests

Uh oh!

flutter-dashboard bot commented Oct 16, 2024

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

gspencergoog left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc Oct 21, 2024

Choose a reason for hiding this comment

Uh oh!

bleroux Oct 22, 2024

Choose a reason for hiding this comment

Uh oh!

jacobsimionato commented Oct 22, 2024

Uh oh!

jacobsimionato commented Oct 22, 2024

Uh oh!

gspencergoog commented Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jacobsimionato commented Oct 23, 2024

Uh oh!

bleroux commented Oct 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bleroux commented Oct 16, 2024 •

edited

Loading

gspencergoog commented Oct 22, 2024 •

edited

Loading