Make sure everything in the Cupertino page transition can be linear when back swiping #28629

xster · 2019-02-28T04:35:46Z

Description

Currently, the route transition can become disconnected between the top and the bottom pages of a Cupertino transition because they can follow different curves. They should both be linearToEaseOut if animated but both be linear if driven by a back swipe.

We should be checking for the participating animations for whether they're part of a swipe rather than checking the route.

Related Issues

#28529

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
My PR includes tests for all changed/updated/fixed behaviors (See [Test Coverage]).
All existing and new tests are passing.
I updated/added relevant documentation (doc comments with ///).
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
I signed the [CLA].
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read [Handling breaking changes]).
No, this is not a breaking change.

xster · 2019-02-28T04:36:27Z

@HansMuller want to get your feedback before I add the tests

HansMuller · 2019-02-28T21:58:44Z

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

@@ -188,12 +188,21 @@ class CupertinoPageRoute<T> extends PageRoute<T> {

  /// True if a Cupertino pop gesture is currently underway for [route].
  ///
+  /// This returns true for both the top route and the bottom route of a


I'm just parachuting into this but ... it's not clear what a "Cupertino pop gesture" is. I don't see a definition elsewhere.

I assume that you just mean that this route is in the middle of running its animation as a result of being popped off the navigator's stack, or its in the middle of running its "secondary" animation because being exposed because the route above it was popped.

This is still confusing since exactly what "pop gesture" means isn't self evident. Since we're just returning the value of route.navigator.userGestureInProgress you could just say as much and provide an explanation there.

HansMuller · 2019-02-28T22:06:24Z

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

+  static Animation<double> _animationControllerForTransitionAnimation(Animation<double> animation) {
+    Animation<double> controller = animation;
+    while(controller is ProxyAnimation || controller is TrainHoppingAnimation) {
+      final Animation<double> proxy = controller;


This works because we believe that the implementation only creates these two animation types? If someone changed the implementation and started using FooAnimationWrapper or, worse still _FooAnimationWrapper, we'd just silently fail, right?

Took a different approach

HansMuller · 2019-02-28T22:44:47Z

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

-  static final Set<PageRoute<dynamic>> _popGestureInProgress = Set<PageRoute<dynamic>>();
+  static bool isPopGestureInProgress(PageRoute<dynamic> route) {
+    return _popGestureInProgress.contains(_animationControllerForTransitionAnimation(route.animation))
+        || _popGestureInProgress.contains(_animationControllerForTransitionAnimation(route.secondaryAnimation));


I'm not sure why _animationControllerForTransitionAnimation will always return route.secondaryAnimation if a route is using it. It seems like it would in simple cases, but all cases?

Took a different approach

xster · 2019-03-05T01:18:27Z

So I took a different approach and didn't need to add a previous route tracker on the route itself.

Navigator has a userGestureInProgress that I can use. Removed a static from the routes class. Added a bunch of tests. Also fixed https://github.com/flutter/flutter/pull/28756/files#r262295964. It diffbases on top of #28855.

HansMuller

This looks mostly fine although I think future generations will appreciate a little less lingo.

HansMuller · 2019-03-08T18:16:24Z

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

@@ -188,12 +188,21 @@ class CupertinoPageRoute<T> extends PageRoute<T> {

  /// True if a Cupertino pop gesture is currently underway for [route].
  ///
+  /// This returns true for both the top route and the bottom route of a


This is still confusing since exactly what "pop gesture" means isn't self evident. Since we're just returning the value of route.navigator.userGestureInProgress you could just say as much and provide an explanation there.

HansMuller · 2019-03-08T18:25:55Z

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

@@ -646,9 +658,9 @@ class _CupertinoBackGestureController<T> {
      controller.removeStatusListener(_handleStatusChanged);
    }
    _animating = false;
+    onEnded();


Not sure why we now want to do this first. Previously it seemed like effectively an else clause. Why would we want to unconditionally dispose the route before removing it?

I thought I wrote it somewhere in the PR but didn't. This is a bug on everything downstream from didStopUserGesture such as NavigatorObserver.didStopUserGesture or NavigatorState.userGestureInProgress. It never worked because the route would be disposed before the back controller would be disposed. We were just lucky that those APIs weren't really used.

Added new tests.

HansMuller · 2019-03-08T18:27:41Z

packages/flutter/lib/src/widgets/routes.dart

@@ -114,6 +114,12 @@ abstract class TransitionRoute<T> extends OverlayRoute<T> {
  AnimationController get controller => _controller;
  AnimationController _controller;

+  /// The animation for the route being pushed on top of this route. This
+  /// animation lets this route coordinate with the entrance and exit transition
+  /// of routes pushed on top of this route.


of the route being pushed on top of

Doesn't the secondary animation also apply to the route that's being exposed because the route above it is being popped?

I just moved this block from below but all these docs are wrong. Re-editing them.

It's not "routes pushed on top of this route" plural, it's just the one route pushed on top of this one?

Thanks, indeed it should be singular

HansMuller · 2019-03-08T18:29:17Z

packages/flutter/test/cupertino/route_test.dart

-    // and then press the button. MaterialPageTransition duration is 300ms,
-    // 275 = 300 * 0.75.
+    // Run the dismiss animation 60%, which exposes the route "push" button,
+    // and then press the button. A drag dropped animation is 400ms when dropped


What is a "drag dropped animation" and where does 400ms come from?

I moved this comment further down

HansMuller · 2019-03-08T18:31:04Z

packages/flutter/test/cupertino/route_test.dart

    await gesture.up();
-    await tester.pump(const Duration(milliseconds: 275)); // partially dismiss "route"
-    expect(find.text('route'), findsOneWidget);
+    await tester.pump(); // Trigger the dropped snapping animation.


What is a "dropped snapping" animation?

Generally speaking I think it would be helpful to explain this stuff in terms that would make sense to a developer who only knows that it's possible to drag a platform-iOS route from the left edge to initiate a navigator pop.

HansMuller · 2019-03-08T18:36:57Z

packages/flutter/test/cupertino/route_test.dart

+    );
+
+    tester.state<NavigatorState>(find.byType(Navigator)).push(route2);
+


It would be helpful to explain that the numbers below are based on animating a Foo curve over a duration of Bar

Doh, found #29068 as I looked into it

Added more comments here

HansMuller · 2019-03-08T18:38:17Z

packages/flutter/test/cupertino/route_test.dart

+    expect(tester.getTopLeft(find.text('2')).dx, moreOrLessEquals(659, epsilon: 1));
+  });
+
+  testWidgets('Dragged pop gesture is linear', (WidgetTester tester) async {


Why are all of the epsilons needed if we're not curving the animation?

Just to avoid dealing with all the irrational numbers from dividing by 3. But it's only needed on the bottom route. Tweaking.

…hen back swiping

goderbauer

This looks good to me. I'll leave the final review to Hans.

HansMuller

Sorry about the delay, LGTM

HansMuller · 2019-03-27T22:41:20Z

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

-  /// True if a Cupertino pop gesture is currently underway for [route].
+  /// True if an iOS-style back swipe pop gesture is currently underway for [route].
+  ///
+  /// This just check for the route's [NavigatorState.userGestureInProgress].


This just checks the route's ...

HansMuller · 2019-03-27T22:42:38Z

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

-  static bool isPopGestureInProgress(PageRoute<dynamic> route) => _popGestureInProgress.contains(route);
-  static final Set<PageRoute<dynamic>> _popGestureInProgress = <PageRoute<dynamic>>{};
+  static bool isPopGestureInProgress(PageRoute<dynamic> route) {
+    return route.navigator.userGestureInProgress;


I'm surprised that we need this method, but OK. I like the fact that it's possible to just ask the navigator.

We absolutely don't need this method since it adds more indirection confusion. But we can't break the API.

HansMuller · 2019-03-27T22:46:47Z

packages/flutter/lib/src/widgets/routes.dart

@@ -114,6 +114,12 @@ abstract class TransitionRoute<T> extends OverlayRoute<T> {
  AnimationController get controller => _controller;
  AnimationController _controller;

+  /// The animation for the route being pushed on top of this route. This
+  /// animation lets this route coordinate with the entrance and exit transition
+  /// of routes pushed on top of this route.


It's not "routes pushed on top of this route" plural, it's just the one route pushed on top of this one?

xster requested a review from HansMuller February 28, 2019 04:35

googlebot added the cla: yes label Feb 28, 2019

xster force-pushed the linear-swipe branch from c749a4a to 786728c Compare February 28, 2019 04:38

zoechi added framework flutter/packages/flutter repository. See also f: labels. f: cupertino flutter/packages/flutter/cupertino repository f: routes Navigator, Router, and related APIs. labels Feb 28, 2019

HansMuller reviewed Feb 28, 2019

View reviewed changes

xster added the work in progress; do not review label Mar 1, 2019

xster force-pushed the linear-swipe branch 2 times, most recently from aef23e8 to 74658fe Compare March 5, 2019 00:58

xster requested a review from HansMuller March 5, 2019 01:16

xster removed the work in progress; do not review label Mar 5, 2019

xster force-pushed the linear-swipe branch 2 times, most recently from 4e0fe10 to 55ea847 Compare March 7, 2019 22:51

HansMuller reviewed Mar 8, 2019

View reviewed changes

xster force-pushed the linear-swipe branch from 90bf03b to 4b5068c Compare March 8, 2019 23:19

xster requested a review from HansMuller March 8, 2019 23:22

xster added 6 commits March 20, 2019 15:00

Make sure everything in the Cupertino page transition can be linear w…

f4ff91c

…hen back swiping

lint

f44a17e

add some tests

236aa7a

test

39789ac

Change mechanism to use navigator.userGestureInProgress instead

303b4fb

review

ee43060

xster force-pushed the linear-swipe branch from 4b5068c to ee43060 Compare March 20, 2019 22:01

xster requested a review from goderbauer March 21, 2019 20:47

goderbauer reviewed Mar 22, 2019

View reviewed changes

HansMuller approved these changes Mar 27, 2019

View reviewed changes

review

9d7ae72

xster merged commit d166a8d into flutter:master Mar 29, 2019

xster deleted the linear-swipe branch March 29, 2019 22:56

github-actions bot locked as resolved and limited conversation to collaborators Aug 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure everything in the Cupertino page transition can be linear when back swiping #28629

Make sure everything in the Cupertino page transition can be linear when back swiping #28629

xster commented Feb 28, 2019 •

edited

Loading

xster commented Feb 28, 2019

HansMuller Feb 28, 2019

HansMuller Mar 8, 2019

xster Mar 8, 2019

HansMuller Feb 28, 2019

xster Mar 5, 2019

HansMuller Feb 28, 2019

xster Mar 5, 2019

xster commented Mar 5, 2019

HansMuller left a comment

HansMuller Mar 8, 2019

HansMuller Mar 8, 2019

xster Mar 8, 2019

HansMuller Mar 8, 2019

xster Mar 8, 2019

HansMuller Mar 27, 2019

xster Mar 27, 2019

HansMuller Mar 8, 2019

xster Mar 8, 2019

HansMuller Mar 8, 2019

xster Mar 8, 2019

HansMuller Mar 8, 2019

xster Mar 8, 2019

xster Mar 8, 2019

HansMuller Mar 8, 2019

xster Mar 8, 2019

goderbauer left a comment

HansMuller left a comment

HansMuller Mar 27, 2019

xster Mar 27, 2019

HansMuller Mar 27, 2019

xster Mar 27, 2019

HansMuller Mar 27, 2019

		);

		tester.state<NavigatorState>(find.byType(Navigator)).push(route2);

Make sure everything in the Cupertino page transition can be linear when back swiping #28629

Make sure everything in the Cupertino page transition can be linear when back swiping #28629

Conversation

xster commented Feb 28, 2019 • edited Loading

Description

Related Issues

Checklist

Breaking Change

xster commented Feb 28, 2019

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

xster commented Mar 5, 2019

HansMuller 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

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

Choose a reason for hiding this comment

goderbauer left a comment

Choose a reason for hiding this comment

HansMuller 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

xster commented Feb 28, 2019 •

edited

Loading