diff --git a/packages/flutter/lib/src/widgets/navigator.dart b/packages/flutter/lib/src/widgets/navigator.dart index 51128b27cf2..f149ea10ff3 100644 --- a/packages/flutter/lib/src/widgets/navigator.dart +++ b/packages/flutter/lib/src/widgets/navigator.dart @@ -2697,11 +2697,8 @@ class Navigator extends StatefulWidget { // \ / / // idle--+-----+ // / \ -// / +------+ -// / | | -// / | complete* -// | | / -// pop* remove* +// / \ +// pop* remove* // / \ // / removing# // popping# | @@ -2738,7 +2735,6 @@ enum _RouteLifecycle { // // routes that should be included in route announcement and should still listen to transition changes. pop, // we'll want to call didPop - complete, // we'll want to call didComplete, remove, // we'll want to run didReplace/didRemove etc // routes should not be included in route announcement but should still listen to transition changes. popping, // we're waiting for the route to call finalizeRoute to switch to dispose @@ -2874,38 +2870,14 @@ class _RouteEntry extends RouteTransitionRecord { lastAnnouncedPoppedNextRoute = poppedRoute; } - /// Process the to-be-popped route. - /// - /// A route can be marked for pop by transition delegate or Navigator.pop, - /// this method actually pops the route by calling Route.didPop. - /// - /// Returns true if the route is popped; otherwise, returns false if the route - /// refuses to be popped. - bool handlePop({ required NavigatorState navigator, required Route? previousPresent }) { + void handlePop({ required NavigatorState navigator, required Route? previousPresent }) { assert(navigator != null); assert(navigator._debugLocked); assert(route._navigator == navigator); currentState = _RouteLifecycle.popping; - if (route._popCompleter.isCompleted) { - // This is a page-based route popped through the Navigator.pop. The - // didPop should have been called. No further action is needed. - assert(hasPage); - assert(pendingResult == null); - return true; - } - if (!route.didPop(pendingResult)) { - currentState = _RouteLifecycle.idle; - return false; - } - pendingResult = null; - return true; - } - - void handleComplete() { - route.didComplete(pendingResult); - pendingResult = null; - assert(route._popCompleter.isCompleted); // implies didComplete was called - currentState = _RouteLifecycle.remove; + navigator._observedRouteDeletions.add( + _NavigatorPopObservation(route, previousPresent), + ); } void handleRemoval({ required NavigatorState navigator, required Route? previousPresent }) { @@ -2920,6 +2892,8 @@ class _RouteEntry extends RouteTransitionRecord { } } + bool doingPop = false; + void didAdd({ required NavigatorState navigator, required bool isNewFirst}) { route.didAdd(); currentState = _RouteLifecycle.idle; @@ -2928,12 +2902,13 @@ class _RouteEntry extends RouteTransitionRecord { } } - Object? pendingResult; - void pop(T? result) { assert(isPresent); - pendingResult = result; - currentState = _RouteLifecycle.pop; + doingPop = true; + if (route.didPop(result) && doingPop) { + currentState = _RouteLifecycle.pop; + } + doingPop = false; } bool _reportRemovalToObserver = true; @@ -2963,8 +2938,9 @@ class _RouteEntry extends RouteTransitionRecord { return; assert(isPresent); _reportRemovalToObserver = !isReplaced; - pendingResult = result; - currentState = _RouteLifecycle.complete; + route.didComplete(result); + assert(route._popCompleter.isCompleted); // implies didComplete was called + currentState = _RouteLifecycle.remove; } void finalize() { @@ -3787,11 +3763,8 @@ class NavigatorState extends State with TickerProviderStateMixin, Res assert(() { _debugLocked = false; return true; }()); } - bool _flushingHistory = false; - void _flushHistoryUpdates({bool rearrangeOverlay = true}) { assert(_debugLocked && !_debugUpdatingPage); - _flushingHistory = true; // Clean up the list, sending updates to the routes that changed. Notably, // we don't send the didChangePrevious/didChangeNext updates to those that // did not change at this point, because we're not yet sure exactly what the @@ -3855,35 +3828,21 @@ class NavigatorState extends State with TickerProviderStateMixin, Res canRemoveOrAdd = true; break; case _RouteLifecycle.pop: - if (!entry.handlePop( - navigator: this, - previousPresent: _getRouteBefore(index, _RouteEntry.willBePresentPredicate)?.route)){ - assert(entry.currentState == _RouteLifecycle.idle); - continue; - } if (!seenTopActiveRoute) { if (poppedRoute != null) entry.handleDidPopNext(poppedRoute); poppedRoute = entry.route; } - _observedRouteDeletions.add( - _NavigatorPopObservation(entry.route, _getRouteBefore(index, _RouteEntry.willBePresentPredicate)?.route), + entry.handlePop( + navigator: this, + previousPresent: _getRouteBefore(index, _RouteEntry.willBePresentPredicate)?.route, ); - if (entry.currentState == _RouteLifecycle.dispose) { - // The pop finished synchronously. This can happen if transition - // duration is zero. - continue; - } assert(entry.currentState == _RouteLifecycle.popping); canRemoveOrAdd = true; break; case _RouteLifecycle.popping: // Will exit this state when animation completes. break; - case _RouteLifecycle.complete: - entry.handleComplete(); - assert(entry.currentState == _RouteLifecycle.remove); - continue; case _RouteLifecycle.remove: if (!seenTopActiveRoute) { if (poppedRoute != null) @@ -3918,6 +3877,7 @@ class NavigatorState extends State with TickerProviderStateMixin, Res entry = previous; previous = index > 0 ? _history[index - 1] : null; } + // Informs navigator observers about route changes. _flushObserverNotifications(); @@ -3950,7 +3910,6 @@ class NavigatorState extends State with TickerProviderStateMixin, Res if (bucket != null) { _serializableHistory.update(_history); } - _flushingHistory = false; } void _flushObserverNotifications() { @@ -4887,18 +4846,17 @@ class NavigatorState extends State with TickerProviderStateMixin, Res }()); final _RouteEntry entry = _history.lastWhere(_RouteEntry.isPresentPredicate); if (entry.hasPage) { - if (widget.onPopPage!(entry.route, result) && entry.currentState == _RouteLifecycle.idle) { - // The entry may have been disposed if the pop finishes synchronously. - assert(entry.route._popCompleter.isCompleted); + if (widget.onPopPage!(entry.route, result)) entry.currentState = _RouteLifecycle.pop; - } } else { entry.pop(result); - assert (entry.currentState == _RouteLifecycle.pop); } - if (entry.currentState == _RouteLifecycle.pop) + if (entry.currentState == _RouteLifecycle.pop) { + // Flush the history if the route actually wants to be popped (the pop + // wasn't handled internally). _flushHistoryUpdates(rearrangeOverlay: false); - assert(entry.currentState == _RouteLifecycle.idle || entry.route._popCompleter.isCompleted); + assert(entry.route._popCompleter.isCompleted); + } assert(() { _debugLocked = false; return true; @@ -5014,13 +4972,15 @@ class NavigatorState extends State with TickerProviderStateMixin, Res assert(() { wasDebugLocked = _debugLocked; _debugLocked = true; return true; }()); assert(_history.where(_RouteEntry.isRoutePredicate(route)).length == 1); final _RouteEntry entry = _history.firstWhere(_RouteEntry.isRoutePredicate(route)); - assert(entry.currentState == _RouteLifecycle.popping); - entry.finalize(); - // finalizeRoute can be called during _flushHistoryUpdates if a pop - // finishes synchronously. - if (!_flushingHistory) + if (entry.doingPop) { + // We were called synchronously from Route.didPop(), but didn't process + // the pop yet. Let's do that now before finalizing. + entry.currentState = _RouteLifecycle.pop; _flushHistoryUpdates(rearrangeOverlay: false); - + } + assert(entry.currentState != _RouteLifecycle.pop); + entry.finalize(); + _flushHistoryUpdates(rearrangeOverlay: false); assert(() { _debugLocked = wasDebugLocked!; return true; }()); } diff --git a/packages/flutter/lib/src/widgets/routes.dart b/packages/flutter/lib/src/widgets/routes.dart index 28893c5260d..f019d29b17d 100644 --- a/packages/flutter/lib/src/widgets/routes.dart +++ b/packages/flutter/lib/src/widgets/routes.dart @@ -129,9 +129,7 @@ abstract class TransitionRoute extends OverlayRoute { // // This situation arises when dealing with the Cupertino dismiss gesture. @override - bool get finishedWhenPopped => _controller!.status == AnimationStatus.dismissed && !_popFinalized; - - bool _popFinalized = false; + bool get finishedWhenPopped => _controller!.status == AnimationStatus.dismissed; /// The animation that drives the route's transition and the previous route's /// forward transition. @@ -208,7 +206,6 @@ abstract class TransitionRoute extends OverlayRoute { // removing the route and disposing it. if (!isActive) { navigator!.finalizeRoute(this); - _popFinalized = true; } break; } diff --git a/packages/flutter/test/widgets/navigator_test.dart b/packages/flutter/test/widgets/navigator_test.dart index fbcf2ab5d32..0c84e287735 100644 --- a/packages/flutter/test/widgets/navigator_test.dart +++ b/packages/flutter/test/widgets/navigator_test.dart @@ -2969,32 +2969,6 @@ void main() { expect(primaryAnimationOfRouteOne.status, AnimationStatus.dismissed); }); - testWidgets('Pop no animation page does not crash', (WidgetTester tester) async { - // Regression Test for https://github.com/flutter/flutter/issues/86604. - Widget buildNavigator(bool secondPage) { - return Directionality( - textDirection: TextDirection.ltr, - child: Navigator( - pages: >[ - const ZeroDurationPage( - child: Text('page1'), - ), - if (secondPage) - const ZeroDurationPage( - child: Text('page2'), - ), - ], - onPopPage: (Route route, dynamic result) => false, - ), - ); - } - await tester.pumpWidget(buildNavigator(true)); - expect(find.text('page2'), findsOneWidget); - - await tester.pumpWidget(buildNavigator(false)); - expect(find.text('page1'), findsOneWidget); - }); - testWidgets('can work with pageless route', (WidgetTester tester) async { final GlobalKey navigator = GlobalKey(); List myPages = [ @@ -3925,9 +3899,6 @@ class NavigatorObservation { final String? previous; final String? current; final String operation; - - @override - String toString() => 'NavigatorObservation($operation, $current, $previous)'; } class BuilderPage extends Page { @@ -3943,43 +3914,3 @@ class BuilderPage extends Page { ); } } - -class ZeroDurationPage extends Page { - const ZeroDurationPage({required this.child}); - - final Widget child; - - @override - Route createRoute(BuildContext context) { - return ZeroDurationPageRoute(page: this); - } -} - -class ZeroDurationPageRoute extends PageRoute { - ZeroDurationPageRoute({required ZeroDurationPage page}) - : super(settings: page); - - @override - Duration get transitionDuration => Duration.zero; - - ZeroDurationPage get _page => settings as ZeroDurationPage; - - @override - Widget buildPage(BuildContext context, Animation animation, Animation secondaryAnimation) { - return _page.child; - } - - @override - Widget buildTransitions(BuildContext context, Animation animation, Animation secondaryAnimation, Widget child) { - return child; - } - - @override - bool get maintainState => false; - - @override - Color? get barrierColor => null; - - @override - String? get barrierLabel => null; -}