Pop fix (tomgilder#218)

integration_test_app/integration_test/navigation_test_shared.dart

@@ -434,6 +434,55 @@ void replaceTests({required void Function(String) expectUrl}) {
     expect(find.byType(PageTwo), findsOneWidget);
   });
 
+  testWidgets(
+      'Navigator pop then forward works correctly via delegate popUntil',
+      (tester) async {
+    final app = MyApp();
+    runApp(app);
+    await tester.pumpAndSettle();
+
+    final history = app.delegate.history;
+
+    // Push: / -> /one
+    await tester.tap(find.text('Push page one'));
+    await tester.pump();
+    await tester.pumpAndSettle();
+    expect(find.byType(PageOne), findsOneWidget);
+
+    // Push: /one -> /one/two
+    await tester.tap(find.text('Push /one/two'));
+    await tester.pump();
+    await tester.pumpAndSettle();
+    expectUrl('/one/two');
+    expect(find.byType(PageTwo), findsOneWidget);
+
+    // Pop with delegate: /one/two -> /one
+    app.delegate.popUntil((r) => r.fullPath == '/one');
+    await tester.pump();
+    await tester.pumpAndSettle();
+    expectUrl('/one');
+    expect(find.byType(PageOne), findsOneWidget);
+    expect(history.canGoBack, isTrue);
+    expect(history.canGoForward, isTrue);
+
+    // Go forward: /one -> /one/two
+    window.history.forward();
+    await tester.pump();
+    await tester.pumpAndSettle();
+    expectUrl('/one/two');
+    expect(find.byType(PageTwo), findsOneWidget);
+    expect(history.canGoBack, isTrue);
+    expect(history.canGoForward, isFalse);
+    expect(history.forward(), isFalse);
+
+    // Try to go forward again: shouldn't do anything
+    window.history.forward();
+    await tester.pump();
+    await tester.pumpAndSettle();
+    expectUrl('/one/two');
+    expect(find.byType(PageTwo), findsOneWidget);
+  });
+
   testWidgets(
       'Pop does not use history.back() if previous route not in history stack',
       (tester) async {

lib/routemaster.dart

@@ -352,21 +352,45 @@ class RoutemasterDelegate extends RouterDelegate<RouteData>
   /// An optional value can be passed to the previous route via the [result]
   /// parameter.
   @optionalTypeArgs
-  Future<bool> pop<T extends Object?>([T? result]) {
+  Future<bool> pop<T extends Object?>([T? result]) async {
     assert(!_isDisposed);
-    return _state.stack.maybePop<T>(result);
+    final popResult = await _state.stack.maybePop<T>(result);
+
+    if (popResult) {
+      _updateCurrentConfiguration(updateHistory: false);
+    }
+
+    return popResult;
   }
 
   /// Calls [pop] repeatedly whilst the [predicate] function returns true.
   ///
   /// If [predicate] immediately returns false, pop won't be called.
   Future<void> popUntil(bool Function(RouteData routeData) predicate) async {
+    var hasPopped = false;
+
+    Future<bool> doPop() async {
+      final popResult = await _state.stack.maybePop();
+      if (popResult) {
+        hasPopped = true;
+      }
+      return popResult;
+    }
+
     do {
       final currentPages = _state.stack._getCurrentPages();
       if (currentPages.isEmpty || predicate(currentPages.last.routeData)) {
+        if (hasPopped) {
+          _updateCurrentConfiguration(updateHistory: false);
+        }
+
         return;
       }
-    } while (await _state.stack.maybePop());
+    } while (await doPop());
+
+    if (hasPopped) {
+      _updateCurrentConfiguration(updateHistory: false);
+    }
   }
 
   /// Pushes [path] into the navigation tree.
@@ -453,7 +477,9 @@ class RoutemasterDelegate extends RouterDelegate<RouteData>
           state: _state,
           routeData: currentConfiguration!,
           child: navigatorBuilder != null
-              ? navigatorBuilder!(context, _state.stack)
+              ? Builder(builder: (context) {
+                  return navigatorBuilder!(context, _state.stack);
+                })
               : PageStackNavigator(
                   stack: _state.stack,
                   transitionDelegate: transitionDelegate ??
@@ -489,6 +515,7 @@ class RoutemasterDelegate extends RouterDelegate<RouteData>
     bool isBrowserHistoryNavigation = false,
     bool isReplacement = false,
     RequestSource requestSource = RequestSource.internal,
+    bool updateHistory = true,
   }) {
     final currentPages = _state.stack._getCurrentPages();
 
@@ -497,7 +524,7 @@ class RoutemasterDelegate extends RouterDelegate<RouteData>
       final routeData = pageEntry.routeData;
       final currentRouteData = _state.currentConfiguration!;
 
-      if (!isBrowserHistoryNavigation) {
+      if (!isBrowserHistoryNavigation && updateHistory) {
         _state.history._didNavigate(
           route: routeData,
           isReplacement: isReplacement,

lib/src/pages/page_stack.dart

@@ -11,24 +11,13 @@ class PageStack extends ChangeNotifier {
       return;
     }
 
-    _listenedToRoutes.forEach((route) {
-      route.removeListener(notifyListeners);
-    });
-
-    _listenedToRoutes = newPages.whereType<Listenable>().toList()
-      ..forEach((route) {
-        route.addListener(notifyListeners);
-      });
-
     __pageContainers = newPages;
     notifyListeners();
   }
 
   /// The count of how many pages this stack will generate.
   int get length => _pageContainers.length;
 
-  List<Listenable> _listenedToRoutes = [];
-
   /// A map so we can keep track of each page's route data. This can be used by
   /// users to get the current page's [RouteData] via `RouteData.of(context)`.
   Map<Page, RouteData> _routeMap = {};
@@ -105,7 +94,8 @@ class PageStack extends ChangeNotifier {
     }
   }
 
-  /// Passed to [Navigator] widgets for them to inform this stack of a pop
+  /// Passed to [Navigator] widgets for the Navigator to inform this stack when
+  /// a page is popped.
   bool onPopPage(
       Route<dynamic> route, dynamic result, Routemaster routemaster) {
     if (route.didPop(result)) {
@@ -115,7 +105,8 @@ class PageStack extends ChangeNotifier {
         newRoute: _pageContainers.last.routeData,
       );
 
-      // We don't need to notify listeners, the Navigator will rebuild itself
+      // We don't need to call notifyListeners() listeners, the Navigator will
+      // rebuild its page list automatically.
       return true;
     }
 
@@ -132,11 +123,13 @@ class PageStack extends ChangeNotifier {
     final lastRoute = _pageContainers.last;
     if (lastRoute is MultiChildPageContainer &&
         await lastRoute.maybePop(result)) {
+      notifyListeners();
       return SynchronousFuture(true);
     }
 
     // Child wasn't interested, ask the navigator
     if (await _attachedNavigator?.maybePop(result) == true) {
+      notifyListeners();
       return SynchronousFuture(true);
     }
 

lib/src/route_history.dart

@@ -121,15 +121,19 @@ class RouteHistory {
   }
 
   void _onPopPage({required RouteData newRoute}) {
-    final previousRouteMatches = _index > 0 && _history[_index - 1] == newRoute;
+    final poppingToPreviousHistoryRoute =
+        _index > 0 && _history[_index - 1] == newRoute;
 
-    if (previousRouteMatches) {
+    if (poppingToPreviousHistoryRoute) {
       if (kIsWeb && SystemNav.enabled) {
         // Use system navigation so forward button works
         SystemNav.back(); // coverage:ignore-line
       } else {
         _index--;
-        _navigate(_history[_index]);
+        _state.delegate._updateCurrentConfiguration(
+          isReplacement: true,
+          updateHistory: false,
+        );
       }
     } else {
       _state.delegate._updateCurrentConfiguration(