flutter / flutter

Flutter makes it easy and fast to build beautiful apps for mobile and beyond
https://flutter.dev
BSD 3-Clause "New" or "Revised" License
164.24k stars 27.1k forks source link

[go_router]: dialogs and bottom sheets are not shown after calling go or goNamed. #146578

Open eli1stark opened 4 months ago

eli1stark commented 4 months ago

Steps to reproduce

  1. go_router: v12.1.3
  2. Launch sample
  3. Press 'Go to page B' button
  4. Press 'Go Page A and Show dialog' button
  5. Dialog is not shown

It seems that the go_router API is not truly synchronous, as stated in the documentation, which can lead to issues like this. The asynchronous behavior is hidden behind the API, and the client assumes that it's synchronous. In this case, it seems a race condition occurs between goNamed and showDialog.

Adding some arbitrary delay between calls fixes the issue, but this is not a great solution because you can never know what is a sufficient time needed to await go and goNamed to avoid the issue on all devices and platforms.

Expected results

Dialog must be shown.

Actual results

Dialog is not shown.

Code sample

Code sample ```dart import 'package:flutter/material.dart'; import 'package:go_router/go_router.dart'; final router = GoRouter( routes: [ GoRoute( name: 'A', path: '/', builder: (_, __) => const PageA(), ), GoRoute( name: 'B', path: '/B', builder: (_, __) => const PageB(), ), ], ); void main() => runApp(const App()); class App extends StatelessWidget { const App({super.key}); @override Widget build(BuildContext context) { return MaterialApp.router( routerConfig: router, ); } } class PageA extends StatelessWidget { const PageA({super.key}); @override Widget build(BuildContext context) { return Scaffold( backgroundColor: Colors.white, body: Column( mainAxisAlignment: MainAxisAlignment.center, children: [ Center( child: TextButton( onPressed: () { context.pushNamed('B'); }, child: const Text('Go to page B'), ), ), ], ), ); } } class PageB extends StatelessWidget { const PageB({super.key}); @override Widget build(BuildContext context) { return Scaffold( backgroundColor: Colors.grey, body: Column( mainAxisAlignment: MainAxisAlignment.center, children: [ Center( child: TextButton( onPressed: () { final ctx = router.routerDelegate.navigatorKey.currentContext!; ctx.goNamed('A'); showDialog( context: ctx, builder: (_) => const AlertDialog( title: Text('Dialog Title'), content: Text('This is my content'), ), ); }, child: const Text('Go Page A and Show dialog'), ), ), ], ), ); } } ```

Flutter Doctor output

Doctor output ```console Doctor summary (to see all details, run flutter doctor -v): [✓] Flutter (Channel stable, 3.16.7, on macOS 14.2.1 23C71 darwin-arm64, locale en-US) [✓] Android toolchain - develop for Android devices (Android SDK version 33.0.0) [✓] Xcode - develop for iOS and macOS (Xcode 15.3) [✓] Chrome - develop for the web [✓] Android Studio (version 2022.3) [✓] VS Code (version 1.87.2) [✓] Connected device (4 available) [✓] Network resources • No issues found! ```
darshankawar commented 4 months ago

It seems that the go_router API is not truly synchronous, as stated in the documentation, which can lead to issues like this. The asynchronous behavior is hidden behind the API, and the client assumes that it's synchronous. In this case, it seems a race condition occurs between goNamed and showDialog.

Adding some arbitrary delay between calls fixes the issue, but this is not a great solution because you can never know what is a sufficient time needed to await go and goNamed to avoid the issue on all devices and platforms.

@eli1stark This issue once resolved will also solve your case. Please check the same and confirm if you can follow-up in it.

eli1stark commented 4 months ago

This is different, they are awaiting for result of the page, which won't get completed until the page is popped, if I understand it correctly, at least this is how push and pushNamed works and they want to introduce a similar concept to go and goNamed.

The problem here is that go_router jumps into racing condition with Navigator 1.0 (when I push dialog) which makes it incompatible for certain use cases like mine. A first solution would be to make execution of go and goNamed truly synchronous, there is no reason for them to be asynchronous behind the curtains, a 2nd solution is to add ability to push dialogs and bottom sheets through go_router API, so it can queue properly calls and avoid racing conditions.

If we go with the 2nd case docs should state that go_router is not fully compatible with the Navigator 1.0 and everything must be done through go_router API. If I call router.go and then call immediately router.push it works as expected and page is pushed correctly, however it's not the same if I mix the calls with Navigator 1.0 like showDialog or showModalBottomSheet and so on.

eli1stark commented 4 months ago

Here are examples where the router is racing or incompatible with Navigator 1.0 (I abstracted the examples a little bit for simplicity):

GoRouter.go('A')
GoRouter.push('B') // works (page 'B' is shown)
GoRouter.go('A')
Navigator.push('B') // doesn't work  (page 'B' is not shown)
GoRouter.go('A')
Navigator.showDialog() // doesn't work (dialog is not shown)
GoRouter.go('A')
Navigator.showModalBottomSheet() // doesn't work (bottom sheet is not shown)
GoRouter.push('B')
Navigator.pop() // doesn't work (There is nothing to pop is thrown)
Navigator.push('B')
Navigator.pop() // works (page 'B' is popped without issues)

In all occurrences where it doesn't work it should work, otherwise go_router docs should mention incompatibility issues with Navigator 1.0.

From the Docs:

GoRouter leverages the Router API to provide backward compatibility with the Navigator, so any calls to Navigator.of(context).push() or Navigator.of(context).pop() will continue to work, but these destinations aren't deep-linkable. You can gradually add more routes to the GoRouter configuration.

darshankawar commented 4 months ago

Thanks for the update. I was able to replicate the reported behavior.

stable, master flutter doctor -v ``` [!] Flutter (Channel stable, 3.19.5, on macOS 12.2.1 21D62 darwin-x64, locale en-GB) • Flutter version 3.19.5 on channel stable at /Users/dhs/documents/fluttersdk/flutter ! Warning: `flutter` on your path resolves to /Users/dhs/Documents/Fluttersdk/flutter/bin/flutter, which is not inside your current Flutter SDK checkout at /Users/dhs/documents/fluttersdk/flutter. Consider adding /Users/dhs/documents/fluttersdk/flutter/bin to the front of your path. ! Warning: `dart` on your path resolves to /Users/dhs/Documents/Fluttersdk/flutter/bin/dart, which is not inside your current Flutter SDK checkout at /Users/dhs/documents/fluttersdk/flutter. Consider adding /Users/dhs/documents/fluttersdk/flutter/bin to the front of your path. • Upstream repository https://github.com/flutter/flutter.git • Framework revision 300451adae (4 days ago), 2024-03-27 21:54:07 -0500 • Engine revision e76c956498 • Dart version 3.3.3 • DevTools version 2.31.1 • If those were intentional, you can disregard the above warnings; however it is recommended to use "git" directly to perform update checks and upgrades. [!] Xcode - develop for iOS and macOS (Xcode 12.3) • Xcode at /Applications/Xcode.app/Contents/Developer ! Flutter recommends a minimum Xcode version of 13. Download the latest version or update via the Mac App Store. • CocoaPods version 1.11.2 [✓] Chrome - develop for the web • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome [✓] VS Code (version 1.62.0) • VS Code at /Applications/Visual Studio Code.app/Contents • Flutter extension version 3.21.0 [✓] Connected device (5 available) • SM G975F (mobile) • RZ8M802WY0X • android-arm64 • Android 11 (API 30) • Darshan's iphone (mobile) • 21150b119064aecc249dfcfe05e259197461ce23 • ios • iOS 14.4.1 18D61 • iPhone 12 Pro Max (mobile) • A5473606-0213-4FD8-BA16-553433949729 • ios • com.apple.CoreSimulator.SimRuntime.iOS-14-3 (simulator) • macOS (desktop) • macos • darwin-x64 • Mac OS X 10.15.4 19E2269 darwin-x64 • Chrome (web) • chrome • web-javascript • Google Chrome 98.0.4758.80 [✓] HTTP Host Availability • All required HTTP hosts are available ! Doctor found issues in 1 category. [!] Flutter (Channel master, 3.22.0-7.0.pre.14, on macOS 12.2.1 21D62 darwin-x64, locale en-GB) • Flutter version 3.22.0-7.0.pre.14 on channel master at /Users/dhs/documents/fluttersdk/flutter ! Warning: `flutter` on your path resolves to /Users/dhs/Documents/Fluttersdk/flutter/bin/flutter, which is not inside your current Flutter SDK checkout at /Users/dhs/documents/fluttersdk/flutter. Consider adding /Users/dhs/documents/fluttersdk/flutter/bin to the front of your path. ! Warning: `dart` on your path resolves to /Users/dhs/Documents/Fluttersdk/flutter/bin/dart, which is not inside your current Flutter SDK checkout at /Users/dhs/documents/fluttersdk/flutter. Consider adding /Users/dhs/documents/fluttersdk/flutter/bin to the front of your path. • Upstream repository https://github.com/flutter/flutter.git • Framework revision 74b65d4e4c (4 hours ago), 2024-04-09 21:26:14 -0400 • Engine revision eaf73cd39c • Dart version 3.5.0 (build 3.5.0-36.0.dev) • DevTools version 2.34.1 • If those were intentional, you can disregard the above warnings; however it is recommended to use "git" directly to perform update checks and upgrades. [!] Android toolchain - develop for Android devices (Android SDK version 30.0.3) • Android SDK at /Users/dhs/Library/Android/sdk ✗ cmdline-tools component is missing Run `path/to/sdkmanager --install "cmdline-tools;latest"` See https://developer.android.com/studio/command-line for more details. ✗ Android license status unknown. Run `flutter doctor --android-licenses` to accept the SDK licenses. See https://flutter.dev/docs/get-started/install/macos#android-setup for more details. [✓] Xcode - develop for iOS and macOS (Xcode 13.2.1) • Xcode at /Applications/Xcode.app/Contents/Developer • Build 13C100 • CocoaPods version 1.11.2 [✓] Chrome - develop for the web • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome [✓] IntelliJ IDEA Ultimate Edition (version 2021.3.2) • IntelliJ at /Applications/IntelliJ IDEA.app • Flutter plugin version 65.1.4 • Dart plugin version 213.7228 [✓] VS Code (version 1.62.0) • VS Code at /Applications/Visual Studio Code.app/Contents • Flutter extension version 3.29.0 [✓] Connected device (3 available) • Darshan's iphone (mobile) • 21150b119064aecc249dfcfe05e259197461ce23 • ios • iOS 15.3.1 19D52 • macOS (desktop) • macos • darwin-x64 • macOS 12.2.1 21D62 darwin-x64 • Chrome (web) • chrome • web-javascript • Google Chrome 109.0.5414.119 [✓] Network resources • All expected network resources are available. ! Doctor found issues in 1 category. [!] Xcode - develop for iOS and macOS (Xcode 12.3) • Xcode at /Applications/Xcode.app/Contents/Developer ! Flutter recommends a minimum Xcode version of 13. Download the latest version or update via the Mac App Store. • CocoaPods version 1.11.2 [✓] Chrome - develop for the web • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome [✓] VS Code (version 1.62.0) • VS Code at /Applications/Visual Studio Code.app/Contents • Flutter extension version 3.21.0 [✓] Connected device (5 available) • SM G975F (mobile) • RZ8M802WY0X • android-arm64 • Android 11 (API 30) • Darshan's iphone (mobile) • 21150b119064aecc249dfcfe05e259197461ce23 • ios • iOS 14.4.1 18D61 • iPhone 12 Pro Max (mobile) • A5473606-0213-4FD8-BA16-553433949729 • ios • com.apple.CoreSimulator.SimRuntime.iOS-14-3 (simulator) • macOS (desktop) • macos • darwin-x64 • Mac OS X 10.15.4 19E2269 darwin-x64 • Chrome (web) • chrome • web-javascript • Google Chrome 98.0.4758.80 [✓] HTTP Host Availability • All required HTTP hosts are available ! Doctor found issues in 1 category. ```
chunhtai commented 2 months ago

Yes, I think we should do something about this.

dkbast commented 2 months ago

I'm running into the same issue but in reverse - in my case I have a bottom navigation bar which uses goNamed to switch "tabs" on one of the tabs I use "showModalBottomSheet" which does not cover the bottom navigation, so when I tap a nav item the bottomSheet is still shown, AND the route changes behind it which is quite confusing. Would expect goNamed to pop the modal bottom sheet.