Skip to content

Automatically handle back gesture in nested Navigators - #152330

Closed
justinmc wants to merge 56 commits into
flutter:masterfrom
justinmc:automatic-pop-scope
Closed

justinmc wants to merge 56 commits into
flutter:masterfrom
justinmc:automatic-pop-scope

Conversation

@justinmc

@justinmc justinmc commented Jul 25, 2024 •

Copy link
Copy Markdown
Contributor

There is a known problem with using nested Navigator widgets in Flutter. System back events will still go to the root Navigator instead of the nested one. The solution has been to use WillPopScope (and later PopScope/NavigatorPopHandler) to catch the system back and handle it manually. See the docs on NavigatorPopHandler for more on this problem and how we recommend solving it.

This PR is exploring the idea: why doesn't Flutter handle this complexity automatically for users? The navigation scenarios may be varied enough that Flutter can't solve this automatically without breaking some of them, but I hope to find out if that's true.

Fixes #145290
Fixes #153672

Discovered while working on this: #152578

Multiple nested Navigators

I thought this would require more work, but it seems maybePop correctly doesn't pop the inactive Navigator.

Multiple nested Navigators example
import 'package:flutter/material.dart';

void main() async {
  runApp(const _MyApp());
}

class _MyApp extends StatelessWidget {
  const _MyApp();

  // This widget is the root of your application.
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      theme: ThemeData(
        useMaterial3: false,
        /*
        pageTransitionsTheme: const PageTransitionsTheme(
          builders: <TargetPlatform, PageTransitionsBuilder>{
            TargetPlatform.android: PredictiveBackPageTransitionsBuilder(),
          },
        ),
        */
      ),
      title: 'Flutter Demo',
      routes: {
        '/': (BuildContext context) {
          return _MyNestedNavigator();
        },
        'nav1leaf': (BuildContext context) {
          return const Nav1LeafPage();
        },
      },
    );
  }
}

class _MyNestedNavigator extends StatelessWidget {
  _MyNestedNavigator();

  final GlobalKey<NavigatorState> _nestedNavigatorKey = GlobalKey<NavigatorState>();

  @override
  Widget build(BuildContext context) {
    return Navigator(
      key: _nestedNavigatorKey,
      initialRoute: 'nav2home',
      onGenerateRoute: (RouteSettings settings) {
        return switch (settings.name) {
          'nav2home' => MaterialPageRoute(
            builder: (BuildContext context) => const MyHomePage(),
          ),
          'nav2leaf' => MaterialPageRoute(
            builder: (BuildContext context) => const Nav2LeafPage(),
          ),
          'nav3' => MaterialPageRoute(
            builder: (BuildContext context) => _MyNestedNestedNavigator(),
          ),
          _ => MaterialPageRoute(
            builder: (BuildContext context) => const Text('404'),
          ),
        };
      },
    );
  }
}

class MyHomePage extends StatelessWidget {
  const MyHomePage({super.key});

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      backgroundColor: Colors.blueAccent,
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.center,
          children: <Widget>[
            const Text('Home page of Navigator 2'),
            const SizedBox(height: 16, width: double.infinity),
            ElevatedButton(
              child: const Text('Open nav 2 leaf'),
              onPressed: () => Navigator.of(context).pushNamed('nav2leaf'),
            ),
          ],
        ),
      ),
    );
  }
}

class Nav2LeafPage extends StatelessWidget {
  const Nav2LeafPage({super.key});

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      backgroundColor: Colors.blue,
      body: Column(
        mainAxisAlignment: MainAxisAlignment.center,
        crossAxisAlignment: CrossAxisAlignment.center,
        children: [
          const Text('Leaf page of Navigator 2'),
          const SizedBox(height: 16, width: double.infinity),
          ElevatedButton(
            child: const Text('Open nav 3 home'),
            onPressed: () => Navigator.of(context).pushNamed('nav3'),
          ),
          const SizedBox(height: 16, width: double.infinity),
          ElevatedButton(
            child: const Text('Open nav 1 leaf'),
            onPressed: () => Navigator.of(context, rootNavigator: true).pushNamed('nav1leaf'),
          ),
        ],
      ),
    );
  }
}

class Nav1LeafPage extends StatelessWidget {
  const Nav1LeafPage({super.key});

  @override
  Widget build(BuildContext context) {
    return const Scaffold(
      body: Column(
        mainAxisAlignment: MainAxisAlignment.center,
        crossAxisAlignment: CrossAxisAlignment.center,
        children: [
          Text('Leaf of Navigator 1'),
        ],
      ),
    );
  }
}

class _MyNestedNestedNavigator extends StatelessWidget {
  _MyNestedNestedNavigator();

  final GlobalKey<NavigatorState> _nestedNavigatorKey = GlobalKey<NavigatorState>();

  @override
  Widget build(BuildContext context) {
    return Navigator(
      key: _nestedNavigatorKey,
      initialRoute: 'nav3home',
      onGenerateRoute: (RouteSettings settings) {
        return switch (settings.name) {
          'nav3home' => MaterialPageRoute(
            builder: (BuildContext context) => const NestedNestedNavigatorHomePage(),
          ),
          'nav3leaf' => MaterialPageRoute(
            builder: (BuildContext context) => const Nav3LeafPage(),
          ),
          _ => MaterialPageRoute(
            builder: (BuildContext context) => const Text('404'),
          ),
        };
      },
    );
  }
}

class NestedNestedNavigatorHomePage extends StatelessWidget {
  const NestedNestedNavigatorHomePage({super.key});

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      backgroundColor: Colors.amber,
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.center,
          children: <Widget>[
            const Text('Home page of Navigator 3'),
            const SizedBox(height: 16, width: double.infinity),
            ElevatedButton(
              child: const Text('Open nav 3 leaf'),
              onPressed: () => Navigator.of(context).pushNamed('nav3leaf'),
            ),
          ],
        ),
      ),
    );
  }
}

class Nav3LeafPage extends StatelessWidget {
  const Nav3LeafPage({super.key});

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      backgroundColor: Colors.amberAccent,
      body: Column(
        mainAxisAlignment: MainAxisAlignment.center,
        crossAxisAlignment: CrossAxisAlignment.center,
        children: [
          const Text('Leaf page of Navigator 3'),
          const SizedBox(height: 16, width: double.infinity),
          ElevatedButton(
            child: const Text('Open nav 1 leaf'),
            onPressed: () => Navigator.of(context, rootNavigator: true).pushNamed('nav1leaf'),
          ),
        ],
      ),
    );
  }
}

TODO

  • More tests, if it looks like this PR is viable.

@justinmc justinmc self-assigned this Jul 25, 2024
@flutter-dashboard

Copy link
Copy Markdown

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

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.

@justinmc
justinmc marked this pull request as draft July 25, 2024 17:41
@github-actions github-actions Bot added framework flutter/packages/flutter repository. See also f: labels. f: routes Navigator, Router, and related APIs. labels Jul 25, 2024
@justinmc
justinmc force-pushed the automatic-pop-scope branch from ae6a6e8 to 09c5b70 Compare July 25, 2024 17:55
… active

The tabs are not managed by routes, they are just shown and hidden, so
this widget needs special custom handling of backs. This is a good
example of when we can't do this automatically.
@github-actions github-actions Bot added the p: cupertino_ui cupertino_ui package in flutter/packages label Jul 26, 2024
justinmc added 3 commits July 29, 2024 13:55
They don't need to do PopScope themselves anymore.
…ons do work

Red herring, it only happens in complex interleaving scenarios, and that
is a bug in master too.
@github-actions github-actions Bot added the p: material_ui material_ui package in flutter/packages label Jul 30, 2024
@justinmc
justinmc marked this pull request as ready for review July 30, 2024 21:20
@chunhtai
chunhtai self-requested a review July 30, 2024 22:30
@justinmc

Copy link
Copy Markdown
Contributor Author

@maRci002 I'm interested in your thoughts here if you have a chance to take a look.


// If this is a nested Navigator, handle system backs here so that the root
// Navigator doesn't get all of them.
if (widget.handlesBacksWhenNested && Navigator.maybeOf(context, rootNavigator: true) != this) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Navigator.maybeOf may be costly in this situation since this will walk ancestor chain every rebuild, I will probably put the check in didChangeDependencies

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment thread packages/flutter/lib/src/material/predictive_back_page_transitions_builder.dart Outdated
// Manually use a NavigatorPopHandler to handle backs only on the active
// tab, instead of letting Navigator use its default back handling for
// nested Navigators.
handlesBacksWhenNested: false,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is unfortunate. if there are multiple parallel navigators, what decide which one receives the pop?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

See this test for how tabbed navigation is typically done here:

testWidgets('System back navigation inside of tabs', (WidgetTester tester) async {

Each tab has its own nested Navigator wrapped by NavigatorPopHandler. Only the single active tab's NavigatorPopHandler is enabled though, so it's the only one that receives a call to onPop.

Or for the more generic case, where you have 2 sibling Navigators each wrapped with a PopScope, and both nested under a root Navigator? Both will be called. I think that's probably what we want.

void onPopInvokedWithResult(bool didPop, T? result) {

But for the even more generic case where you have two WidgetsBindingObservers listening for didPopRoute, only the one that was registered first will be called. I think that's very arbitrary and probably a mistake. It would probably solve a lot of the complexity with PopScope etc. if it was able to just call it on the current route or something.

Future<bool> handlePopRoute() async {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@chunhtai I wonder what you think about this handlesBacksWhenNested parameter versus scrapping this PR and adding NavigatorPopHandler to go_router?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the current approach of adding this parameter may be easier for developer

@maRci002

maRci002 commented Aug 1, 2024 •

Copy link
Copy Markdown
Contributor

Here are some contexts for managing the back button in Flutter:

  • Using Back Button (without predictive back):

    • WidgetsBinding.handlePopRoute is invoked at very first

      Future<void> handlePopRoute() async {
      for (final WidgetsBindingObserver observer in List<WidgetsBindingObserver>.of(_observers)) {
      if (await observer.didPopRoute()) {
      return;
      }
      }
      SystemNavigator.pop();
      }

    • Non-router version:

    • Router version:

      • RootBackButtonDispatcher.didPopRoute is triggered
        Future<bool> didPopRoute() => invokeCallback(Future<bool>.value(false));
      • To delegate the back button to a nested Navigator, you should override RouterDelegate.popRoute. Depending on your AppState (which should know which Navigator is active, e.g., when a BottomNavigationBar is used where each tab has its own Navigator), find the appropriate NavigatorState by GlobalKey and call pop / maybePop on it.
        Future<bool> popRoute();
  • Using Predictive Back:

    • The following methods manage predictive back actions: WidgetsBinding._handleStartBackGesture / _handleUpdateBackGestureProgress / _handleCommitBackGesture / _handleCancelBackGesture.
    • The method _handleStartBackGesture sets the _backGestureObserver, which handles the rest of the predictive back actions
      Future<bool> _handleStartBackGesture(Map<String?, Object?> arguments) {
      _backGestureObserver = null;
      final PredictiveBackEvent backEvent = PredictiveBackEvent.fromMap(arguments);
      for (final WidgetsBindingObserver observer in List<WidgetsBindingObserver>.of(_observers)) {
      if (observer.handleStartBackGesture(backEvent)) {
      _backGestureObserver = observer;
      return Future<bool>.value(true);
      }
      }
      return Future<bool>.value(false);
      }

Currently, we can delegate the back button to the appropriate Navigator, but we cannot delegate predictive back events to the appropriate Navigator. This pull request also aims to handle back button delegations?

I am unsure whether the current solution is capable of determining which Navigator should handle system gestures, since we are registering WidgetsBindingObservers in order. This means that MaterialApp / RootBackButtonDispatcher will always have priority over system gestures

void addObserver(WidgetsBindingObserver observer) => _observers.add(observer);

The solution has been to use WillPopScope (and later PopScope/NavigatorPopHandler) to catch the system back and handle it manually.

I believe the approach is good and we shouldn't break it.

We might need a flag on the Navigator, like handlesPredictiveBack, and _PredictiveBackGestureDetectorState._isEnabled could also check for this with route.navigator?.widget.handlesPredictiveBack, and that’s it. This value could be true by default, so we don’t disrupt existing functionality, and we could add this flag to MaterialApp's constructor to proxy the value to Navigator.

/// True when the predictive back gesture is enabled.
bool get _isEnabled {
return widget.route.isCurrent
&& widget.route.popGestureEnabled;
}

@justinmc

justinmc commented Aug 2, 2024

Copy link
Copy Markdown
Contributor Author

@maRci002 What would that flag do?

This PR was not attempting to fix the problem with predictive back route transitions.

I'm just realizing that this PR does not fix the problem with ShellRoute either! Because go_router does not have a root Navigator. I need to reproduce that problem with just MaterialApp.router and no go_router so I fully understand it.

@github-actions github-actions Bot added d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos labels Aug 8, 2024
@justinmc
justinmc requested a review from chunhtai September 16, 2025 18:07
@justinmc

Copy link
Copy Markdown
Contributor Author

@chunhtai This is finally ready for re-review. I was worried that this PR would be too breaking, but now go_router’s breakages are fixed, the failing Google tests are very minimal (and I have a g3fix), and there are no customer test failures. I think I’m confident enough to try merging this. It should be much easier for users in the majority of cases, where they don’t want to think about handling predictive back gestures manually for their nested Navigators.

/// * [PopScope], which provides even more control over system back behavior.
/// * [CupertinoTabView], which sets this parameter to false in order to
/// provide custom back handling for its nested [Navigator]s.
final bool handlesBacksWhenNested;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just a thought, should this be tristate to make things easier? in case of null, it automatically check whether this is nested by calling Navigator.of

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's what true does currently, see _isRootNavigator and _getIsRootNavigator.

I think there is really no third state to be handled. When the Navigator is not root, then it doesn't need a PopScope widget, it will handle back gestures anyway. So an unqualified handlesBacks set to true would mean the same thing as handlesBacksWhenNested is true. And it's not really possible to not handle backs at all for a root Navigator.

Maybe the "WhenNested" part of the name makes this confusing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

sgtm

late bool _isRootNavigator;

bool _lastCanPopCached = false; // Because history is empty to start.
bool get _lastCanPop => _lastCanPopCached;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

consider move the _lastCanPopCached into the middle
getter
private property
setter

// If this Navigator handles back gestures, then rebuild when canPop changes
// so that the PopScope can be updated.
if (_handlesBackGestures && value != _lastCanPop) {
ServicesBinding.instance.addPostFrameCallback((Duration timestamp) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we should check for schedule phase I image we may not always changeing this in build state in the future


// If this is a nested Navigator, handle system backs so that the root
// Navigator doesn't get all of them.
bool get _handlesBackGestures => widget.handlesBacksWhenNested && !_isRootNavigator;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we are checking _isRootNavigator, does the root navigator need to set this to false?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The root navigator itself will have _isRootNavigator set to false, so _handlesBackGestures will always be false for the root navigator. So it doesn't matter what handlesBackWhenNested is for the root navigator.

Does that answer your question? Maybe the naming makes this confusing...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

in that case, I don't think we need to set handlesBackWhenNested: false in WidgetsApp


bool _lastCanPopCached = false; // Because history is empty to start.
bool get _lastCanPop => _lastCanPopCached;
set _lastCanPop(bool value) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

not something need to be addressed now. we now has canPop, _lastCanPop, feels a bit weird. and when to use which in the navigatorState. Consider consolidate them (maybe a breaking change or deprcating canPop()) and turn them into changenotifier

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'll add a comment here to help explain lastCanPop in the meantime.

// Navigator doesn't get all of them.
if (widget.handlesBacksWhenNested && Navigator.maybeOf(context, rootNavigator: true) != this) {
return PopScope(
canPop: !canPop(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you add a comment here says rebuild will be triggered when _lastCanPop changed

@justinmc
justinmc requested a review from chunhtai September 26, 2025 21:40
chunhtai
chunhtai previously approved these changes Sep 29, 2025

@chunhtai chunhtai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, just one suggestion

debugLabel: 'Navigator Scope',
autofocus: true,
child: Navigator(
handlesBacksWhenNested: false,

@chunhtai chunhtai Sep 29, 2025 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is not needed since handlesBacksWhenNested: true can still handle the root case. While uncommon, but I have seen people doing MaterialApp inside a MaterialApp...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think I did this specifically to work around a weird setup like that in the Google tests. I will try it and see if it passes, maybe that's out of date or I'm misremembering.


// If this is a nested Navigator, handle system backs so that the root
// Navigator doesn't get all of them.
bool get _handlesBackGestures => widget.handlesBacksWhenNested && !_isRootNavigator;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

in that case, I don't think we need to set handlesBackWhenNested: false in WidgetsApp

@Piinks

Piinks commented Dec 8, 2025

Copy link
Copy Markdown
Contributor

Greetings from stale PR triage! 👋
Is this change still on your radar?

@Sshlale

This comment was marked as off-topic.

@dkwingsmt
dkwingsmt marked this pull request as draft March 25, 2026 18:19
@flutter-dashboard

Copy link
Copy Markdown

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

creatorpiyush pushed a commit to creatorpiyush/packages that referenced this pull request Jun 10, 2026
This test was failing in flutter/flutter#152330. That PR adds a PopScope inside of nested Navigators. When a back gesture happens, the PopScope takes 1 frame to rerender and update the state of its pop handling. The test was breaking because it performed two back gestures in one frame. Putting a pump in between them fixes it when the PR is  merged (and works fine without the PR too).

It took me so many hours to realize this one little pump was the fix I needed 😭 .
@Piinks Piinks added the Decoupling: Not ready to port yet Instructions will be provided when this is ready to move to flutter/packages. label Jun 24, 2026
@Piinks

Piinks commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

I've marked this PR as not ready to port to flutter/packages yet.
We'll provide instructions to move this change over to material_ui/cupertino_ui once ready to receive PRs. Thank you!

The changes not in the material or cupertino libraries will remain here.

@Piinks Piinks added the Decoupling: Split PR The PR will need to be split to separate Material & Cupertino changes label Jun 24, 2026
@Piinks

Piinks commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Advice for PR splitting: Since this touches material/cupertino along with other parts of flutter/flutter, it will need to be split up.

If the material/cupertino changes are dependent on the other non-material/cupertino changes in this PR, then the material/cupertino changes will need to wait for them to roll to stable before they can be ported over to material_ui/cupertino_ui in flutter/packages (which uses the stable branch).

  • If that is the case here, consider opening a new PR with the non-material/cupertino changes to move forward with so that waiting period can be reduced. Up to you!

@reidbaker

Copy link
Copy Markdown
Contributor

@justinmc what is the status of this pr do you want review?

@Piinks

Piinks commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

When ready, material_ui and cupertino_ui are now accepting contributions. The portions of this PR that affect the material and cupertino libraries can be ported over to flutter/packages. Guidance and instructions for easily porting over can be found in #188444

@Piinks Piinks added Decoupling: Port to flutter/packages This PR is ready to be ported to flutter/packages. We will provide instructions to do so. and removed Decoupling: Not ready to port yet Instructions will be provided when this is ready to move to flutter/packages. labels Aug 18, 2026
@justinmc

Copy link
Copy Markdown
Contributor Author

I'll close this since it does have a few places where it touches Cupertino.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos Decoupling: Port to flutter/packages This PR is ready to be ported to flutter/packages. We will provide instructions to do so. Decoupling: Split PR The PR will need to be split to separate Material & Cupertino changes f: routes Navigator, Router, and related APIs. framework flutter/packages/flutter repository. See also f: labels. p: cupertino_ui cupertino_ui package in flutter/packages p: material_ui material_ui package in flutter/packages

Projects

None yet

7 participants