Skip to content
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

SelectionArea's selection should not be cleared on loss of window focus #148067

Merged
merged 8 commits into from May 20, 2024

Conversation

Renzo-Olivares
Copy link
Contributor

@Renzo-Olivares Renzo-Olivares commented May 9, 2024

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.

@github-actions 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
@@ -454,7 +454,11 @@ class SelectableRegionState extends State<SelectableRegion> with TextSelectionDe
if (kIsWeb) {
PlatformSelectableRegionContextMenu.detach(_selectionDelegate);
}
_clearSelection();
if (SchedulerBinding.instance.lifecycleState == AppLifecycleState.resumed) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this for someone minimize the windows and resume it?

Also does this work for embedded flutter view?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

_clearSelection();
if (SchedulerBinding.instance.lifecycleState == AppLifecycleState.resumed) {
// We should only clear the selection when this SelectableRegion loses
// focus while the application is currently running.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@gspencergoog gspencergoog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

32384589-a60f0e74-c078-11e7-9bc1-e5b5287aea9d

@Renzo-Olivares
Copy link
Contributor Author

@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
Copy link
Contributor

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
Copy link
Contributor Author

Renzo-Olivares commented May 10, 2024

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
Copy link
Contributor

gspencergoog commented May 10, 2024

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
Copy link
Contributor Author

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

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
Copy link
Contributor

gspencergoog commented May 10, 2024

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
Copy link
Contributor Author

Renzo-Olivares commented May 10, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

Thanks for the suggestion! Done.

Copy link
Contributor

@gspencergoog gspencergoog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still LGTM

/// a test environment where the [BuildOwner] which initializes its own
/// [FocusManager], may not have the accurate platform context during its
/// initialization. In this case it is necessary for the test framework to call
/// method after it has setup the test variant for a given test, so the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// [FocusManager], may not have the accurate platform context during its
/// initialization. In this case it is necessary for the test framework to call
/// method after it has setup the test variant for a given test, so the
/// [FocusManager] can accurately listen to application lifecycle changes if
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Renzo-Olivares 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 auto-submit bot merged commit 5890a2f into flutter:master May 20, 2024
74 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 21, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request May 21, 2024
flutter/flutter@02a6c91...d02292d

2024-05-21 ian@hixie.ch Make FileSystem dependency explicit througout (more). (flutter/flutter#148095)
2024-05-20 magder@google.com Remove add-to-app bitcode warning (flutter/flutter#148587)
2024-05-20 rmolivares@renzo-olivares.dev SelectionArea's selection should not be cleared on loss of window focus (flutter/flutter#148067)
2024-05-20 katelovett@google.com [wiki migration] Engine team pages (flutter/flutter#148696)
2024-05-20 goderbauer@google.com Manual roll camera dependency (flutter/flutter#148426)
2024-05-20 katelovett@google.com [wiki migration] Framework team pages (flutter/flutter#148721)
2024-05-20 engine-flutter-autoroll@skia.org 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 louisehsu@google.com Fixes incorrect read/write permissions on Flutter.framework and FlutterMacOS.framework (flutter/flutter#148580)
2024-05-20 engine-flutter-autoroll@skia.org Roll Flutter Engine from c6fecf65fbf3 to a8fb9daae8d0 (3 revisions) (flutter/flutter#148700)
2024-05-20 jason-simmons@users.noreply.github.com Remove the no-shuffle tag on the flutter_tools create_test suite (flutter/flutter#148688)
2024-05-20 andrewrkolos@gmail.com log incoming vm service messages in `FlutterVMService::runInView` (flutter/flutter#148596)
2024-05-20 sokolovskyi.konstantin@gmail.com Add tests for shared_app_data.#.dart API examples. (flutter/flutter#147830)
2024-05-20 sokolovskyi.konstantin@gmail.com Add tests for logical_key_set.0.dart API example. (flutter/flutter#147735)
2024-05-20 katelovett@google.com [wiki migration] Ecosystem team pages (flutter/flutter#148589)
2024-05-20 sokolovskyi.konstantin@gmail.com Fix painting API examples tests directories structure. (flutter/flutter#148177)
2024-05-20 102401667+Dimilkalathiya@users.noreply.github.com fixes `CupertinoModalPopupRoute` (flutter/flutter#147823)
2024-05-20 nate.w5687@gmail.com Implement new `AnimationStatus` getters (flutter/flutter#148570)
2024-05-20 nate.w5687@gmail.com Reland "`if` chains � `switch` expressions" (flutter/flutter#148634)
2024-05-20 gspencergoog@users.noreply.github.com Factor out `RawView`, make `View` listen to engine generated view focus events (flutter/flutter#143259)
2024-05-20 zanderso@users.noreply.github.com 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 rmistry@google.com,stuartmorgan@google.com,tarrinneal@google.com 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
hello-coder-xu added a commit to hello-coder-xu/flutter that referenced this pull request May 22, 2024
…-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 a8fb9daae8d0 to c2ef01f6f1ab (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 c6fecf65fbf3 to a8fb9daae8d0 (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)
  ...
TecHaxter pushed a commit to TecHaxter/flutter_packages that referenced this pull request May 22, 2024
…r#6778)

flutter/flutter@02a6c91...d02292d

2024-05-21 ian@hixie.ch Make FileSystem dependency explicit througout (more). (flutter/flutter#148095)
2024-05-20 magder@google.com Remove add-to-app bitcode warning (flutter/flutter#148587)
2024-05-20 rmolivares@renzo-olivares.dev SelectionArea's selection should not be cleared on loss of window focus (flutter/flutter#148067)
2024-05-20 katelovett@google.com [wiki migration] Engine team pages (flutter/flutter#148696)
2024-05-20 goderbauer@google.com Manual roll camera dependency (flutter/flutter#148426)
2024-05-20 katelovett@google.com [wiki migration] Framework team pages (flutter/flutter#148721)
2024-05-20 engine-flutter-autoroll@skia.org 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 louisehsu@google.com Fixes incorrect read/write permissions on Flutter.framework and FlutterMacOS.framework (flutter/flutter#148580)
2024-05-20 engine-flutter-autoroll@skia.org Roll Flutter Engine from c6fecf65fbf3 to a8fb9daae8d0 (3 revisions) (flutter/flutter#148700)
2024-05-20 jason-simmons@users.noreply.github.com Remove the no-shuffle tag on the flutter_tools create_test suite (flutter/flutter#148688)
2024-05-20 andrewrkolos@gmail.com log incoming vm service messages in `FlutterVMService::runInView` (flutter/flutter#148596)
2024-05-20 sokolovskyi.konstantin@gmail.com Add tests for shared_app_data.#.dart API examples. (flutter/flutter#147830)
2024-05-20 sokolovskyi.konstantin@gmail.com Add tests for logical_key_set.0.dart API example. (flutter/flutter#147735)
2024-05-20 katelovett@google.com [wiki migration] Ecosystem team pages (flutter/flutter#148589)
2024-05-20 sokolovskyi.konstantin@gmail.com Fix painting API examples tests directories structure. (flutter/flutter#148177)
2024-05-20 102401667+Dimilkalathiya@users.noreply.github.com fixes `CupertinoModalPopupRoute` (flutter/flutter#147823)
2024-05-20 nate.w5687@gmail.com Implement new `AnimationStatus` getters (flutter/flutter#148570)
2024-05-20 nate.w5687@gmail.com Reland "`if` chains � `switch` expressions" (flutter/flutter#148634)
2024-05-20 gspencergoog@users.noreply.github.com Factor out `RawView`, make `View` listen to engine generated view focus events (flutter/flutter#143259)
2024-05-20 zanderso@users.noreply.github.com 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 rmistry@google.com,stuartmorgan@google.com,tarrinneal@google.com 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: tests "flutter test", flutter_test, or one of our tests autosubmit Merge PR when tree becomes green via auto submit App f: focus Focus traversal, gaining or losing focus framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants