Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion packages/go_router/lib/src/delegate.dart
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,6 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList>
while (walker is ShellRouteMatch) {
final NavigatorState potentialCandidate =
walker.navigatorKey.currentState!;

final ModalRoute<dynamic>? modalRoute = ModalRoute.of(
potentialCandidate.context,
);
Expand Down
10 changes: 9 additions & 1 deletion packages/go_router/lib/src/route.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1569,7 +1569,15 @@ class StatefulNavigationShellState extends State<StatefulNavigationShell>
)
.toList();

return widget.containerBuilder(context, widget, children);
return PopScope(
canPop: widget.currentIndex == 0,
onPopInvokedWithResult: (bool didPop, Object? result) {
if (!didPop && widget.currentIndex != 0) {
goBranch(0);
}
},
child: widget.containerBuilder(context, widget, children),
);
Comment on lines +1572 to +1580
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The current implementation of PopScope might interfere with nested PopScope widgets within a branch.

If a child route on a non-root branch (e.g., Tab B) uses a PopScope to block a pop (for instance, to show a "Discard changes?" confirmation dialog), the didPop value in this shell-level PopScope will be false. Consequently, goBranch(0) will be triggered, switching the user to the root branch even though the intention was to stay on the current branch and handle the pop locally.

To fix this, you should check if the current branch's navigator can actually pop before deciding to switch branches. If navigator.canPop() is true but didPop is false, it implies a nested PopScope blocked the pop, and the shell should not intervene.

    return PopScope(
      canPop: widget.currentIndex == 0,
      onPopInvokedWithResult: (bool didPop, Object? result) {
        if (!didPop && widget.currentIndex != 0) {
          final NavigatorState? navigator =
              widget.shellRouteContext.navigatorKey.currentState;
          // Only switch to the root branch if the current branch navigator
          // cannot pop further. If it can pop but didn't, a nested PopScope
          // likely blocked it.
          if (navigator != null && !navigator.canPop()) {
            goBranch(0);
          }
        }
      },
      child: widget.containerBuilder(context, widget, children),
    );

}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
changelog: |
- Fix StatefulShellRoute PopScope behavior to orchestrate back button pops to the root branch when inner navigators cannot pop.
version: patch
186 changes: 186 additions & 0 deletions packages/go_router/test/shell_route_pop_scope_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,186 @@
// Copyright 2013 The Flutter Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'package:flutter/material.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:go_router/go_router.dart';

import 'test_helpers.dart';

void main() {
testWidgets(
'PopScope in StatefulShellRoute branch works on subsequent visits',
(WidgetTester tester) async {
int tabBPopCount = 0;

final routes = <RouteBase>[
StatefulShellRoute.indexedStack(
builder:
(
BuildContext context,
GoRouterState state,
StatefulNavigationShell navigationShell,
) {
return Scaffold(
body: navigationShell,
bottomNavigationBar: BottomNavigationBar(
currentIndex: navigationShell.currentIndex,
onTap: (int index) => navigationShell.goBranch(index),
items: const <BottomNavigationBarItem>[
BottomNavigationBarItem(
icon: Icon(Icons.home),
label: 'A',
),
BottomNavigationBarItem(
icon: Icon(Icons.business),
label: 'B',
),
],
),
);
},
branches: <StatefulShellBranch>[
StatefulShellBranch(
routes: <RouteBase>[
GoRoute(
path: '/tabA',
builder: (BuildContext context, GoRouterState state) =>
const DummyScreen(key: ValueKey<String>('tabA')),
),
],
),
StatefulShellBranch(
routes: <RouteBase>[
GoRoute(
path: '/tabB',
builder: (BuildContext context, GoRouterState state) {
return PopScope(
canPop: false,
onPopInvokedWithResult: (bool didPop, Object? result) {
tabBPopCount++;
if (!didPop) {
context.go('/tabA');
}
Comment on lines +60 to +64
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The current implementation of onPopInvokedWithResult in this test handles the navigation to /tabA itself. This masks the effect of the fix being introduced in StatefulNavigationShellState, which is designed to handle cases where a PopScope prevents a pop but doesn't perform any navigation.

To make this test more accurately reflect the scenario your fix is targeting, I suggest removing the navigation logic from the test's PopScope. The test will then verify that the new PopScope in StatefulNavigationShellState correctly navigates to the root branch.

                      onPopInvokedWithResult: (bool didPop, Object? result) {
                        tabBPopCount++;
                      },

},
child: const DummyScreen(key: ValueKey<String>('tabB')),
);
},
Comment on lines +58 to +68
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

This test case includes an explicit PopScope within the route definition that manually handles the branch switch. While this verifies that user-defined PopScope widgets still work, it doesn't exercise the new automatic branch-switching logic added to StatefulNavigationShell in route.dart. Consider adding a test case where the routes do not have a PopScope, to verify that the back button still correctly switches to the root branch by default.

),
],
),
],
),
];

final GoRouter router = await createRouter(
routes,
tester,
initialLocation: '/tabA',
);

// 1. Visit Tab A
expect(find.byKey(const ValueKey<String>('tabA')), findsOneWidget);
expect(find.byKey(const ValueKey<String>('tabB')), findsNothing);

// 2. Switch to Tab B
router.go('/tabB');
await tester.pumpAndSettle();
expect(find.byKey(const ValueKey<String>('tabB')), findsOneWidget);

// 3. Press back button on Tab B (First Visit)
await simulateAndroidBackButton(tester);
await tester.pumpAndSettle();

// Should have switched back to Tab A
expect(find.byKey(const ValueKey<String>('tabA')), findsOneWidget);
expect(tabBPopCount, 1);

// 4. Switch to Tab B again (Second Visit)
router.go('/tabB');
await tester.pumpAndSettle();
expect(find.byKey(const ValueKey<String>('tabB')), findsOneWidget);

// 5. Press back button on Tab B (Second Visit)
await simulateAndroidBackButton(tester);
await tester.pumpAndSettle();

// Verify that the PopScope was triggered again
expect(tabBPopCount, 2);

// Should have switched back to Tab A again
expect(find.byKey(const ValueKey<String>('tabA')), findsOneWidget);
},
);

testWidgets(
'StatefulShellRoute switches to root branch by default on back button',
(WidgetTester tester) async {
final routes = <RouteBase>[
StatefulShellRoute.indexedStack(
builder:
(
BuildContext context,
GoRouterState state,
StatefulNavigationShell navigationShell,
) {
return Scaffold(
body: navigationShell,
bottomNavigationBar: BottomNavigationBar(
currentIndex: navigationShell.currentIndex,
onTap: (int index) => navigationShell.goBranch(index),
items: const <BottomNavigationBarItem>[
BottomNavigationBarItem(
icon: Icon(Icons.home),
label: 'A',
),
BottomNavigationBarItem(
icon: Icon(Icons.business),
label: 'B',
),
],
),
);
},
branches: <StatefulShellBranch>[
StatefulShellBranch(
routes: <RouteBase>[
GoRoute(
path: '/tabA',
builder: (BuildContext context, GoRouterState state) =>
const DummyScreen(key: ValueKey<String>('tabA')),
),
],
),
StatefulShellBranch(
routes: <RouteBase>[
GoRoute(
path: '/tabB',
builder: (BuildContext context, GoRouterState state) =>
const DummyScreen(key: ValueKey<String>('tabB')),
),
],
),
],
),
];

final GoRouter router = await createRouter(
routes,
tester,
initialLocation: '/tabA',
);

expect(find.byKey(const ValueKey<String>('tabA')), findsOneWidget);

router.go('/tabB');
await tester.pumpAndSettle();
expect(find.byKey(const ValueKey<String>('tabB')), findsOneWidget);

await simulateAndroidBackButton(tester);
await tester.pumpAndSettle();

expect(find.byKey(const ValueKey<String>('tabA')), findsOneWidget);
},
);
}
Loading