Fix gestures not responding on swipe back #116694

MitchellGoodwin · 2022-12-07T23:51:23Z

Addresses #73026 and #92978. Summary of the two issues: during the back swipe animation, users gestures are locked until the animation completes. However, the animation follows this curve so during the back half of the animation, it looks to the user that the animation is complete and they can use a gesture, but it is still imperceptibly going.

A possible solution is to adjust the animation curve, but as it was carefully made to match iOS, I decided instead to change the listener for the animation to re-enable gestures near the end of the animation, instead of at it. A side effect of removing the listener while it's in flight is that it seems to very slightly speed it up at the very end to were it theoretically should be, hence some of the value changes in the tests.

Before, the behavior if you tap soon after the animation finishes, the button will not respond:

Screen.Recording.2022-12-07.at.3.22.44.PM.mov

After it is responsive much sooner, but will still not respond while the majority of the animation is playing:

Screen.Recording.2022-12-07.at.3.33.45.PM.mov

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.

LongCatIsLooong

The approach lgtm just a few nits.

@chunhtai could you take a look?

LongCatIsLooong · 2022-12-08T18:34:25Z

packages/flutter/lib/src/cupertino/route.dart

+      // Only apply the value listener if going back to the previous route, as
+      // allowing clicks on the top route while in motion may trigger errors
+      // for going to a route that is already being moved to.
+      if (!animateForward) {


It looks like based on whether we're animating forward, we need either the animationValueCallback or the animationStatusCallback, but never both?

Ah nvm it looks like the route has access to the controller too.

My worry is somehow the animation would avoid the animationValueCallback and the user would be locked out of gestures, especially because the animation duration can vary a lot. I was worried it would somehow avoid checking the 0.2 value. It was possible from testing locally if the value was extreme, like 0.00001, so I didn't want to risk it. That was my thought process, anyway.

If it's unnecessary, I'm open to removing it though.

LongCatIsLooong · 2022-12-08T18:36:24Z

packages/flutter/lib/src/cupertino/route.dart

+      // when the animation visibly ends.
+      animationValueCallback = () {
+        if (controller.value < 0.02) {
+          if (navigator.userGestureInProgress) {


Do we need this check? Since this callback is called only once, this doesn't seem necessary?

Probably not. This is a fragment from experimenting with other implementations that were unnecessarily complicated. I left it in for safety, but I don't think it would be necessary.

LongCatIsLooong · 2022-12-08T18:37:06Z

packages/flutter/lib/src/cupertino/route.dart

@@ -788,13 +788,38 @@ class _CupertinoBackGestureController<T> {
    }

    if (controller.isAnimating) {
+      late VoidCallback animationValueCallback;
+      late AnimationStatusListener animationStatusCallback;


nit: consider using an inline function for late binding, so the binding is also final.

LongCatIsLooong · 2022-12-08T19:15:00Z

On second thought, would it be safer if we change the curve used for the animation? The comment claims that

    // This curve has been determined through rigorously eyeballing native iOS
    // animations.

but since it looks like the animation is basically stationary towards the end maybe the eyeballing isn't as accurate as it sounded.

chunhtai · 2022-12-08T19:37:06Z

packages/flutter/lib/src/cupertino/route.dart

+      // false near the end, but just before so the app can be interacted with
+      // when the animation visibly ends.
+      animationValueCallback = () {
+        if (controller.value < 0.02) {


why 0.02? we try to avoid these magic numbers inline in the code, probably store this with in a const variable and add some comments.

chunhtai · 2022-12-08T19:56:14Z

packages/flutter/lib/src/cupertino/route.dart

+      // depends on userGestureInProgress. Switch the userGestureInProgress to
+      // false near the end, but just before so the app can be interacted with
+      // when the animation visibly ends.
+      animationValueCallback = () {


If we could make CupertinoPageTransition's animation curve to be resilient between user drag and ballistic animation. We can just call navigator.didStopUserGesture(); directly without this 0.02 workaround right? It seems to me that will be a better solution if we can make it so. One way to do it is make CupertinoPageTransition a statefulwidget and only update the _primaryPositionAnimation and _secondaryPositionAnimation when the input animation status changes. WDYT?

As in on drag end, we'd update _primaryPositionAnimation? navigator.didStopUserGesture() isn't called elsewhere and I'm not sure where we'd move it to in that case. By calling it directly do you mean differently from the status listener that already existed?

No I think the reason we can't call navigator.didStopUserGesture() is that it will cause CupertinoRouteTransitionMixin.isPopGestureInProgress to return false, which will then make CupertinoPageTransition to switch its _primaryPositionAnimation/_secondaryPositionAnimation from linear transition to CurvedAnimation thus may result in a jump. See

flutter/packages/flutter/lib/src/cupertino/route.dart

Line 472 in bebea0c

_primaryPositionAnimation =

If we can make it so that CupertinoPageTransition will not switch the animation when CupertinoRouteTransitionMixin.isPopGestureInProgress change from true to false if the animation is ongoing, we can directly call navigator.didStopUserGesture() here which will unblock user from tapping the screen.

Ok, that makes sense. That explains some things I was seeing in the tests.

MitchellGoodwin · 2022-12-08T20:48:36Z

On second thought, would it be safer if we change the curve used for the animation? The comment claims that
    // This curve has been determined through rigorously eyeballing native iOS
    // animations.
but since it looks like the animation is basically stationary towards the end maybe the eyeballing isn't as accurate as it sounded.

From reading #22862 it looks like the native transition does have a bit of a long tail where it doesn't look like it's doing anything to the human eye, though we are a little off. This PR #104643 which hasn't been updated in a while changed the curve to match the visible curve a bit better, but I was wanting to find a solution that left in that section were it's settling very slightly.

LongCatIsLooong · 2022-12-08T21:20:35Z

From what I'm seeing this is a CASpringAnimation, so the curve should be a sine function (when I tested it the initial velocity was set to 0 but I'm not sure if that's going to be the case if you fling) instead of a Bézier curve. https://master-api.flutter.dev/flutter/animation/Curves/fastLinearToSlowEaseIn-constant.html definitely looks a bit too flat once it's past the t/2 mark

MitchellGoodwin · 2022-12-08T21:47:50Z

From @OscarFarrer using a tool to compare the frames, the curve seems to be something like

Which doesn't match the curve in use, and doesn't seem to match anything we have at the moment.

Edit: this doesn't work very smoothly with fast swipes.

MitchellGoodwin · 2022-12-13T18:26:01Z

I'm going to switch over to working on building the route transition comparison tool (#22862) in order to have better confidence that the animation matches on iOS as it should, without taking shots in the dark. Closing this for now.

MitchellGoodwin added 2 commits December 7, 2022 11:10

Allow user gestures near end of swipe back

301e93c

Documentation clarification

6a08830

flutter-dashboard bot added f: cupertino flutter/packages/flutter/cupertino repository f: routes Navigator, Router, and related APIs. framework flutter/packages/flutter repository. See also f: labels. labels Dec 7, 2022

Update page test

51ad9b0

flutter-dashboard bot added the f: material design flutter/packages/flutter/material repository. label Dec 8, 2022

MitchellGoodwin added 2 commits December 8, 2022 08:46

Add back space

3caecc5

Remove white space

002ec27

MitchellGoodwin marked this pull request as ready for review December 8, 2022 17:32

MitchellGoodwin changed the title ~~Gestures on swipe back~~ Fix gestures not responding on swipe back Dec 8, 2022

MitchellGoodwin requested a review from LongCatIsLooong December 8, 2022 17:33

LongCatIsLooong reviewed Dec 8, 2022

View reviewed changes

chunhtai self-requested a review December 8, 2022 19:31

chunhtai reviewed Dec 8, 2022

View reviewed changes

MitchellGoodwin closed this Dec 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix gestures not responding on swipe back #116694

Fix gestures not responding on swipe back #116694

MitchellGoodwin commented Dec 7, 2022 •

edited

LongCatIsLooong left a comment

LongCatIsLooong Dec 8, 2022

LongCatIsLooong Dec 8, 2022

MitchellGoodwin Dec 8, 2022

MitchellGoodwin Dec 8, 2022

LongCatIsLooong Dec 8, 2022

MitchellGoodwin Dec 8, 2022

LongCatIsLooong Dec 8, 2022

LongCatIsLooong commented Dec 8, 2022 •

edited

chunhtai Dec 8, 2022

chunhtai Dec 8, 2022

MitchellGoodwin Dec 8, 2022

chunhtai Dec 8, 2022 •

edited

MitchellGoodwin Dec 8, 2022

MitchellGoodwin commented Dec 8, 2022

LongCatIsLooong commented Dec 8, 2022 •

edited

MitchellGoodwin commented Dec 8, 2022 •

edited

MitchellGoodwin commented Dec 13, 2022

Fix gestures not responding on swipe back #116694

Fix gestures not responding on swipe back #116694

Conversation

MitchellGoodwin commented Dec 7, 2022 • edited

Pre-launch Checklist

LongCatIsLooong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LongCatIsLooong commented Dec 8, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunhtai Dec 8, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MitchellGoodwin commented Dec 8, 2022

LongCatIsLooong commented Dec 8, 2022 • edited

MitchellGoodwin commented Dec 8, 2022 • edited

MitchellGoodwin commented Dec 13, 2022

MitchellGoodwin commented Dec 7, 2022 •

edited

LongCatIsLooong commented Dec 8, 2022 •

edited

chunhtai Dec 8, 2022 •

edited

LongCatIsLooong commented Dec 8, 2022 •

edited

MitchellGoodwin commented Dec 8, 2022 •

edited