From 7f7ef8a44fb2d583d3c176a5e455834fb8688831 Mon Sep 17 00:00:00 2001 From: Omar Hanafy Date: Fri, 8 Nov 2024 21:38:41 +0200 Subject: [PATCH] [go_router] resolved issue #140869 Fixed WillPopScope/PopScope doesn't trigger with back button navigation on root screens. Added a test for PopScope, and fixed some test failures in the delegate_test. --- packages/go_router/CHANGELOG.md | 4 ++ packages/go_router/lib/src/delegate.dart | 52 +++++++++++++--------- packages/go_router/pubspec.yaml | 2 +- packages/go_router/test/delegate_test.dart | 38 ++++++++++++++++ 4 files changed, 74 insertions(+), 22 deletions(-) diff --git a/packages/go_router/CHANGELOG.md b/packages/go_router/CHANGELOG.md index 78abc879a9e9..35fd6f02fc06 100644 --- a/packages/go_router/CHANGELOG.md +++ b/packages/go_router/CHANGELOG.md @@ -1,3 +1,7 @@ +## 14.5.0 + +- Fixed `PopScope`, and `WillPopScop` was not handled properly in the Root routes. + ## 14.4.1 - Adds `missing_code_block_language_in_doc_comment` lint. diff --git a/packages/go_router/lib/src/delegate.dart b/packages/go_router/lib/src/delegate.dart index 94496657ed78..56326e4faf78 100644 --- a/packages/go_router/lib/src/delegate.dart +++ b/packages/go_router/lib/src/delegate.dart @@ -57,9 +57,13 @@ class GoRouterDelegate extends RouterDelegate Future popRoute() async { final NavigatorState? state = _findCurrentNavigator(); if (state != null) { - return state.maybePop(); + final bool didPop = await state.maybePop(); // Call maybePop() directly + if (didPop) { + return true; // Return true if maybePop handled the pop + } } - // This should be the only place where the last GoRoute exit the screen. + + // Fallback to onExit if maybePop did not handle the pop final GoRoute lastRoute = currentConfiguration.last.route; if (lastRoute.onExit != null && navigatorKey.currentContext != null) { return !(await lastRoute.onExit!( @@ -68,6 +72,7 @@ class GoRouterDelegate extends RouterDelegate .buildState(_configuration, currentConfiguration), )); } + return false; } @@ -89,7 +94,7 @@ class GoRouterDelegate extends RouterDelegate /// Pops the top-most route. void pop([T? result]) { final NavigatorState? state = _findCurrentNavigator(); - if (state == null) { + if (state == null || !state.canPop()) { throw GoError('There is nothing to pop'); } state.pop(result); @@ -97,18 +102,21 @@ class GoRouterDelegate extends RouterDelegate NavigatorState? _findCurrentNavigator() { NavigatorState? state; - if (navigatorKey.currentState?.canPop() ?? false) { - state = navigatorKey.currentState; - } + state = + navigatorKey.currentState; // Set state directly without canPop check + RouteMatchBase walker = currentConfiguration.matches.last; while (walker is ShellRouteMatch) { final NavigatorState potentialCandidate = walker.navigatorKey.currentState!; - if (!ModalRoute.of(potentialCandidate.context)!.isCurrent) { - // There is a pageless route on top of the shell route. it needs to be - // popped first. + + final ModalRoute? modalRoute = + ModalRoute.of(potentialCandidate.context); + if (modalRoute == null || !modalRoute.isCurrent) { + // Stop if there is a pageless route on top of the shell route. break; } + if (potentialCandidate.canPop()) { state = walker.navigatorKey.currentState; } @@ -117,14 +125,6 @@ class GoRouterDelegate extends RouterDelegate return state; } - void _debugAssertMatchListNotEmpty() { - assert( - currentConfiguration.isNotEmpty, - 'You have popped the last page off of the stack,' - ' there are no pages left to show', - ); - } - bool _handlePopPageWithRouteMatch( Route route, Object? result, RouteMatchBase match) { if (route.willHandlePopInternally) { @@ -154,6 +154,14 @@ class GoRouterDelegate extends RouterDelegate return false; } + void _debugAssertMatchListNotEmpty() { + assert( + currentConfiguration.isNotEmpty, + 'You have popped the last page off of the stack,' + ' there are no pages left to show', + ); + } + void _completeRouteMatch(Object? result, RouteMatchBase match) { RouteMatchBase walker = match; while (walker is ShellRouteMatch) { @@ -162,12 +170,14 @@ class GoRouterDelegate extends RouterDelegate if (walker is ImperativeRouteMatch) { walker.complete(result); } + + // Unconditionally remove the match from the current configuration currentConfiguration = currentConfiguration.remove(match); + notifyListeners(); - assert(() { - _debugAssertMatchListNotEmpty(); - return true; - }()); + + // Ensure the configuration is not empty + _debugAssertMatchListNotEmpty(); } /// The top [GoRouterState], the state of the route that was diff --git a/packages/go_router/pubspec.yaml b/packages/go_router/pubspec.yaml index 7bfc3f9297f6..94c1899e3df0 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: 14.4.1 +version: 14.5.0 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 diff --git a/packages/go_router/test/delegate_test.dart b/packages/go_router/test/delegate_test.dart index 6513a1c9665f..75db026fca61 100644 --- a/packages/go_router/test/delegate_test.dart +++ b/packages/go_router/test/delegate_test.dart @@ -92,6 +92,44 @@ void main() { false); }); + testWidgets('PopScope intercepts back button on root route', + (WidgetTester tester) async { + bool didPop = false; + + final GoRouter goRouter = GoRouter( + initialLocation: '/', + routes: [ + GoRoute( + path: '/', + builder: (_, __) => PopScope( + onPopInvokedWithResult: (bool result, _) { + didPop = true; + }, + canPop: false, + child: const Text('Home'), + ), + ), + ], + ); + + addTearDown(goRouter.dispose); + + await tester.pumpWidget(MaterialApp.router( + routerConfig: goRouter, + )); + + expect(find.text('Home'), findsOneWidget); + + // Simulate back button press + await tester.binding.handlePopRoute(); + + await tester.pumpAndSettle(); + + // Verify that PopScope intercepted the back button + expect(didPop, isTrue); + expect(find.text('Home'), findsOneWidget); + }); + testWidgets('pops more than matches count should return false', (WidgetTester tester) async { final GoRouter goRouter = await createGoRouter(tester)