Skip to content

Cleans up navigator pop and remove logic #175612

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
auto-submit merged 3 commits into from
Sep 25, 2025
Merged

Cleans up navigator pop and remove logic #175612

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
11b7c30
Cleans up navigator pop and remove logic
chunhtai Sep 18, 2025
ccd2f27
update
chunhtai Sep 23, 2025
95978f9
Merge branch 'master' into issues/170250
chunhtai Sep 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 31 additions & 26 deletions packages/flutter/lib/src/widgets/navigator.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -3302,10 +3306,6 @@ class _RouteEntry extends RouteTransitionRecord {
return false;
}
route.onPopInvokedWithResult(true, pendingResult);
if (pageBased) {
final Page<Object?> page = route.settings as Page<Object?>;
navigator.widget.onDidRemovePage?.call(page);
}
pendingResult = null;
return true;
}
Expand All @@ -3322,8 +3322,14 @@ class _RouteEntry extends RouteTransitionRecord {
required Route<dynamic>? 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

@justinmc justinmc Sep 23, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

push or pop

Since this is handleRemoval, does that mean this is always a pop? Or am I misundestanding?

@chunhtai chunhtai Sep 23, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a mechanism that when you suddenly add 2 or more route on top using page api, the top-most route will be pushed with the animation, and the routes in between are added without animation. However, to avoid suddenly jump of animation, the these routes in between will remain in adding state and wait for the top-most route finish page transition.

this line is referring the top-most route has page transition ongoing, in this case any page below that are waiting to be added will stuck in adding state and not installed yet (thus route._navigator == null). In this case if another page update triggered to remove these route, we can just dispose them directly

@justinmc justinmc Sep 23, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thank you for explaining!

// 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));
}
Expand All @@ -3343,28 +3349,25 @@ class _RouteEntry extends RouteTransitionRecord {

Object? pendingResult;

void pop<T>(T? result) {
void pop<T>(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>(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>(T result, {required bool isReplaced, required bool imperativeRemoval}) {
if (currentState.index >= _RouteLifecycle.remove.index) {
return;
}
assert(isPresent);
_reportRemovalToObserver = !isReplaced;
pendingResult = result;
currentState = _RouteLifecycle.complete;
this.imperativeRemoval = imperativeRemoval;
}

void finalize() {
Expand Down Expand Up @@ -3529,7 +3532,7 @@ class _RouteEntry extends RouteTransitionRecord {
final bool popResult = route.didPop(result);
assert(!popResult);
}
pop<dynamic>(result);
pop<dynamic>(result, imperativeRemoval: false);

@justinmc justinmc Sep 23, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So onDidRemovePage will not be called when it is triggered by a pop?

Do you think it would make sense to explain in the docs of onDidRemovePage all of the cases where it will be called?

@chunhtai chunhtai Sep 23, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

markForPop is only called when Navigator.pages api update

@justinmc justinmc Sep 23, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, I read this as a method declaration when I was reviewing the diff.

_isWaitingForExitingDecision = false;
}

Expand All @@ -3541,7 +3544,7 @@ class _RouteEntry extends RouteTransitionRecord {
'been made or it does not require an explicit decision on how to transition '
'out.',
);
complete<dynamic>(result);
complete<dynamic>(result, isReplaced: false, imperativeRemoval: false);
_isWaitingForExitingDecision = false;
}

Expand Down Expand Up @@ -4491,21 +4494,21 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin, Res
navigator: this,
previousPresent: _getRouteBefore(index, _RouteEntry.willBePresentPredicate)?.route,
);
assert(entry.currentState == _RouteLifecycle.removing);
assert(entry.currentState.index >= _RouteLifecycle.removing.index);

@justinmc justinmc Sep 23, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I worry about someone naively reordering the entries in _RouteLifecycle and breaking this. Would it be practical to assert the specific values that it can be? Or maybe just add a comment to _RouteLifecycle about the order if there isn't one already?

@chunhtai chunhtai Sep 23, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only page that are <= idle can be moved around using page api

flutter/packages/flutter/lib/src/widgets/navigator.dart

Line 3182 in 6cd85a9

if (!willBePresent) {

either way this assert is to make sure handleRemoval doesn't somehow bring the route back to idle

@justinmc justinmc Sep 23, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I see this kind of order comparison is done in many other places, sounds good.

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<Object?>);
}
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<Object?>);
}
entry = next;
case _RouteLifecycle.disposing:
case _RouteLifecycle.disposed:
Expand Down Expand Up @@ -5185,7 +5188,9 @@ class NavigatorState extends State<Navigator> 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(() {
Expand Down Expand Up @@ -5284,7 +5289,7 @@ class NavigatorState extends State<Navigator> 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;
}
Expand Down Expand Up @@ -5369,7 +5374,7 @@ class NavigatorState extends State<Navigator> 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;
Expand Down Expand Up @@ -5459,7 +5464,7 @@ class NavigatorState extends State<Navigator> 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;
Expand Down Expand Up @@ -5582,7 +5587,7 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin, Res
entry.route.onPopInvokedWithResult(true, result);
}
} else {
entry.pop<T>(result);
entry.pop<T>(result, imperativeRemoval: true);
assert(entry.currentState == _RouteLifecycle.pop);
}
if (entry.currentState == _RouteLifecycle.pop) {
Expand Down Expand Up @@ -5634,7 +5639,7 @@ class NavigatorState extends State<Navigator> 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;
Expand Down Expand Up @@ -5671,7 +5676,7 @@ class NavigatorState extends State<Navigator> 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;
Expand Down
127 changes: 127 additions & 0 deletions packages/flutter/test/widgets/navigator_on_did_remove_page_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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<void> buildPages(
List<Page<void>> pages,
WidgetTester tester, {
GlobalKey<NavigatorState>? navKey,
required List<Page<void>> 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<Page<void>> removedPages = <Page<void>>[];

const MaterialPage<void> page = MaterialPage<void>(
key: ValueKey<String>('page'),
child: Text('page'),
);
const MaterialPage<void> page1 = MaterialPage<void>(
key: ValueKey<String>('page1'),
child: Text('page1'),
);
const MaterialPage<void> page2 = MaterialPage<void>(
key: ValueKey<String>('page2'),
child: Text('page2'),
);
const MaterialPage<void> page3 = MaterialPage<void>(
key: ValueKey<String>('page3'),
child: Text('page3'),
);
const MaterialPage<void> page4 = MaterialPage<void>(
key: ValueKey<String>('page4'),
child: Text('page4'),
);
const MaterialPage<void> page5 = MaterialPage<void>(
key: ValueKey<String>('page5'),
child: Text('page5'),
);
const MaterialPage<void> page6 = MaterialPage<void>(
key: ValueKey<String>('page6'),
child: Text('page6'),
);
await buildPages(<Page<void>>[page], tester, removedPage: removedPages);

expect(find.text('page'), findsOneWidget);

await buildPages(<Page<void>>[page, page1, page2, page3], tester, removedPage: removedPages);
await buildPages(<Page<void>>[page, page4, page5, page6], tester, removedPage: removedPages);
await tester.pumpAndSettle();
expect(find.text('page6'), findsOneWidget);
expect(removedPages, isEmpty);

await buildPages(<Page<void>>[page, page4, page5], tester, removedPage: removedPages);
await tester.pumpAndSettle();
expect(find.text('page5'), findsOneWidget);
expect(removedPages, isEmpty);

await buildPages(<Page<void>>[page], tester, removedPage: removedPages);
await tester.pumpAndSettle();
expect(find.text('page'), findsOneWidget);
expect(removedPages, isEmpty);
});

testWidgets('pop calls onDidRemovePage', (WidgetTester tester) async {

@justinmc justinmc Sep 23, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see above that pop sets imperativeRemoval to false, so how does it end up calling onDidRemovePage?

@chunhtai chunhtai Sep 23, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see the reply to the imperativeRemoval comment

final GlobalKey<NavigatorState> key = GlobalKey<NavigatorState>();
final List<Page<void>> removedPage = <Page<void>>[];

const MaterialPage<void> page = MaterialPage<void>(
key: ValueKey<String>('page'),
child: Text('page'),
);
const MaterialPage<void> page1 = MaterialPage<void>(
key: ValueKey<String>('page1'),
child: Text('page1'),
);
await buildPages(<Page<void>>[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, <Page<void>>[page1]);
});

testWidgets('pushReplacement calls onDidRemovePage', (WidgetTester tester) async {
final GlobalKey<NavigatorState> key = GlobalKey<NavigatorState>();
final List<Page<void>> removedPage = <Page<void>>[];

const MaterialPage<void> page = MaterialPage<void>(
key: ValueKey<String>('page'),
child: Text('page'),
);
const MaterialPage<void> page1 = MaterialPage<void>(
key: ValueKey<String>('page1'),
child: Text('page1'),
);
await buildPages(<Page<void>>[page, page1], tester, removedPage: removedPage, navKey: key);

expect(find.text('page1'), findsOneWidget);

key.currentState!.pushReplacement(
MaterialPageRoute<void>(
builder: (_) {
return const Text('new page');
},
),
);
await tester.pumpAndSettle();
expect(find.text('new page'), findsOneWidget);
expect(removedPage, <Page<void>>[page1]);
});

@justinmc justinmc Sep 23, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should any of the cases where onDidRemovePage is not called that are worth testing? Or maybe they're already covered elsewhere...

@chunhtai chunhtai Sep 23, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they only case it shouldn't be called is page api. For testing onDidRemovePage called, I really only need to test pop and replace since all the other variants in terms call these two methods

}
Loading