From 210f6f1f91846e079147ad885290846209d0551f Mon Sep 17 00:00:00 2001 From: swi-oberhauser Date: Wed, 14 May 2025 11:17:01 +0200 Subject: [PATCH 1/6] add tests --- packages/go_router/test/delegate_test.dart | 136 +++++++++++++++++++++ 1 file changed, 136 insertions(+) diff --git a/packages/go_router/test/delegate_test.dart b/packages/go_router/test/delegate_test.dart index ad628610fd1..dd74b059bd2 100644 --- a/packages/go_router/test/delegate_test.dart +++ b/packages/go_router/test/delegate_test.dart @@ -76,6 +76,57 @@ Future createGoRouterWithStatefulShellRoute( return router; } +Future createGoRouterWithStatefulShellRouteAndPopScopes( + WidgetTester tester, { + bool canPopShellRouteBuilder = true, + bool canPopBranch = true, + bool canPopBranchSubRoute = true, + PopInvokedWithResultCallback? onPopShellRouteBuilder, + PopInvokedWithResultCallback? onPopBranch, + PopInvokedWithResultCallback? onPopBranchSubRoute, +}) async { + final GoRouter router = GoRouter( + initialLocation: '/c', + routes: [ + StatefulShellRoute.indexedStack( + branches: [ + StatefulShellBranch(routes: [ + GoRoute( + path: '/c', + builder: (_, __) => PopScope( + onPopInvokedWithResult: onPopBranch, + canPop: canPopBranch, + child: const Text('Home')), + routes: [ + 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('removes the last element', (WidgetTester tester) async { @@ -130,6 +181,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) From 2d55e2b2aabf4998f3f805e2c624ec84ffdbef4f Mon Sep 17 00:00:00 2001 From: swi-oberhauser Date: Tue, 13 May 2025 17:10:26 +0200 Subject: [PATCH 2/6] fix: PopScope.onPopInvokedWithResult not called in branch routes --- packages/go_router/lib/src/delegate.dart | 34 ++++++++++++++---------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/packages/go_router/lib/src/delegate.dart b/packages/go_router/lib/src/delegate.dart index aa95fdb39eb..a4eaf94e1df 100644 --- a/packages/go_router/lib/src/delegate.dart +++ b/packages/go_router/lib/src/delegate.dart @@ -55,8 +55,8 @@ class GoRouterDelegate extends RouterDelegate @override Future popRoute() async { - final NavigatorState? state = _findCurrentNavigator(); - if (state != null) { + final List 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,25 @@ class GoRouterDelegate extends RouterDelegate /// Pops the top-most route. void pop([T? result]) { - final NavigatorState? state = _findCurrentNavigator(); - if (state == null || !state.canPop()) { + final Iterable 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 + /// 1. Pop routes within branches of shell navigation which `canPop` + /// 2. Pop parent route + /// 3. Pop branch routes (which are exit routes as they cannot be popped) + List _findCurrentNavigators() { + final List states = []; + if (navigatorKey.currentState != null) { + states.add(navigatorKey + .currentState!); // Set state directly without canPop check + } RouteMatchBase walker = currentConfiguration.matches.last; while (walker is ShellRouteMatch) { @@ -119,13 +127,11 @@ class GoRouterDelegate extends RouterDelegate // Stop if there is a pageless route on top of the shell route. break; } - - if (potentialCandidate.canPop()) { - state = walker.navigatorKey.currentState; - } + states.insert( + potentialCandidate.canPop() ? 0 : states.length, potentialCandidate); walker = walker.matches.last; } - return state; + return states; } bool _handlePopPageWithRouteMatch( From 7dbe8f660c8cb57359f1812ef57a3297c1f44fd1 Mon Sep 17 00:00:00 2001 From: swi-oberhauser Date: Wed, 14 May 2025 11:25:20 +0200 Subject: [PATCH 3/6] alter structure --- packages/go_router/lib/src/delegate.dart | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/packages/go_router/lib/src/delegate.dart b/packages/go_router/lib/src/delegate.dart index a4eaf94e1df..f02a4605eb1 100644 --- a/packages/go_router/lib/src/delegate.dart +++ b/packages/go_router/lib/src/delegate.dart @@ -105,15 +105,17 @@ class GoRouterDelegate extends RouterDelegate states.first.pop(result); } - /// Get a prioritized list of NavigatorStates - /// 1. Pop routes within branches of shell navigation which `canPop` - /// 2. Pop parent route - /// 3. Pop branch routes (which are exit routes as they cannot be popped) + /// 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 List _findCurrentNavigators() { final List states = []; if (navigatorKey.currentState != null) { - states.add(navigatorKey - .currentState!); // Set state directly without canPop check + // Set state directly without canPop check + states.add(navigatorKey.currentState!); } RouteMatchBase walker = currentConfiguration.matches.last; @@ -127,8 +129,7 @@ class GoRouterDelegate extends RouterDelegate // Stop if there is a pageless route on top of the shell route. break; } - states.insert( - potentialCandidate.canPop() ? 0 : states.length, potentialCandidate); + states.insert(0, potentialCandidate); walker = walker.matches.last; } return states; From f2b12ec4db6fe3b7994d7a4d7c0a384dfc5ff752 Mon Sep 17 00:00:00 2001 From: swi-oberhauser Date: Wed, 14 May 2025 11:29:19 +0200 Subject: [PATCH 4/6] changelog --- packages/go_router/CHANGELOG.md | 4 ++++ packages/go_router/pubspec.yaml | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/go_router/CHANGELOG.md b/packages/go_router/CHANGELOG.md index a8a2c2d3944..f66330cd92a 100644 --- a/packages/go_router/CHANGELOG.md +++ b/packages/go_router/CHANGELOG.md @@ -1,3 +1,7 @@ +## 15.1.3 + +- Fixes calling `PopScope.onPopInvokedWithResult` in branch routes. + ## 15.1.2 - Fixes focus request propagation from `GoRouter` to `Navigator` by properly handling the `requestFocus` parameter. diff --git a/packages/go_router/pubspec.yaml b/packages/go_router/pubspec.yaml index dcf98a3d983..11b415b4d31 100644 --- a/packages/go_router/pubspec.yaml +++ b/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.1.2 +version: 15.1.3 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 From 231d4e5628574ba90668a057afb1b2fc5a1f85dd Mon Sep 17 00:00:00 2001 From: swi-oberhauser Date: Mon, 19 May 2025 12:26:15 +0200 Subject: [PATCH 5/6] use reversed list --- packages/go_router/lib/src/delegate.dart | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/go_router/lib/src/delegate.dart b/packages/go_router/lib/src/delegate.dart index f02a4605eb1..f39643e2731 100644 --- a/packages/go_router/lib/src/delegate.dart +++ b/packages/go_router/lib/src/delegate.dart @@ -129,10 +129,10 @@ class GoRouterDelegate extends RouterDelegate // Stop if there is a pageless route on top of the shell route. break; } - states.insert(0, potentialCandidate); + states.add(potentialCandidate); walker = walker.matches.last; } - return states; + return states.reversed.toList(growable: false); } bool _handlePopPageWithRouteMatch( From 476b9121eaf9eb5e2ed1b57b059e2a8236817be9 Mon Sep 17 00:00:00 2001 From: swi-oberhauser Date: Mon, 19 May 2025 12:30:30 +0200 Subject: [PATCH 6/6] use Iterable as return type --- packages/go_router/lib/src/delegate.dart | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/go_router/lib/src/delegate.dart b/packages/go_router/lib/src/delegate.dart index f39643e2731..3598a2237f2 100644 --- a/packages/go_router/lib/src/delegate.dart +++ b/packages/go_router/lib/src/delegate.dart @@ -55,7 +55,7 @@ class GoRouterDelegate extends RouterDelegate @override Future popRoute() async { - final List states = _findCurrentNavigators(); + final Iterable states = _findCurrentNavigators(); for (final NavigatorState state in states) { final bool didPop = await state.maybePop(); // Call maybePop() directly if (didPop) { @@ -111,7 +111,7 @@ class GoRouterDelegate extends RouterDelegate /// 1. Sub route within branches of shell navigation /// 2. Branch route /// 3. Parent route - List _findCurrentNavigators() { + Iterable _findCurrentNavigators() { final List states = []; if (navigatorKey.currentState != null) { // Set state directly without canPop check @@ -132,7 +132,7 @@ class GoRouterDelegate extends RouterDelegate states.add(potentialCandidate); walker = walker.matches.last; } - return states.reversed.toList(growable: false); + return states.reversed; } bool _handlePopPageWithRouteMatch(