diff --git a/packages/flutter/lib/src/widgets/navigator.dart b/packages/flutter/lib/src/widgets/navigator.dart index 1453c92d766..65c199c13e4 100644 --- a/packages/flutter/lib/src/widgets/navigator.dart +++ b/packages/flutter/lib/src/widgets/navigator.dart @@ -3163,6 +3163,10 @@ class _RouteEntry extends RouteTransitionRecord { _RoutePlaceholder? lastAnnouncedNextRoute = notAnnounced; // last argument to Route.didChangeNext int? lastFocusNode; // The last focused semantic node for the route entry. + // Whether this route is removed without using a Navigator.pages api. + // For example, Navigator.pop or Navigator.pushReplacement. + bool imperativeRemoval = false; + /// Restoration ID to be used for the encapsulating route when restoration is /// enabled for it or null if restoration cannot be enabled for it. String? get restorationId { @@ -3302,10 +3306,6 @@ class _RouteEntry extends RouteTransitionRecord { return false; } route.onPopInvokedWithResult(true, pendingResult); - if (pageBased) { - final Page page = route.settings as Page; - navigator.widget.onDidRemovePage?.call(page); - } pendingResult = null; return true; } @@ -3322,8 +3322,14 @@ class _RouteEntry extends RouteTransitionRecord { required Route? previousPresent, }) { assert(navigator._debugLocked); - assert(route._navigator == navigator); - currentState = _RouteLifecycle.removing; + if (route._navigator == navigator) { + currentState = _RouteLifecycle.removing; + } else { + // This route is still waiting to be added while a top-most push or pop + // animation is still on-going. In this case, this route can be disposed + // directly since nothing has been initialized yet. + currentState = _RouteLifecycle.dispose; + } if (_reportRemovalToObserver) { navigator._observedRouteDeletions.add(_NavigatorRemoveObservation(route, previousPresent)); } @@ -3343,21 +3349,17 @@ class _RouteEntry extends RouteTransitionRecord { Object? pendingResult; - void pop(T? result) { + void pop(T? result, {required bool imperativeRemoval}) { assert(isPresent); pendingResult = result; currentState = _RouteLifecycle.pop; + this.imperativeRemoval = imperativeRemoval; } bool _reportRemovalToObserver = true; // Route completes with `result` and is removed. - void complete(T result, {bool isReplaced = false}) { - assert( - !pageBased || isWaitingForExitingDecision, - 'A page-based route cannot be completed using imperative api, provide a ' - 'new list without the corresponding Page to Navigator.pages instead. ', - ); + void complete(T result, {required bool isReplaced, required bool imperativeRemoval}) { if (currentState.index >= _RouteLifecycle.remove.index) { return; } @@ -3365,6 +3367,7 @@ class _RouteEntry extends RouteTransitionRecord { _reportRemovalToObserver = !isReplaced; pendingResult = result; currentState = _RouteLifecycle.complete; + this.imperativeRemoval = imperativeRemoval; } void finalize() { @@ -3529,7 +3532,7 @@ class _RouteEntry extends RouteTransitionRecord { final bool popResult = route.didPop(result); assert(!popResult); } - pop(result); + pop(result, imperativeRemoval: false); _isWaitingForExitingDecision = false; } @@ -3541,7 +3544,7 @@ class _RouteEntry extends RouteTransitionRecord { 'been made or it does not require an explicit decision on how to transition ' 'out.', ); - complete(result); + complete(result, isReplaced: false, imperativeRemoval: false); _isWaitingForExitingDecision = false; } @@ -4491,21 +4494,21 @@ class NavigatorState extends State with TickerProviderStateMixin, Res navigator: this, previousPresent: _getRouteBefore(index, _RouteEntry.willBePresentPredicate)?.route, ); - assert(entry.currentState == _RouteLifecycle.removing); + assert(entry.currentState.index >= _RouteLifecycle.removing.index); continue; case _RouteLifecycle.removing: if (!canRemoveOrAdd && next != null) { // We aren't allowed to remove this route yet. break; } - if (entry.pageBased) { - widget.onDidRemovePage?.call(entry.route.settings as Page); - } entry.currentState = _RouteLifecycle.dispose; continue; case _RouteLifecycle.dispose: // Delay disposal until didChangeNext/didChangePrevious have been sent. toBeDisposed.add(_history.removeAt(index)); + if (entry.pageBased && entry.imperativeRemoval) { + widget.onDidRemovePage?.call(entry.route.settings as Page); + } entry = next; case _RouteLifecycle.disposing: case _RouteLifecycle.disposed: @@ -5185,7 +5188,9 @@ class NavigatorState extends State with TickerProviderStateMixin, Res 'Navigator has no active routes to replace.', ); assert(entry.currentState == _RouteLifecycle.pushReplace); - _history.lastWhere(_RouteEntry.isPresentPredicate).complete(result, isReplaced: true); + _history + .lastWhere(_RouteEntry.isPresentPredicate) + .complete(result, isReplaced: true, imperativeRemoval: true); _history.add(entry); _flushHistoryUpdates(); assert(() { @@ -5284,7 +5289,7 @@ class NavigatorState extends State with TickerProviderStateMixin, Res _history.add(entry); while (index >= 0 && !predicate(_history[index].route)) { if (_history[index].isPresent) { - _history[index].complete(null); + _history[index].complete(null, isReplaced: false, imperativeRemoval: true); } index -= 1; } @@ -5369,7 +5374,7 @@ class NavigatorState extends State with TickerProviderStateMixin, Res ); final bool wasCurrent = oldRoute.isCurrent; _history.insert(index + 1, entry); - _history[index].complete(null, isReplaced: true); + _history[index].complete(null, isReplaced: true, imperativeRemoval: true); _flushHistoryUpdates(); assert(() { _debugLocked = false; @@ -5459,7 +5464,7 @@ class NavigatorState extends State with TickerProviderStateMixin, Res } assert(index >= 0, 'There are no routes below the specified anchorRoute.'); _history.insert(index + 1, entry); - _history[index].complete(null, isReplaced: true); + _history[index].complete(null, isReplaced: true, imperativeRemoval: true); _flushHistoryUpdates(); assert(() { _debugLocked = false; @@ -5582,7 +5587,7 @@ class NavigatorState extends State with TickerProviderStateMixin, Res entry.route.onPopInvokedWithResult(true, result); } } else { - entry.pop(result); + entry.pop(result, imperativeRemoval: true); assert(entry.currentState == _RouteLifecycle.pop); } if (entry.currentState == _RouteLifecycle.pop) { @@ -5634,7 +5639,7 @@ class NavigatorState extends State with TickerProviderStateMixin, Res assert(route._navigator == this); final bool wasCurrent = route.isCurrent; final _RouteEntry entry = _history.firstWhere(_RouteEntry.isRoutePredicate(route)); - entry.complete(result); + entry.complete(result, isReplaced: false, imperativeRemoval: true); _flushHistoryUpdates(rearrangeOverlay: false); assert(() { _debugLocked = false; @@ -5671,7 +5676,7 @@ class NavigatorState extends State with TickerProviderStateMixin, Res index -= 1; } assert(index >= 0, 'There are no routes below the specified anchorRoute.'); - _history[index].complete(result); + _history[index].complete(result, isReplaced: false, imperativeRemoval: true); _flushHistoryUpdates(rearrangeOverlay: false); assert(() { _debugLocked = false; diff --git a/packages/flutter/test/widgets/navigator_on_did_remove_page_test.dart b/packages/flutter/test/widgets/navigator_on_did_remove_page_test.dart new file mode 100644 index 00000000000..2e20adb5acc --- /dev/null +++ b/packages/flutter/test/widgets/navigator_on_did_remove_page_test.dart @@ -0,0 +1,127 @@ +// Copyright 2014 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; + +void main() { + Future buildPages( + List> pages, + WidgetTester tester, { + GlobalKey? navKey, + required List> removedPage, + }) { + return tester.pumpWidget( + MediaQuery( + data: MediaQueryData.fromView(tester.view), + child: Directionality( + textDirection: TextDirection.ltr, + child: Navigator(key: navKey, pages: pages, onDidRemovePage: removedPage.add), + ), + ), + ); + } + + testWidgets('Page API will not call onDidRemovePage', (WidgetTester tester) async { + final List> removedPages = >[]; + + const MaterialPage page = MaterialPage( + key: ValueKey('page'), + child: Text('page'), + ); + const MaterialPage page1 = MaterialPage( + key: ValueKey('page1'), + child: Text('page1'), + ); + const MaterialPage page2 = MaterialPage( + key: ValueKey('page2'), + child: Text('page2'), + ); + const MaterialPage page3 = MaterialPage( + key: ValueKey('page3'), + child: Text('page3'), + ); + const MaterialPage page4 = MaterialPage( + key: ValueKey('page4'), + child: Text('page4'), + ); + const MaterialPage page5 = MaterialPage( + key: ValueKey('page5'), + child: Text('page5'), + ); + const MaterialPage page6 = MaterialPage( + key: ValueKey('page6'), + child: Text('page6'), + ); + await buildPages(>[page], tester, removedPage: removedPages); + + expect(find.text('page'), findsOneWidget); + + await buildPages(>[page, page1, page2, page3], tester, removedPage: removedPages); + await buildPages(>[page, page4, page5, page6], tester, removedPage: removedPages); + await tester.pumpAndSettle(); + expect(find.text('page6'), findsOneWidget); + expect(removedPages, isEmpty); + + await buildPages(>[page, page4, page5], tester, removedPage: removedPages); + await tester.pumpAndSettle(); + expect(find.text('page5'), findsOneWidget); + expect(removedPages, isEmpty); + + await buildPages(>[page], tester, removedPage: removedPages); + await tester.pumpAndSettle(); + expect(find.text('page'), findsOneWidget); + expect(removedPages, isEmpty); + }); + + testWidgets('pop calls onDidRemovePage', (WidgetTester tester) async { + final GlobalKey key = GlobalKey(); + final List> removedPage = >[]; + + const MaterialPage page = MaterialPage( + key: ValueKey('page'), + child: Text('page'), + ); + const MaterialPage page1 = MaterialPage( + key: ValueKey('page1'), + child: Text('page1'), + ); + await buildPages(>[page, page1], tester, removedPage: removedPage, navKey: key); + + expect(find.text('page1'), findsOneWidget); + + key.currentState!.pop(); + await tester.pumpAndSettle(); + expect(find.text('page'), findsOneWidget); + expect(removedPage, >[page1]); + }); + + testWidgets('pushReplacement calls onDidRemovePage', (WidgetTester tester) async { + final GlobalKey key = GlobalKey(); + final List> removedPage = >[]; + + const MaterialPage page = MaterialPage( + key: ValueKey('page'), + child: Text('page'), + ); + const MaterialPage page1 = MaterialPage( + key: ValueKey('page1'), + child: Text('page1'), + ); + await buildPages(>[page, page1], tester, removedPage: removedPage, navKey: key); + + expect(find.text('page1'), findsOneWidget); + + key.currentState!.pushReplacement( + MaterialPageRoute( + builder: (_) { + return const Text('new page'); + }, + ), + ); + await tester.pumpAndSettle(); + expect(find.text('new page'), findsOneWidget); + expect(removedPage, >[page1]); + }); +} diff --git a/packages/flutter/test/widgets/navigator_test.dart b/packages/flutter/test/widgets/navigator_test.dart index b69d131040d..067a81335a1 100644 --- a/packages/flutter/test/widgets/navigator_test.dart +++ b/packages/flutter/test/widgets/navigator_test.dart @@ -250,40 +250,91 @@ void main() { expect(tester.widget(find.byType(Overlay)).clipBehavior, Clip.none); }); - testWidgets('Navigator should call observer.didPop when pops page-based routes', ( - WidgetTester tester, - ) async { + testWidgets('Navigator can set clip behavior', (WidgetTester tester) async { const MaterialPage page = MaterialPage(child: Text('page')); - const MaterialPage page1 = MaterialPage(child: Text('page1')); - final List> pages = >[page, page1]; - final _MockNavigatorObserver observer = _MockNavigatorObserver(); - final GlobalKey nav = GlobalKey(); + await tester.pumpWidget( + MediaQuery( + data: MediaQueryData.fromView(tester.view), + child: Directionality( + textDirection: TextDirection.ltr, + child: Navigator(pages: const >[page], onPopPage: (_, _) => false), + ), + ), + ); + // Default to hard edge. + expect(tester.widget(find.byType(Overlay)).clipBehavior, Clip.hardEdge); + await tester.pumpWidget( MediaQuery( data: MediaQueryData.fromView(tester.view), child: Directionality( textDirection: TextDirection.ltr, child: Navigator( - key: nav, - pages: pages, - observers: [observer], - onPopPage: (Route route, dynamic result) { - route.didPop(result); - pages.remove(route.settings); - return true; - }, + pages: const >[page], + clipBehavior: Clip.none, + onPopPage: (_, _) => false, ), ), ), ); + expect(tester.widget(find.byType(Overlay)).clipBehavior, Clip.none); + }); - expect(find.text('page1'), findsOneWidget); + testWidgets('Can remove page before they are added', (WidgetTester tester) async { + final GlobalKey key = GlobalKey(); + Future buildPages(List> pages) { + return tester.pumpWidget( + MediaQuery( + data: MediaQueryData.fromView(tester.view), + child: Directionality( + textDirection: TextDirection.ltr, + child: Navigator(key: key, pages: pages, onDidRemovePage: (_) {}), + ), + ), + ); + } + + const MaterialPage page = MaterialPage( + key: ValueKey('page'), + child: Text('page'), + ); + const MaterialPage page1 = MaterialPage( + key: ValueKey('page1'), + child: Text('page1'), + ); + const MaterialPage page2 = MaterialPage( + key: ValueKey('page2'), + child: Text('page2'), + ); + const MaterialPage page3 = MaterialPage( + key: ValueKey('page3'), + child: Text('page3'), + ); + const MaterialPage page4 = MaterialPage( + key: ValueKey('page4'), + child: Text('page4'), + ); + const MaterialPage page5 = MaterialPage( + key: ValueKey('page5'), + child: Text('page5'), + ); + const MaterialPage page6 = MaterialPage( + key: ValueKey('page6'), + child: Text('page6'), + ); + await buildPages(>[page]); - observer._invocations.clear(); - nav.currentState!.pop(); - await tester.pumpAndSettle(); expect(find.text('page'), findsOneWidget); - observer._checkInvocations(const [#didPop, #didChangeTop]); + + await buildPages(>[page, page1, page2, page3]); + + // In mid transition; + await tester.pump(const Duration(microseconds: 150)); + // Cause page1 and page2 to be removed before they are added. + await buildPages(>[page, page4, page5, page6]); + await tester.pumpAndSettle(); + expect(find.text('page6'), findsOneWidget); + expect(tester.takeException(), isNull); }); testWidgets('Zero transition page-based route correctly notifies observers when it is popped', (