Skip to content

Commit 8dad1d6

Browse files
Gustl22Gustl22
authored
[go_router] fix: PopScope.onPopInvokedWithResult not called in branch routes (flutter#9245)
1 parent 39c588a commit 8dad1d6

4 files changed

Lines changed: 162 additions & 16 deletions

File tree

‎packages/go_router/CHANGELOG.md‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,7 @@
1+
## 15.2.2
2+
3+
- Fixes calling `PopScope.onPopInvokedWithResult` in branch routes.
4+
15
## 15.2.1
26

37
* Fixes Popping state and re-rendering scaffold at the same time doesn't update the URL on web.
@@ -11,7 +15,6 @@
1115
* Updates minimum supported SDK version to Flutter 3.27/Dart 3.6.
1216
* Fixes typo in API docs.
1317

14-
1518
## 15.1.2
1619

1720
- Fixes focus request propagation from `GoRouter` to `Navigator` by properly handling the `requestFocus` parameter.

‎packages/go_router/lib/src/delegate.dart‎

Lines changed: 21 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -55,8 +55,8 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList>
5555

5656
@override
5757
Future<bool> popRoute() async {
58-
final NavigatorState? state = _findCurrentNavigator();
59-
if (state != null) {
58+
final Iterable<NavigatorState> states = _findCurrentNavigators();
59+
for (final NavigatorState state in states) {
6060
final bool didPop = await state.maybePop(); // Call maybePop() directly
6161
if (didPop) {
6262
return true; // Return true if maybePop handled the pop
@@ -96,17 +96,27 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList>
9696

9797
/// Pops the top-most route.
9898
void pop<T extends Object?>([T? result]) {
99-
final NavigatorState? state = _findCurrentNavigator();
100-
if (state == null || !state.canPop()) {
99+
final Iterable<NavigatorState> states = _findCurrentNavigators().where(
100+
(NavigatorState element) => element.canPop(),
101+
);
102+
if (states.isEmpty) {
101103
throw GoError('There is nothing to pop');
102104
}
103-
state.pop(result);
105+
states.first.pop(result);
104106
}
105107

106-
NavigatorState? _findCurrentNavigator() {
107-
NavigatorState? state;
108-
state =
109-
navigatorKey.currentState; // Set state directly without canPop check
108+
/// Get a prioritized list of NavigatorStates,
109+
/// which either can pop or are exit routes.
110+
///
111+
/// 1. Sub route within branches of shell navigation
112+
/// 2. Branch route
113+
/// 3. Parent route
114+
Iterable<NavigatorState> _findCurrentNavigators() {
115+
final List<NavigatorState> states = <NavigatorState>[];
116+
if (navigatorKey.currentState != null) {
117+
// Set state directly without canPop check
118+
states.add(navigatorKey.currentState!);
119+
}
110120

111121
RouteMatchBase walker = currentConfiguration.matches.last;
112122
while (walker is ShellRouteMatch) {
@@ -119,13 +129,10 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList>
119129
// Stop if there is a pageless route on top of the shell route.
120130
break;
121131
}
122-
123-
if (potentialCandidate.canPop()) {
124-
state = walker.navigatorKey.currentState;
125-
}
132+
states.add(potentialCandidate);
126133
walker = walker.matches.last;
127134
}
128-
return state;
135+
return states.reversed;
129136
}
130137

131138
bool _handlePopPageWithRouteMatch(

‎packages/go_router/pubspec.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
name: go_router
22
description: A declarative router for Flutter based on Navigation 2 supporting
33
deep linking, data-driven routes and more
4-
version: 15.2.1
4+
version: 15.2.2
55
repository: https://github.com/flutter/packages/tree/main/packages/go_router
66
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+go_router%22
77

‎packages/go_router/test/delegate_test.dart‎

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,57 @@ Future<GoRouter> createGoRouterWithStatefulShellRoute(
7676
return router;
7777
}
7878

79+
Future<GoRouter> createGoRouterWithStatefulShellRouteAndPopScopes(
80+
WidgetTester tester, {
81+
bool canPopShellRouteBuilder = true,
82+
bool canPopBranch = true,
83+
bool canPopBranchSubRoute = true,
84+
PopInvokedWithResultCallback<bool>? onPopShellRouteBuilder,
85+
PopInvokedWithResultCallback<bool>? onPopBranch,
86+
PopInvokedWithResultCallback<bool>? onPopBranchSubRoute,
87+
}) async {
88+
final GoRouter router = GoRouter(
89+
initialLocation: '/c',
90+
routes: <RouteBase>[
91+
StatefulShellRoute.indexedStack(
92+
branches: <StatefulShellBranch>[
93+
StatefulShellBranch(routes: <RouteBase>[
94+
GoRoute(
95+
path: '/c',
96+
builder: (_, __) => PopScope(
97+
onPopInvokedWithResult: onPopBranch,
98+
canPop: canPopBranch,
99+
child: const Text('Home')),
100+
routes: <RouteBase>[
101+
GoRoute(
102+
path: 'c1',
103+
builder: (_, __) => PopScope(
104+
onPopInvokedWithResult: onPopBranchSubRoute,
105+
canPop: canPopBranchSubRoute,
106+
child: const Text('SubRoute'),
107+
),
108+
),
109+
]),
110+
]),
111+
],
112+
builder: (BuildContext context, GoRouterState state,
113+
StatefulNavigationShell navigationShell) =>
114+
PopScope(
115+
onPopInvokedWithResult: onPopShellRouteBuilder,
116+
canPop: canPopShellRouteBuilder,
117+
child: navigationShell,
118+
),
119+
),
120+
],
121+
);
122+
123+
addTearDown(router.dispose);
124+
await tester.pumpWidget(MaterialApp.router(
125+
routerConfig: router,
126+
));
127+
return router;
128+
}
129+
79130
void main() {
80131
group('pop', () {
81132
testWidgets('restore() update currentConfiguration in pop()',
@@ -152,6 +203,91 @@ void main() {
152203
expect(find.text('Home'), findsOneWidget);
153204
});
154205

206+
testWidgets(
207+
'PopScope intercepts back button on StatefulShellRoute builder route',
208+
(WidgetTester tester) async {
209+
bool didPopShellRouteBuilder = false;
210+
bool didPopBranch = false;
211+
bool didPopBranchSubRoute = false;
212+
213+
await createGoRouterWithStatefulShellRouteAndPopScopes(
214+
tester,
215+
canPopShellRouteBuilder: false,
216+
onPopShellRouteBuilder: (_, __) => didPopShellRouteBuilder = true,
217+
onPopBranch: (_, __) => didPopBranch = true,
218+
onPopBranchSubRoute: (_, __) => didPopBranchSubRoute = true,
219+
);
220+
221+
expect(find.text('Home'), findsOneWidget);
222+
await tester.binding.handlePopRoute();
223+
await tester.pumpAndSettle();
224+
225+
// Verify that PopScope intercepted the back button
226+
expect(didPopShellRouteBuilder, isTrue);
227+
expect(didPopBranch, isFalse);
228+
expect(didPopBranchSubRoute, isFalse);
229+
230+
expect(find.text('Home'), findsOneWidget);
231+
});
232+
233+
testWidgets(
234+
'PopScope intercepts back button on StatefulShellRoute branch route',
235+
(WidgetTester tester) async {
236+
bool didPopShellRouteBuilder = false;
237+
bool didPopBranch = false;
238+
bool didPopBranchSubRoute = false;
239+
240+
await createGoRouterWithStatefulShellRouteAndPopScopes(
241+
tester,
242+
canPopBranch: false,
243+
onPopShellRouteBuilder: (_, __) => didPopShellRouteBuilder = true,
244+
onPopBranch: (_, __) => didPopBranch = true,
245+
onPopBranchSubRoute: (_, __) => didPopBranchSubRoute = true,
246+
);
247+
248+
expect(find.text('Home'), findsOneWidget);
249+
await tester.binding.handlePopRoute();
250+
await tester.pumpAndSettle();
251+
252+
// Verify that PopScope intercepted the back button
253+
expect(didPopShellRouteBuilder, isFalse);
254+
expect(didPopBranch, isTrue);
255+
expect(didPopBranchSubRoute, isFalse);
256+
257+
expect(find.text('Home'), findsOneWidget);
258+
});
259+
260+
testWidgets(
261+
'PopScope intercepts back button on StatefulShellRoute branch sub route',
262+
(WidgetTester tester) async {
263+
bool didPopShellRouteBuilder = false;
264+
bool didPopBranch = false;
265+
bool didPopBranchSubRoute = false;
266+
267+
final GoRouter goRouter =
268+
await createGoRouterWithStatefulShellRouteAndPopScopes(
269+
tester,
270+
canPopBranchSubRoute: false,
271+
onPopShellRouteBuilder: (_, __) => didPopShellRouteBuilder = true,
272+
onPopBranch: (_, __) => didPopBranch = true,
273+
onPopBranchSubRoute: (_, __) => didPopBranchSubRoute = true,
274+
);
275+
276+
goRouter.push('/c/c1');
277+
await tester.pumpAndSettle();
278+
279+
expect(find.text('SubRoute'), findsOneWidget);
280+
await tester.binding.handlePopRoute();
281+
await tester.pumpAndSettle();
282+
283+
// Verify that PopScope intercepted the back button
284+
expect(didPopShellRouteBuilder, isFalse);
285+
expect(didPopBranch, isFalse);
286+
expect(didPopBranchSubRoute, isTrue);
287+
288+
expect(find.text('SubRoute'), findsOneWidget);
289+
});
290+
155291
testWidgets('pops more than matches count should return false',
156292
(WidgetTester tester) async {
157293
final GoRouter goRouter = await createGoRouter(tester)

0 commit comments

Comments
 (0)