Refactors page API

[chunhtai]

fixes #137458

Chagnes:

1. Navigator.pop will always pop page based route
2. add a onDidRemovePage callback to replace onPopPage
3. Page.canPop and Page.onPopInvoked mirrors the PopScope, but in Page class.

migration guide flutter/website#10523

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
• I signed the CLA.
• I listed at least one issue that this PR fixes in the description above.
• 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.

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie or stuartmorgan on the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[chunhtai]

I will add test later if this approach looks good

[justinmc]

I've been away from predictive back for awhile, but this approach looks good to me. It seems a lot simpler with no onPopPage and the same canPop and onPopInvoked API everywhere. Especially because it works with system backs out of the box as users will expect.

Thank you for refactoring this!

[justinmc]

Good call pulling this out into a function.

[goderbauer]

Nice reafactoring, indeed!

[justinmc]

The name sounds good to me.

[justinmc]

Nit: Is there anything that could be deduplicated with a macro in these docs? Here and for canPop.

[chunhtai]

the content is slightly different. I can't find a good way to dedup these doc

[chunhtai]

@justinmc There is one thing I am not too sure about the popscope. Is there a way for PopScope to know what result the original pop used? For example if the pop is generated by Navigator.maybePop('some text'), if you want to launch a confirmation dialog in popScope, there isn't seem to be a way to get the string as pop result if user click confirm

[goderbauer]

Nice reafactoring, indeed!

[btrautmann]

Hi 👋 Any idea when this might land? 🙏

[justinmc]

@justinmc There is one thing I am not too sure about the popscope. Is there a way for PopScope to know what result the original pop used? For example if the pop is generated by Navigator.maybePop('some text'), if you want to launch a confirmation dialog in popScope, there isn't seem to be a way to get the string as pop result if user click confirm

I pasted an example below that hopefully covers what you mean.

I don't think you can get the result of the pop that triggered onPopInvoked from inside of onPopInvoked. I'm not sure we want to enable that. I think that's the same as it was with WillPopScope. See the example, where I show the dialog from inside of onPopInvoked and receive the result there.

Example

import 'package:flutter/material.dart';

void main() => runApp(const NavigatorPopHandlerApp());

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

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      initialRoute: '/home',
      routes: <String, WidgetBuilder>{
        '/home': (BuildContext context) => const _HomePage(),
        '/two': (BuildContext context) => const _PageTwo(),
      },
    );
  }
}

class _HomePage extends StatefulWidget {
  const _HomePage();

  @override
  State<_HomePage> createState() => _HomePageState();
}

class _HomePageState extends State<_HomePage> {

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.center,
          children: <Widget>[
            const Text('Page One'),
            TextButton(
              onPressed: () {
                Navigator.of(context).pushNamed('/two');
              },
              child: const Text('Next page'),
            ),
          ],
        ),
      ),
    );
  }
}

class _PageTwo extends StatefulWidget {
  const _PageTwo();

  @override
  State<_PageTwo> createState() => _PageTwoState();
}

class _PageTwoState extends State<_PageTwo> {
  Future<bool?> _showBackDialog() {
    return showDialog<bool>(
      context: context,
      builder: (BuildContext context) {
        return AlertDialog(
          title: const Text('Are you sure?'),
          content: const Text(
            'Are you sure you want to leave this page?',
          ),
          actions: <Widget>[
            TextButton(
              style: TextButton.styleFrom(
                textStyle: Theme.of(context).textTheme.labelLarge,
              ),
              child: const Text('Nevermind'),
              onPressed: () {
                Navigator.pop(context, false);
              },
            ),
            TextButton(
              style: TextButton.styleFrom(
                textStyle: Theme.of(context).textTheme.labelLarge,
              ),
              child: const Text('Leave'),
              onPressed: () {
                Navigator.pop(context, true);
              },
            ),
          ],
        );
      },
    );
  }

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.center,
          children: <Widget>[
            const Text('Page Two'),
            PopScope(
              canPop: false,
              onPopInvoked: (bool didPop) async {
                if (didPop) {
                  return;
                }
                final NavigatorState navigator = Navigator.of(context);
                final bool? shouldPop = await _showBackDialog();
                if (shouldPop ?? false) {
                  navigator.pop();
                }
              },
              child: TextButton(
                onPressed: () {
                  _showBackDialog();
                },
                child: const Text('Go back'),
              ),
            ),
          ],
        ),
      ),
    );
  }
}

[justinmc]

@btrautmann The author is on leave until April, but maybe I can take this over if people need it.

[chunhtai]

I pasted an example below that hopefully covers what you mean.

I don't think you can get the result of the pop that triggered onPopInvoked from inside of onPopInvoked. I'm not sure we want to enable that. I think that's the same as it was with WillPopScope. See the example, where I show the dialog from inside of onPopInvoked and receive the result there.

Example

See this example.

void main() {
  runApp(MaterialApp(
   home: Home(),
  ));
}

class Home extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Center(
        child: TextButton(
                onPressed: () async {
                  final myMessage = await Navigator.of(context).push(MaterialPageRoute(builder: (_) => MyWidget()));
                  print(myMessage);
                },
                child: const Text('push'),
              ),
      ),
    );
  }
}


class MyWidget extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.center,
          children: <Widget>[
            const Text('Page Two'),
            PopScope(
              canPop: false,
              onPopInvoked: (bool didPop) async {
                if (didPop) {
                  return;
                }
                final NavigatorState navigator = Navigator.of(context);
                final bool? shouldPop = await _showBackDialog();
                if (shouldPop ?? false) {
                  // I want to be able to pass original result 'my result' to the pending future in Home!!
                }
              },
              child: TextButton(
                onPressed: () {
                  Navigator.pop(context, 'my result');
                },
                child: const Text('Go back'),
              ),
            ),
          ],
        ),
      ),
    );
  }
}

I have a pr up for adding this feature #139164
I will reopen it once i come back.

[justinmc]

@chunhtai Thanks, I see what you mean. Indeed I don't think it's possible to do that right now without your PR. I think we should put both PRs on hold for now until you get back.

[chunhtai]

This is ready for review @justinmc @goderbauer

[johnninja]

I've just encountered this issue, and it coincided with an update; I'm waiting online. lol.

[justinmc]

LGTM with nits 👍

[justinmc]

"The [canPop] and [onPopInvoked]" => "The [canPop] and [onPopInvoked] parameters"

[justinmc]

"this [canPop] and [onPopInvoked]" => "the [canPop] and [onPopInvoked] parameters"

[justinmc]

So this is called when a system back happens I guess?

[justinmc]

Anything else that should be tested here besides the example tests?

[auto-submit]

auto label is removed for flutter/flutter/137792, due to - The status or check suite Linux web_skwasm_tests_6 has failed. Please fix the issues identified (or deflake) before re-applying this label.

[auto-submit]

auto label is removed for flutter/flutter/137792, due to - The status or check suite Windows build_tests_4_7 has failed. Please fix the issues identified (or deflake) before re-applying this label.