-
Notifications
You must be signed in to change notification settings - Fork 29.2k
Sliver Main Axis Group #126596
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sliver Main Axis Group #126596
Conversation
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thkim1011 is this ready for a review?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some sample code that illustrates the pinned case?
/// A sliver that places multiple sliver children in a linear array along the | ||
/// main axis. | ||
/// | ||
/// Typically, the slivers will be laid out one at a time. Slivers that have been |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what case would this order of layout not occur?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the documentation to remove this line.
/// main axis. | ||
/// | ||
/// Typically, the slivers will be laid out one at a time. Slivers that have been | ||
/// scrolled past partially or entirely will be provided a nonzero scrollOffset that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this referring to the pinned header case? I am not sure I follow. Also, if scrollOffset is a reference, can you add a [breadcrumb]?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the documentation.
/// the total layout extent of the sliver by painting slivers that are "out of | ||
/// bounds" with a negative [SliverPhysicalParentData.paintOffset]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I see now. It is bit difficult to discern which sliver you are referring to, the group as a whole or its children, can you clarify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I clarified this point.
|
||
@override | ||
void paint(PaintingContext context, Offset offset) { | ||
RenderSliver? child = lastChild; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you share why this is the painting order?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think visitChildForSemantics expects the children to be visited in paint order, so that may need to be overridden to work with this reverse paint order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. This is the paint order because this is how Viewport
paints its sliver children. I think it's necessary for SliverPersistentHeader
to be painted on top of the widgets that come after it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you going to implement visitChildrenForSemantics?
/// the main axis, one after another. | ||
/// | ||
/// For pinned sliver children, the behavior is that the pinned sliver should | ||
/// scroll up after all of the main content has been scrolled through. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a See also
for SliverCrossAxisGroup? And then add one for the cross axis group that refers back here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
/// A sliver that places multiple sliver children in a linear array along | ||
/// the main axis, one after another. | ||
/// | ||
/// For pinned sliver children, the behavior is that the pinned sliver should |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you reference the API you are referring to here? Folks may not know what a pinned sliver child is in this context. Some sample code of this exact case would be really great actually. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
/// The layout algorithm lays out slivers one by one. If the sliver is visible | ||
/// on the [Viewport] due to a sufficiently high [SliverConstraints.scrollOffset], | ||
/// then we compute a valid [SliverConstraints.scrollOffset] and | ||
/// [SliverConstraints.remainingPaintExtent] based on the total [SliverConstraints.scrollOffset] | ||
/// of all the sliver children laid out so far. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what this paragraph is saying, can you elaborate?
@Piinks I was wondering how to achieve the following. There are four nested listviews inside another external/parent listview. All vertical. When user starts scrolling the first nested listview and it reaches the end of scroll, now when trying to scroll the same nested listview will make the outer listview start scrolling. Is there a inbuilt solution for this in Flutter? Please let me know. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
41a6d2d
to
adb1f06
Compare
auto label is removed for flutter/flutter, pr: 126596, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
flutter/flutter@6e254a3...da127f1 2023-06-09 hans.muller@gmail.com Updated material button theme tests for Material3 (flutter/flutter#128543) 2023-06-09 engine-flutter-autoroll@skia.org Roll Flutter Engine from cb93477008d6 to 93afba901b3b (2 revisions) (flutter/flutter#128573) 2023-06-09 6655696+guidezpl@users.noreply.github.com Improve defaults generation with logging, stats, and token validation (flutter/flutter#128244) 2023-06-09 whesse@google.com [testing] Make the FLUTTER_STORAGE_BASE_URL warning non-fatal (flutter/flutter#128335) 2023-06-09 danny@tuppeny.com [flutter_tools] [DAP] Don't try to restart/reload if app hasn't started yet (flutter/flutter#128267) 2023-06-09 engine-flutter-autoroll@skia.org Roll Flutter Engine from 8f9e608d39ab to cb93477008d6 (3 revisions) (flutter/flutter#128568) 2023-06-09 tessertaha@gmail.com Replace `MaterialButton` from test classes (flutter/flutter#128466) 2023-06-09 tessertaha@gmail.com Fix `showBottomSheet` doesn't remove scrim when draggable sheet is dismissed (flutter/flutter#128455) 2023-06-09 engine-flutter-autoroll@skia.org Manual roll Flutter Engine from a5f7d5d75ff2 to 8f9e608d39ab (31 revisions) (flutter/flutter#128554) 2023-06-09 ychris@google.com Revert "test owners: cyanglaz -> vashworth" (flutter/flutter#128462) 2023-06-09 43054281+camsim99@users.noreply.github.com [Android] Bump integration tests using `compileSdkVersion` 31 to 33 (flutter/flutter#128072) 2023-06-09 dkwingsmt@users.noreply.github.com Remove single view assumption from MouseTracker, and unify its hit testing code flow (flutter/flutter#127060) 2023-06-09 christopherfujino@gmail.com [flutter_tools] Precache after channel switch (flutter/flutter#118129) 2023-06-08 leigha.jarett@gmail.com Adding migration guide for Material 3 colors (flutter/flutter#128429) 2023-06-08 gspencergoog@users.noreply.github.com Add `AppLifecycleListener`, with support for application exit handling (flutter/flutter#123274) 2023-06-08 thkim1011@users.noreply.github.com Sliver Main Axis Group (flutter/flutter#126596) 2023-06-08 31859944+LongCatIsLooong@users.noreply.github.com Reduce `_DoubleClampVisitor` false positives (flutter/flutter#128539) 2023-06-08 leigha.jarett@gmail.com Advise developers to use OverflowBar instead of ButtonBar (flutter/flutter#128437) 2023-06-08 jacksongardner@google.com Reland "Migrate benchmarks to package:web" (flutter/flutter#128266) 2023-06-08 53684884+mhbdev@users.noreply.github.com Navigator.pop before PopupMenuItem onTap call (flutter/flutter#127446) 2023-06-08 leroux_bruno@yahoo.fr Fix navigation rail with long labels misplaced highlights (flutter/flutter#128324) 2023-06-08 tessertaha@gmail.com Update `chip.dart` to use set of `MaterialState` (flutter/flutter#128507) 2023-06-08 jcollins@google.com Update flutter to dartdoc 6.3.0 and hide Icons implementation from doc pages (flutter/flutter#128442) 2023-06-08 31859944+LongCatIsLooong@users.noreply.github.com Disable blinking cursor when `EditableText.showCursor` is false (flutter/flutter#127562) 2023-06-08 41930132+hellohuanlin@users.noreply.github.com [floating_cursor_selection]add more comments on the tricky part (flutter/flutter#127227) 2023-06-08 goderbauer@google.com Move RenderObjectElement.updateChildren to Element (flutter/flutter#128458) 2023-06-08 goderbauer@google.com Fix PointerEventConverter doc (flutter/flutter#128452) 2023-06-08 engine-flutter-autoroll@skia.org Roll Packages from a84b2c2 to e13b8c4 (9 revisions) (flutter/flutter#128508) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
I noticed issue #108289 regarding sticky headers was closed, since @Piinks mentioned that SliverMainAxisGroup resolved it. I just tested SliverMainAxisGroup using two pinned SliverPersistentHeaders in the main branch here using the following code and the sticky header feature still doesn't work:
|
@Areopagitics probably you should wrap each header in different SliverMainAxisGroup in order to make it work |
@rMozes Thank you so much! I'm guessing, though, that this feature will take another month or so to land in the stable branch. Here is some sample code for Sticky Headers that others can test here for future reference:
|
I've been testing |
This is correct. Regarding the behavior of
https://api.flutter.dev/flutter/widgets/ScrollView/center.html Please file an issue request if you would like to see this work differently. Comments on old PRs are typically not responded to. |
Fixes #145068 The original tests for SliverMainAxisGroup did not actually check where the child was painted (#126596). Followed the same pattern in RenderSliverMultiBoxAdaptor: https://github.com/flutter/flutter/blob/11c034f0371eb28576d41a0c218ccae6b38e7702/packages/flutter/lib/src/rendering/sliver_multi_box_adaptor.dart#L666
This widget implements the ability to place slivers one after another in a single ScrollView in a way that all child slivers are drawn within the bounds of the group itself (i.e. SliverPersistentHeaders aren't drawn outside of the scroll extent provided by all of the child slivers). The design document for SliverMainAxisGroup can be found here.
Fixes #33137.
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.