Cupertino navigation bars transitionBetweenRoutes fidelity update #164956
Conversation
| ); | ||
|
|
||
| Widget child = bottomNavBarBottom.child; | ||
| final Curve animationCurve = | ||
| animation.status == AnimationStatus.forward | ||
| ? Curves.easeOutQuart |
There was a problem hiding this comment.
The forward case looks a little fast. Ideally, the navbar contents would not go ahead of the content behind it, I think it's a little distracting. The reverse case looks good, in my opinion.
It looks like the Hero widget applies it's own curve as well. Is that interfering at all?
How does this look when dragging the whole time? As long as the drag gesture is still active, the base curve of the transition is linear.
There was a problem hiding this comment.
The forward case looks a little fast. Ideally, the navbar contents would not go ahead of the content behind it, I think it's a little distracting. The reverse case looks good, in my opinion.
yeah it feels too fast and the fact that it goes ahead of the content makes it look bad visually, especially when manually dragging to pop a page.
I also found a bug when swiping between pages, if I start swiping from the furthest point the large title doesn't start animating immediately creating an unpleasing visual bug, but if I start swiping to pop from a point far from the edge it animates as expected, here's a screen recording of this behaviour:
There was a problem hiding this comment.
I also found a bug when swiping between pages, if I start swiping from the furthest point the large title doesn't start animating immediately creating an unpleasing visual bug, but if I start swiping to pop from a point far from the edge it animates as expected, here's a screen recording of this behaviour
That's likely because the page transition is on a linear curve during the drag, while the navbar is still on a flipped easeOutQuart curve.
There was a problem hiding this comment.
Is it possible for the header to simply follow the content's x coordinate? If we can't get the exact curve then simply doing this should be acceptable.
There was a problem hiding this comment.
Is it possible for the header to simply follow the content's x coordinate? If we can't get the exact curve then simply doing this should be acceptable.
I also thought of this, it sounds like a reasonable thing to do if the curve requires lots of work to look somewhat right, also since this is a temporary change and will be tweaked again as mentioned in the issue description to use a spring simulation.
There was a problem hiding this comment.
I tried all the curves we have, none of them could follow the content. There's something funky happening behind the scenes that I can't put a finger on. @MitchellGoodwin mentioned that the Hero might be applying its own curve, maybe that's why.
Either way I used custom curves to approximate the native animation.
There was a problem hiding this comment.
it look more polished now, looks good for when there's only a large title, but with the search field not so much, can you make the search field that's on the page that's getting another page pushed on to (the previous page), slide with the same curve as the pages content because currently there a lot of overlapping happening with them especially when manually dragging, things are feeling better overall, the pushed title felt flimsy before this pr now it feels snappy which is good.
| ); | ||
|
|
||
| Widget child = bottomNavBarBottom.child; | ||
| final Curve animationCurve = |
There was a problem hiding this comment.
the title being ahead of the page looks jarring, maybe a custom animation curve could be used instead of the current built-in ones if they can't match current needs, because it seems like it needs a very specific curve, based on my observation of the native side, maybe you can use the base of the curve that the page content animates in, but tweak it a bit, make it slightly slower at the beginning and faster at the end but not to fast to the point where it doesn't overflow out of the page's content.
| ); | ||
|
|
||
| Widget child = bottomNavBarBottom.child; | ||
| final Curve animationCurve = | ||
| animation.status == AnimationStatus.forward | ||
| ? Curves.easeOutQuart |
There was a problem hiding this comment.
Is it possible for the header to simply follow the content's x coordinate? If we can't get the exact curve then simply doing this should be acceptable.
I also thought of this, it sounds like a reasonable thing to do if the curve requires lots of work to look somewhat right, also since this is a temporary change and will be tweaked again as mentioned in the issue description to use a spring simulation.
There was a problem hiding this comment.
looks awesome now, the curves are snappy like native and aren't overlapping aggressively like before, the bug where if you start swiping to pop from the devices border is still present, the title doesn't start animating until you swipe a longer distance, also the the large title when animating to the previous page title in the back button needs updating now seems it needs to match the speed the large title that's entering and leaving, other than that, everything looks and works fine.
| // The bottom widget animates linearly during a backswipe by a user gesture. | ||
| userGestureInProgress | ||
| // The parent of the hero animation, which is the route animation. | ||
| ? (animation as CurvedAnimation).parent |
There was a problem hiding this comment.
I think we should check to see if it is a CurvedAnimation. That way if it changes in the Hero implementation in the future, this doesn't break.
There was a problem hiding this comment.
should we check or assert?
There was a problem hiding this comment.
Good question. I think assert would be better, so this code doesn't just hang around if that happens.
| // The large title animates linearly during a backswipe by a user gesture. | ||
| userGestureInProgress | ||
| // The parent of the hero animation, which is the route animation. | ||
| ? (animation as CurvedAnimation).parent |
|
just checked the latest changes and it looks good, I also agree with Mitchels comment:
things look good so far, it isn't worth putting more effort to match the native side down to the smallest detail if it's going to be revisited and get implemented with a spring simulation sometime later. |
|
can you make it that the chevron finishes animating as soon as the page push animation finishes because currently it starts animating when the push animation finishes unlike native. |
Hi @MaherSafadii can you confirm if this has been fixed? |
it works properly now, the only noticeable thing left is the large title animating to the previous button is a bit slow, should it be fixes in this PR or is it for another one? |
I see that as well, taking a look. |
This should be fixed now. |
There was a problem hiding this comment.
It's way better now, it feels snappy, but there seems to be some aggressive font weight and text position changing that makes it look jarring especially in normal speed, here are some screen recordings that show it clearly:
Slow motion:
https://github.com/user-attachments/assets/9ccecad0-9a8b-4680-b7c7-2f4b6166850b
Normal speed:
https://github.com/user-attachments/assets/e645070b-01c2-4a36-aeb4-810f0175c935
another thing I just remembered is that on native when you push to a page, the default behaviour for the previous button title is it to be the large title that's set in the previous page, but in flutter by default its an empty string with just a chevron, it feels like a chore every time to make it match the correct behaviour by adding a line of code every time, making it behave like native would create a better experience and also for localization it would help since it takes the string from the large text from the previous page, so if its localized it will automatically be localized in the previous button.
Not 100% sure but I think this is an impeller text rendering issue. |
|
I think native iOS nav bars keep the background color per page native.iOS.background.movso instead of removing the background completely, we should just have both nav bars keep their individual backgrounds (vs. previously where they shared the same background) nav.bar.readd.background.movThis also prevents an issue where transitions from a page that has been scrolled shows the scrollview's content underneath the nav bar mid transition. |
I think this is because it switches from 'CupertinoSystemDisplay' and 'CupertinoSystemText', which each have different letter spacing and font weight handling. That might be a bit of a rabbit hole, so I'd recommend leaving that for a separate issue and PR. |
In the before Flutter video, drag happens after the 17 second mark.
Flutter before
Flutter.before.mov
Flutter after
Flutter.after.4.mov
Flutter back swipe drag gesture
flutter.after.drag.2.mov
Native iOS
native.mov
Flutter scaled back chevron and large title after
flutter.scaled.and.label.mov
Native iOS scaled back chevron and large title
native.scaled.back.chevron.mov
Native iOS is probably using a spring simulation. This is the closest curve we have to the native transition, but should be updated in the future with the exact values.
Fixes Cupertino navigation bars transitionBetweenRoutes fidelity update