Allow "from" hero state to survive hero animation in a push transition by LongCatIsLooong · Pull Request #32842 · flutter/flutter

LongCatIsLooong · 2019-05-16T22:41:12Z

Description

When a route push transition happens, Hero widgets involved in the animation are all replaced by placeholder widgets (SizedBoxes). This removes their elements (as well as state) from the tree which might not be desirable for the from Heroes if they are stateful.

Related Issues

Fixes #31503 and #32356, somewhat mitigates #31473

Tests

I added the following tests:

Hero works with images that don't have both width and height specified
From hero's state should be preserved, heroes work well with child widgets that has global keys

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 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

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): https://groups.google.com/forum/#!topic/flutter-announce/b2Cd0iKOh08
No, this is not a breaking change.

HansMuller

This looks good. Just some trivial formatting feedback.

HansMuller · 2019-05-17T18:05:48Z

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

-    setState(() {
-      _placeholderSize = box.size;
-    });
+    setState(() => _placeholderSize = box.size);


We usually only use => for one-liners that return a value. That's why the original version of this statement was formatted with { }

Same comment below

Done. But technically the expression evaluates to box.size, and setState does use the returned value (check it's runtime type). Not sure where to draw the line.

It's true that setState() checks the return value, but only to warn the user if they've mistakenly returned a Future. I think one-liners that implement a void method should get curly braced.

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

packages/flutter/test/widgets/heroes_test.dart

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

xster · 2019-05-17T19:57:42Z

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

      _proxyAnimation.parent = manifest.animation;

-    manifest.fromHero.startFlight();
+    manifest.fromHero.startFlight(shouldIncludedChildInPlaceholder: manifest.type == HeroFlightDirection.push);


We need to be slightly careful here. If the user uses a shuttleBuilder, it could be reasonable for the user to expect that he/she can just use some globalkey that used to be in the from Hero inside the custom shuttleBuilder. That might break if we kept the original tree still under the Hero

going to add a Limitations section in flightShuttleBuilder's documentation. Is this considered a breaking change?

Then you'd have to explain that that's only true for the Hero of the from route in the event of a push navigation. Perhaps add another argument to Hero for preservePlaceholderStateDuringFlight that defaults to true and gets overridden when a custom placeholderBuilder is given?

I think I did:

/// ## Limitations
///
/// Currently if a widget built by [flightShuttleBuilder] takes part in a
/// [Navigator] push transition, that widget or its descendants must not have
/// any [GlobalKey] that is used in the source Hero's descendant widgets.

~~Isn't flightShuttleBuilder == null always equal to preservePlaceholderStateDuringFlight?~~
In that case maybe we should add a Size parameter to placeholderBuilder?

xster · 2019-05-17T20:05:50Z

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

+      ? Visibility(
+          visible: false,
+          maintainState: true,
+          maintainAnimation: true,


ummm this is a bit unfortunate. @HansMuller it's not super clear to me why this is needed looking at the cascading logic in Visibility. Do you know?

Not sure what you mean by "cascading logic". I agree that keeping the from Hero's animation running while it is hidden seems unnecessary.

I just meant this series of logic

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

Line 69 in e2042ba

assert(maintainState == true || maintainAnimation == false, 'Cannot maintain animations if the state is not also maintained.'),

Replacing the visibility widget with SizedBox(Offstage(TickerMode)) seems to work

xster

LGTM

xster · 2019-05-20T20:31:55Z

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

+      ? Visibility(
+          visible: false,
+          maintainState: true,
+          maintainAnimation: true,


I just meant this series of logic

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

Line 69 in e2042ba

assert(maintainState == true || maintainAnimation == false, 'Cannot maintain animations if the state is not also maintained.'),

xster · 2019-05-20T20:34:00Z

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

      _proxyAnimation.parent = manifest.animation;

-    manifest.fromHero.startFlight();
+    manifest.fromHero.startFlight(shouldIncludedChildInPlaceholder: manifest.type == HeroFlightDirection.push);


Then you'd have to explain that that's only true for the Hero of the from route in the event of a push navigation. Perhaps add another argument to Hero for preservePlaceholderStateDuringFlight that defaults to true and gets overridden when a custom placeholderBuilder is given?

…ter into maintain-hero-state

Piinks · 2019-05-28T16:52:39Z

Is there a link to the breaking change announcement?

LongCatIsLooong · 2019-05-28T17:14:11Z

@Piinks there isn't one yet. I'll post the announcement once the changes are finalized.

HansMuller

LGTM - just some suggestions about the docs

HansMuller · 2019-06-03T19:45:55Z

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

 /// [MaterialRectArcTween].
 typedef CreateRectTween = Tween<Rect> Function(Rect begin, Rect end);

+/// A builder that builds a placeholder widget given a child and a [Size]


A function that builds a [Hero] placeholder widget ...

HansMuller · 2019-06-03T19:46:47Z

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

+/// A builder that builds a placeholder widget given a child and a [Size]
+///
+/// The child can optionally be part of the returned widget tree. The returned
+/// widget should typically be sized to [heroSize], if it doesn't do so


sized => constrained

HansMuller · 2019-06-03T19:50:16Z

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

+  ///
+  /// If a widget built by [flightShuttleBuilder] takes part in a [Navigator]
+  /// push transition, that widget or its descendants must not have any
+  /// [GlobalKey] that is used in the source Hero's descendant widgets, as both


Better to break the "as both" point into a new sentence:

...widgets. Both subtrees will be included ... animation, and global keys must be unique across the entire widget tree.

HansMuller · 2019-06-03T19:52:00Z

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

-  /// By default, an empty SizedBox keeping the Hero child's original size is
-  /// left in place once the Hero shuttle has taken flight.
-  final TransitionBuilder placeholderBuilder;
+  /// By default the placeholder widget left in place is an empty [SizedBox]


Please leave out "left in place"

...placeholder widget is an empty...

HansMuller · 2019-06-03T19:55:18Z

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

-  final TransitionBuilder placeholderBuilder;
+  /// By default the placeholder widget left in place is an empty [SizedBox]
+  /// keeping the Hero child's original size, unless this Hero is a source Hero
+  /// of a [Navigator] push transition, in which case the placeholder will be a


This is run-on. Maybe:
...push transition. If this hero the source of a push transition, the [child] will be a descendant of the placeholder. The child will be kept [Offstage] during the hero's flight.

Piinks · 2019-06-05T20:37:31Z

Hey @LongCatIsLooong , this is great. It looks like your branch needs to be updated to fix those codegen errors that are causing test failures.

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

Hixie · 2019-06-06T02:25:43Z

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

+  /// [GlobalKey] that is used in the source Hero's descendant widgets. That is
+  /// because both subtrees will be included in the widget tree during the Hero
+  /// flight animation, and [GlobalKey]s must be unique across the entire widget
+  /// tree.


i don't understand this. Are you saying the child will be included twice in the tree? Because that won't work if the widget is stateful... the whole point of the Hero logic is that it only be included once.

Hixie · 2019-06-06T02:27:28Z

cc @goderbauer

…ter into maintain-hero-state

* master: (24 commits) [flutter_tool,fuchsia] Update the install flow for packaging migration. (flutter#34447) SliverFillRemaining flag for different use cases (flutter#33627) SizedBox documentation (flutter#34424) Change API doc link to api.dart.dev (flutter#34388) 2589785 Roll src/third_party/skia 87e885038893..c3252a04b377 (3 commits) (flutter/engine#9327) (flutter#34484) ace5d59 Fix rawTypes errors in Android embedding classes (flutter/engine#9326) (flutter#34481) bf0def6 Roll src/third_party/skia 4c4945a25248..87e885038893 (1 commits) (flutter/engine#9325) (flutter#34471) Roll engine f1d821d..6f5347c (13 commits) (flutter#34466) Allow "from" hero state to survive hero animation in a push transition (flutter#32842) Roll pub dependencies (flutter#33677) Skip flaky test on Windows (flutter#34464) Allow flaky tests to pass or fail and mark web tests as flaky (flutter#34456) Dont depend on web SDK unless running tests on chrome (flutter#34457) Fix semantics_tester (flutter#34368) Include raw value in Diagnostics json for basic types (flutter#34417) Refactor Gradle plugin (flutter#34353) Allow web tests to fail in cirrus config (flutter#34436) skip bottom_sheet (flutter#34430) Remove unused flag `--target-platform` from `flutter run` (flutter#34369) Extract DiagnosticsNode serializer from WidgetInspector (flutter#34012) ...

dnfield · 2019-06-14T19:39:37Z

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

+    }
+
+    return SizedBox(
+      width: _placeholderSize?.width,


This breaks getting sematnics for a FloatingActionButton, e.g.

await tester.pumpWidget(MaterialApp( home: FloatingActionButton( onPressed: () {}, ))); expect( tester.getSemantics(find.byType(FloatingActionButton)), matchesSemantics( hasTapAction: true, hasEnabledState: true, isButton: true, isEnabled: true, ));

Now returns the semantics node for the parent of the widget rather than the semantics node for the widget.

Fix for this is in #34512

LongCatIsLooong added 6 commits May 15, 2019 18:48

WIP

844a7e8

add tests

79d3e63

comments

4b7db33

add more comments

b5fbc92

more tests

ad4f463

comments

01c54d7

googlebot added the cla: yes label May 16, 2019

indentation

0cd105f

LongCatIsLooong requested review from HansMuller and xster May 17, 2019 01:00

HansMuller reviewed May 17, 2019

View reviewed changes

xster reviewed May 17, 2019

View reviewed changes

Piinks added framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. a: animation Animation APIs labels May 18, 2019

review

03c4f35

LongCatIsLooong requested review from HansMuller and xster May 18, 2019 22:29

xster reviewed May 20, 2019

View reviewed changes

LongCatIsLooong added 5 commits May 21, 2019 10:42

Update heroes.dart

29daa8e

SizedBox >>> Offstage >>> TickerMode

0ca8020

Merge branch 'maintain-hero-state' of github.com:LongCatIsLooong/flut…

1e15bbb

…ter into maintain-hero-state

thank you analyzer

adfb56b

change placeholdBuilder

cc5a47f

LongCatIsLooong added the c: API break Backwards-incompatible API changes label May 23, 2019

LongCatIsLooong requested a review from xster May 23, 2019 21:44

Try to maintain the element tree height during hero animation

bedffec

HansMuller approved these changes Jun 3, 2019

View reviewed changes

update doc

290b823

Hixie reviewed Jun 6, 2019

View reviewed changes

packages/flutter/lib/src/widgets/heroes.dart Outdated Show resolved Hide resolved

Hixie reviewed Jun 6, 2019

View reviewed changes

LongCatIsLooong added 3 commits June 5, 2019 20:19

Update heroes.dart

a581ee1

Merge branch 'master' into maintain-hero-state

18489b7

Merge branch 'maintain-hero-state' of github.com:LongCatIsLooong/flut…

a5e15b1

…ter into maintain-hero-state

LongCatIsLooong merged commit 731e981 into flutter:master Jun 14, 2019

LongCatIsLooong deleted the maintain-hero-state branch June 14, 2019 03:41

LongCatIsLooong mentioned this pull request Jun 14, 2019

Hero Widget transitionOnUserGestures animates wrong. #32356

Closed

dnfield reviewed Jun 14, 2019

View reviewed changes

dnfield mentioned this pull request Jun 14, 2019

Make sure fab semantics end up on top #34512

Merged

9 tasks

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

Conversation

LongCatIsLooong commented May 16, 2019 • 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!

HansMuller 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

LongCatIsLooong May 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Piinks commented May 28, 2019

Uh oh!

LongCatIsLooong commented May 28, 2019

Uh oh!

HansMuller 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!

Piinks commented Jun 5, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Jun 6, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong commented May 16, 2019 •

edited

Loading

LongCatIsLooong May 21, 2019 •

edited

Loading