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

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

Fixes CupertinoSliverNavigationBar should snap between large and middle title
Fixes CupertinoSliverNavigationBar 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

packages/flutter/lib/src/cupertino/nav_bar.dart

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

packages/flutter/lib/src/cupertino/nav_bar.dart

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

packages/flutter/lib/src/cupertino/nav_bar.dart

+  @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.

packages/flutter/lib/src/cupertino/nav_bar.dart

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.

packages/flutter/test/cupertino/nav_bar_test.dart

Piinks

LGTM

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

packages/flutter/lib/src/cupertino/nav_bar.dart

+    }
+
+    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)

Snap CupertinoSliverNavigationBar large title

6cdd492

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: cupertino flutter/packages/flutter/cupertino repository 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 widget Oct 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

packages/flutter/test/cupertino/nav_bar_test.dart Show resolved Hide resolved

packages/flutter/test/cupertino/nav_bar_test.dart Show resolved Hide resolved

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-submit bot merged commit 83a0b85 into flutter:master Oct 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-queue bot 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

83a0b85 Snap 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

83a0b85 Snap 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

83a0b85 Snap 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

83a0b85 Snap 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

83a0b85 Snap 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

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