fix cupertino page route dismisses hero transition when swipe to the … #58024

chunhtai · 2020-05-26T17:47:43Z

…edge

Description

When doing cupertino back swipe, the page animation will be dismissed/completed when the gesture reach either end of the screen. This will cause the hero controller to remove the overlay entry of the flighting hero. If the user keep holding the finger and swipe, the hero animation will not respond because it is no longer in the overlay

Related Issues

Fixes #57581

Tests

I added the following tests:

See files

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.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: Replace with a link to your migration guide

chunhtai · 2020-05-26T17:51:12Z

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

@@ -633,7 +636,8 @@ class _CupertinoBackGestureController<T> {
  /// The drag gesture has changed by [fractionalDelta]. The total range of the
  /// drag should be 0.0 to 1.0.
  void dragUpdate(double delta) {
-    controller.value -= delta;
+    final double newValue = controller.value - delta;


I don't like this solution. Ideally the animation should be smart enough to not send animation status update when there is an ongoing gesture. However, I can't think of a good way to do it because the animation is provided from TransitionRoute and we also need to send animation status update when the gesture is lifted.

So the clamp prevents the animation from finishing by keeping its value between 0.0 and 1.0? I can't think of a better approach off the top of my head...

justinmc

LGTM, just some nits.

justinmc · 2020-05-28T16:55:33Z

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

@@ -37,6 +37,9 @@ const Color _kModalBarrierColor = CupertinoDynamicColor.withBrightness(
 // The duration of the transition used when a modal popup is shown.
 const Duration _kModalPopupTransitionDuration = Duration(milliseconds: 335);

+const double _kdragingAnimationUpperLimit = 0.998;
+const double _kdragingAnimationLowerLimit = 0.002;


The letter after _k should be capitalized for these two, and dragging should have two letter g's.

Also maybe add a comment about where these values came from, like whether they're just what felt right to you, or if they were calculated somehow, etc.

justinmc · 2020-05-28T16:56:42Z

packages/flutter/test/cupertino/page_test.dart

    await tester.pump();
+    await tester.pump(const Duration(milliseconds: 50));


Could pumpAndSettle work here?

Same for two other cases below.

I want to make sure the ballistic scroll is small enough that can be neglect by human eyes.

Ah I see, sounds good.

justinmc · 2020-05-28T17:07:16Z

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

@@ -633,7 +636,8 @@ class _CupertinoBackGestureController<T> {
  /// The drag gesture has changed by [fractionalDelta]. The total range of the
  /// drag should be 0.0 to 1.0.


Is this comment about 0.0-1.0 still accurate with the changes?

justinmc · 2020-05-28T17:19:23Z

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

@@ -633,7 +636,8 @@ class _CupertinoBackGestureController<T> {
  /// The drag gesture has changed by [fractionalDelta]. The total range of the
  /// drag should be 0.0 to 1.0.
  void dragUpdate(double delta) {
-    controller.value -= delta;
+    final double newValue = controller.value - delta;


So the clamp prevents the animation from finishing by keeping its value between 0.0 and 1.0? I can't think of a better approach off the top of my head...

xster

What's triggering the removal of the hero overlay? Is the HorizontalDragGestureRecognizer calling onEnd somehow?

xster · 2020-05-28T17:30:20Z

packages/flutter/test/material/page_test.dart

    await tester.pump();
+    await tester.pump(const Duration(milliseconds: 50));


So this causes the side effect that there will always be a ballistic scroll at the end of a drag no matter where we dropped it off?

yes, I try to update the test to make sure it just requires a small pump to finish the ballistic scroll that can be neglected by human eyes

xster · 2020-05-28T17:30:32Z

packages/flutter/test/cupertino/route_test.dart

@@ -1133,6 +1133,76 @@ void main() {
    expect(nestedObserver.popupCount, 0);
  });

+  testWidgets('showCupertinoModalPopup uses root navigator by default', (WidgetTester tester) async {


Is this description right?

no its not, thanks for catching it

chunhtai · 2020-05-28T18:07:05Z

What's triggering the removal of the hero overlay? Is the HorizontalDragGestureRecognizer calling onEnd somehow?

It trigger removal when the animation status become dismissed or completed. The animation of the route is AnimationController, that it will update its status to dismissed if its value reach 0 or completed if reach 1.0

flutter/packages/flutter/lib/src/animation/animation_controller.dart

Line 398 in 5663e02

void _internalSetValue(double newValue) {

xster · 2020-05-28T20:44:53Z

ah ya, that's unfortunate. Looks like native iOS allows for the page to be fully swiped to the edge then back too.

Changing the whole animation controller behavior is probably too big of a change. What about making use of navigator didStartUserGesture and somehow not hooking the animation status listener until the gesture stopped?

chunhtai · 2020-05-28T21:23:31Z

ah ya, that's unfortunate. Looks like native iOS allows for the page to be fully swiped to the edge then back too.

Changing the whole animation controller behavior is probably too big of a change. What about making use of navigator didStartUserGesture and somehow not hooking the animation status listener until the gesture stopped?

thank you. I think i found a better solution and avoid the breaking change. @justinmc @xster ready for review

chunhtai · 2020-05-28T21:37:41Z

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

+    // This also relies on the animation to update its status at the end of the
+    // gesture. See the _CupertinoBackGestureController.dragEnd for how
+    // cupertino page route achieves that.
+    if (manifest.fromRoute?.navigator?.userGestureInProgress == true)


One thing that troubles me in this approach is that we push the responsibility, that gesture should trigger animation update when it ends, to the route transition class. I am worrying the third party library developer might trip on this

I think we already crossed that bridge. Since Navigator and Hero are both from the widgets package and heroes.dart is already making use of the user gesture state.

Does this still work if the user dragged it to the end and dropped it exactly there?

yes we have tests to ensure that

flutter/packages/flutter/test/material/page_test.dart

Line 541 in fed18fc

testWidgets('test edge swipe then drop back at ending point works', (WidgetTester tester) async {

flutter/packages/flutter/test/material/page_test.dart

Line 506 in fed18fc

testWidgets('test edge swipe then drop back at starting point works', (WidgetTester tester) async {

chunhtai · 2020-06-05T20:31:05Z

@justinmc the pr has changed a lot since your last review, can you take another look?

justinmc

LGTM, just two nits.

justinmc · 2020-06-05T20:54:19Z

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

@@ -533,6 +533,14 @@ class _HeroFlight {
  }

  void _handleAnimationUpdate(AnimationStatus status) {
+    // The animation will not finish until the user lift their finger, so we


Suggested change

// The animation will not finish until the user lift their finger, so we

// The animation will not finish until the user lifts their finger, so we

justinmc · 2020-06-05T21:03:52Z

packages/flutter/test/cupertino/route_test.dart

+    box = tester.renderObject(find.byKey(container)) as RenderBox;
+    final double firstPosition = box.localToGlobal(Offset.zero).dx;
+    // Checks the hero is in-transit.
+    expect(finalPosition > firstPosition, isTrue);


Nit: Same thing, but maybe the error message is more informative if you do this:

Suggested change

expect(finalPosition > firstPosition, isTrue);

expect(finalPosition, greaterThan(firstPosition));

…edge

flutter#58024) * fix cupertino page route dismisses hero transition when swipe to the edge * add more comment * addressing comments

fluttergithubbot added f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels May 26, 2020

googlebot added the cla: yes label May 26, 2020

chunhtai added the c: API break Backwards-incompatible API changes label May 26, 2020

chunhtai commented May 26, 2020

View reviewed changes

chunhtai requested review from goderbauer, xster and justinmc May 26, 2020 17:51

justinmc approved these changes May 28, 2020

View reviewed changes

xster reviewed May 28, 2020

View reviewed changes

chunhtai force-pushed the issues/57581 branch from afff7b8 to 8f99b30 Compare May 28, 2020 20:22

chunhtai force-pushed the issues/57581 branch from 8f99b30 to 5680ed9 Compare May 28, 2020 21:09

chunhtai commented May 28, 2020

View reviewed changes

justinmc approved these changes Jun 5, 2020

View reviewed changes

chunhtai removed the c: API break Backwards-incompatible API changes label Jun 5, 2020

chunhtai added 3 commits June 5, 2020 15:03

fix cupertino page route dismisses hero transition when swipe to the …

2e26fc9

…edge

add more comment

40412bb

addressing comments

946f088

chunhtai force-pushed the issues/57581 branch from 97087bb to 946f088 Compare June 5, 2020 22:03

chunhtai added the waiting for tree to go green label Jun 5, 2020

chunhtai merged commit 5eaaad4 into flutter:master Jun 5, 2020

dkwingsmt mentioned this pull request Sep 29, 2020

ModalRoute no longer disposes resources until its child widget is unmounted #66697

Closed

13 tasks

This was referenced Oct 15, 2020

Delay route disposal to after the next frame #68202

Closed

Allow OverlayEntry to lock route disposal #68255

Closed

goderbauer mentioned this pull request Oct 26, 2020

Delay Route disposal until OverlayEntries are unmounted #68913

Merged

13 tasks

github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021

		await tester.pump();
		await tester.pump(const Duration(milliseconds: 50));

		@@ -633,7 +636,8 @@ class _CupertinoBackGestureController<T> {
		/// The drag gesture has changed by [fractionalDelta]. The total range of the
		/// drag should be 0.0 to 1.0.

	// The animation will not finish until the user lift their finger, so we
	// The animation will not finish until the user lifts their finger, so we

	expect(finalPosition > firstPosition, isTrue);
	expect(finalPosition, greaterThan(firstPosition));

fix cupertino page route dismisses hero transition when swipe to the … #58024

fix cupertino page route dismisses hero transition when swipe to the … #58024

Uh oh!

Conversation

chunhtai commented May 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai commented May 28, 2020

Uh oh!

xster commented May 28, 2020

Uh oh!

chunhtai commented May 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai commented Jun 5, 2020

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chunhtai commented May 26, 2020 •

edited

Loading