Snap CupertinoSliverNavigationBar large title and bottom widget by victorsanni · Pull Request #156381 · flutter/flutter

victorsanni · 2024-10-08T01:22:26Z

FixesCupertinoSliverNavigationBar should snap between large and middle title
FixesCupertinoSliverNavigationBar needs to implement snapping

Flutter (with this PR):

Screen.Recording.2024-10-16.at.4.26.18.PM.mov

Native:

Screen.Recording.2024-10-16.at.4.28.17.PM.mov

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

MitchellGoodwin

Was ScrollEndNotification unable to be used for this? Just following up on this comment: #126028 (comment)

MitchellGoodwin · 2024-10-14T22:59:45Z

+ : 0.0,
+ // Eyeballed on an iPhone 16 simulator running iOS 18.
+ duration: const Duration(milliseconds: 100),
+ curve: Curves.easeOut,


To my eye, the animation isn't snappy enough. I wonder if the curve should have a faster start. On iOS it might be a spring animation.

Yeah, tbh I don't know what animation curve or values are used. The native one seems more snappy, but not too fast at the same time. If only there was a way to get these values...

Maybe Curves.easeOutExpo? That one seems to start off faster. There is a helpful video with graphs and sample animation for every curve at https://api.flutter.dev/flutter/animation/Curves-class.html

I have a strong suspicion that it's a spring animation. That's what iOS uses for releasing a partially dragged page transition. There is a way to check with Xcode debugging, but I do not remember off the top of my head. @dkwingsmt was working with using the spring simulation for an animation: #155575
But my opinion for this PR is it's fine to use a curve we already have for now, but one that's closer. fastEaseInToSlowEaseOut is what the Cupertino routes use.

easeOutExpo started a little bit too fast, but fastEaseInToSlowEaseOut was closer-ish. It's not perfect but definitely better.

victorsanni · 2024-10-14T23:20:41Z

Was ScrollEndNotification unable to be used for this?

I was having difficulty making scroll notifications and scroll position play well together. I added a listener to be called on ScrollNotifications, but when I tried to animate the scroll position within that listener, it didn't work. So I added a separate listener for the scroll position, and tried making both callbacks talk to each other, but that also didn't work.

I agree that responding to ScrollEndNotifications seems like the most intuitive way to do this. I think Hans' examples worked with ScrollNotifications because it had control of the parent CustomScrollView and so could wrap it directly in a NotificationListener. But it's trickier to do (but maybe there is a way to do it) when we're entirely within a sliver within the CustomScrollView and can only look up the tree with inherited widgets.

Piinks · 2024-10-15T17:02:50Z

+ : 0.0,
+ // Eyeballed on an iPhone 16 simulator running iOS 18.
+ duration: const Duration(milliseconds: 100),
+ curve: Curves.easeOut,


Maybe Curves.easeOutExpo? That one seems to start off faster. There is a helpful video with graphs and sample animation for every curve at https://api.flutter.dev/flutter/animation/Curves-class.html

Piinks · 2024-10-15T17:03:43Z

+ @override
+ void didChangeDependencies() {
+ super.didChangeDependencies();
+ _scrollableState?.position.isScrollingNotifier.removeListener(_handleScrollChange);


Using the isScrollingNotifier makes sense to me. It will only notify when scrolling starts and stops, which is exactly what is needed here. 👍 Using scroll notifications would mean a lot of notifications we would not need.

MitchellGoodwin

LGTM!

victorsanni · 2024-10-16T19:48:40Z

Converting to draft... Since the bottom to nav bar PR has been merged, I'm working to make it snap for both the search bar and the large title.

victorsanni · 2024-10-16T19:50:49Z

Snapping only the large title causes problems in NavigationBarBottomMode.automatic, since the search bar scrolls first instead of the large title (so we should be using the search bar height to snap first, not the large title height).

MitchellGoodwin

Renewing my LGTM with some nits.

Piinks

LGTM

b3nni97 · 2024-10-20T22:02:02Z

+ }
+
+ if (target != null) {
+ position.animateTo(


Using 'animateTo' can be very risky as it breaks the entire scrollable widget if one attempts to scroll within the 300ms timeframe (refer to issue #14452). This issue is easily reproducible by increasing the animation duration and trying to scroll the CustomScrollView during that period. Subsequently, assertions fail, such as '_hold == null || _drag == null'.

…y scrolled in .always bottom mode (#162425) Forgot to add these in #156381. Spotted it when debugging #159120.

…et (flutter/flutter#156381)

…y scrolled in .always bottom mode (#162425) Forgot to add these in flutter/flutter#156381. Spotted it when debugging flutter/flutter#159120.

Snap CupertinoSliverNavigationBar large title
6cdd492

github-actionsBot added framework flutter/packages/flutter repository. See also f: labels. p: cupertino_ui cupertino_ui package in flutter/packages labels Oct 8, 2024

victorsanni added 4 commits October 9, 2024 08:09

Fix crash on desktop
aef4cf5

Merge branch 'master' into snap-large-title
5baed16

Add tests
773f5bd

Merge branch 'master' into snap-large-title
4fb206f

victorsanni marked this pull request as ready for review October 11, 2024 20:56

victorsanni requested review from MitchellGoodwin and Piinks October 11, 2024 20:56

victorsanni added 3 commits October 11, 2024 13:58

Tweak scroll gesture name
1971c82

Tweak test comments
7b25f7c

Merge branch 'master' into snap-large-title
bc8868d

MitchellGoodwin reviewed Oct 14, 2024
View reviewed changes

Piinks reviewed Oct 15, 2024
View reviewed changes

victorsanni added 5 commits October 15, 2024 10:57

Add position.hasPixels
64b107a

Merge branch 'master' into snap-large-title
efb9519

Tweak animation values
67c7851

Merge branch 'master' into snap-large-title
fc8daa9

Remove trailing spaces
fba6bbc

victorsanni requested review from MitchellGoodwin and Piinks October 16, 2024 19:24

MitchellGoodwin approved these changes Oct 16, 2024
View reviewed changes

victorsanni marked this pull request as draft October 16, 2024 19:48

victorsanni added 3 commits October 16, 2024 12:51

Merge branch 'master' into snap-large-title
d0cb2d9

Add functionality and tests for bottom widget
ae48f84

Merge branch 'master' into snap-large-title
fd06636

victorsanni marked this pull request as ready for review October 16, 2024 22:36

victorsanni changed the title ~~Snap CupertinoSliverNavigationBar large title~~Snap CupertinoSliverNavigationBar large title and bottom widgetOct 16, 2024

victorsanni added 2 commits October 17, 2024 12:06

Clip bottom in automatic mode
2842819

Merge branch 'master' into snap-large-title
c2f9670

MitchellGoodwin approved these changes Oct 17, 2024
View reviewed changes

Comment threadpackages/flutter/test/cupertino/nav_bar_test.dart
Comment threadpackages/flutter/test/cupertino/nav_bar_test.dart

victorsanni added 4 commits October 18, 2024 10:04

Ensure correct bottom position in tests
d3f6b24

Merge branch 'master' into snap-large-title
6a818bf

Remove unneeded MediaQuerys
f289f62

Merge branch 'master' into snap-large-title
3bfdf27

Piinks approved these changes Oct 18, 2024
View reviewed changes

victorsanni added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 18, 2024

auto-submitBot merged commit 83a0b85 into flutter:masterOct 18, 2024

victorsanni deleted the snap-large-title branch October 18, 2024 20:32

Piinks mentioned this pull request Oct 18, 2024
Flutter Framework Team: Notable Commits #121415
Closed

b3nni97 reviewed Oct 20, 2024
View reviewed changes

victorsanni mentioned this pull request Jan 30, 2025
Add tests to confirm CupertinoSliverNavigationBar snaps when partially scrolled in .always bottom mode #162425
Merged

github-merge-queueBot pushed a commit that referenced this pull request Jan 30, 2025
Add tests to confirm CupertinoSliverNavigationBar snaps when partiall…
5d70532
…y scrolled in .always bottom mode (#162425) Forgot to add these in #156381. Spotted it when debugging #159120.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 12, 2025
83a0b85Snap CupertinoSliverNavigationBar large title and bottom widg…
9a82593
…et (flutter/flutter#156381)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025
83a0b85Snap CupertinoSliverNavigationBar large title and bottom widg…
763cbb3
…et (flutter/flutter#156381)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025
83a0b85Snap CupertinoSliverNavigationBar large title and bottom widg…
1d4d43b
…et (flutter/flutter#156381)

victorsanni mentioned this pull request Feb 14, 2025
Assertion Error using CustomScrollView with CupertinoSliverNavigationBar (Flutter 3.29) #163297
Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 6, 2025
83a0b85Snap CupertinoSliverNavigationBar large title and bottom widg…
328e121
…et (flutter/flutter#156381)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2025
83a0b85Snap CupertinoSliverNavigationBar large title and bottom widg…
aa714e7
…et (flutter/flutter#156381)

techouse mentioned this pull request Jun 7, 2025
CupertinoSliverNavigationBar's collapse animation can get stuck in an infinite loop #164458
Closed

Uh oh!

Conversation

victorsanni commented Oct 8, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Flutter (with this PR):

Native:

Pre-launch Checklist

Uh oh!

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

victorsanni commented Oct 14, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MitchellGoodwin left a comment

Choose a reason for hiding this comment

Uh oh!

victorsanni commented Oct 16, 2024

Uh oh!

victorsanni commented Oct 16, 2024

Uh oh!

MitchellGoodwin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

victorsanni commented Oct 8, 2024•
edited
Loading