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

[Android] Save back handling state in Activity/Fragment bundle #56715

Merged
merged 12 commits into from
Dec 4, 2024

Conversation

gmackall
Copy link
Member

@gmackall gmackall commented Nov 19, 2024

Fixes flutter/flutter#159158, and fixes b/355556397

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 and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@gmackall gmackall changed the title [Android][wip] Save back handling state in Activity/Fragment bundle [Android] Save back handling state in Activity/Fragment bundle Nov 20, 2024
@gmackall gmackall marked this pull request as ready for review November 20, 2024 18:00
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

LGTM from my side 👍 . This would have taken me quite a long time to figure out on my own so thank you!

So this is unrelated to predictive back being unsupported on FlutterFragmentActivity?

@gmackall
Copy link
Member Author

LGTM from my side 👍 . This would have taken me quite a long time to figure out on my own so thank you!

So this is unrelated to predictive back being unsupported on FlutterFragmentActivity?

Yes, that is correct. Though from what I can tell, predictive back when leaving the app seems to work with a FlutterFragmentActivity? I plan to do more testing, but I encountered the same behavior you described here when I initially tried it
flutter/flutter#149753 (comment)

Separately, I noticed that #52302 seems to have unintentionally had the effect of not allowing people to "opt out" of predictive back. This is actually aligned with what the android docs say should happen:
https://developer.android.com/guide/navigation/custom-back/predictive-back-gesture

Note: OnBackPressedCallback is always called regardless of the value of android:enableOnBackInvokedCallback. In other words, disabling the system animation doesn't affect your app's back handling logic if it uses OnBackPressedCallback.

But this wasn't actually true for flutter apps before #52302, because we were not calling super, and FlutterFragmentActivity extends a FragmentActivity which in turn extends a ComponentActivity, which uses the old onBackPressed to invoke the new back handling:

   override fun onBackPressed() {
        onBackPressedDispatcher.onBackPressed()
    }

So while the docs imply that removing the onBackPressed in FlutterFragmentActivity shouldn't have had an effect, that wasn't true because in our case we were consuming the back event and ignoring the warning

  @Override
  @SuppressWarnings("MissingSuperCall")
  public void onBackPressed() {
    flutterFragment.onBackPressed();
  }

What all this means is that apps that aren't opting in to predictive back had their back handling migrated to the new code path automatically, which is part of what happened in b/355556397: the bug this pr fixes affects all of FFA/FA/FF, but they noticed a behavior difference between FlutterActivity and FlutterFragmentActivity because of

  1. the bug this pr fixes
  2. the fact that FFA uniquely is forced into the new back handling codepath (they would see the same behavior if using FlutterActivity and opting in with android:enableOnBackInvokedCallback=true)

@gmackall
Copy link
Member Author

I also think we may want to restore the ability for people to opt out, so that FlutterActivity and FlutterFragmentActivity have the same behavior wrt the android:enableOnBackInvokedCallback (despite this sort of running counter to what the android docs suggest)
#56565 (still need to find out the best way to write a test that interacts with the xml attribute)
Does that make sense to you?

@reidbaker
Copy link
Contributor

I also think we may want to restore the ability for people to opt out, so that FlutterActivity and FlutterFragmentActivity have the same behavior wrt the android:enableOnBackInvokedCallback (despite this sort of running counter to what the android docs suggest) #56565 (still need to find out the best way to write a test that interacts with the xml attribute) Does that make sense to you?

For now I think we should make decisions as if we will be supporting predictive back and legacy back forever.

@justinmc
Copy link
Contributor

justinmc commented Dec 2, 2024

Thanks for the detailed explanation on what's going on here. I'm on board with allowing opt-out and supporting both predictive and non-predictive back.

@gmackall gmackall requested a review from reidbaker December 3, 2024 18:09
@@ -1071,6 +1072,12 @@ public void onAttach(@NonNull Context context) {
@Override
public void onCreate(@Nullable Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
if (savedInstanceState != null) {
boolean frameworkHandlesBack =
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to change the pr now but when we have a second variable then all of this code should go into a "save everything" and "restore everything" methods.

@gmackall gmackall added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 4, 2024
@auto-submit auto-submit bot merged commit 1e6864c into flutter:main Dec 4, 2024
32 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 4, 2024
github-merge-queue bot pushed a commit to flutter/flutter that referenced this pull request Dec 4, 2024
…159776)

flutter/engine@938f816...1e6864c

2024-12-04 34871572+gmackall@users.noreply.github.com [Android] Save
back handling state in Activity/Fragment bundle (flutter/engine#56715)
2024-12-03 robert.ancell@canonical.com Split keyevent channel into own
class (flutter/engine#56911)
2024-12-03 robert.ancell@canonical.com Add tests for errors encoding
message channel request and method calls. (flutter/engine#56914)
2024-12-03 skia-flutter-autoroll@skia.org Roll Skia from 8dc8bdc364f5 to
e02d856f86fb (3 revisions) (flutter/engine#56926)
2024-12-03 jonahwilliams@google.com [Impeller] invalidate cached atlas
data, take 2. (flutter/engine#56925)
2024-12-03 jason-simmons@users.noreply.github.com Add typeface_proxy
dependency to the Skia build script for the Android font manager
(flutter/engine#56924)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC bdero@google.com,zra@google.com on the revert to ensure that a
human
is aware of the problem.

To file a bug in Flutter:
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
nick9822 pushed a commit to nick9822/flutter that referenced this pull request Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Back handling state is lost on activity recreation when using a cached engine
3 participants