Allows page removal that contains Localhistoryentry

[chunhtai]

fixes #97836
fixes #134752
fixes #118645

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.

[chunhtai]

looking at the code, it looks like popscope/willpopscope are going through Navigator.maybePop mechanism and are still allowed to be popped through Navigator.pop. So it is not handle pop internally.

[chunhtai]

All the other solutions I can think of would require adding new API and break other people, or messing with Navigator/Route class.

[goderbauer]

Wondering why we do this in the markForPop and not in the actual handlePop where the route is actually popped?

Also, should we be asserting somewhere that handlePop must actually return true if we're removing a page? This change basically disallows that a page-based route can veto its return, right?

[goderbauer]

Wondering if this can lead to strange animations: It will basically run route-internal pop animations (e.g. to close drawers) while the regular route pop-animation runs, no?

[goderbauer]

Would it make sense to actually add new API to the route for this to tell it: "Hey, you have to go away, if you want to do some internal pops, do them, but after that you have to go away". This would basically leave the decision if it wants to pop drawers in an animated way while the whole route is popped to the route itself.

I would image that in most case where you remove a page with an open drawer you don't really want to run the drawer close animation while the page goes away?

[chunhtai]

handlePop

handlePop can be called as a result of various reason, either a navigator.pop or page update. If we handle it there, I would need to add a flag and drill it down from markForPop to handlePop which is not trivial.

should we be asserting somewhere that handlePop must actually return true

It is possible the handlePop is a result of Navigator.pop on a page-based route. so we can't assert it return true.

It will basically run route-internal pop animations (e.g. to close drawers) while the regular route pop-animation runs.

I don't have a counter argument to this, but if they want to do something special (if they really care) they should do it before they update the Navigator.pages.

Would it make sense to actually add new API to the route for this to tell it: "Hey, you have to go away, if you want to do some internal pops, do them, but after that you have to go away"

I have thought about adding something like void cleanUpInternalPopHandler() and asserts willHandlePopInternally to return false after that, but I figure this will be another breaking change to people and will increase complexity of Route class which is already very confusing with all the canpop, popDisposition, willhandlePop...etc.

I figure the current solution is a good enough solution that will likely solve most if not all use cases. Prior to this change, the page update will just be broken. So this will only be an improvement.

If they really want to customize how route is forced to be removed, we can always add the new API later and remove this while loop didPop.

[goderbauer]

LGTM