Support Floating+ SliverAppBars in NestedScrollViews #57707
Conversation
Piinks
added
framework
fluttergithubbot
added
the
f: material design
Piinks
removed
the
f: material design
fluttergithubbot
added
the
f: material design
packages/flutter/lib/src/rendering/sliver_persistent_header.dart
Outdated
Show resolved
Hide resolved
| layoutExtent: layoutExtent.clamp(0.0, clampedPaintExtent) as double, | ||
| maxPaintExtent: maxExtent + stretchOffset, | ||
| maxScrollObstructionExtent: maxExtent, | ||
| maxScrollObstructionExtent: minExtent, |
There was a problem hiding this comment.
Why this change? It seems like maxExtend should be the max scroll obstraction?
There was a problem hiding this comment.
This is a separate bug that eluded me for a while.
The portion of the app bar that is actually pinned is the minExtent, while the maxExtent of a floating-pinned app bar accounts for any expanded portion that will float/collapse/expand when scrolled, so it only reduces the scrollable area by the minExtent for a floating-pinned app bar.
A regular pinned app bar is the same (maxScrollObstructionExtent: minExtent), and a floating app bar has maxScrollObstructionExtent: 0.0, so I believe it should be the minExtent.
As it was with maxExtent, the expanded portion of the app bar would not float/collapse/expand as expected in a NestedScrollView.
There was a problem hiding this comment.
Update: There was concern that since maxScrollObstructionExtent (used by the SliverOverlap* widgets in a NestedScrollView) is also used in viewport.dart it may affect a11y traversal in scrollables, which was the motivation for introducing the property in #11878. I've been checking A11y traversal this morning against this change and have found it is working as intended. We should add some more tests around this expectation (the floating/pinned config), as there appear to only be tests for pinned SliverAppBars currently.
| /// | ||
| /// This is useful for an outer scrollable containing a [SliverAppBar] that | ||
| /// is expected to float. This cannot be null. | ||
| final bool floatHeaderSlivers; |
There was a problem hiding this comment.
Should we give this flag a more generic name to indicate that this will prioritize the outer (header) scrollable? It's not necessarily tied to floatingslivers, right?
There was a problem hiding this comment.
I agree. I have some thoughts though. I think this is the only use case where you would want the outer scrollable to be prioritized before the inner when scrolling in reverse, it's definitely unique. By using a name unrelated to 'floating', will it make discoverability harder? Would it be difficult for someone to understand that when trying to float a nested app bar that this flag (i.e. prioritizeOuterScrollableInReverseDirection) would be their answer?
| header?.maybeStopSnapAnimation(_position.userScrollDirection); | ||
| else | ||
| header?.maybeStartSnapAnimation(_position.userScrollDirection); |
There was a problem hiding this comment.
As discussed over VC: Can you remind my why just calling maybeStartSnapAnimation doesn't work when floatHeaderSlivers is set to true?
There was a problem hiding this comment.
This is due to the fact that as the scroll offset is changing, the snap animation can be broken when animating closed. When the app bar goes to animate closed, it is animating from the _effectiveScrollOffset to the maxExtent of the RenderSliverFloatingPersistentHeader, however if the actual scrollOffset is less than the maxExtent the animation gets stuck. In the case of NestedScrollView, the scrollOffset will always be <= maxExtent since the maxExtent is actually the maxScrollExtent of the entire scrollable. Usually in a CustomScrollView, the scrollOffset is much greater since the slivers that follow the app bar are part of the same scrollable.
For example, if you checkout this branch and run the sample without the nestedSnap flag on the SliverAppBar you can reproduce this:
- I scroll away an outer nested floating app bar fully out of view, it will be at both its maxExtent of the render object and the maxScrollExtent of the scroll view.
scrollOffset: 224.0, _effectiveScrollOffset: 224.0 - I scroll in reverse, floating the app bar into view by ~90.0 pixels,
scrollOffset: 132.0, _effectiveScrollOffset: 132.0 - Once I release the drag, the animation is triggered, animating from the
_effectiveScrollOffset --> 0.0, revealing the entire app bar. The actual scrollOffset does not change during the animation, so when the animation completes we have
scrollOffset: 132.0, _effectiveScrollOffset: 0.0. - Now, I scroll forward, starting to float some of the app bar out of view by ~10.0 pixels
scrollOffset: 142.0, _effectiveScrollOffset: 10.0 - Once I release the drag, the animation will be triggered to close the app bar, sliding it out of view, animating from the
_effectiveScrollOffset --> maxExtent (224.0). The scrollOffset does not change during this animation, so the animation gets stuck when it reaches the actual scrollOffset of 142.0, never reaching 224.0, leaving part of the app bar still exposed. After animating:
scrollOffset: 142.0, _effectiveScrollOffset: 142.0(not the intended result) 🤕 🤒
Animating the position directly solves this issue.
There was a problem hiding this comment.
Discussed this further over VC today...
We talked about landing in the else block here (on master branch):
but the animation is actually breaking down prior to that.
The animation gets stuck because of this line (on master branch):
The _effectiveScrollOffset gets clamped to be within the bounds of the current real scroll offset during layout, so when trying to animate to (example) 224.0, it can't exceed an actual scroll offset of (example) 132.0.
There was a problem hiding this comment.
I see, that problem would probably go away if we .clamp(0.0, maxExtent). However, that would presumably change the behavior in the non-nested case when you're scrolling close to the top.
There was a problem hiding this comment.
Update: We discussed using animateTo instead of setPixels. I used setPixels instead of animateTo since this specific animation is triggered by the listener on the scrolling activity. Calling animateTo for this animation results in an infinite loop as it just re-triggers itself.
Codecov Report
@@ Coverage Diff @@
## master #57707 +/- ##
==========================================
+ Coverage 70.14% 71.21% +1.07%
==========================================
Files 208 208
Lines 23316 23229 -87
==========================================
+ Hits 16354 16543 +189
+ Misses 6962 6686 -276
Continue to review full report at Codecov.
|
| if (_position != null) | ||
| _position.isScrollingNotifier.removeListener(_isScrollingListener); | ||
| _controller?.dispose(); | ||
| _controller = null; // lazily recreated if we're reattached. |
There was a problem hiding this comment.
Regarding the comment: After "dispose" is called a state object should never reattach.
| header?.maybeStopSnapAnimation(_position.userScrollDirection); | ||
| else | ||
| header?.maybeStartSnapAnimation(_position.userScrollDirection); |
There was a problem hiding this comment.
I see, that problem would probably go away if we .clamp(0.0, maxExtent). However, that would presumably change the behavior in the non-nested case when you're scrolling close to the top.
|
The solution for the snap animation is unfavorable, so we are going to remove it and document that the snap animation is a known limitation. I'll also file a bug for it and link everything together. I'm going to close this to preserve the implementation, and open a new PR without the snapping. |
Now, is just waiting until the great work you've done in #59187 is merged with master, and then we can all celebrate it 🍾 ? |
5 participants
Description
This PR adds support for floating the outer header slivers of a
NestedScrollViewover the inner scroll view. 🎉This is achieved by introducing
NestedScrollView.floatHeaderSlivers, a boolean flag for specifying that the header is expected to float. When true, theNestedScrollView's scrolling coordinator will prioritize the outer scroll view before passing any leftover delta to the inner scroll view.Normally, when an app bar floats, the render object keeps track of a mock scroll offset (_effectiveScrollOffset), since as the app bar moves in and out of view, its position can vary in accordance with the actual scroll offset. Instead of following the mocking convention, as a separate scroll view, we can just change the real offset when floating from a
NestedScrollView.Also in this change is another new flag,
FloatingHeaderSnapConfiguration.nestedSnap. Since we are changing the actual scroll offset of the outer scroll view as we float, the snap animation of a floating app bar will need to be handled differently forNestedScrollViews whenfloatHeaderSliversis true. In order to properly snap in and out as expected, instead of animating the mock scroll offset (_effectiveScrollOffset) of theRenderSliverFloatingPersistentHeader, we animate on the actual position of the outer scroll view.Using none of these flags maintains the existing snapping animation without floating, in case anyone may want to keep what they already have in place. :)
There is documentation and sample applications to illustrate using these flags in conjunction with the many configurations of a sliver app bar. This change does not address stretching sliver app bars. That is being tracked in #54059, and will come in a follow up change.
Finally, this also makes a small change to the
SliverGeometry.maxScrollObstructionof theRenderSliverFloatingPinnedPersistentHeader. The previous geometry set the maxExtent, when it should be the minExtent. Using the maxExtent prevents an expanded, pinned, floating app bar from collapsing as expected.Related Issues
Fixes #49560
Fixes #29264
Fixes #17518
Fixes #20877
Related change: #55069
Tests
I added the following tests:
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Did any tests fail when you ran them? Please read Handling breaking changes.