Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How should Cupertino back gesture interact with onPopPage #137458

Closed
2 tasks done
chunhtai opened this issue Oct 27, 2023 · 3 comments · Fixed by #137792
Closed
2 tasks done

How should Cupertino back gesture interact with onPopPage #137458

chunhtai opened this issue Oct 27, 2023 · 3 comments · Fixed by #137792
Labels
f: routes Navigator, Router, and related APIs. framework flutter/packages/flutter repository. See also f: labels. P2 Important issues not at the top of the work list team-framework Owned by Framework team triaged-framework Triaged by Framework team

Comments

@chunhtai
Copy link
Contributor

Is there an existing issue for this?

Steps to reproduce

Currently the Cupertino back gesture assumes its pop will always remove the route, however it missed the case where onPopPage can still veto the pop. Right now, this will cause the page to hang.

The question is what should they do in this case. I can think several ways forward.

  1. onPopPage can veto the pop, then we have to fix the cupertino back swipe to not freeze the screen if it is veto'd
  2. page based route will disable cupertino back swipe.
  3. cupertino backswipe can't be veto by onPopPage.

It seems to me (2) or (3) is closer to the current cupertino back swipe behavior, but (2) may be too aggressive. If we want to implement (3) we will need a new API in Navigator to still give customer a chance to clean up their page list.

Actual results

fix the issue.

Logs

Logs
<!-- Paste your logs here -->

Flutter Doctor output

Doctor output
<!-- Paste your output here -->
@chunhtai chunhtai added framework flutter/packages/flutter repository. See also f: labels. f: routes Navigator, Router, and related APIs. team-framework Owned by Framework team labels Oct 27, 2023
@goderbauer
Copy link
Member

In general, I think that vetoing a route that is dismissed via a back swipe is bad UX. From a user perspective, the expectation is that a back swipe will always be successful. If for a particular route you cannot guarantee that, it probably shouldn't support the back swipe (and instead only be dismissible by clicking a button).

I agree, that disabling the swipe for all page-based routes is too aggressive, though.

It sounds like 3 may be the right option here? Do you have a proposal for what that API would look like, though?

We probably will also run into the same problem with Android's new in-app predictable back, which is pretty similar to the current back swipe in iOS.

Do we have an easy way to disable the backswipe for a particular route if one wants to do that to be able to veto its pop?

@chunhtai
Copy link
Contributor Author

It sounds like 3 may be the right option here? Do you have a proposal for what that API would look like, though?

Yes, I think so, but this will be the most breaking way to handle this out of the 3

One idea is to add a new onPageDidPop callback, but I am struggling to figure out a way that is less breaking. It seems to me this onDidPopPage will need to be called if onPopPage return true, but that will make the migration a bit tricky. Customer may need to move part of the onPopPage implementation to onDidPopPage; otherwise they may be cleaning up the page list twice unknowingly

We probably will also run into the same problem with Android's new in-app predictable back, which is pretty similar to the current back swipe in iOS.

Yes the PopScope is enabling the predictive back, but the onPopPage can still veto it. It does seem like the best choice will be to go with (3). it we go with (2) the page apis will have to wire special logic to disable each of force pop mechanisms.

Do we have an easy way to disable the backswipe for a particular route if one wants to do that to be able to veto its pop?

They will have to build a WillPopScope or PopScope in the route's widget subtree. The alternative is to add more parameter to Page class like Page.popGestureEnabled, I think that can be useful in some way, but I really don't want to further complicated the pop related API....

@goderbauer goderbauer added P2 Important issues not at the top of the work list triaged-framework Triaged by Framework team labels Oct 31, 2023
@chunhtai chunhtai mentioned this issue Nov 2, 2023
8 tasks
auto-submit bot pushed a commit that referenced this issue May 13, 2024
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
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
f: routes Navigator, Router, and related APIs. framework flutter/packages/flutter repository. See also f: labels. P2 Important issues not at the top of the work list team-framework Owned by Framework team triaged-framework Triaged by Framework team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants