From d166a8d81e22c56af4e4e4c268c64c42e51a3287 Mon Sep 17 00:00:00 2001 From: xster Date: Fri, 29 Mar 2019 15:56:18 -0700 Subject: [PATCH] Make sure everything in the Cupertino page transition can be linear when back swiping (#28629) --- packages/flutter/lib/src/cupertino/route.dart | 93 ++++---- packages/flutter/lib/src/widgets/routes.dart | 12 +- .../flutter/test/cupertino/route_test.dart | 205 +++++++++++++++++- packages/flutter/test/material/page_test.dart | 21 +- 4 files changed, 272 insertions(+), 59 deletions(-) diff --git a/packages/flutter/lib/src/cupertino/route.dart b/packages/flutter/lib/src/cupertino/route.dart index eba6a787945..ad4993ac628 100644 --- a/packages/flutter/lib/src/cupertino/route.dart +++ b/packages/flutter/lib/src/cupertino/route.dart @@ -180,22 +180,19 @@ class CupertinoPageRoute extends PageRoute { return nextRoute is CupertinoPageRoute && !nextRoute.fullscreenDialog; } - @override - void dispose() { - _popGestureInProgress.remove(this); - super.dispose(); - } - - /// 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 the route's [NavigatorState.userGestureInProgress]. /// /// See also: /// /// * [popGestureEnabled], which returns true if a user-triggered pop gesture /// would be allowed. - static bool isPopGestureInProgress(PageRoute route) => _popGestureInProgress.contains(route); - static final Set> _popGestureInProgress = >{}; + static bool isPopGestureInProgress(PageRoute route) { + return route.navigator.userGestureInProgress; + } - /// True if a Cupertino pop gesture is currently underway for this route. + /// True if an iOS-style back swipe pop gesture is currently underway for this route. /// /// See also: /// @@ -233,10 +230,15 @@ class CupertinoPageRoute extends PageRoute { if (route.fullscreenDialog) return false; // If we're in an animation already, we cannot be manually swiped. - if (route.controller.status != AnimationStatus.completed) + if (route.animation.status != AnimationStatus.completed) + return false; + // If we're being popped into, we also cannot be swiped until the pop above + // it completes. This translates to our secondary animation being + // dismissed. + if (route.secondaryAnimation.status != AnimationStatus.dismissed) return false; // If we're in a gesture already, we cannot start another. - if (_popGestureInProgress.contains(route)) + if (isPopGestureInProgress(route)) return false; // Looks like a back gesture would be welcome! @@ -266,9 +268,7 @@ class CupertinoPageRoute extends PageRoute { // gesture is detected. The returned controller handles all of the subsequent // drag events. static _CupertinoBackGestureController _startPopGesture(PageRoute route) { - assert(!_popGestureInProgress.contains(route)); assert(_isPopGestureEnabled(route)); - _popGestureInProgress.add(route); _CupertinoBackGestureController backController; backController = _CupertinoBackGestureController( @@ -277,7 +277,6 @@ class CupertinoPageRoute extends PageRoute { onEnded: () { backController?.dispose(); backController = null; - _popGestureInProgress.remove(route); }, ); return backController; @@ -313,9 +312,12 @@ class CupertinoPageRoute extends PageRoute { return CupertinoPageTransition( primaryRouteAnimation: animation, secondaryRouteAnimation: secondaryAnimation, + // Check if the route has an animation that's currently participating + // in a back swipe gesture. + // // In the middle of a back gesture drag, let the transition be linear to // match finger motions. - linearTransition: _popGestureInProgress.contains(route), + linearTransition: isPopGestureInProgress(route), child: _CupertinoBackGestureDetector( enabledCallback: () => _isPopGestureEnabled(route), onStartPopGesture: () => _startPopGesture(route), @@ -354,28 +356,38 @@ class CupertinoPageTransition extends StatelessWidget { @required this.child, @required bool linearTransition, }) : assert(linearTransition != null), - _primaryPositionAnimation = (linearTransition ? primaryRouteAnimation : - // The curves below have been rigorously derived from plots of native - // iOS animation frames. Specifically, a video was taken of a page - // transition animation and the distance in each frame that the page - // moved was measured. A best fit bezier curve was the fitted to the - // point set, which is linearToEaseIn. Conversely, easeInToLinear is the - // reflection over the origin of linearToEaseIn. - CurvedAnimation( - parent: primaryRouteAnimation, - curve: Curves.linearToEaseOut, - reverseCurve: Curves.easeInToLinear, - ) - ).drive(_kRightMiddleTween), - _secondaryPositionAnimation = CurvedAnimation( - parent: secondaryRouteAnimation, - curve: Curves.linearToEaseOut, - reverseCurve: Curves.easeInToLinear, - ).drive(_kMiddleLeftTween), - _primaryShadowAnimation = CurvedAnimation( - parent: primaryRouteAnimation, - curve: Curves.linearToEaseOut, - ).drive(_kGradientShadowTween), + _primaryPositionAnimation = + (linearTransition + ? primaryRouteAnimation + : CurvedAnimation( + // The curves below have been rigorously derived from plots of native + // iOS animation frames. Specifically, a video was taken of a page + // transition animation and the distance in each frame that the page + // moved was measured. A best fit bezier curve was the fitted to the + // point set, which is linearToEaseIn. Conversely, easeInToLinear is the + // reflection over the origin of linearToEaseIn. + parent: primaryRouteAnimation, + curve: Curves.linearToEaseOut, + reverseCurve: Curves.easeInToLinear, + ) + ).drive(_kRightMiddleTween), + _secondaryPositionAnimation = + (linearTransition + ? secondaryRouteAnimation + : CurvedAnimation( + parent: secondaryRouteAnimation, + curve: Curves.linearToEaseOut, + reverseCurve: Curves.easeInToLinear, + ) + ).drive(_kMiddleLeftTween), + _primaryShadowAnimation = + (linearTransition + ? primaryRouteAnimation + : CurvedAnimation( + parent: primaryRouteAnimation, + curve: Curves.linearToEaseOut, + ) + ).drive(_kGradientShadowTween), super(key: key); // When this page is coming in to cover another page. @@ -618,7 +630,6 @@ class _CupertinoBackGestureController { animateForward = velocity > 0 ? false : true; else animateForward = controller.value > 0.5 ? true : false; - if (animateForward) { // The closer the panel is to dismissing, the shorter the animation is. // We want to cap the animation time, but we want to use a linear curve @@ -650,9 +661,9 @@ class _CupertinoBackGestureController { controller.removeStatusListener(_handleStatusChanged); } _animating = false; + onEnded(); if (status == AnimationStatus.dismissed) - route.navigator.removeRoute(route); // this will cause the route to get disposed, which will dispose us - onEnded(); // this will call dispose if popping the route failed to do so + route.navigator.removeRoute(route); // This also disposes the route. } void dispose() { diff --git a/packages/flutter/lib/src/widgets/routes.dart b/packages/flutter/lib/src/widgets/routes.dart index eb23e0095f9..798d960d8c3 100644 --- a/packages/flutter/lib/src/widgets/routes.dart +++ b/packages/flutter/lib/src/widgets/routes.dart @@ -114,6 +114,12 @@ abstract class TransitionRoute extends OverlayRoute { 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 route pushed on top of this route. + Animation get secondaryAnimation => _secondaryAnimation; + final ProxyAnimation _secondaryAnimation = ProxyAnimation(kAlwaysDismissedAnimation); + /// Called to create the animation controller that will drive the transitions to /// this route from the previous one, and back to the previous route from this /// one. @@ -164,12 +170,6 @@ abstract class TransitionRoute extends OverlayRoute { changedInternalState(); } - /// 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. - Animation get secondaryAnimation => _secondaryAnimation; - final ProxyAnimation _secondaryAnimation = ProxyAnimation(kAlwaysDismissedAnimation); - @override void install(OverlayEntry insertionPoint) { assert(!_transitionCompleter.isCompleted, 'Cannot install a $runtimeType after disposing it.'); diff --git a/packages/flutter/test/cupertino/route_test.dart b/packages/flutter/test/cupertino/route_test.dart index dd152840ae3..4250fcd9dd9 100644 --- a/packages/flutter/test/cupertino/route_test.dart +++ b/packages/flutter/test/cupertino/route_test.dart @@ -309,9 +309,8 @@ void main() { expect(find.text('route'), findsNothing); - // Run the dismiss animation 75%, which exposes the route "push" button, - // 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. await tester.tap(find.text('push')); await tester.pumpAndSettle(); @@ -319,10 +318,27 @@ void main() { expect(find.text('push'), findsNothing); gesture = await tester.startGesture(const Offset(5, 300)); - await gesture.moveBy(const Offset(400, 0)); // drag halfway + 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); + // Trigger the snapping animation. + // Since the back swipe drag was brought to >=50% of the screen, it will + // self snap to finish the pop transition as the gesture is lifted. + // + // This drag drop animation is 400ms when dropped exactly halfway + // (800 / [pixel distance remaining], see + // _CupertinoBackGestureController.dragEnd). It follows a curve that is very + // steep initially. + await tester.pump(); + expect( + tester.getTopLeft(find.ancestor(of: find.text('route'), matching: find.byType(CupertinoPageScaffold))), + const Offset(400, 0), + ); + // Let the dismissing snapping animation go 60%. + await tester.pump(const Duration(milliseconds: 240)); + expect( + tester.getTopLeft(find.ancestor(of: find.text('route'), matching: find.byType(CupertinoPageScaffold))).dx, + moreOrLessEquals(798, epsilon: 1), + ); await tester.tap(find.text('push')); await tester.pumpAndSettle(); expect(find.text('route'), findsOneWidget); @@ -431,4 +447,181 @@ void main() { await tester.pump(const Duration(milliseconds: 40)); expect(tester.getTopLeft(find.byType(Placeholder)).dy, closeTo(600.0, 0.1)); }); + + testWidgets('Animated push/pop is not linear', (WidgetTester tester) async { + await tester.pumpWidget( + const CupertinoApp( + home: Text('1'), + ), + ); + + final CupertinoPageRoute route2 = CupertinoPageRoute( + builder: (BuildContext context) { + return const CupertinoPageScaffold( + child: Text('2'), + ); + } + ); + + tester.state(find.byType(Navigator)).push(route2); + // The whole transition is 400ms based on CupertinoPageRoute.transitionDuration. + // Break it up into small chunks. + + await tester.pump(); + await tester.pump(const Duration(milliseconds: 50)); + expect(tester.getTopLeft(find.text('1')).dx, moreOrLessEquals(-87, epsilon: 1)); + expect(tester.getTopLeft(find.text('2')).dx, moreOrLessEquals(537, epsilon: 1)); + + await tester.pump(const Duration(milliseconds: 50)); + expect(tester.getTopLeft(find.text('1')).dx, moreOrLessEquals(-166, epsilon: 1)); + expect(tester.getTopLeft(find.text('2')).dx, moreOrLessEquals(301, epsilon: 1)); + + await tester.pump(const Duration(milliseconds: 50)); + // Translation slows down as time goes on. + expect(tester.getTopLeft(find.text('1')).dx, moreOrLessEquals(-220, epsilon: 1)); + expect(tester.getTopLeft(find.text('2')).dx, moreOrLessEquals(141, epsilon: 1)); + + // Finish the rest of the animation + await tester.pump(const Duration(milliseconds: 250)); + + tester.state(find.byType(Navigator)).pop(); + await tester.pump(); + await tester.pump(const Duration(milliseconds: 50)); + expect(tester.getTopLeft(find.text('1')).dx, moreOrLessEquals(-179, epsilon: 1)); + expect(tester.getTopLeft(find.text('2')).dx, moreOrLessEquals(262, epsilon: 1)); + + await tester.pump(const Duration(milliseconds: 50)); + expect(tester.getTopLeft(find.text('1')).dx, moreOrLessEquals(-100, epsilon: 1)); + expect(tester.getTopLeft(find.text('2')).dx, moreOrLessEquals(499, epsilon: 1)); + + await tester.pump(const Duration(milliseconds: 50)); + // Translation slows down as time goes on. + expect(tester.getTopLeft(find.text('1')).dx, moreOrLessEquals(-47, epsilon: 1)); + expect(tester.getTopLeft(find.text('2')).dx, moreOrLessEquals(659, epsilon: 1)); + }); + + testWidgets('Dragged pop gesture is linear', (WidgetTester tester) async { + await tester.pumpWidget( + const CupertinoApp( + home: Text('1'), + ), + ); + + final CupertinoPageRoute route2 = CupertinoPageRoute( + builder: (BuildContext context) { + return const CupertinoPageScaffold( + child: Text('2'), + ); + } + ); + + tester.state(find.byType(Navigator)).push(route2); + + await tester.pumpAndSettle(); + + expect(find.text('1'), findsNothing); + expect(tester.getTopLeft(find.text('2')).dx, moreOrLessEquals(0)); + + final TestGesture swipeGesture = await tester.startGesture(const Offset(5, 100)); + + await swipeGesture.moveBy(const Offset(100, 0)); + await tester.pump(); + expect(tester.getTopLeft(find.text('1')).dx, moreOrLessEquals(-233, epsilon: 1)); + expect(tester.getTopLeft(find.text('2')).dx, moreOrLessEquals(100)); + + await swipeGesture.moveBy(const Offset(100, 0)); + await tester.pump(); + expect(tester.getTopLeft(find.text('1')).dx, moreOrLessEquals(-200)); + expect(tester.getTopLeft(find.text('2')).dx, moreOrLessEquals(200)); + + // Moving by the same distance each time produces linear movements on both + // routes. + await swipeGesture.moveBy(const Offset(100, 0)); + await tester.pump(); + expect(tester.getTopLeft(find.text('1')).dx, moreOrLessEquals(-166, epsilon: 1)); + expect(tester.getTopLeft(find.text('2')).dx, moreOrLessEquals(300)); + }); + + testWidgets('Pop gesture snapping is not linear', (WidgetTester tester) async { + await tester.pumpWidget( + const CupertinoApp( + home: Text('1'), + ), + ); + + final CupertinoPageRoute route2 = CupertinoPageRoute( + builder: (BuildContext context) { + return const CupertinoPageScaffold( + child: Text('2'), + ); + } + ); + + tester.state(find.byType(Navigator)).push(route2); + + await tester.pumpAndSettle(); + + final TestGesture swipeGesture = await tester.startGesture(const Offset(5, 100)); + + await swipeGesture.moveBy(const Offset(500, 0)); + await swipeGesture.up(); + await tester.pump(); + expect(tester.getTopLeft(find.text('1')).dx, moreOrLessEquals(-100)); + expect(tester.getTopLeft(find.text('2')).dx, moreOrLessEquals(500)); + + await tester.pump(const Duration(milliseconds: 50)); + expect(tester.getTopLeft(find.text('1')).dx, moreOrLessEquals(-19, epsilon: 1)); + expect(tester.getTopLeft(find.text('2')).dx, moreOrLessEquals(744, epsilon: 1)); + + await tester.pump(const Duration(milliseconds: 50)); + // Rate of change is slowing down. + expect(tester.getTopLeft(find.text('1')).dx, moreOrLessEquals(-4, epsilon: 1)); + expect(tester.getTopLeft(find.text('2')).dx, moreOrLessEquals(787, epsilon: 1)); + }); + + testWidgets('Snapped drags forwards and backwards should signal didStopUserGesture', (WidgetTester tester) async { + final GlobalKey navigatorKey = GlobalKey(); + await tester.pumpWidget( + CupertinoApp( + navigatorKey: navigatorKey, + home: const Text('1'), + ), + ); + + final CupertinoPageRoute route2 = CupertinoPageRoute( + builder: (BuildContext context) { + return const CupertinoPageScaffold( + child: Text('2'), + ); + } + ); + + navigatorKey.currentState.push(route2); + await tester.pumpAndSettle(); + + await tester.dragFrom(const Offset(5, 100), const Offset(100, 0)); + await tester.pump(); + expect(tester.getTopLeft(find.text('2')).dx, moreOrLessEquals(100)); + expect(navigatorKey.currentState.userGestureInProgress, true); + + // Didn't drag far enough to snap into dismissing this route. + // Each 100px distance takes 100ms to snap back. + await tester.pump(const Duration(milliseconds: 101)); + // Back to the page covering the whole screen. + expect(tester.getTopLeft(find.text('2')).dx, moreOrLessEquals(0)); + expect(navigatorKey.currentState.userGestureInProgress, false); + + await tester.dragFrom(const Offset(5, 100), const Offset(500, 0)); + await tester.pump(); + expect(tester.getTopLeft(find.text('2')).dx, moreOrLessEquals(500)); + expect(navigatorKey.currentState.userGestureInProgress, true); + + // Did go far enough to snap out of this route. + await tester.pump(const Duration(milliseconds: 301)); + // Back to the page covering the whole screen. + expect(find.text('2'), findsNothing); + // First route covers the whole screen. + expect(tester.getTopLeft(find.text('1')).dx, moreOrLessEquals(0)); + expect(navigatorKey.currentState.userGestureInProgress, false); + }); } diff --git a/packages/flutter/test/material/page_test.dart b/packages/flutter/test/material/page_test.dart index fd0ff4025d9..ebdce13f2de 100644 --- a/packages/flutter/test/material/page_test.dart +++ b/packages/flutter/test/material/page_test.dart @@ -597,9 +597,9 @@ void main() { expect(find.text('route'), findsNothing); - // Run the dismiss animation 75%, which exposes the route "push" button, - // 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 + // exactly halfway. It follows a curve that is very steep initially. await tester.tap(find.text('push')); await tester.pumpAndSettle(); @@ -607,10 +607,19 @@ void main() { expect(find.text('push'), findsNothing); gesture = await tester.startGesture(const Offset(5, 300)); - await gesture.moveBy(const Offset(400, 0)); // drag halfway + 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); + await tester.pump(); // Trigger the dropped snapping animation. + expect( + tester.getTopLeft(find.ancestor(of: find.text('route'), matching: find.byType(Scaffold))), + const Offset(400, 0), + ); + // Let the dismissing snapping animation go 60%. + await tester.pump(const Duration(milliseconds: 240)); + expect( + tester.getTopLeft(find.ancestor(of: find.text('route'), matching: find.byType(Scaffold))).dx, + moreOrLessEquals(798, epsilon: 1), + ); await tester.tap(find.text('push')); await tester.pumpAndSettle(); expect(find.text('route'), findsOneWidget);