FlutterFragment predictive back #52302
Conversation
| // can be changed by calling setFrameworkHandlesBack. For example, the | ||
| // framework will call this automatically in a typical app when it has | ||
| // routes to pop. | ||
| onBackPressedCallback.setEnabled(false); |
There was a problem hiding this comment.
You probably also need to update the implementation of popSystemNavigator() to restore the previous state of the callback-enablement, instead of unconditionally re-enabling it on line 1673.
There was a problem hiding this comment.
@math1man Thanks for jumping back in here, I've been trying to catch back up on your comments from the last PR. Pushing a fix for that now.
There was a problem hiding this comment.
No worries. Honestly, I think besides this comment everything looks good. Happy to look into anything specific you might be concerned about though.
|
Could @math1man or @reidbaker point me in the right direction for how to test this? Sorry I'm pretty inexperienced here, I'm struggling to even get into a real workflow. It seems like I need to recompile every time I touch the engine code (but not the test code). I'm doing: But for one, I don't know how to view the output of Log statements I put in the test or in the engine code. They never show up in the test output. I added some code in FlutterFragmentTest to verify popSystemNavigator is enabling/disabling the callback, but it doesn't seem to fail if I revert the PR's changes, and I'm struggling to understand why not without the benefit of logs. |
|
Honestly @matanlurey is who I go to for engine help but tomorrow I can take a look. |
|
@camsim99 helped me out on Discord, the answer is |
|
Sometimes |
|
Ah thanks, I was using |
|
@reidbaker This is ready for a final check when you have a chance. |
|
Thank you I will get this on my list |
|
Thanks! No rush from me. |
| // By default, Android handles backs, and predictive back is enabled. This | ||
| // can be changed by calling setFrameworkHandlesBack. For example, the | ||
| // framework will call this automatically in a typical app when it has | ||
| // routes to pop. |
There was a problem hiding this comment.
I am still confused by this code. It looks like we are on a true branch of ARG_SHOULD_AUTOMATICALLY_HANDLE_ON_BACK_PRESSED.
Is this just saying that onBackPressedCallback.setEnabled should be the opposite of the argument passed to ARG_SHOULD_AUTOMATICALLY_HANDLE_ON_BACK_PRESSED?
There was a problem hiding this comment.
I'm going to push a commit to reword this comment to try to make it more clear.
ARG_SHOULD_AUTOMATICALLY_HANDLE_ON_BACK_PRESSED means that predictive back is enabled. onBackPressedCallback enabled means that Flutter is handling back gestures (typically because Flutter has Flutter routes to on its navigation stack). Disabled means that Android is handling back gestures, so it will either pop the Activity or go back to the phone's home screen.
So when the app starts, by default we let Android handle backs. If/when the Flutter app wants to change this, it calls setFrameworkHandlesBack and enables this callback.
| @@ -318,8 +320,15 @@ public void itDelegatesOnBackPressedAutomaticallyWhenEnabled() { | |||
| TestDelegateFactory delegateFactory = new TestDelegateFactory(mockDelegate); | |||
| fragment.setDelegateFactory(delegateFactory); | |||
|
|
|||
| // Calling onBackPressed now will still be handled by Android (the default), | |||
There was a problem hiding this comment.
reading this test shouldn't the back pressed be handled by the framework because of the code on 306?
There was a problem hiding this comment.
My comment above should help explain this a bit. shouldAutomaticallyHandleOnBackPressed enables predictive back, while setFrameworkHandlesBack determines whether Android or the framework handle the back gesture.
This reverts commit c364b29.
…sions) (#149220) Manual roll requested by jacksongardner@google.com flutter/engine@d032390...b26e1b0 2024-05-28 34871572+gmackall@users.noreply.github.com Manual revert of #53001 (flutter/engine#53075) 2024-05-28 chinmaygarde@google.com Remove --ios-cpu flag. Only the arm64 variant is supported. (flutter/engine#53044) 2024-05-28 skia-flutter-autoroll@skia.org Roll Skia from 229d94a8807e to ac454b80130c (1 revision) (flutter/engine#53074) 2024-05-28 jonahwilliams@google.com [Impeller] make strokes slightly lighter. (flutter/engine#53067) 2024-05-28 skia-flutter-autoroll@skia.org Roll Skia from 23ddbb590e44 to 229d94a8807e (2 revisions) (flutter/engine#53071) 2024-05-28 jmccandless@google.com FlutterFragment predictive back (flutter/engine#52302) 2024-05-28 skia-flutter-autoroll@skia.org Roll Skia from 02c359cf8233 to 23ddbb590e44 (2 revisions) (flutter/engine#53070) 2024-05-28 skia-flutter-autoroll@skia.org Roll Skia from 4f91b3865441 to 02c359cf8233 (1 revision) (flutter/engine#53069) 2024-05-28 30870216+gaaclarke@users.noreply.github.com [Impeller] shrunk the buffer for the rrect_blur (flutter/engine#53068) 2024-05-28 skia-flutter-autoroll@skia.org Roll Skia from 91cd2b48377a to 4f91b3865441 (4 revisions) (flutter/engine#53066) 2024-05-28 34871572+gmackall@users.noreply.github.com Upgrade all[most] androidx dependencies to latest (flutter/engine#53001) 2024-05-28 skia-flutter-autoroll@skia.org Roll Skia from 0c2c490021b7 to 91cd2b48377a (3 revisions) (flutter/engine#53065) 2024-05-28 skia-flutter-autoroll@skia.org Roll Skia from 545203f95d4e to 0c2c490021b7 (2 revisions) (flutter/engine#53063) 2024-05-28 skia-flutter-autoroll@skia.org Roll Skia from 74b4d97be6ab to 545203f95d4e (1 revision) (flutter/engine#53062) 2024-05-28 skia-flutter-autoroll@skia.org Roll Skia from 848d9498fd68 to 74b4d97be6ab (1 revision) (flutter/engine#53061) 2024-05-28 johnniwinther@google.com Remove use of --nnbd-agnostic (flutter/engine#53055) 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 jacksongardner@google.com,rmistry@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
…sions) (flutter#149220) Manual roll requested by jacksongardner@google.com flutter/engine@d032390...b26e1b0 2024-05-28 34871572+gmackall@users.noreply.github.com Manual revert of flutter#53001 (flutter/engine#53075) 2024-05-28 chinmaygarde@google.com Remove --ios-cpu flag. Only the arm64 variant is supported. (flutter/engine#53044) 2024-05-28 skia-flutter-autoroll@skia.org Roll Skia from 229d94a8807e to ac454b80130c (1 revision) (flutter/engine#53074) 2024-05-28 jonahwilliams@google.com [Impeller] make strokes slightly lighter. (flutter/engine#53067) 2024-05-28 skia-flutter-autoroll@skia.org Roll Skia from 23ddbb590e44 to 229d94a8807e (2 revisions) (flutter/engine#53071) 2024-05-28 jmccandless@google.com FlutterFragment predictive back (flutter/engine#52302) 2024-05-28 skia-flutter-autoroll@skia.org Roll Skia from 02c359cf8233 to 23ddbb590e44 (2 revisions) (flutter/engine#53070) 2024-05-28 skia-flutter-autoroll@skia.org Roll Skia from 4f91b3865441 to 02c359cf8233 (1 revision) (flutter/engine#53069) 2024-05-28 30870216+gaaclarke@users.noreply.github.com [Impeller] shrunk the buffer for the rrect_blur (flutter/engine#53068) 2024-05-28 skia-flutter-autoroll@skia.org Roll Skia from 91cd2b48377a to 4f91b3865441 (4 revisions) (flutter/engine#53066) 2024-05-28 34871572+gmackall@users.noreply.github.com Upgrade all[most] androidx dependencies to latest (flutter/engine#53001) 2024-05-28 skia-flutter-autoroll@skia.org Roll Skia from 0c2c490021b7 to 91cd2b48377a (3 revisions) (flutter/engine#53065) 2024-05-28 skia-flutter-autoroll@skia.org Roll Skia from 545203f95d4e to 0c2c490021b7 (2 revisions) (flutter/engine#53063) 2024-05-28 skia-flutter-autoroll@skia.org Roll Skia from 74b4d97be6ab to 545203f95d4e (1 revision) (flutter/engine#53062) 2024-05-28 skia-flutter-autoroll@skia.org Roll Skia from 848d9498fd68 to 74b4d97be6ab (1 revision) (flutter/engine#53061) 2024-05-28 johnniwinther@google.com Remove use of --nnbd-agnostic (flutter/engine#53055) 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 jacksongardner@google.com,rmistry@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
Remake of flutter/engine#56565. Quoting from an old comment, slightly edited: ------------ flutter/engine#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 flutter/engine#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](https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:activity/activity/src/main/java/androidx/activity/ComponentActivity.kt;l=587?q=ComponentActivity): ```kotlin 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 ```java @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. FlutterFragmentActivity was uniquely is forced into the new back handling codepath by flutter/engine#52302, which this PR fixes. ------------ ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: Gray Mackall <mackall@google.com>
Following up on #39208 and #42789, which added predictive back support for FlutterActivity.
My test app for this is: https://github.com/justinmc/flutter-add-to-app
I tried this both with the Flutter app as the main activity, and with it as a secondary activity after an initial Android route. Predictive back worked in both cases. However, this PR does not include support for in-app route transitions in Flutter.
Related: flutter/flutter#109558, and my comment flutter/flutter#109558 (comment).
b/242216228