[animations] Prevent SharedAxisTransition from resetting ongoing animations on entering page

[vasilich6107]

This is a POC of the fix for flutter/flutter#55826

Investigation story :)

After I noticed the CircularProgressIndicator restart I start digging into the issues and found this - https://tppr.me/KSBAK

It renders the child component three times(1 for SignIn and 2 times for Courses) moving 'next' and only one time(SignIn widget) while moving back. As far as I know Flutter works with animations in the way that it does not perform useless rerenders and just works with the whole child.

When I removed all the transitions and left only the return child; in both switch cases everything started working as expected(without animations obviously)

switch (_effectiveAnimationStatus) {
case AnimationStatus.forward:
return child;
case AnimationStatus.dismissed:
case AnimationStatus.reverse:
case AnimationStatus.completed:
return child;
}

Then I replaced all the _EnterTransition with _ExitTransition and repeated the test - everything became smooth(the animation became a little bit different)

The main goal of this POC PR is to make the widget tree identical.

While performing the animations the widget tree changes from _EnterTransition to _ExitTransition, and the _ExitTransition adds additional Container to that tree. This changes was the root cause of useless rerenders.

If you debug the current released version you can see the the 'happy path' of single render in 'back' transition occurs cause widget.child passes through the identical _EnterTransition(or _ExitTransition I do not remember the precise details)

I decided to combine this 2 classes to guarantee that the widget tree will remain the same.

This is a result

[vasilich6107]

@shihaohong, @goderbauer
Could you review my PR?

[shihaohong]

Hi @vasilich6107, thank you for filing the issues (with great gifs!) and submitting this POC PR.

Could you summarize what this PR aims to do in the first comment? It's hard to tell just from looking at the code what your approach to fixing this is, and the PR doesn't point out the root cause of the problem.

This helps with reviewing the code for me as well, since I can then verify that the intended behavior matches the code changes. Thank you!

[vasilich6107]

Updated the comment

[goderbauer]

Haven't looked at the code in detail yet, but in order to accept the PR we do require tests to ensure that this never regresses again in the future. Can you add some tests, please?

[vasilich6107]

I’ll try to)

[vasilich6107]

It would be great if you point me on same existing tests that verify rendering)

[shihaohong]

Looking at the explanation and then the code for a few minutes, it's still not apparent to me how this change introduced that fixes the problem. I think it was mentioned that adding a Container caused a change in the render tree and triggered the additional re-render, but I don't see any part of the PR that removes a Container widget.

I think once the reasoning for why the fix works is clear, writing the test should be much more straightforward since I'm not sure how to test for a behavior I do not understand. Off the top of my head, checking the state to make sure the entering widget tree (the incoming page) does not lose state throughout the animation is one way (although I remember we have a test for that, so maybe it's something different or the test doesn't catch this particular scenario).

Another solution that comes to mind is introducing a key? That way, even if the widget tree changes, as long as the subtree remains, it does not get completely re-rendered from scratch.

[vasilich6107]

I was working to figure out how to fix this for almost two days)
After I found a fix I did not have More time to experiment with another solutions.
May be there could be a better solution...

Current PR pipes the child widget through the single _Transition class which guaranties that the widget tree will be the same no matter how you change output. As far as Container was used to provide a fillColor I leaved it for both cases(entering and exiting).

Could you provide a links to the tests that you are talking about?

[shihaohong]

The test I was thinking about was this one in particular.

[vasilich6107]

Thanks. I'll try to find some time to test the approach with key)

[vasilich6107]

@shihaohong@goderbauer
Added test
https://github.com/flutter/packages/pull/146/files#diff-ca7f59dbfd46169f7fa7e5467a016f1aR164

[vasilich6107]

Any comments, thoughts?

[shihaohong]

@vasilich6107 Thank you for your patience, I've been working on a different set of issues and finally got some time to get back to code reviews :)

I just tried my hand at this, and what I mean by adding a key is as follows to your application. Basically, by adding a key to the page you're transitioning to, you're telling Flutter that you want to retain the state in your keyed widget subtree and move them in your widget tree rather than rebuilding them from scratch.

Given that there's a working solution without modifying the animations package, the question now, becomes: Do we want to prevent this from occurring for users out of the box in the animations package, or would it preferable for users to provide keys to prevent losing track of state? What are the pros and cons of doing so?

@goderbauer can maybe chime in more on this. Personally, I prefer to leave it up to the user to decide whether or not to provide keys for their widgets. My primary concern is that it may take up unnecessary space (I'm not sure what the performance and size implications of adding keys haphazardly might be). Once we introduce a keyed widget into the transition, it might be difficult to remove it without it being a breaking change to our users.

A good article on keys in Flutter: https://medium.com/flutter/keys-what-are-they-good-for-13cb51742e7d

[shihaohong]

Also, if we opt to not provide a key by default, we could add a recommendation to the API documentation for the transitions and recommend adding keys when this becomes a problem

[vasilich6107]

Personally I think that if we have a working solution that does not require providing additional key's from user it will be better to apply it.
Better experience out of the box is more valuable to my mind.

What are the pros of leaving the SharedAxisTransition 'as is'?

[shihaohong]

@vasilich6107 I think my concern was that there was some use case where users do not want to have a keyed widget for the incoming page. I'm not sure what those situations might be from my lack of experience with using/not using keys, so I want to be careful in the event there are some undesirable side effects (Questions I'm asking myself are along the lines of "In what situation would a developer not want a key here?"). I'll experiment a little more with it and see.

If I can't find any problems, I 100% agree with you that providing this convenience out of the box would be ideal.

[goderbauer]

While ensuring that the tree structure is the same for both animations would work, it really makes the code more complicated to read and understand. I think we should just wrap the child here in a KeyedSubtree widget that applies a global key to it:

packages/packages/animations/lib/src/shared_axis_transition.dart

Line 387 in c19aef9

child: widget.child,

[goderbauer]

Checking how often build was called is not appropriate. The framework is free to call build as often as it wants. Instead, you should check that the state object associated with the widget hasn't changed after the transition has started.

[goderbauer]

Having said all this, I just saw #141, which fixes this in a very elegant way without using keys and it also removes some of the code duplication that we have in our transitions. I think that PR is the right way forward from here.

[vasilich6107]

Great to hear!
I’ll close mine in favor of #141