-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Add ability for ModalRoutes to ignore pointers during transitions and do so on Cupertino routes
#95757
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
Add ability for ModalRoutes to ignore pointers during transitions and do so on Cupertino routes
#95757
Conversation
7d513c3 to
be7690f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about _shouldIgnoreFocusRequest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes thank you for bringing this up, I had this question as well and forgot to comment on it—the code before was written such that it's following the same logic, but I couldn't quite tell if that was incidental to the fact that they happened to share the same conditions at the time.
It sounds like you'd suggest that we have the focusNodeScope should also be set to the value of the notifier? For context, the cases where _ignorePointer is true is currently a super set of the cases where _shouldIgnoreFocusRequest is true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For context, the cases where _ignorePointer is true is currently a super set of the cases where _shouldIgnoreFocusRequest is true
Is this true? I am not sure if it is already fixed, but if I remember correctly, the cupertino back swipe can swipe the animation to completed/dismissed if you swipe to the edges but still holding your finger...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this true? I am not sure if it is already fixed, but if I remember correctly, the cupertino back swipe can swipe the animation to completed/dismissed if you swipe to the edges but still holding your finger...
Ah ok good point—I'm not sure. From what I can tell looking at the code, it seems like you're probably right about that, but I'm having trouble verifying. If that's the case, then I think I'll also need to update _maybeUpdateIgnorePointer to take this into account, because at the moment that logic is assuming that all user gestures in progress are caught by the logic of whether the animation statuses are forward/reverse. I'll run an example and double-check.
So if that's the case, what does that mean for this focusScopeNode?
--
I do also get some failing tests when I do update the logic:
focus traverse correct when pop multiple page simultaneouslyfocus traversal is correct when popping multiple pages simultaneously - with focused children
Thoughts? I'm a bit unfamiliar with how the focus stuff works but happy to look into it further
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup you're correct: when you start a gesture, and then take it back to the start, if you hold your finger down then the gesture will be in progress but my current logic wouldn't ignore pointers there. I'll make an update to that, because I think it probably should continue to block interactions since that's the current behavior
chunhtai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
This pull request is not suitable for automatic merging in its current state.
|
goderbauer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
secondary LGTM
|
This pull request is not suitable for automatic merging in its current state.
|
|
Hi @willlockwood, I'm sorry, the bot didn't land your PR in time for some reason. Can you resolve the merge conflicts and we will try to land it again? |
|
@willlockwood This is so close to landing, just needs conflicts to be resolved — any chance you have a moment to do that? |
c4e7e55 to
4ceb00d
Compare
|
Nevermind, looks like it's mine 😅 |
|
Can you see if this is a real failure? |
|
The implementation was correct, but the new test broke because the default android transitions builder changed, making the widget I was tapping in my test no longer visible during the transition 👍🏻 |
|
@chunhtai Do you know if this can now be again marked waiting for the tree to go green? |
…itions and do so on `Cupertino` routes (flutter/flutter#95757)
…itions and do so on `Cupertino` routes (flutter/flutter#95757)
|
SkiaPerf noted an improvement to transition build times on this change: Is that expected? |
|
@zanderso One possibility may be the IgnorePointer of the Route is rebuilt less frequently after this patch. but I am not sure whether it would cause the drop |
…itions and do so on `Cupertino` routes (flutter#95757)" This reverts commit 4c0b0be.
…itions and do so on `Cupertino` routes (flutter#95757)" (flutter#104520) This reverts commit 4c0b0be.
|
So is the main issue here the suspicious perf improvement? Just wanna make sure I'm focused on the right thing when I revisit this |
|
No, I don't think that was the reason for the revert. @chunhtai and/or @XilaiZhang will need to give more details about that. |
|
yes this was the error we saw earlier the error was generated by some internal tests written by teams other than flutter. I am not sure if there are open source version of these tests? |
Gotcha, thank you for the information! @XilaiZhang or @chunhtai do you know if that's something you can help me figure out? I'm looking at the implementation again, trying to piece together exactly how this exception would be thrown, but it's difficult without some more information (e.g., even just knowing which |
…nd do so on `Cupertino` routes (flutter#95757)
…itions and do so on `Cupertino` routes (flutter#95757)" (flutter#104520) This reverts commit 4c0b0be.
…itions and do so on `Cupertino` routes (flutter/flutter#95757)
…itions and do so on `Cupertino` routes (flutter/flutter#95757)
This PR fixes #54090
Summary
Currently, the Flutter implementation of
TransitionRoute/ModalRouteensures that, for all material and Cupertino page route transitions, if the route is the current route, then the route is interactive. When the route isn't current, it can't be interacted with. This is the correct logic for android, but on iOS, transitions must be fully complete before the current route can process tap events.This PR adds in the ability for routes to opt-in to the behavior described for native iOS: when the current route is transitioning (e.g., a route has just been pushed on top of it, or the route is in its own entrance animation, etc.), it can be set to ignore pointers until the transition is over.
This behavior would now be configurable via the
ignorePointerDuringTransitionsfield onModalRoute.The implementation here is based on routes tracking whether pointers should be ignored via a notifier that could update any time the
animation's orsecondaryAnimation's status changes. This should be probably the most performant approach, firstly because rebuilds on the overlay's_ModalScope's existingIgnorePointerwill only happen when the value of whether the route should intercept pointers changes, and because whether routes should be interactive could actually change any time there is a change in an animation status.With this change, it's no longer necessary to track for user gestures in
_ModalScope, because that logic is already covered by the AnimationStatus-based approach described above.Alternatives considered:
userGestureInProgress(but this gets very messy because of how often secondary animations get updated without an animation completing its full cycle)TransitionRouteat all (this became necessary because we need to consult thenextRoute's animation status to decide whether the current route should ignore pointers, for example, after popping a fullscreen dialog route)Recordings (slowed down for ease of observation):
Native iOS
Native iOS behavior:
Screen.Recording.2021-12-23.at.4.58.03.PM.mov
Flutter (before)
Cupertino:
Screen.Recording.2021-12-23.at.5.06.38.PM.mov
Material:
Screen.Recording.2021-12-23.at.5.07.42.PM.mov
Flutter (after)
Cupertino:
Screen.Recording.2021-12-23.at.5.01.12.PM.mov
Material:
Screen.Recording.2021-12-23.at.5.03.49.PM.mov
Pre-launch Checklist
///).