[go_router] fix: PopScope not honored for initial Routes inside ShellRoutes

[ElectricCookie]

This includes fixes to honor ShellRoutes with PopScope routes #140869 see the last couple comments.

Pot. also touches #8045

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene page, which explains my responsibilities.
• I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
• I signed the CLA.
• The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
• I linked to at least one issue that this PR fixes in the description above.
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
• I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is test-exempt.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[chunhtai]

I think the issue is that the navigatorState.canPop returns false even if there is a popscope on the root route, I think that may be a bug and should probably fixed in the framework side

Hi @justinmc , since you are working on popscope, WDYT?

[chunhtai]

why is this needed? this should be called automatically in _handlePopPageWithRouteMatch which will be called during Navigator.maybePop

[ElectricCookie]

Not sure, I migrated this basically from the original implementation.

Edit: removing this code will cause on_exit_test.dart:241 to fail because returning false from the onExit method will no longer prevent popping.

[ElectricCookie]

I think the issue is that the navigatorState.canPop returns false even if there is a popscope on the root route, I think that may be a bug and should probably fixed in the framework side

Hi @justinmc , since you are working on popscope, WDYT?

This was probably the root cause of the current implementation failing, yes. While looking at the documentation of canPop it mentions its not considering the Pop entries, so I'm interested to see what the reasoning for this was.

A nice functionality to have on the framework would be to get the pop disposition of the routes / navigator without calling pop.

[justinmc]

I think the issue is that the navigatorState.canPop returns false even if there is a popscope on the root route, I think that may be a bug and should probably fixed in the framework side

Hi @justinmc , since you are working on popscope, WDYT?

I think that this is not necessarily a bug. I think the real solution here is that there should be a PopScope widget in the internals of ShellRoute (between the root Navigator and ShellRoute's Navigator). Here is how this situation is typically handled:

Canonical nested Navigator setup

Navigator(
  ...
  child: PopScope( // or NavigatorPopHandler does this automatically.
    canPop: !nestedNavigator.canPop(),
    onPopInvokedWithResult: (bool didPop, Object? result) {
      if (didPop) {
        return;
      }
      nestedNavigator.maybePop();
    },
    child: Navigator(
      child: ...,
        // A page where the app developer wants to block pops.
        child: PopScope(
          canPop: false,
          onPopInvokedWithResult: (bool didPop, Object? result) {
            if (didPop) {
              return;
            }
            print('Called onPopInvokedWithResult in the nested PopScope');
          },
        )
    )
  )
)

See also NavigatorPopHandler's docs and code.

Solution 1: in go_router

go_router could solve this by adding a PopScope widget into ShellRoute, above the nested Navigator. I think that's the correct approach rather than what's currently in this PR, if I'm understanding correctly.

Solution 2: in the framework

The Flutter framework could automatically add a PopScope widget inside of Navigator if it detects that the Navigator is nested. This solution is implemented in flutter/flutter#152330.

Game plan

I'm about to take a vacation for the rest of the year, but if everyone here agrees, then in January I think we should:

• Attempt to land Automatically handle back gesture in nested Navigators flutter#152330 and see if it breaks anything, since it's a pretty opinionated change.
  • Verify that go_router's ShellRoutes and the situation described in this PR are all fixed.
  • Close this PR.
• If we can't land Automatically handle back gesture in nested Navigators flutter#152330, then we should investigate adding PopScope to ShellRoute.
  • This PR could be modified to take that approach if @ElectricCookie is in agreement.

[ElectricCookie]

I wanna preface this with the fact that I probably do not know enough about Navigators to comment about changes to the flutter framework itself. However, I found it odd that there seems to be a second parallel system established (?) to handle passing the navigation events and pop behaviour next to the action intent framework.

@justinmc wouldn't it be possible to inherit all the functionality from that system to manage basically route focus and the event propagation? Or am I in complete ignorance, that this is already the way its done internally?

I'm unsure on the effects of adding a PopScope to the ShellRoute, but I can imagine it would basically be a very similar approach to the current one but instead of finding the pop-able navigators when the event is fired you would compute the canPop property during layout. I think it might then still be necessary/benefitial to interrogate the pop-disposition of the navigator without popping.

Nevertheless, I'm happy to update the PR whenever there is an agreed solution that addresses the framework consistency since this fix is of high importance to the products I'm working on.

[utopicnarwhal]

For those who just want to use/test this fix in their apps, just update your "pubspec.yaml" to contain this:

dependency_overrides:
  go_router: # Issue described here: https://github.com/flutter/packages/pull/8162 remove the override when it's resolved.
    git:
      url: https://github.com/ElectricCookie/flutter_packages
      ref: main
      path: packages/go_router

[justinmc]

@ElectricCookie I agree that it would be nice if navigation and pops were done via Actions and Intents. See how NotificationListener is used to communicate navigation changes: https://github.com/flutter/flutter/blob/27ba2f279075a9a66b48ce78d94d2999b06d6e26/packages/flutter/lib/src/widgets/navigator_pop_handler.dart#L119. Android needs to know whether the Flutter app should handle the pop before the pop happens, though.

I will sync with @chunhtai next week.

[brams-9]

hi @ElectricCookie , is this PR will be merged anytime soon?

i already fork this and use it in my project, but kinda need this PR to be merged and use the official new version

[ElectricCookie]

hi @ElectricCookie , is this PR will be merged anytime soon?

i already fork this and use it in my project, but kinda need this PR to be merged and use the official new version

This is out of my control. Waiting on @justinmc and @chunhtai :)

[chunhtai]

Hi @justinmc I think the issue here is different from the situation you describe.

The issue is that the canpop doesn't correctly reflect the status of navigator. The doc said

Whether the navigator that most tightly encloses the given context can be popped.

The initial route cannot be popped off the navigator, which implies that this function returns true only if popping the navigator would not remove the initial route.

if the initial route has PopScope with canpop=false, Navigator.canPop should return true because popping the navigator will not end up removing the initial route.

[chunhtai]

simply repro

// 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';

/// Flutter code sample for [Scrollbar].

void main() {
  runApp(const ScrollbarExampleApp());
}

final GlobalKey<NavigatorState> nav = GlobalKey<NavigatorState>();

class ScrollbarExampleApp extends StatelessWidget {
  const ScrollbarExampleApp({super.key});

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      navigatorKey: nav,
      home: PopScope(
        canPop: false,
        child: Scaffold(
          appBar: AppBar(title: const Text('Scrollbar Sample')),
          body: Center(
            child: ElevatedButton(
              onPressed: () {
                print(
                  'navigator can Pop should be true! but got ${nav.currentState!.canPop()}',
                );
              },
              child: Text('canPop?'),
            ),
          ),
        ),
      ),
    );
  }
}

[justinmc]

I think we need to decide on the definition of willHandlePopInternally and popDisposition.

@chunhtai what do you think about the assumption in this test? It seems to think that willHandlePopInternally shouldn't consider PopScope because it concerns programmatic popping. It will fail if we make willHandlePopInternally consider PopScope (flutter/flutter#163884).

https://github.com/flutter/flutter/blob/043b71954ce73f727f3cb6f5ccd549cc005b1310/packages/flutter/test/widgets/navigator_test.dart#L3721-L3723

      // PopScope only prevents user trigger action, e.g. Navigator.maybePop.
      // The page can still be popped by the system if it needs to.
      expect(route.willHandlePopInternally, false);

[chunhtai]

waiting for flutter/flutter#163884 to land

[chunhtai]

I think we don't need this pr once the framework side is merged. closing this one