Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions packages/go_router/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 16.2.3

- Fixes an issue where iOS back gesture pops entire ShellRoute instead of the active sub-route.

## 16.2.2

- Fixes broken links in readme.
Expand Down
35 changes: 21 additions & 14 deletions packages/go_router/lib/src/builder.dart
Original file line number Diff line number Diff line change
Expand Up @@ -293,20 +293,27 @@ class _CustomNavigatorState extends State<_CustomNavigator> {
List<NavigatorObserver>? observers,
String? restorationScopeId,
) {
return _CustomNavigator(
// The state needs to persist across rebuild.
key: GlobalObjectKey(navigatorKey.hashCode),
navigatorRestorationId: restorationScopeId,
navigatorKey: navigatorKey,
matches: match.matches,
matchList: matchList,
configuration: widget.configuration,
observers: observers ?? const <NavigatorObserver>[],
onPopPageWithRouteMatch: widget.onPopPageWithRouteMatch,
// This is used to recursively build pages under this shell route.
errorBuilder: widget.errorBuilder,
errorPageBuilder: widget.errorPageBuilder,
requestFocus: widget.requestFocus,
return PopScope(
Copy link
Contributor

Choose a reason for hiding this comment

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

@justinmc is this the right way to prevent an iOS backgesture? or should we use NavigatorPopHandler?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like Justin is on vacation. Is there anybody else we can loop in or should we wait for him?

Screenshot 2025-09-09 at 19 38 54

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's wait for him. I "think" we should use NavigatorPopHandler and I also remember Justin was also fixing something about the nested navigator in go_router

Copy link
Contributor

Choose a reason for hiding this comment

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

@LukasMirbt Could you check if flutter/flutter#152330 fixes this issue? If so I'd prefer to wait for that PR. If not, we should confirm that any fix made inside of go_router here will still work after that PR lands.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@justinmc flutter/flutter#152330 does seem to fix this issue 🎉

However, I don't see a compelling reason to wait for your PR since it will most likely take a couple of months before it hits stable, even once it's merged, and the issue linked in this PR has over 50 upvotes.

All the go_router tests + regression tests seem to pass (on your fork) both with and without the fix added in this PR. I propose we add a TODO to remove the PopScope once your fix has hit stable and merge this PR in the meantime.

Let me know if you disagree!

Copy link
Contributor

Choose a reason for hiding this comment

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

Can the change here and flutter change coexist? If so, can you add a test to make sure android back button work correctly so that when stable roll into package won't break go_router?

Otherwise, adding a todo and merge this now sgtm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can the change here and flutter change coexist?

Yes, it seems like the fix is forward compatible 👍 The tests are passing with flutter/flutter#152330.

If so, can you add a test to make sure android back button work correctly so that when stable roll into package won't break go_router?

Done ✅

Otherwise, adding a todo and merge this now sgtm

TODO added as well!

// Prevent ShellRoute from being popped, for example
// by an iOS back gesture, when the route has active sub-routes.
// TODO(LukasMirbt): Remove when minimum flutter version includes
// https://github.com/flutter/flutter/pull/152330.
canPop: match.matches.length == 1,
child: _CustomNavigator(
// The state needs to persist across rebuild.
key: GlobalObjectKey(navigatorKey.hashCode),
navigatorRestorationId: restorationScopeId,
navigatorKey: navigatorKey,
matches: match.matches,
matchList: matchList,
configuration: widget.configuration,
observers: observers ?? const <NavigatorObserver>[],
onPopPageWithRouteMatch: widget.onPopPageWithRouteMatch,
// This is used to recursively build pages under this shell route.
errorBuilder: widget.errorBuilder,
errorPageBuilder: widget.errorPageBuilder,
requestFocus: widget.requestFocus,
),
);
},
);
Expand Down
2 changes: 1 addition & 1 deletion packages/go_router/pubspec.yaml
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
name: go_router
description: A declarative router for Flutter based on Navigation 2 supporting
deep linking, data-driven routes and more
version: 16.2.2
version: 16.2.3
repository: https://github.com/flutter/packages/tree/main/packages/go_router
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+go_router%22

Expand Down
165 changes: 165 additions & 0 deletions packages/go_router/test/shell_route_system_back_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,165 @@
// Copyright 2013 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/foundation.dart';
import 'package:flutter/material.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:go_router/go_router.dart';

import 'test_helpers.dart';

// Regression test for https://github.com/flutter/flutter/issues/120353
void main() {
group('iOS back gesture inside a ShellRoute', () {
testWidgets('pops the top sub-route '
'when there is an active sub-route', (WidgetTester tester) async {
debugDefaultTargetPlatformOverride = TargetPlatform.iOS;

await tester.pumpWidget(const _TestApp());
expect(find.text('Home'), findsOneWidget);

await tester.tap(find.byType(FilledButton));
await tester.pumpAndSettle();
expect(find.text('Post'), findsOneWidget);

await tester.tap(find.byType(FilledButton));
await tester.pumpAndSettle();
expect(find.text('Comment'), findsOneWidget);

await simulateIosBackGesture(tester);
await tester.pumpAndSettle();
expect(find.text('Post'), findsOneWidget);

debugDefaultTargetPlatformOverride = null;
});

testWidgets('pops ShellRoute '
'when there are no active sub-routes', (WidgetTester tester) async {
debugDefaultTargetPlatformOverride = TargetPlatform.iOS;

await tester.pumpWidget(const _TestApp());
expect(find.text('Home'), findsOneWidget);

await tester.tap(find.byType(FilledButton));
await tester.pumpAndSettle();
expect(find.text('Post'), findsOneWidget);

await simulateIosBackGesture(tester);
await tester.pumpAndSettle();
expect(find.text('Home'), findsOneWidget);

debugDefaultTargetPlatformOverride = null;
});
});

group('Android back button inside a ShellRoute', () {
testWidgets('pops the top sub-route '
'when there is an active sub-route', (WidgetTester tester) async {
await tester.pumpWidget(const _TestApp());
expect(find.text('Home'), findsOneWidget);

await tester.tap(find.byType(FilledButton));
await tester.pumpAndSettle();
expect(find.text('Post'), findsOneWidget);

await tester.tap(find.byType(FilledButton));
await tester.pumpAndSettle();
expect(find.text('Comment'), findsOneWidget);

await simulateAndroidBackButton(tester);
await tester.pumpAndSettle();
expect(find.text('Post'), findsOneWidget);
});

testWidgets('pops ShellRoute '
'when there are no active sub-routes', (WidgetTester tester) async {
await tester.pumpWidget(const _TestApp());
expect(find.text('Home'), findsOneWidget);

await tester.tap(find.byType(FilledButton));
await tester.pumpAndSettle();
expect(find.text('Post'), findsOneWidget);

await simulateAndroidBackButton(tester);
await tester.pumpAndSettle();
expect(find.text('Home'), findsOneWidget);
});
});
}

class _TestApp extends StatefulWidget {
const _TestApp();

@override
State<_TestApp> createState() => _TestAppState();
}

class _TestAppState extends State<_TestApp> {
final GoRouter _router = GoRouter(
routes: <GoRoute>[
GoRoute(
path: '/',
builder: (BuildContext context, GoRouterState state) {
return Scaffold(
appBar: AppBar(title: const Text('Home')),
body: Center(
child: FilledButton(
onPressed: () {
GoRouter.of(context).go('/post');
},
child: const Text('Go to Post'),
),
),
);
},
routes: <RouteBase>[
ShellRoute(
builder: (BuildContext context, GoRouterState state, Widget child) {
return child;
},
routes: <GoRoute>[
GoRoute(
path: '/post',
builder: (BuildContext context, GoRouterState state) {
return Scaffold(
appBar: AppBar(title: const Text('Post')),
body: Center(
child: FilledButton(
onPressed: () {
GoRouter.of(context).go('/post/comment');
},
child: const Text('Comment'),
),
),
);
},
routes: <GoRoute>[
GoRoute(
path: 'comment',
builder: (BuildContext context, GoRouterState state) {
return Scaffold(
appBar: AppBar(title: const Text('Comment')),
);
},
),
],
),
],
),
],
),
],
);

@override
void dispose() {
_router.dispose();
super.dispose();
}

@override
Widget build(BuildContext context) {
return MaterialApp.router(routerConfig: _router);
}
}
Loading