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
162.54k stars 26.72k forks source link

go_router's canPop is updated one frame late #124637

Open justinmc opened 1 year ago

justinmc commented 1 year ago

GoRouter's canPop method gives out of date results until the following frame after the navigation.

Steps to reproduce

  1. Run the app below, which listens to GoRouter and logs its state.
  2. Navigate to the next route.

Expected: When GoRouter's listener is called, its canPop is true, because there are two routes on the stack.

GoRouter changed. Location: /one. canPop? true
After the post frame callback. Location: /one. canPop? true

Actual: GoRouter's listener is called, and the location is correct, but canPop is still out of date and gives false. After a post frame callback, it is up to date and returns true.

GoRouter changed. Location: /one. canPop? false
After the post frame callback. Location: /one. canPop? true
Example app ```dart // 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/scheduler.dart'; import 'package:go_router/go_router.dart'; import 'package:flutter/material.dart'; import 'package:flutter/services.dart'; void main() => runApp(_MyApp()); class _MyApp extends StatefulWidget { @override State<_MyApp> createState() => _MyAppState(); } class _MyAppState extends State<_MyApp> { final GoRouter _router = GoRouter( routes: [ GoRoute( path: '/', builder: (BuildContext context, GoRouterState state) => _LinksPage( title: 'Home page', backgroundColor: Colors.indigo, buttons: [ TextButton( onPressed: () { context.push('/one'); }, child: const Text('Go to one'), ), ], ), ), GoRoute( path: '/one', builder: (BuildContext context, GoRouterState state) => _LinksPage( title: 'Page one', backgroundColor: Colors.indigo.withRed(255), buttons: [ TextButton( onPressed: () { context.push('/one/two'); }, child: const Text('Go to one/two'), ), ], ), ), GoRoute( path: '/one/two', builder: (BuildContext context, GoRouterState state) => _LinksPage( title: 'Page one/two', backgroundColor: Colors.indigo.withBlue(255), ), ), ], ); void _routerChanged() { print('GoRouter changed. Location: ${_router.location}. canPop? ${_router.canPop()}'); SchedulerBinding.instance.addPostFrameCallback((Duration duration) { print('After the post frame callback. Location: ${_router.location}. canPop? ${_router.canPop()}'); SystemNavigator.updateNavigationStackStatus(_router.canPop()); }); } @override void initState() { super.initState(); _router.addListener(_routerChanged); } @override void dispose() { _router.removeListener(_routerChanged); super.dispose(); } @override Widget build(BuildContext context) { return MaterialApp.router( routerConfig: _router, ); } } class _LinksPage extends StatelessWidget { const _LinksPage ({ required this.backgroundColor, this.buttons = const [], required this.title, }); final Color backgroundColor; final List buttons; final String title; @override Widget build(BuildContext context) { return Scaffold( backgroundColor: backgroundColor, body: Center( child: Column( mainAxisAlignment: MainAxisAlignment.center, children: [ Text(title), ...buttons, if (GoRouter.of(context).canPop()) TextButton( onPressed: () { context.pop(); }, child: const Text('Go back'), ), ], ), ), ); } } ```

Filed this issue after discovering while working on predictive back and talking to @chunhtai offline.

darshankawar commented 1 year ago

Verified on latest versions using the example code provided and can see the same reported behavior.

stable, master flutter doctor -v ``` [!] Flutter (Channel stable, 3.7.12, on macOS 12.2.1 21D62 darwin-x64, locale en-GB) • Flutter version 3.7.12 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 4d9e56e694 (3 days ago), 2023-04-17 21:47:46 -0400 • Engine revision 1a65d409c7 • Dart version 2.19.6 • DevTools version 2.20.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.10.0-12.0.pre.68, on macOS 12.2.1 21D62 darwin-x64, locale en-GB) • Flutter version 3.10.0-12.0.pre.68 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 8de77e89b7 (53 minutes ago), 2023-04-25 23:53:04 -0400 • Engine revision 610c57781b • Dart version 3.1.0 (build 3.1.0-42.0.dev) • DevTools version 2.23.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. ```
ValentinVignal commented 1 year ago

This might come from https://github.com/flutter/flutter/issues/119677 and be a duplicate

justinmc commented 1 year ago

I think you're right!

Amir-P commented 1 year ago

@ValentinVignal @darshankawar The problem is not in GoRouter.canPop(). In fact it's working correctly since Navigator.canPop() returns false in that moment too. You can validate it yourself:

final rootKey = GlobalKey<NavigatorState>();

class _MyAppState extends State<_MyApp> {
  final GoRouter _router = GoRouter(
    navigatorKey: rootKey,
    ...
  );

  void _routerChanged() {
    print(
        'GoRouter changed. Location: ${_router.location}. canPop? ${_router.canPop()}. nav.canPop? ${rootKey.currentState?.canPop()}');
    SchedulerBinding.instance.addPostFrameCallback((Duration duration) {
      print(
          'After the post frame callback. Location: ${_router.location}. canPop? ${_router.canPop()}. nav.canPop? ${rootKey.currentState?.canPop()}');
    });
  }
}
chunhtai commented 1 year ago

it is that the goRouter notify its listener one frame early, the route change will take one frame to propagate the change to navigator before its canPop will change.

P1Rh0 commented 2 weeks ago

Hey :) Is there any update on this? I'm trying to add a back button to pages where canPop is true - for the first route in a stack I'm getting canPop = false on first render. Eg for Sub routes a b and c... initial route /a correctly returns canPop = false navigating to /a/b also returns canPop = false on first render so the back button isn't displayed (incorrectly as obviously we can go back to /a now) Navigating another level to /a/b/c we see the back button is visible as canPop = true (I assume due to now picking up on the previous route?) Clicking the back button to go back to /a/b then puts you back to the second screen but now canPop = true on first render and the back button correctly appears

leanang commented 1 week ago

Hey :) Is there any update on this? I'm trying to add a back button to pages where canPop is true - for the first route in a stack I'm getting canPop = false on first render. Eg for Sub routes a b and c... initial route /a correctly returns canPop = false navigating to /a/b also returns canPop = false on first render so the back button isn't displayed (incorrectly as obviously we can go back to /a now) Navigating another level to /a/b/c we see the back button is visible as canPop = true (I assume due to now picking up on the previous route?) Clicking the back button to go back to /a/b then puts you back to the second screen but now canPop = true on first render and the back button correctly appears

class NaviBackButton extends StatefulWidget {
  const NaviBackButton({
    super.key,
  });
  @override
  State<NaviBackButton> createState() => _NaviBackButtonState();
}

class _NaviBackButtonState extends State<NaviBackButton> {
  late bool _canPop = context.canPop();

  @override
  Widget build(BuildContext context) {
    // https://github.com/flutter/flutter/issues/124637
    SchedulerBinding.instance.addPostFrameCallback((Duration duration) {
      setState(() {
        _canPop = context.canPop();
      });
    });

    return IconButton(
      onPressed: _canPop ? () => context.pop() : null,
      icon: const Icon(Icons.chevron_left, size: 24),
    );
  }
}