The logic for determining the reverseCurve seems incorrect. When a reverseCurve is provided on the Hero widget, it should be used as is for the reverse animation. However, the current implementation applies .flipped to it, which is likely not the intended behavior. The reverseCurve for the CurvedAnimation should only be curve.flipped when the hero's reverseCurve property is null.

I suggest adjusting the logic for both the push and pop cases to correctly handle the provided reverseCurve.

    switch (type) {
      case HeroFlightDirection.push:
        parent = toRoute.animation!;
        curve = toHero.widget.curve;
        reverseCurve = toHero.widget.reverseCurve ?? curve.flipped;
      case HeroFlightDirection.pop:
        parent = fromRoute.animation!;
        curve = fromHero.widget.curve;
        reverseCurve = fromHero.widget.reverseCurve ?? curve.flipped;
    }

discussion on #180100 (comment)

The expected curve for the pop animation is incorrect when reverseCurve is not specified. When reverseCurve is null, the pop animation should use the flipped version of the forward curve. The test currently expects it to use the forward curve itself.

This makes the test 'popped hero curve is the flipped version of [curve] if reverseCurve is not specified' incorrect, as it asserts against the wrong curve.

    final Animatable<Size?> tween = SizeTween(
      begin: const Size(200, 200),
      end: const Size(100, 100),
    ).chain(CurveTween(curve: Curves.fastOutSlowIn));

The original implementation uses curves instead of flipped.

This test contains a significant amount of duplicated code within the for loop, specifically the MaterialApp and its routes definition. This can be refactored to improve readability and maintainability.

Consider extracting the MaterialApp creation into a helper function that takes the curve as a parameter. This would make the test logic clearer and easier to manage.

I'd prefer to keep the current implementation as-is to minimize changes.

Recreate MaterialApp with a unique key to avoid conflicts from previous curve tests.

Nit: Another thing you could do is to move the for loop outside of testWidgets altogether, so that it creates a new test for each curve. Then I think you wouldn't need the key, and the test output might be a little easier to read.

Thank you for the review! I updated the code as you suggested. Both the code and the test output are now easier to read—this was really helpful!
3701d0d

this doesn't look right

shouldn't this be

reverseCurve = toHero.widget.reverseCurve ?? toHero.widget.curve.flipped;

same for line 484
#159393 (comment)

I see the feedback in the previous PR, but I believe the current implementation is correct.
The reverseCurve needs to be flipped because at line 723, the animation is wrapped with ReverseAnimation for pop direction.
When ReverseAnimation reverses the animation, the curve also needs to be flipped to match the reversed direction, regardless of whether reverseCurve is explicitly provided or defaults to curve.

The tests also appear to be passing correctly.

Can we remove the ReverseAnimation wrapping instead of having this complication? If not, this should be documented in the line above this.

I’m sorry to reverse my earlier position, but I now think flipping reverseCurve inside Hero is not the right approach.

1. As you pointed out, this makes the behavior harder to reason about and increases implementation complexity.
2. In the Flutter SDK, it’s generally expected that the caller flips the reverse curve so that the easing profile remains the same in both the forward and reverse directions.
   • For example, _CupertinoPageTransitionState flips the reverse curve as well.
   • Aligning Hero’s behavior with these existing APIs should make the overall behavior more predictable.

Based on that, I updated the implementation to stop flipping reverseCurve inside Hero.　
9cf4b32

Can we directly say something like Hero.curve.flipped?

Fixed in fdd9db6

Can we remove the ReverseAnimation wrapping instead of having this complication? If not, this should be documented in the line above this.

I wouldn't change this test, maybe rename to "Default Hero animation is fastOutSlowIn" and add a separate test for this PR.

Fixed in 6a568d5
I also reverted the pop transition test back to its default-value test.　5a2c1f9

To link to the documentation. This might need a doc import of package:flutter/animation.dart to work.

Fixed in 6377019

or similar.

I tried [Hero.curve.flipped], but it didn’t link, so I used [Hero.curve].flipped instead.
6377019

Our convention is to define helper methods at the very end/top of the file and outside the main method, not between testWidgets calls.

Fixed in bfbbe1f

Nit: Another thing you could do is to move the for loop outside of testWidgets altogether, so that it creates a new test for each curve. Then I think you wouldn't need the key, and the test output might be a little easier to read.