[go_router] Refactors GoRouter.pop to handle pageless route (#2879)

* Refactors GoRouter.pop to handle pageless route

* update

packages/go_router/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 5.2.1
+
+- Refactors `GoRouter.pop` to be able to pop individual pageless route with result.
+
 ## 5.2.0
 
 - Fixes `GoRouterState.location` and `GoRouterState.param` to return correct value.

packages/go_router/lib/src/builder.dart

@@ -54,7 +54,7 @@ class RouteBuilder {
   Widget build(
     BuildContext context,
     RouteMatchList matchList,
-    VoidCallback pop,
+    PopPageCallback onPopPage,
     bool routerNeglect,
   ) {
     if (matchList.isEmpty) {
@@ -69,14 +69,14 @@ class RouteBuilder {
           try {
             final Map<Page<Object?>, GoRouterState> newRegistry =
                 <Page<Object?>, GoRouterState>{};
-            final Widget result = tryBuild(context, matchList, pop,
+            final Widget result = tryBuild(context, matchList, onPopPage,
                 routerNeglect, configuration.navigatorKey, newRegistry);
             _registry.updateRegistry(newRegistry);
             return GoRouterStateRegistryScope(
                 registry: _registry, child: result);
           } on _RouteBuilderError catch (e) {
-            return _buildErrorNavigator(
+            return _buildErrorNavigator(context, e, matchList.uri, onPopPage,
-                context, e, matchList.uri, pop, configuration.navigatorKey);
+                configuration.navigatorKey);
           }
         },
       ),
@@ -91,7 +91,7 @@ class RouteBuilder {
   Widget tryBuild(
     BuildContext context,
     RouteMatchList matchList,
-    VoidCallback pop,
+    PopPageCallback onPopPage,
     bool routerNeglect,
     GlobalKey<NavigatorState> navigatorKey,
     Map<Page<Object?>, GoRouterState> registry,
@@ -99,9 +99,9 @@ class RouteBuilder {
     return builderWithNav(
       context,
       _buildNavigator(
-        pop,
+        onPopPage,
-        buildPages(
+        buildPages(context, matchList, onPopPage, routerNeglect, navigatorKey,
-            context, matchList, pop, routerNeglect, navigatorKey, registry),
+            registry),
         navigatorKey,
         observers: observers,
       ),
@@ -114,15 +114,15 @@ class RouteBuilder {
   List<Page<Object?>> buildPages(
       BuildContext context,
       RouteMatchList matchList,
-      VoidCallback onPop,
+      PopPageCallback onPopPage,
       bool routerNeglect,
       GlobalKey<NavigatorState> navigatorKey,
       Map<Page<Object?>, GoRouterState> registry) {
     try {
       final Map<GlobalKey<NavigatorState>, List<Page<Object?>>> keyToPage =
           <GlobalKey<NavigatorState>, List<Page<Object?>>>{};
-      _buildRecursive(context, matchList, 0, onPop, routerNeglect, keyToPage,
+      _buildRecursive(context, matchList, 0, onPopPage, routerNeglect,
-          navigatorKey, registry);
+          keyToPage, navigatorKey, registry);
       return keyToPage[navigatorKey]!;
     } on _RouteBuilderError catch (e) {
       return <Page<Object?>>[
@@ -135,7 +135,7 @@ class RouteBuilder {
     BuildContext context,
     RouteMatchList matchList,
     int startIndex,
-    VoidCallback pop,
+    PopPageCallback onPopPage,
     bool routerNeglect,
     Map<GlobalKey<NavigatorState>, List<Page<Object?>>> keyToPages,
     GlobalKey<NavigatorState> navigatorKey,
@@ -163,8 +163,8 @@ class RouteBuilder {
 
       keyToPages.putIfAbsent(goRouteNavKey, () => <Page<Object?>>[]).add(page);
 
-      _buildRecursive(context, matchList, startIndex + 1, pop, routerNeglect,
+      _buildRecursive(context, matchList, startIndex + 1, onPopPage,
-          keyToPages, navigatorKey, registry);
+          routerNeglect, keyToPages, navigatorKey, registry);
     } else if (route is ShellRoute) {
       // The key for the Navigator that will display this ShellRoute's page.
       final GlobalKey<NavigatorState> parentNavigatorKey = navigatorKey;
@@ -184,12 +184,12 @@ class RouteBuilder {
       final int shellPageIdx = keyToPages[parentNavigatorKey]!.length;
 
       // Build the remaining pages
-      _buildRecursive(context, matchList, startIndex + 1, pop, routerNeglect,
+      _buildRecursive(context, matchList, startIndex + 1, onPopPage,
-          keyToPages, shellNavigatorKey, registry);
+          routerNeglect, keyToPages, shellNavigatorKey, registry);
 
       // Build the Navigator
       final Widget child = _buildNavigator(
-          pop, keyToPages[shellNavigatorKey]!, shellNavigatorKey);
+          onPopPage, keyToPages[shellNavigatorKey]!, shellNavigatorKey);
 
       // Build the Page for this route
       final Page<Object?> page =
@@ -203,7 +203,7 @@ class RouteBuilder {
   }
 
   Navigator _buildNavigator(
-    VoidCallback pop,
+    PopPageCallback onPopPage,
     List<Page<Object?>> pages,
     Key? navigatorKey, {
     List<NavigatorObserver> observers = const <NavigatorObserver>[],
@@ -213,13 +213,7 @@ class RouteBuilder {
       restorationScopeId: restorationScopeId,
       pages: pages,
       observers: observers,
-      onPopPage: (Route<dynamic> route, dynamic result) {
+      onPopPage: onPopPage,
-        if (!route.didPop(result)) {
-          return false;
-        }
-        pop();
-        return true;
-      },
     );
   }
 
@@ -393,10 +387,14 @@ class RouteBuilder {
       );
 
   /// Builds a Navigator containing an error page.
-  Widget _buildErrorNavigator(BuildContext context, _RouteBuilderError e,
+  Widget _buildErrorNavigator(
-      Uri uri, VoidCallback pop, GlobalKey<NavigatorState> navigatorKey) {
+      BuildContext context,
+      _RouteBuilderError e,
+      Uri uri,
+      PopPageCallback onPopPage,
+      GlobalKey<NavigatorState> navigatorKey) {
     return _buildNavigator(
-      pop,
+      onPopPage,
       <Page<Object?>>[
         _buildErrorPage(context, e, uri),
       ],

packages/go_router/lib/src/delegate.dart

@@ -11,6 +11,7 @@ import 'builder.dart';
 import 'configuration.dart';
 import 'match.dart';
 import 'matching.dart';
+import 'misc/errors.dart';
 import 'typedefs.dart';
 
 /// GoRouter implementation of [RouterDelegate].
@@ -59,38 +60,19 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList>
   final Map<String, int> _pushCounts = <String, int>{};
   final RouteConfiguration _configuration;
 
+  _NavigatorStateIterator _createNavigatorStateIterator() =>
+      _NavigatorStateIterator(_matchList, navigatorKey.currentState!);
+
   @override
   Future<bool> popRoute() async {
-    // Iterate backwards through the RouteMatchList until seeing a GoRoute with
+    final _NavigatorStateIterator iterator = _createNavigatorStateIterator();
-    // a non-null parentNavigatorKey or a ShellRoute with a non-null
+    while (iterator.moveNext()) {
-    // parentNavigatorKey and pop from that Navigator instead of the root.
+      final bool didPop = await iterator.current.maybePop();
-    final int matchCount = _matchList.matches.length;
+      if (didPop) {
-    for (int i = matchCount - 1; i >= 0; i -= 1) {
+        return true;
-      final RouteMatch match = _matchList.matches[i];
-      final RouteBase route = match.route;
-
-      if (route is GoRoute && route.parentNavigatorKey != null) {
-        final bool didPop =
-            await route.parentNavigatorKey!.currentState!.maybePop();
-
-        // Continue if didPop was false.
-        if (didPop) {
-          return didPop;
-        }
-      } else if (route is ShellRoute) {
-        final bool didPop = await route.navigatorKey.currentState!.maybePop();
-
-        // Continue if didPop was false.
-        if (didPop) {
-          return didPop;
-        }
       }
     }
-
+    return false;
-    // Use the root navigator if no ShellRoute Navigators were found and didn't
-    // pop
-    final NavigatorState navigator = navigatorKey.currentState!;
-    return navigator.maybePop();
   }
 
   /// Pushes the given location onto the page stack
@@ -117,29 +99,25 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList>
 
   /// Returns `true` if the active Navigator can pop.
   bool canPop() {
-    // Loop through navigators in reverse and call canPop()
+    final _NavigatorStateIterator iterator = _createNavigatorStateIterator();
-    final int matchCount = _matchList.matches.length;
+    while (iterator.moveNext()) {
-    for (int i = matchCount - 1; i >= 0; i -= 1) {
+      if (iterator.current.canPop()) {
-      final RouteMatch match = _matchList.matches[i];
+        return true;
-      final RouteBase route = match.route;
-      if (route is GoRoute && route.parentNavigatorKey != null) {
-        final bool canPop =
-            route.parentNavigatorKey!.currentState?.canPop() ?? false;
-
-        // Continue if canPop is false.
-        if (canPop) {
-          return canPop;
-        }
-      } else if (route is ShellRoute) {
-        final bool canPop = route.navigatorKey.currentState?.canPop() ?? false;
-
-        // Continue if canPop is false.
-        if (canPop) {
-          return canPop;
-        }
       }
     }
-    return navigatorKey.currentState?.canPop() ?? false;
+    return false;
+  }
+
+  /// Pops the top-most route.
+  void pop<T extends Object?>([T? result]) {
+    final _NavigatorStateIterator iterator = _createNavigatorStateIterator();
+    while (iterator.moveNext()) {
+      if (iterator.current.canPop()) {
+        iterator.current.pop<T>(result);
+        return;
+      }
+    }
+    throw GoError('There is nothing to pop');
   }
 
   void _debugAssertMatchListNotEmpty() {
@@ -150,14 +128,16 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList>
     );
   }
 
-  /// Pop the top page off the GoRouter's page stack.
+  bool _onPopPage(Route<Object?> route, Object? result) {
-  void pop() {
+    if (!route.didPop(result)) {
+      return false;
+    }
     _matchList.pop();
     assert(() {
       _debugAssertMatchListNotEmpty();
       return true;
     }());
-    notifyListeners();
+    return true;
   }
 
   /// Replaces the top-most page of the page stack with the given one.
@@ -187,7 +167,7 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList>
     return builder.build(
       context,
       _matchList,
-      pop,
+      _onPopPage,
       routerNeglect,
     );
   }
@@ -204,6 +184,84 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList>
   }
 }
 
+/// An iterator that iterates through navigators that [GoRouterDelegate]
+/// created from the inner to outer.
+///
+/// The iterator starts with the navigator that hosts the top-most route. This
+/// navigator may not be the inner-most navigator if the top-most route is a
+/// pageless route, such as a dialog or bottom sheet.
+class _NavigatorStateIterator extends Iterator<NavigatorState> {
+  _NavigatorStateIterator(this.matchList, this.root)
+      : index = matchList.matches.length;
+
+  final RouteMatchList matchList;
+  int index = 0;
+  final NavigatorState root;
+  @override
+  late NavigatorState current;
+
+  @override
+  bool moveNext() {
+    if (index < 0) {
+      return false;
+    }
+    for (index -= 1; index >= 0; index -= 1) {
+      final RouteMatch match = matchList.matches[index];
+      final RouteBase route = match.route;
+      if (route is GoRoute && route.parentNavigatorKey != null) {
+        final GlobalKey<NavigatorState> parentNavigatorKey =
+            route.parentNavigatorKey!;
+        final ModalRoute<Object?>? parentModalRoute =
+            ModalRoute.of(parentNavigatorKey.currentContext!);
+        // The ModalRoute can be null if the parentNavigatorKey references the
+        // root navigator.
+        if (parentModalRoute == null) {
+          index = -1;
+          assert(root == parentNavigatorKey.currentState);
+          current = root;
+          return true;
+        }
+        // It must be a ShellRoute that holds this parentNavigatorKey;
+        // otherwise, parentModalRoute would have been null. Updates the index
+        // to the ShellRoute
+        for (index -= 1; index >= 0; index -= 1) {
+          final RouteBase route = matchList.matches[index].route;
+          if (route is ShellRoute) {
+            if (route.navigatorKey == parentNavigatorKey) {
+              break;
+            }
+          }
+        }
+        // There may be a pageless route on top of ModalRoute that the
+        // NavigatorState of parentNavigatorKey is in. For example, an open
+        // dialog. In that case we want to find the navigator that host the
+        // pageless route.
+        if (parentModalRoute.isCurrent == false) {
+          continue;
+        }
+
+        current = parentNavigatorKey.currentState!;
+        return true;
+      } else if (route is ShellRoute) {
+        // Must have a ModalRoute parent because the navigator ShellRoute
+        // created must not be the root navigator.
+        final ModalRoute<Object?> parentModalRoute =
+            ModalRoute.of(route.navigatorKey.currentContext!)!;
+        // There may be pageless route on top of ModalRoute that the
+        // parentNavigatorKey is in. For example an open dialog.
+        if (parentModalRoute.isCurrent == false) {
+          continue;
+        }
+        current = route.navigatorKey.currentState!;
+        return true;
+      }
+    }
+    assert(index == -1);
+    current = root;
+    return true;
+  }
+}
+
 /// The route match that represent route pushed through [GoRouter.push].
 // TODO(chunhtai): Removes this once imperative API no longer insert route match.
 class ImperativeRouteMatch extends RouteMatch {

packages/go_router/lib/src/router.dart

@@ -142,7 +142,7 @@ class GoRouter extends ChangeNotifier implements RouterConfig<RouteMatchList> {
   String get location => _location;
   String _location = '/';
 
-  /// Returns `true` if there is more than 1 page on the stack.
+  /// Returns `true` if there is at least two or more route can be pop.
   bool canPop() => _routerDelegate.canPop();
 
   void _handleStateMayChange() {
@@ -272,13 +272,16 @@ class GoRouter extends ChangeNotifier implements RouterConfig<RouteMatchList> {
     );
   }
 
-  /// Pop the top page off the GoRouter's page stack.
+  /// Pop the top-most route off the current screen.
-  void pop() {
+  ///
+  /// If the top-most route is a pop up or dialog, this method pops it instead
+  /// of any GoRoute under it.
+  void pop<T extends Object?>([T? result]) {
     assert(() {
       log.info('popping $location');
       return true;
     }());
-    _routerDelegate.pop();
+    _routerDelegate.pop<T>(result);
   }
 
   /// Refresh the route.

packages/go_router/pubspec.yaml

@@ -1,7 +1,7 @@
 name: go_router
 description: A declarative router for Flutter based on Navigation 2 supporting
   deep linking, data-driven routes and more
-version: 5.2.0
+version: 5.2.1
 repository: https://github.com/flutter/packages/tree/main/packages/go_router
 issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+go_router%22
 

packages/go_router/test/builder_test.dart

@@ -346,7 +346,7 @@ class _BuilderTestWidget extends StatelessWidget {
   @override
   Widget build(BuildContext context) {
     return MaterialApp(
-      home: builder.tryBuild(context, matches, () {}, false,
+      home: builder.tryBuild(context, matches, (_, __) => false, false,
           routeConfiguration.navigatorKey, <Page<Object?>, GoRouterState>{}),
       // builder: (context, child) => ,
     );

packages/go_router/test/delegate_test.dart

@@ -36,24 +36,21 @@ void main() {
     testWidgets('removes the last element', (WidgetTester tester) async {
       final GoRouter goRouter = await createGoRouter(tester)
         ..push('/error');
+      await tester.pumpAndSettle();
 
-      goRouter.routerDelegate.addListener(expectAsync0(() {}));
       final RouteMatch last = goRouter.routerDelegate.matches.matches.last;
-      goRouter.routerDelegate.pop();
+      await goRouter.routerDelegate.popRoute();
       expect(goRouter.routerDelegate.matches.matches.length, 1);
       expect(goRouter.routerDelegate.matches.matches.contains(last), false);
     });
 
-    testWidgets('throws when it pops more than matches count',
+    testWidgets('pops more than matches count should return false',
         (WidgetTester tester) async {
       final GoRouter goRouter = await createGoRouter(tester)
         ..push('/error');
-      expect(
+      await tester.pumpAndSettle();
-        () => goRouter.routerDelegate
+      await goRouter.routerDelegate.popRoute();
-          ..pop()
+      expect(await goRouter.routerDelegate.popRoute(), isFalse);
-          ..pop(),
-        throwsA(isAssertionError),
-      );
     });
   });
 

packages/go_router/test/go_router_test.dart

@@ -2549,15 +2549,6 @@ void main() {
   });
 
   group('Imperative navigation', () {
-    testWidgets('pop triggers pop on routerDelegate',
-        (WidgetTester tester) async {
-      final GoRouter router = await createGoRouter(tester)
-        ..push('/error');
-      router.routerDelegate.addListener(expectAsync0(() {}));
-      router.pop();
-      await tester.pump();
-    });
-
     group('canPop', () {
       testWidgets(
         'It should return false if Navigator.canPop() returns false.',
@@ -2736,7 +2727,55 @@ void main() {
           expect(router.canPop(), true);
         },
       );
+      testWidgets('Pageless route should include in can pop',
+          (WidgetTester tester) async {
+        final GlobalKey<NavigatorState> root =
+            GlobalKey<NavigatorState>(debugLabel: 'root');
+        final GlobalKey<NavigatorState> shell =
+            GlobalKey<NavigatorState>(debugLabel: 'shell');
+
+        final GoRouter router = GoRouter(
+          navigatorKey: root,
+          routes: <RouteBase>[
+            ShellRoute(
+              navigatorKey: shell,
+              builder:
+                  (BuildContext context, GoRouterState state, Widget child) {
+                return Scaffold(
+                  body: Center(
+                    child: Column(
+                      children: <Widget>[
+                        const Text('Shell'),
+                        Expanded(child: child),
+                      ],
+                    ),
+                  ),
+                );
+              },
+              routes: <RouteBase>[
+                GoRoute(
+                  path: '/',
+                  builder: (_, __) => const Text('A Screen'),
+                ),
+              ],
+            ),
+          ],
+        );
+
+        await tester.pumpWidget(MaterialApp.router(routerConfig: router));
+
+        expect(router.canPop(), isFalse);
+        expect(find.text('A Screen'), findsOneWidget);
+        expect(find.text('Shell'), findsOneWidget);
+        showDialog(
+            context: root.currentContext!,
+            builder: (_) => const Text('A dialog'));
+        await tester.pumpAndSettle();
+        expect(find.text('A dialog'), findsOneWidget);
+        expect(router.canPop(), isTrue);
+      });
     });
+
     group('pop', () {
       testWidgets(
         'Should pop from the correct navigator when parentNavigatorKey is set',
@@ -2815,6 +2854,74 @@ void main() {
           expect(find.text('Shell'), findsNothing);
         },
       );
+
+      testWidgets('Should pop dialog if it is present',
+          (WidgetTester tester) async {
+        final GlobalKey<NavigatorState> root =
+            GlobalKey<NavigatorState>(debugLabel: 'root');
+        final GlobalKey<NavigatorState> shell =
+            GlobalKey<NavigatorState>(debugLabel: 'shell');
+
+        final GoRouter router = GoRouter(
+          initialLocation: '/a',
+          navigatorKey: root,
+          routes: <GoRoute>[
+            GoRoute(
+              path: '/',
+              builder: (BuildContext context, _) {
+                return const Scaffold(
+                  body: Text('Home'),
+                );
+              },
+              routes: <RouteBase>[
+                ShellRoute(
+                  navigatorKey: shell,
+                  builder: (BuildContext context, GoRouterState state,
+                      Widget child) {
+                    return Scaffold(
+                      body: Center(
+                        child: Column(
+                          children: <Widget>[
+                            const Text('Shell'),
+                            Expanded(child: child),
+                          ],
+                        ),
+                      ),
+                    );
+                  },
+                  routes: <RouteBase>[
+                    GoRoute(
+                      path: 'a',
+                      builder: (_, __) => const Text('A Screen'),
+                    ),
+                  ],
+                ),
+              ],
+            ),
+          ],
+        );
+
+        await tester.pumpWidget(MaterialApp.router(routerConfig: router));
+
+        expect(router.canPop(), isTrue);
+        expect(find.text('A Screen'), findsOneWidget);
+        expect(find.text('Shell'), findsOneWidget);
+        expect(find.text('Home'), findsNothing);
+        final Future<bool?> resultFuture = showDialog<bool>(
+            context: root.currentContext!,
+            builder: (_) => const Text('A dialog'));
+        await tester.pumpAndSettle();
+        expect(find.text('A dialog'), findsOneWidget);
+        expect(router.canPop(), isTrue);
+
+        router.pop<bool>(true);
+        await tester.pumpAndSettle();
+        expect(find.text('A Screen'), findsOneWidget);
+        expect(find.text('Shell'), findsOneWidget);
+        expect(find.text('A dialog'), findsNothing);
+        final bool? result = await resultFuture;
+        expect(result, isTrue);
+      });
     });
   });
 }

packages/go_router/test/test_helpers.dart

@@ -130,7 +130,7 @@ class GoRouterPopSpy extends GoRouter {
   bool popped = false;
 
   @override
-  void pop() {
+  void pop<T extends Object?>([T? result]) {
     popped = true;
   }
 }