SelectionArea's selection should not be cleared on loss of window focus by Renzo-Olivares · Pull Request #148067 · flutter/flutter

Renzo-Olivares · 2024-05-09T16:40:47Z

This change fixes an issue where SelectionArea would clear its selection when the application window lost focus by first checking if the application is running. This is needed because FocusManager is aware of the application lifecycle as of #142930 , and triggers a focus lost if the application is not active.

Also fixes an issue where the FocusManager was not being reset on tests at the right time, causing it always to build with TargetPlatform.android as its context.

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.

chunhtai · 2024-05-09T17:16:06Z

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

is this for someone minimize the windows and resume it?

Also does this work for embedded flutter view?

Yeah this is for that case, or when someone clicks on a different applications window. I'm not too familiar with embedded flutter views and how they tie into the app lifecycle, but it seems at least for Android that a developer has to manually bridge the lifecycle events from their host application to the FlutterView https://docs.flutter.dev/add-to-app/android/add-flutter-view . I think if the lifecycle events are correctly bridged to their FlutterView that this should also work for them.

With regards to an embedded FlutterView in HTML, I used the sample https://github.com/flutter/samples/tree/main/web_embedding/ng-flutter to verify that the FlutterView was correctly receiving lifecycle events. Clicking onto an element in the host web page did not seem to trigger any changes to the app lifecycle. I only observed application lifecycle events triggered when clicking on a new tab, clicking on a different window, or different application outside the browser.

chunhtai

LGTM

chunhtai · 2024-05-09T20:14:06Z

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

maybe mention in which situation the lifecycle would not be resume.

gspencergoog

Renzo-Olivares · 2024-05-10T17:25:55Z

@chunhtai @gspencergoog I managed to track down the google testing issues. Some users modify the FocusManager.highlightStrategy in the setUp method of tests. This was easy enough to get around by saving the highlight strategy. However, the second issue is that some users add listeners to FocusManager in setUp. If we recreate an entirely new FocusManager at test time we lose all the listeners the user added. Not sure what the best way forward is here, this is my current attempt at fixing this 80ef35f but it exposes a public method on FocusManager.

Some other things I can think of is exposing some method in the TestWidgetsFlutterBinding that a user can call to rebuild FocusManager and that they should expect that any of their changes to FocusManager are not retained in that case. This means that anyone that wants an accurate FocusManager that listens to the application lifecycle would need to call this method in their test, which seems a little fragile.

gspencergoog · 2024-05-10T18:49:52Z

However, the second issue is that some users add listeners to FocusManager in setUp

In setUp, or in setUpAll? Because in setUp, they should expect that any state they setup there is destroyed once tearDown is called, and the next test should set it up again.

If they do it in setUpAll, that's more problematic, and the solution there is probably to just do it in setUp instead.

Renzo-Olivares · 2024-05-10T20:55:31Z

They add listeners in setUp, so I think we still need to retain the listeners across each individual test right? Right now there is an issue because we are setting a new FocusManager after setUp is called, which loses all the listeners that were set in setUp for a given test. setUp needs to happen first so FocusManager has the relevant platform context during construction.

gspencergoog · 2024-05-10T20:59:48Z

Ahh. So, could we instead initialize the FocusManager before the setUp, and then set the platform context after?

Since setUp is called before each test, I think the expectation would be that things don't revert after the setUp, but that reverting before the setup is fine (since it'll be reset to baseline).

Renzo-Olivares · 2024-05-10T22:08:59Z

I'm not sure I follow. If we initialize FocusManager before setUp we won't have the correct platform context during construction. The test variant is setup sometime after setUp is called, essentially at the same time the test body is run

flutter/packages/flutter_test/lib/src/widget_tester.dart

Lines 181 to 183 in a8a9b9b

    
           memento = await variant.setUp(value); 
        
           maybeSetupLeakTrackingForTest(experimentalLeakTesting, combinedDescription); 
        
           await callback(tester);

. We would ideally setup the test variant before setUp is called, so inside that method the user already has the correct defaultTargetPlatform for the given test (I think?), then we could initialize the FocusManager before setUp and it would have the correct platform context. That's a bit tricky to do though since the test is run asynchronously, I don't see a place where I could set up the test variant any earlier, unless we change the actual test_api to support running some callback before setUp. What do you think?

From what I understand this is the order of operations for a given test:
testWidgets -> test -> Declarer.test https://github.com/dart-lang/test/blob/84d2a2bcba0d63b9f4bd6cbf14fb7fc38621e65d/pkgs/test_api/lib/src/backend/declarer.dart#L171-L225 -> setUp (the setup method for a given test) -> test body (sets up the test variant and has the actual test logic for a given test).

gspencergoog · 2024-05-10T22:25:32Z

Actually, in effect, I guess I'm proposing the same thing that you are in the PR: your listenToApplicationLifecycleChangesIfSupported function is basically re-doing the FocusManager constructor bits that have to do with platform, which is what I was thinking about when I was saying to initialize it and then set the platform after.

I'm a little leery of adding that as a public API, though: I don't see any use for it outside of tests. Maybe mark it @visibleForTesting?

Renzo-Olivares · 2024-05-10T22:37:34Z

Oh okay I understand.

I agree, I am also not so sure about having it as a public API. The analyzer complains about visibleForTesting because it's not technically being used in a test, even though it's in flutter_test.

gspencergoog · 2024-05-10T23:52:09Z

Yeah, we should really fix the analyzer test to allow @visibleForTesting in the flutter_test repo. In any case, you can just add an exemption for it there.

Renzo-Olivares · 2024-05-11T00:23:46Z

Thanks for the suggestion! Done.

gspencergoog

Still LGTM

gspencergoog · 2024-05-20T22:23:23Z

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

Suggested change

/// method after it has setup the test variant for a given test, so the

/// this method after it has set up the test variant for a given test, so the

gspencergoog · 2024-05-20T22:23:50Z

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

Suggested change

/// [FocusManager] can accurately listen to application lifecycle changes if

/// [FocusManager] can accurately listen to application lifecycle changes, if

…hould also reset focusManager

…ould save it so we can restore it when we rebuild FocusManager to consider the current test variant

…isibleForTesting and exempt its usage in flutter_test

…ndow focus (flutter/flutter#148067)

flutter/flutter@02a6c91...d02292d 2024-05-21 [email protected] Make FileSystem dependency explicit througout (more). (flutter/flutter#148095) 2024-05-20 [email protected] Remove add-to-app bitcode warning (flutter/flutter#148587) 2024-05-20 [email protected] SelectionArea's selection should not be cleared on loss of window focus (flutter/flutter#148067) 2024-05-20 [email protected] [wiki migration] Engine team pages (flutter/flutter#148696) 2024-05-20 [email protected] Manual roll camera dependency (flutter/flutter#148426) 2024-05-20 [email protected] [wiki migration] Framework team pages (flutter/flutter#148721) 2024-05-20 [email protected] Roll Flutter Engine from a8fb9daae8d0 to c2ef01f6f1ab (3 revisions) (flutter/flutter#148722) 2024-05-20 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.5 to 3.25.6 (flutter/flutter#148715) 2024-05-20 49699333+dependabot[bot]@users.noreply.github.com Bump codecov/codecov-action from 4.4.0 to 4.4.1 (flutter/flutter#148714) 2024-05-20 [email protected] Fixes incorrect read/write permissions on Flutter.framework and FlutterMacOS.framework (flutter/flutter#148580) 2024-05-20 [email protected] Roll Flutter Engine from c6fecf65fbf3 to a8fb9daae8d0 (3 revisions) (flutter/flutter#148700) 2024-05-20 [email protected] Remove the no-shuffle tag on the flutter_tools create_test suite (flutter/flutter#148688) 2024-05-20 [email protected] log incoming vm service messages in `FlutterVMService::runInView` (flutter/flutter#148596) 2024-05-20 [email protected] Add tests for shared_app_data.#.dart API examples. (flutter/flutter#147830) 2024-05-20 [email protected] Add tests for logical_key_set.0.dart API example. (flutter/flutter#147735) 2024-05-20 [email protected] [wiki migration] Ecosystem team pages (flutter/flutter#148589) 2024-05-20 [email protected] Fix painting API examples tests directories structure. (flutter/flutter#148177) 2024-05-20 [email protected] fixes `CupertinoModalPopupRoute` (flutter/flutter#147823) 2024-05-20 [email protected] Implement new `AnimationStatus` getters (flutter/flutter#148570) 2024-05-20 [email protected] Reland "`if` chains â�� `switch` expressions" (flutter/flutter#148634) 2024-05-20 [email protected] Factor out `RawView`, make `View` listen to engine generated view focus events (flutter/flutter#143259) 2024-05-20 [email protected] Remove all tests from a02s. Replace with mokey in bringup (flutter/flutter#148563) 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],[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

…-coder-xu/flutter into fix/slider_text_null_error * 'fix/slider_text_null_error' of https://github.com/hello-coder-xu/flutter: (65 commits) Change implementation method fix slider text null error Make FileSystem dependency explicit througout (more). (flutter#148095) Remove add-to-app bitcode warning (flutter#148587) SelectionArea's selection should not be cleared on loss of window focus (flutter#148067) [wiki migration] Engine team pages (flutter#148696) Manual roll camera dependency (flutter#148426) [wiki migration] Framework team pages (flutter#148721) Roll Flutter Engine from a8fb9da to c2ef01f (3 revisions) (flutter#148722) Bump github/codeql-action from 3.25.5 to 3.25.6 (flutter#148715) Bump codecov/codecov-action from 4.4.0 to 4.4.1 (flutter#148714) Fixes incorrect read/write permissions on Flutter.framework and FlutterMacOS.framework (flutter#148580) Roll Flutter Engine from c6fecf6 to a8fb9da (3 revisions) (flutter#148700) Remove the no-shuffle tag on the flutter_tools create_test suite (flutter#148688) log incoming vm service messages in `FlutterVMService::runInView` (flutter#148596) Add tests for shared_app_data.#.dart API examples. (flutter#147830) Add tests for logical_key_set.0.dart API example. (flutter#147735) [wiki migration] Ecosystem team pages (flutter#148589) Fix painting API examples tests directories structure. (flutter#148177) fixes `CupertinoModalPopupRoute` (flutter#147823) ...

flutter/flutter@02a6c91...d02292d 2024-05-21 [email protected] Make FileSystem dependency explicit througout (more). (flutter/flutter#148095) 2024-05-20 [email protected] Remove add-to-app bitcode warning (flutter/flutter#148587) 2024-05-20 [email protected] SelectionArea's selection should not be cleared on loss of window focus (flutter/flutter#148067) 2024-05-20 [email protected] [wiki migration] Engine team pages (flutter/flutter#148696) 2024-05-20 [email protected] Manual roll camera dependency (flutter/flutter#148426) 2024-05-20 [email protected] [wiki migration] Framework team pages (flutter/flutter#148721) 2024-05-20 [email protected] Roll Flutter Engine from a8fb9daae8d0 to c2ef01f6f1ab (3 revisions) (flutter/flutter#148722) 2024-05-20 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.5 to 3.25.6 (flutter/flutter#148715) 2024-05-20 49699333+dependabot[bot]@users.noreply.github.com Bump codecov/codecov-action from 4.4.0 to 4.4.1 (flutter/flutter#148714) 2024-05-20 [email protected] Fixes incorrect read/write permissions on Flutter.framework and FlutterMacOS.framework (flutter/flutter#148580) 2024-05-20 [email protected] Roll Flutter Engine from c6fecf65fbf3 to a8fb9daae8d0 (3 revisions) (flutter/flutter#148700) 2024-05-20 [email protected] Remove the no-shuffle tag on the flutter_tools create_test suite (flutter/flutter#148688) 2024-05-20 [email protected] log incoming vm service messages in `FlutterVMService::runInView` (flutter/flutter#148596) 2024-05-20 [email protected] Add tests for shared_app_data.#.dart API examples. (flutter/flutter#147830) 2024-05-20 [email protected] Add tests for logical_key_set.0.dart API example. (flutter/flutter#147735) 2024-05-20 [email protected] [wiki migration] Ecosystem team pages (flutter/flutter#148589) 2024-05-20 [email protected] Fix painting API examples tests directories structure. (flutter/flutter#148177) 2024-05-20 [email protected] fixes `CupertinoModalPopupRoute` (flutter/flutter#147823) 2024-05-20 [email protected] Implement new `AnimationStatus` getters (flutter/flutter#148570) 2024-05-20 [email protected] Reland "`if` chains â�� `switch` expressions" (flutter/flutter#148634) 2024-05-20 [email protected] Factor out `RawView`, make `View` listen to engine generated view focus events (flutter/flutter#143259) 2024-05-20 [email protected] Remove all tests from a02s. Replace with mokey in bringup (flutter/flutter#148563) 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],[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

…us (flutter#148067) This change fixes an issue where SelectionArea would clear its selection when the application window lost focus by first checking if the application is running. This is needed because `FocusManager` is aware of the application lifecycle as of flutter#142930 , and triggers a focus lost if the application is not active. Also fixes an issue where the `FocusManager` was not being reset on tests at the right time, causing it always to build with `TargetPlatform.android` as its context.

…ndow focus (flutter/flutter#148067)

github-actions bot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. f: focus Focus traversal, gaining or losing focus labels May 9, 2024

Renzo-Olivares force-pushed the sa-applifecycle branch from 78ecd92 to 6277b9c Compare May 9, 2024 16:41

Renzo-Olivares requested review from chunhtai and gspencergoog May 9, 2024 16:48

chunhtai reviewed May 9, 2024

View reviewed changes

chunhtai approved these changes May 9, 2024

View reviewed changes

gspencergoog approved these changes May 10, 2024

View reviewed changes

Renzo-Olivares requested review from chunhtai and gspencergoog May 10, 2024 17:26

Renzo-Olivares force-pushed the sa-applifecycle branch from 80ef35f to 738090d Compare May 10, 2024 17:51

Renzo-Olivares force-pushed the sa-applifecycle branch from 6aed873 to d2ea333 Compare May 11, 2024 00:21

Renzo-Olivares force-pushed the sa-applifecycle branch from d2ea333 to fb56a3c Compare May 14, 2024 18:11

Renzo-Olivares requested review from gspencergoog and removed request for gspencergoog May 20, 2024 19:56

gspencergoog approved these changes May 20, 2024

View reviewed changes

Renzo-Olivares added 4 commits May 20, 2024 15:54

fix clear selection when window focus lost

16f2d65

Add test and fix fous_manager_test, TestWidgetsFlutterBinding.reset s…

41c6845

…hould also reset focusManager

updates

8fd6db3

highlight strategy for focus manager may be set during setUp so we sh…

663fa57

…ould save it so we can restore it when we rebuild FocusManager to consider the current test variant

Renzo-Olivares added 4 commits May 20, 2024 15:54

address reviewer comments

943013d

Second attempt at fixing google testing

0d653ea

mark FocusManager.listenToApplicationLifecycleChangesIfSupported as v…

07a86de

…isibleForTesting and exempt its usage in flutter_test

Address comments

e11be71

Renzo-Olivares force-pushed the sa-applifecycle branch from bf1725b to e11be71 Compare May 20, 2024 22:54

Renzo-Olivares added autosubmit Merge PR when tree becomes green via auto submit App and removed autosubmit Merge PR when tree becomes green via auto submit App labels May 20, 2024

auto-submit bot merged commit 5890a2f into flutter:master May 20, 2024

engine-flutter-autoroll mentioned this pull request May 21, 2024

Roll Flutter from 02a6c91e4d37 to d02292dbc409 (22 revisions) flutter/packages#6778

Merged

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

5890a2f SelectionArea's selection should not be cleared on loss of wi…

f87d98c

…ndow focus (flutter/flutter#148067)

Piinks mentioned this pull request May 31, 2024

Flutter Framework Team: Notable Commits #121415

Closed

This was referenced Jul 1, 2024

[CP-stable] Update FocusManager platform check to include iOS #151144

Closed

Reland "Update FocusManager platform check to include iOS" #148984

Merged

Renzo-Olivares added the cp: stable cherry pick this pull request to stable release candidate branch label Jul 11, 2024

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

5890a2f SelectionArea's selection should not be cleared on loss of wi…

18c85d9

…ndow focus (flutter/flutter#148067)

	/// method after it has setup the test variant for a given test, so the
	/// this method after it has set up the test variant for a given test, so the

	/// [FocusManager] can accurately listen to application lifecycle changes if
	/// [FocusManager] can accurately listen to application lifecycle changes, if

Conversation

Renzo-Olivares commented May 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

chunhtai May 9, 2024

Choose a reason for hiding this comment

Uh oh!

Renzo-Olivares May 9, 2024

Choose a reason for hiding this comment

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

chunhtai May 9, 2024

Choose a reason for hiding this comment

Uh oh!

gspencergoog left a comment

Choose a reason for hiding this comment

Uh oh!

Renzo-Olivares commented May 10, 2024

Uh oh!

gspencergoog commented May 10, 2024

Uh oh!

Renzo-Olivares commented May 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Renzo-Olivares commented May 10, 2024

Uh oh!

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

Uh oh!

Uh oh!

Renzo-Olivares commented May 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gspencergoog commented May 10, 2024

Uh oh!

Renzo-Olivares commented May 11, 2024

Uh oh!

gspencergoog left a comment

Choose a reason for hiding this comment

Uh oh!

gspencergoog May 20, 2024

Choose a reason for hiding this comment

Uh oh!

gspencergoog May 20, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Renzo-Olivares commented May 9, 2024 •

edited

Loading

Renzo-Olivares commented May 10, 2024 •

edited

Loading

gspencergoog commented May 10, 2024 •

edited

Loading

gspencergoog commented May 10, 2024 •

edited

Loading

Renzo-Olivares commented May 10, 2024 •

edited

Loading