[go_router] fix: PopScope.onPopInvokedWithResult not called in branch routes (#9245)

Author: Gustl22

packages/go_router/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 15.2.2
+
+- Fixes calling `PopScope.onPopInvokedWithResult` in branch routes.
+
 ## 15.2.1
 
 * Fixes Popping state and re-rendering scaffold at the same time doesn't update the URL on web.
@@ -11,7 +15,6 @@
 * Updates minimum supported SDK version to Flutter 3.27/Dart 3.6.
 * Fixes typo in API docs.
 
-
 ## 15.1.2
 
 - Fixes focus request propagation from `GoRouter` to `Navigator` by properly handling the `requestFocus` parameter.

packages/go_router/lib/src/delegate.dart

@@ -55,8 +55,8 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList>
 
   @override
   Future<bool> popRoute() async {
-    final NavigatorState? state = _findCurrentNavigator();
-    if (state != null) {
+    final Iterable<NavigatorState> states = _findCurrentNavigators();
+    for (final NavigatorState state in states) {
       final bool didPop = await state.maybePop(); // Call maybePop() directly
       if (didPop) {
         return true; // Return true if maybePop handled the pop
@@ -96,17 +96,27 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList>
 
   /// Pops the top-most route.
   void pop<T extends Object?>([T? result]) {
-    final NavigatorState? state = _findCurrentNavigator();
-    if (state == null || !state.canPop()) {
+    final Iterable<NavigatorState> states = _findCurrentNavigators().where(
+      (NavigatorState element) => element.canPop(),
+    );
+    if (states.isEmpty) {
       throw GoError('There is nothing to pop');
     }
-    state.pop(result);
+    states.first.pop(result);
   }
 
-  NavigatorState? _findCurrentNavigator() {
-    NavigatorState? state;
-    state =
-        navigatorKey.currentState; // Set state directly without canPop check
+  /// Get a prioritized list of NavigatorStates,
+  /// which either can pop or are exit routes.
+  ///
+  /// 1. Sub route within branches of shell navigation
+  /// 2. Branch route
+  /// 3. Parent route
+  Iterable<NavigatorState> _findCurrentNavigators() {
+    final List<NavigatorState> states = <NavigatorState>[];
+    if (navigatorKey.currentState != null) {
+      // Set state directly without canPop check
+      states.add(navigatorKey.currentState!);
+    }
 
     RouteMatchBase walker = currentConfiguration.matches.last;
     while (walker is ShellRouteMatch) {
@@ -119,13 +129,10 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList>
         // Stop if there is a pageless route on top of the shell route.
         break;
       }
-
-      if (potentialCandidate.canPop()) {
-        state = walker.navigatorKey.currentState;
-      }
+      states.add(potentialCandidate);
       walker = walker.matches.last;
     }
-    return state;
+    return states.reversed;
   }
 
   bool _handlePopPageWithRouteMatch(

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: 15.2.1
+version: 15.2.2
 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/delegate_test.dart

@@ -76,6 +76,57 @@ Future<GoRouter> createGoRouterWithStatefulShellRoute(
   return router;
 }
 
+Future<GoRouter> createGoRouterWithStatefulShellRouteAndPopScopes(
+  WidgetTester tester, {
+  bool canPopShellRouteBuilder = true,
+  bool canPopBranch = true,
+  bool canPopBranchSubRoute = true,
+  PopInvokedWithResultCallback<bool>? onPopShellRouteBuilder,
+  PopInvokedWithResultCallback<bool>? onPopBranch,
+  PopInvokedWithResultCallback<bool>? onPopBranchSubRoute,
+}) async {
+  final GoRouter router = GoRouter(
+    initialLocation: '/c',
+    routes: <RouteBase>[
+      StatefulShellRoute.indexedStack(
+        branches: <StatefulShellBranch>[
+          StatefulShellBranch(routes: <RouteBase>[
+            GoRoute(
+                path: '/c',
+                builder: (_, __) => PopScope(
+                    onPopInvokedWithResult: onPopBranch,
+                    canPop: canPopBranch,
+                    child: const Text('Home')),
+                routes: <RouteBase>[
+                  GoRoute(
+                    path: 'c1',
+                    builder: (_, __) => PopScope(
+                      onPopInvokedWithResult: onPopBranchSubRoute,
+                      canPop: canPopBranchSubRoute,
+                      child: const Text('SubRoute'),
+                    ),
+                  ),
+                ]),
+          ]),
+        ],
+        builder: (BuildContext context, GoRouterState state,
+                StatefulNavigationShell navigationShell) =>
+            PopScope(
+          onPopInvokedWithResult: onPopShellRouteBuilder,
+          canPop: canPopShellRouteBuilder,
+          child: navigationShell,
+        ),
+      ),
+    ],
+  );
+
+  addTearDown(router.dispose);
+  await tester.pumpWidget(MaterialApp.router(
+    routerConfig: router,
+  ));
+  return router;
+}
+
 void main() {
   group('pop', () {
     testWidgets('restore() update currentConfiguration in pop()',
@@ -152,6 +203,91 @@ void main() {
       expect(find.text('Home'), findsOneWidget);
     });
 
+    testWidgets(
+        'PopScope intercepts back button on StatefulShellRoute builder route',
+        (WidgetTester tester) async {
+      bool didPopShellRouteBuilder = false;
+      bool didPopBranch = false;
+      bool didPopBranchSubRoute = false;
+
+      await createGoRouterWithStatefulShellRouteAndPopScopes(
+        tester,
+        canPopShellRouteBuilder: false,
+        onPopShellRouteBuilder: (_, __) => didPopShellRouteBuilder = true,
+        onPopBranch: (_, __) => didPopBranch = true,
+        onPopBranchSubRoute: (_, __) => didPopBranchSubRoute = true,
+      );
+
+      expect(find.text('Home'), findsOneWidget);
+      await tester.binding.handlePopRoute();
+      await tester.pumpAndSettle();
+
+      // Verify that PopScope intercepted the back button
+      expect(didPopShellRouteBuilder, isTrue);
+      expect(didPopBranch, isFalse);
+      expect(didPopBranchSubRoute, isFalse);
+
+      expect(find.text('Home'), findsOneWidget);
+    });
+
+    testWidgets(
+        'PopScope intercepts back button on StatefulShellRoute branch route',
+        (WidgetTester tester) async {
+      bool didPopShellRouteBuilder = false;
+      bool didPopBranch = false;
+      bool didPopBranchSubRoute = false;
+
+      await createGoRouterWithStatefulShellRouteAndPopScopes(
+        tester,
+        canPopBranch: false,
+        onPopShellRouteBuilder: (_, __) => didPopShellRouteBuilder = true,
+        onPopBranch: (_, __) => didPopBranch = true,
+        onPopBranchSubRoute: (_, __) => didPopBranchSubRoute = true,
+      );
+
+      expect(find.text('Home'), findsOneWidget);
+      await tester.binding.handlePopRoute();
+      await tester.pumpAndSettle();
+
+      // Verify that PopScope intercepted the back button
+      expect(didPopShellRouteBuilder, isFalse);
+      expect(didPopBranch, isTrue);
+      expect(didPopBranchSubRoute, isFalse);
+
+      expect(find.text('Home'), findsOneWidget);
+    });
+
+    testWidgets(
+        'PopScope intercepts back button on StatefulShellRoute branch sub route',
+        (WidgetTester tester) async {
+      bool didPopShellRouteBuilder = false;
+      bool didPopBranch = false;
+      bool didPopBranchSubRoute = false;
+
+      final GoRouter goRouter =
+          await createGoRouterWithStatefulShellRouteAndPopScopes(
+        tester,
+        canPopBranchSubRoute: false,
+        onPopShellRouteBuilder: (_, __) => didPopShellRouteBuilder = true,
+        onPopBranch: (_, __) => didPopBranch = true,
+        onPopBranchSubRoute: (_, __) => didPopBranchSubRoute = true,
+      );
+
+      goRouter.push('/c/c1');
+      await tester.pumpAndSettle();
+
+      expect(find.text('SubRoute'), findsOneWidget);
+      await tester.binding.handlePopRoute();
+      await tester.pumpAndSettle();
+
+      // Verify that PopScope intercepted the back button
+      expect(didPopShellRouteBuilder, isFalse);
+      expect(didPopBranch, isFalse);
+      expect(didPopBranchSubRoute, isTrue);
+
+      expect(find.text('SubRoute'), findsOneWidget);
+    });
+
     testWidgets('pops more than matches count should return false',
         (WidgetTester tester) async {
       final GoRouter goRouter = await createGoRouter(tester)