Handle Cupertino back gesture interrupted by Navigator push#28756
Handle Cupertino back gesture interrupted by Navigator push#28756HansMuller merged 2 commits intoflutter:masterfrom
Conversation
| @required this.navigator, | ||
| @required this.controller, | ||
| @required this.onEnded, | ||
| @required this.route, |
There was a problem hiding this comment.
since this is private, we can just make it take route (and make it impossible for future maintainers to accidentally give in disjoint instances of these 3 things).
| // back gesture drives this animation to the dismissed status before | ||
| // popping the navigator. | ||
| if (!isCurrent) { | ||
| // removing the route. |
There was a problem hiding this comment.
Add that it itself is responsible for disposing the route and we don't have to do it here.
| gesture = await tester.startGesture(const Offset(5, 300)); | ||
| await gesture.moveBy(const Offset(400, 0)); // drag halfway | ||
| await gesture.up(); | ||
| await tester.pump(const Duration(milliseconds: 275)); // partially dismiss "route" |
There was a problem hiding this comment.
Is this right? Looking at droppedPageForwardAnimationTime and droppedPageBackAnimationTime, it seems like it's a bit more complicated than that.
There was a problem hiding this comment.
I verified that the test is doing the right thing by watching it execute and making sure that (without the fix) it failed exactly as was reported.
There was a problem hiding this comment.
It's not a big deal, I just meant this animation isn't driven by MaterialPageTransition times. It's driven by https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/cupertino/route.dart#L628. It still works. It's just not 75% as written above. I'll clean this up a bit in the next PR.
| await gesture.moveBy(const Offset(400, 0)); // drag halfway | ||
| await gesture.up(); | ||
| await tester.pump(const Duration(milliseconds: 275)); // partially dismiss "route" | ||
| expect(find.text('route'), findsOneWidget); |
There was a problem hiding this comment.
assert where 'route' and 'push' are so if someone refactors the droppedPageForwardAnimationTime/droppedPageBackAnimationTime in the future, we don't accidentally just finish running the whole animation and end up not testing anything here.
There was a problem hiding this comment.
OK, added checks for this.
goderbauer
added
framework
|
LGTM |
| _animating = false; | ||
| if (status == AnimationStatus.dismissed) | ||
| navigator.pop<T>(); // this will cause the route to get disposed, which will dispose us | ||
| route.navigator.removeRoute(route); // this will cause the route to get disposed, which will dispose us |
There was a problem hiding this comment.
Turns out this has a subtle (untested :()problem. This will dispose the route first which means by the time the controller is disposed, the route's reference to the navigator is gone already and we can't call didStopUserGesture. Breaks a few uncommon APIs on the NavigatorObserver. I'll fix it in a next pr.
There was a problem hiding this comment.
Of course, I don’t understand how it all works, but this change caused this problem #29596, I added
route.navigator.pop ();
instead
route.navigator.removeRoute (route);
At first glance, everything works, but what this leads to, I cannot say) you know better,
on stable branch navigator.pop(); =)
There was a problem hiding this comment.
Thanks so much for digging through this @1AlexFix1, I think you're right in that this would break existing NavigatorObserver.didPop.
@HansMuller, let's chat about this next week. Though I'm not sure what is the right solution. Since didPop() actually returns the route that was popped whereas pop() doesn't let you specify which route was popped, this API asymmetry might be the root of the issue.
5 participants
Previously, if a new route was pushed before the iOS "back-swipe" dismiss animation was finished then the
_CupertinoBackGestureControllerwould mistakenly pop the new route when the animation actually did finish.Fixes #28728
Fixes #27334