Skip to content
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.

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

Fix canpop logic to be more robust #73993

Merged
merged 2 commits into from Jan 15, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 2 additions & 8 deletions packages/flutter/lib/src/material/app_bar.dart
Expand Up @@ -709,8 +709,6 @@ class _AppBarState extends State<AppBar> {
Scaffold.of(context).openEndDrawer();
}

bool? hadBackButtonWhenRouteWasActive;

@override
Widget build(BuildContext context) {
assert(!widget.primary || debugCheckHasMediaQuery(context));
Expand All @@ -723,11 +721,7 @@ class _AppBarState extends State<AppBar> {

final bool hasDrawer = scaffold?.hasDrawer ?? false;
final bool hasEndDrawer = scaffold?.hasEndDrawer ?? false;
hadBackButtonWhenRouteWasActive ??= false;
if (parentRoute?.isActive == true) {
hadBackButtonWhenRouteWasActive = parentRoute!.canPop;
}
assert(hadBackButtonWhenRouteWasActive != null);
final bool canPop = parentRoute?.canPop ?? false;
final bool useCloseButton = parentRoute is PageRoute<dynamic> && parentRoute.fullscreenDialog;

final double toolbarHeight = widget.toolbarHeight ?? kToolbarHeight;
Expand Down Expand Up @@ -796,7 +790,7 @@ class _AppBarState extends State<AppBar> {
tooltip: MaterialLocalizations.of(context).openAppDrawerTooltip,
);
} else {
if (!hasEndDrawer && hadBackButtonWhenRouteWasActive!)
if (!hasEndDrawer && canPop)
leading = useCloseButton ? const CloseButton() : const BackButton();
}
}
Expand Down
19 changes: 15 additions & 4 deletions packages/flutter/lib/src/widgets/navigator.dart
Expand Up @@ -464,10 +464,7 @@ abstract class Route<T> {
return currentRouteEntry.route == this;
}

/// Whether this route is the bottom-most route on the navigator.
///
/// If this is true, then [Navigator.canPop] will return false if this route's
/// [willHandlePopInternally] returns false.
/// Whether this route is the bottom-most active route on the navigator.
///
/// If [isFirst] and [isCurrent] are both true then this is the only route on
/// the navigator (and [isActive] will also be true).
Expand All @@ -483,6 +480,20 @@ abstract class Route<T> {
return currentRouteEntry.route == this;
}

/// Whether there is at least one active route underneath this route.
@protected
bool get hasActiveRouteBelow {
if (_navigator == null)
return false;
for (final _RouteEntry entry in _navigator!._history) {
if (entry.route == this)
return false;
if (_RouteEntry.isPresentPredicate(entry))
return true;
}
return false;
}

/// Whether this route is on the navigator.
///
/// If the route is not only active, but also the current route (the top-most
Expand Down
5 changes: 4 additions & 1 deletion packages/flutter/lib/src/widgets/routes.dart
Expand Up @@ -1483,10 +1483,13 @@ abstract class ModalRoute<T> extends TransitionRoute<T> with LocalHistoryRoute<T

/// Whether this route can be popped.
///
/// A route can be popped if there is at least one active route below it, or
/// if [willHandlePopInternally] returns true.
///
/// When this changes, if the route is visible, the route will
/// rebuild, and any widgets that used [ModalRoute.of] will be
/// notified.
bool get canPop => isActive && (!isFirst || willHandlePopInternally);
bool get canPop => hasActiveRouteBelow || willHandlePopInternally;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is much easier to understand than the old version :-)


// Internals

Expand Down
30 changes: 30 additions & 0 deletions packages/flutter/test/cupertino/nav_bar_test.dart
Expand Up @@ -93,6 +93,36 @@ void main() {
expect(find.byType(BackdropFilter), findsOneWidget);
});

testWidgets('Nav bar displays correctly', (WidgetTester tester) async {
final GlobalKey<NavigatorState> navigator = GlobalKey<NavigatorState>();
await tester.pumpWidget(
CupertinoApp(
navigatorKey: navigator,
home: const CupertinoNavigationBar(
middle: Text('Page 1'),
),
),
);
navigator.currentState!.push<void>(CupertinoPageRoute<void>(
builder: (BuildContext context) {
return const CupertinoNavigationBar(
middle: Text('Page 2'),
);
}
));
await tester.pumpAndSettle();
expect(find.byType(CupertinoNavigationBarBackButton), findsOneWidget);
// Pops the page 2
navigator.currentState!.pop();
await tester.pump();
// Needs another pump to trigger the rebuild;
await tester.pump();
// The back button should still persist;
expect(find.byType(CupertinoNavigationBarBackButton), findsOneWidget);
// The app does not crash
expect(tester.takeException(), isNull);
});

testWidgets('Can specify custom padding', (WidgetTester tester) async {
final Key middleBox = GlobalKey();
await tester.pumpWidget(
Expand Down