Support for vetoing an attempt to pop the current route by HansMuller · Pull Request #7488 · flutter/flutter

HansMuller · 2017-01-13T20:37:56Z

It's often useful for a Form widget to warn or even block the user if they attempt to back out of an invalid or unsaved modal form.

The Form willPop callback is called before a Navigator.pop() triggered by the Scaffold's back button or Android's system back button. The value of the callback is a Future<bool>. The enclosing modal route will only be popped if willPop returns true. A simple application of willPop would be to show an alert that asks the user to confirm:

new Form(
  onWillPop: () {
    return showDialog(
      context: context,
      child: new AlertDialog(
        title: new Text('This form has errors'),
        content: new Text('Really leave this form?'),
        actions: <Widget> [
          new FlatButton(
            child: new Text('YES'),
            onPressed: () { Navigator.of(context).pop(true); },
          ),
          new FlatButton(
            child: new Text('NO'),
            onPressed: () { Navigator.of(context).pop(false); },
          ),
        ],
      ),
    );
  },
  // ...
)

In this example showDialog returns true or false depending on the Navigator.pop() value it was dismissed with.

The Route class now has a willPop method with just returns Future<bool>.value(true) by default.

The ModalRoute class overrides willPop and delegates to its _ModalScope. This makes it possible to define willPop from a stateful descendant of a ModalRoute. Typically apps don't define ModalRoute subclasses directly, they just use higher level primitives that create ModalRoutes, like showDialog() or MaterialPageRoute. The ModalRoute scopedWillPopCallback makes it possible for a ModalRoute's stateful descendant, like showDialog's child, to define the value of willPop.

@override
void dependenciesChanged() {
 super.dependenciesChanged();
 final ModalRoute<Null> route = ModalRoute.of(context);
 if (route.isCurrent)
   route.scopedWillPopCallback = askTheUserIfTheyAreSure;
}

Navigator.willPop() now delegates to the current route's willPop method. The Scaffold's back button and the binding for the Android system back button now check Navigator.willPop() before actually popping the current route.

Fixes #6331

abarth · 2017-01-13T20:40:58Z

examples/flutter_gallery/test/smoke_test.dart

Maybe change these to pumpUntilNoTransientCallbacks ?

That's WAY better. I didn't realize that pumpUntil... existed.

abarth · 2017-01-13T20:41:57Z

packages/flutter/lib/src/widgets/app.dart

Don't we need to check mounted here too?

Here's the way I understand didPopRoute's role in all of this.

If didPopRoute() returns true it just means that the pop request has been dealt with. In this case willPop() defeats the request, i.e. it's been dealt with, even if the route was disposed.

abarth · 2017-01-13T20:43:20Z

packages/flutter/lib/src/widgets/form.dart

What if there are two forms in a route? Also, don't we need to unregister the callback on dispose?

You're right (Ian had brought this up as well, but I thought I could avoid it). A Route can have an arbitrary number of Form descendants. So I'll need to manage a list of callbacks, rather than just one.

eseidelGoogle · 2017-01-13T20:43:35Z

What's the expected interaction on iOS? Does this kind of interaction only happen for full-screen dialogs on ios? Presumably this doesn't occur when someone uses the backgesture on ios? :D

abarth · 2017-01-13T20:44:30Z

packages/flutter/lib/src/widgets/navigator.dart

Also need to check mounted here

shouldn't the route check the isCurrent thing?
I think this can all be simplified to just return route.willPop(), if the route verifies isCurrent.

you don't need to check mounted here, that's the caller's problem.

Hixie · 2017-01-13T20:54:51Z

~~The~~ My expectation is was that back gesture pages not use this, and back gesture therefore doesn't go through this path.

edit: see later comment

HansMuller · 2017-01-13T20:57:02Z

@eseidelGoogle Yes. That's what the change in page.dart is for.

Hixie · 2017-01-13T20:57:21Z

packages/flutter/lib/src/widgets/app.dart

you're returning a Future<Future> here. You should just return true, and async will wrap it for you.

Hixie · 2017-01-13T20:59:47Z

packages/flutter/lib/src/widgets/binding.dart

we probably need to take a local copy of _observers here or otherwise handle what happens while this now long-running API gets fiddled with while in flight. We used to have the rule that didPopRoute couldn't change the observers but that no longer works since it's entirely async and minutes could pass.

Good point.

Hixie · 2017-01-13T21:00:22Z

packages/flutter/lib/src/widgets/form.dart

dynamic, not Null. You don't know what the route's return type will be here.

Hixie · 2017-01-13T21:01:01Z

packages/flutter/lib/src/widgets/form.dart

dynamic not Null

Hixie · 2017-01-13T21:01:31Z

packages/flutter/lib/src/widgets/form.dart

why the isCurrent check?

Hixie · 2017-01-13T21:01:49Z

packages/flutter/lib/src/widgets/navigator.dart

Future<bool> willPop async => true;

Hixie · 2017-01-13T21:02:31Z

packages/flutter/lib/src/widgets/navigator.dart

ditto above

Hixie · 2017-01-13T21:03:51Z

packages/flutter/lib/src/widgets/routes.dart

s/,/, and/ and s/;/./

Hixie · 2017-01-13T21:04:13Z

packages/flutter/lib/src/widgets/routes.dart

the route callback mechanism should be a list of callbacks, not a single callback

Hixie · 2017-01-13T21:08:52Z

packages/flutter/lib/src/widgets/routes.dart

why this limitation?

I wasn't sure if the _ModalScope would always exist, even when the route wasn't current. Maybe it always does; I'll remove the isCurrent checks if that's so.

The _ModalScope is just another widget. If you're in the tree in a Navigator in a ModalRoute, then you must have a _ModalScope because the ModalRoute builds it.

Hixie · 2017-01-13T21:32:27Z

In reading the patch I found that the way the patch does the back gesture thing is it just disallows the back gesture if you have any callbacks registered. That seems fine too.

Hixie · 2017-01-17T21:34:26Z

examples/flutter_gallery/lib/demo/text_field_demo.dart

won't this return a Future<Null>?

Oh never mind, I see.

This should be showDialog/*<bool>*/(...

Yes, it returns a Future whose value is the bool passed to the Navgator.pop() that dismisses the Alert dialog.

Right. So it should be showDialog/*<bool>*/(... (rather than just showDialog( like here)

Hixie · 2017-01-17T21:36:29Z

examples/flutter_gallery/test/smoke_test.dart

we should be careful about using pumpUntilNoTransientCallbacks. It can hide bugs. In particular here I think it would be better to just have the two pumps, because then we're clear about how many frames we're expecting. Why would we be getting more?

Scaffold's back button callback now awaits the willPop() method before actually calling Navigator.pop(). That means that tests that back out of a route by tapping the back button now need to pump three times to be consistent with the implementation. I'll add the explicit pumps (back) but binding the test to the implementation in this way doesn't seem that useful to me.

Hixie · 2017-01-17T21:36:44Z

examples/flutter_gallery/test/update_test.dart

Same here, do we fully understand why we're getting more than two?

Hixie · 2017-01-17T21:37:22Z

packages/flutter/lib/src/material/page.dart

This should probably be mentioned in the docs, too.

Hixie · 2017-01-17T21:40:16Z

packages/flutter/lib/src/widgets/form.dart

I don't really understand this comment. There's no test anywhere that it's the same modal route...

This happens in the gallery. Each time the "This form has errors" warning dialog is pushed or popped the underlying Form's dependenciesChanged() method runs. We don't want to keep adding the same callback to the route.

I still need to write a test that simulates this scenario.

Hixie · 2017-01-17T21:40:29Z

packages/flutter/lib/src/widgets/form.dart

why does it matter if it's current?

It doesn't matter. I'd meant to remove that, thanks for repointing it out.

Hixie · 2017-01-17T21:41:03Z

packages/flutter/lib/src/widgets/form.dart

don't you need to check if oldConfig.onWillPop was null?

Hixie · 2017-01-17T21:43:03Z

packages/flutter/lib/src/widgets/routes.dart

Maybe this would be better starting off as null, since it's a rare case.

I had it that way originally. Doing so makes all of the other references to willPopCallbacks messier; I don't think there's much of a win here.

Oh, I see. I didn't realise it was used from outside this class.

Hixie · 2017-01-17T21:43:22Z

packages/flutter/lib/src/widgets/routes.dart

assert that it's not that const that you set in the dispose method.

I've added an assert(mounted)

Hixie · 2017-01-17T21:43:32Z

packages/flutter/lib/src/widgets/routes.dart

Hixie · 2017-01-17T21:44:36Z

packages/flutter/lib/src/widgets/routes.dart

should probably warn that the callback might be called long after the route was unmounted.

never mind, i see this is the private api

Hixie · 2017-01-17T21:46:04Z

packages/flutter/lib/src/widgets/routes.dart

should probably warn that this callback might get called long after the route and the widget tree is disposed.

I've updated the doc.

Hixie · 2017-01-17T21:46:57Z

packages/flutter/lib/src/widgets/routes.dart

if it's null you return true?

if this is intentional, please add a comment as to why...

It was intentional and consistent and unnecessary. I've removed all of them.

Hixie · 2017-01-18T06:00:41Z

LGTM with the one remaining fix (mentioning the effect of registering a willPop callback on the iOS back gesture).

googlebot added the cla: yes label Jan 13, 2017

abarth reviewed Jan 13, 2017

View reviewed changes

Hixie reviewed Jan 13, 2017

View reviewed changes

packages/flutter/lib/src/widgets/form.dart Outdated

Copy link

Contributor

Hixie Jan 13, 2017

Choose a reason for hiding this comment

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

dynamic, not Null. You don't know what the route's return type will be here.

Hixie reviewed Jan 13, 2017

View reviewed changes

packages/flutter/lib/src/widgets/form.dart Outdated

Copy link

Contributor

Hixie Jan 13, 2017

Choose a reason for hiding this comment

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

dynamic not Null

Hixie reviewed Jan 13, 2017

View reviewed changes

packages/flutter/lib/src/widgets/form.dart Outdated

Copy link

Contributor

Hixie Jan 13, 2017

Choose a reason for hiding this comment

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

why the isCurrent check?

Hixie reviewed Jan 13, 2017

View reviewed changes

packages/flutter/lib/src/widgets/navigator.dart Outdated

Copy link

Contributor

Hixie Jan 13, 2017 •

edited

Loading

Choose a reason for hiding this comment

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

Future<bool> willPop async => true;

Hixie reviewed Jan 13, 2017

View reviewed changes

packages/flutter/lib/src/widgets/navigator.dart Outdated

Copy link

Contributor

Hixie Jan 13, 2017

Choose a reason for hiding this comment

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

ditto above

Hixie reviewed Jan 13, 2017

View reviewed changes

packages/flutter/lib/src/widgets/routes.dart Outdated

Copy link

Contributor

Hixie Jan 13, 2017 •

edited

Loading

Choose a reason for hiding this comment

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

s/,/, and/ and s/;/./

Hixie reviewed Jan 13, 2017

View reviewed changes

Hixie reviewed Jan 17, 2017

View reviewed changes

examples/flutter_gallery/test/update_test.dart Outdated

Copy link

Contributor

Hixie Jan 17, 2017

Choose a reason for hiding this comment

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

Same here, do we fully understand why we're getting more than two?

Hixie reviewed Jan 17, 2017

View reviewed changes

packages/flutter/lib/src/material/page.dart Outdated

Copy link

Contributor

Hixie Jan 17, 2017

Choose a reason for hiding this comment

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

This should probably be mentioned in the docs, too.

Hixie reviewed Jan 17, 2017

View reviewed changes

packages/flutter/lib/src/widgets/routes.dart Outdated

Copy link

Contributor

Hixie Jan 17, 2017

Choose a reason for hiding this comment

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

ditto

Hixie reviewed Jan 17, 2017

View reviewed changes

HansMuller force-pushed the willPop branch from 0a4b70d to 3b4f745 Compare January 18, 2017 01:48

eseidelGoogle mentioned this pull request Jan 18, 2017

Support warning users before leaving route containing an unfinished form #6331

Closed

Hans Muller added 4 commits January 18, 2017 10:49

Support for vetoing user attempt to back out of a Modal route

9d4ee67

updated per review feedback

98d7595

Updated per nearly all of the feedback

fca4137

Added some additional doc and a test

9402692

HansMuller force-pushed the willPop branch from 3b4f745 to 9402692 Compare January 18, 2017 18:49

HansMuller merged commit 0ce9917 into flutter:master Jan 18, 2017

github-actions bot locked as resolved and limited conversation to collaborators Aug 14, 2021

Conversation

HansMuller commented Jan 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eseidelGoogle commented Jan 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Jan 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HansMuller commented Jan 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie Jan 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie Jan 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Jan 13, 2017

Uh oh!

Hixie Jan 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

HansMuller commented Jan 13, 2017 •

edited

Loading

Hixie commented Jan 13, 2017 •

edited

Loading

Hixie Jan 13, 2017 •

edited

Loading

Hixie Jan 13, 2017 •

edited

Loading

Hixie Jan 17, 2017 •

edited

Loading