Skip to content

Conversation

@thkim1011
Copy link
Contributor

@thkim1011 thkim1011 commented May 11, 2023

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

  • 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.
  • All existing and new tests are passing.

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

@flutter-dashboard flutter-dashboard bot added f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. labels May 11, 2023
@flutter-dashboard
Copy link

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.

@thkim1011 thkim1011 marked this pull request as draft May 11, 2023 21:27
@goderbauer goderbauer requested a review from Piinks May 16, 2023 22:10
Copy link
Contributor

@Piinks Piinks left a 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?

@github-actions github-actions bot removed framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. labels May 29, 2023
@flutter-dashboard flutter-dashboard bot added f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. labels May 30, 2023
@thkim1011 thkim1011 marked this pull request as ready for review May 30, 2023 03:25
@thkim1011 thkim1011 requested a review from Piinks May 30, 2023 03:25
@github-actions github-actions bot removed f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. labels May 30, 2023
Copy link
Contributor

@Piinks Piinks left a 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
Copy link
Contributor

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?

Copy link
Contributor Author

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
Copy link
Contributor

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]?

Copy link
Contributor Author

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.

Comment on lines 184 to 185
/// the total layout extent of the sliver by painting slivers that are "out of
/// bounds" with a negative [SliverPhysicalParentData.paintOffset].
Copy link
Contributor

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?

Copy link
Contributor Author

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;
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.
Copy link
Contributor

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?

Copy link
Contributor Author

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
Copy link
Contributor

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. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@thkim1011 thkim1011 requested a review from Piinks June 1, 2023 20:36
@github-actions github-actions bot added d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos documentation f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. c: contributor-productivity Team-specific productivity, code health, technical debt. labels Jun 1, 2023
Comment on lines 187 to 191
Copy link
Contributor

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?

@thkim1011 thkim1011 requested a review from Piinks June 6, 2023 20:26
@brajas-quinnox
Copy link

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

Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Piinks Piinks force-pushed the tae/sliver-main-axis-group branch from 41a6d2d to adb1f06 Compare June 7, 2023 18:29
@Piinks Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 7, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jun 8, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Jun 8, 2023

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.

@thkim1011 thkim1011 merged commit f2351f6 into flutter:master Jun 8, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 9, 2023
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Jun 9, 2023
flutter/flutter@6e254a3...da127f1

2023-06-09 [email protected] Updated material button theme tests for Material3 (flutter/flutter#128543)
2023-06-09 [email protected] Roll Flutter Engine from cb93477008d6 to 93afba901b3b (2 revisions) (flutter/flutter#128573)
2023-06-09 [email protected] Improve defaults generation with logging, stats, and token validation (flutter/flutter#128244)
2023-06-09 [email protected] [testing] Make the FLUTTER_STORAGE_BASE_URL warning non-fatal (flutter/flutter#128335)
2023-06-09 [email protected] [flutter_tools] [DAP] Don't try to restart/reload if app hasn't started yet (flutter/flutter#128267)
2023-06-09 [email protected] Roll Flutter Engine from 8f9e608d39ab to cb93477008d6 (3 revisions) (flutter/flutter#128568)
2023-06-09 [email protected] Replace `MaterialButton` from test classes (flutter/flutter#128466)
2023-06-09 [email protected] Fix `showBottomSheet` doesn't remove scrim when draggable sheet is dismissed (flutter/flutter#128455)
2023-06-09 [email protected] Manual roll Flutter Engine from a5f7d5d75ff2 to 8f9e608d39ab (31 revisions) (flutter/flutter#128554)
2023-06-09 [email protected] Revert "test owners: cyanglaz -> vashworth" (flutter/flutter#128462)
2023-06-09 [email protected] [Android] Bump integration tests using `compileSdkVersion` 31 to 33 (flutter/flutter#128072)
2023-06-09 [email protected] Remove single view assumption from MouseTracker, and unify its hit testing code flow (flutter/flutter#127060)
2023-06-09 [email protected] [flutter_tools] Precache after channel switch (flutter/flutter#118129)
2023-06-08 [email protected] Adding migration guide for Material 3 colors (flutter/flutter#128429)
2023-06-08 [email protected] Add `AppLifecycleListener`, with support for application exit handling (flutter/flutter#123274)
2023-06-08 [email protected] Sliver Main Axis Group (flutter/flutter#126596)
2023-06-08 [email protected] Reduce `_DoubleClampVisitor` false positives (flutter/flutter#128539)
2023-06-08 [email protected] Advise developers to use OverflowBar instead of ButtonBar (flutter/flutter#128437)
2023-06-08 [email protected] Reland "Migrate benchmarks to package:web" (flutter/flutter#128266)
2023-06-08 [email protected] Navigator.pop before PopupMenuItem onTap call (flutter/flutter#127446)
2023-06-08 [email protected] Fix navigation rail with long labels misplaced highlights (flutter/flutter#128324)
2023-06-08 [email protected] Update `chip.dart` to use set of `MaterialState` (flutter/flutter#128507)
2023-06-08 [email protected] Update flutter to dartdoc 6.3.0 and hide Icons implementation from doc pages (flutter/flutter#128442)
2023-06-08 [email protected] Disable blinking cursor when `EditableText.showCursor` is false (flutter/flutter#127562)
2023-06-08 [email protected] [floating_cursor_selection]add more comments on the tricky part (flutter/flutter#127227)
2023-06-08 [email protected] Move RenderObjectElement.updateChildren to Element (flutter/flutter#128458)
2023-06-08 [email protected] Fix PointerEventConverter doc (flutter/flutter#128452)
2023-06-08 [email protected] 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 [email protected],[email protected] 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
@Areopagitics
Copy link

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:

import 'package:flutter/material.dart';

void main() => runApp(const SliverMainAxisGroupExampleApp());

class SliverMainAxisGroupExampleApp extends StatelessWidget {
  const SliverMainAxisGroupExampleApp({super.key});

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      home: Scaffold(
        appBar: AppBar(title: const Text('SliverMainAxisGroup Sample')),
        body: const SliverMainAxisGroupExample(),
      ),
    );
  }
}

class Delegate extends SliverPersistentHeaderDelegate {
  final Color backgroundColor;
  final String _title;

  Delegate(this.backgroundColor, this._title);

  @override
  Widget build(BuildContext context, double shrinkOffset, bool overlapsContent) {
    return Container(
      color: backgroundColor,
      child: Center(
        child: Text(
          _title,
          style: const TextStyle(
            color: Colors.black,
            fontSize: 25,
          ),
        ),
      ),
    );
  }

  @override
  double get maxExtent => 50;

  @override
  double get minExtent => 50;

  @override
  bool shouldRebuild(SliverPersistentHeaderDelegate oldDelegate) {
    return true;
  }
}

class SliverMainAxisGroupExample extends StatelessWidget {
  const SliverMainAxisGroupExample({super.key});

  @override
  Widget build(BuildContext context) {
    return CustomScrollView(
      slivers: <Widget>[
        SliverMainAxisGroup(
          slivers: <Widget>[
            SliverPersistentHeader(
              pinned: true,
              floating: false,
              delegate: Delegate(Colors.white, 'Test'),
          ),
            SliverList.builder(
              itemBuilder: (BuildContext context, int index) {
                return Container(
                  color: index.isEven ? Colors.amber[300] : Colors.blue[300],
                  height: 100.0,
                  child: Center(
                    child: Text(
                      'Item $index',
                      style: const TextStyle(fontSize: 24),
                    ),
                  ),
                );
              },
              itemCount: 5,
            ),
            SliverPersistentHeader(
                pinned: true,
                floating: false,
                delegate: Delegate(Colors.white, 'Test2'),
            ),
            SliverList.builder(
              itemBuilder: (BuildContext context, int index) {
                return Container(
                  color: index.isEven ? Colors.amber[300] : Colors.blue[300],
                  height: 100.0,
                  child: Center(
                    child: Text(
                      'Item $index',
                      style: const TextStyle(fontSize: 24),
                    ),
                  ),
                );
              },
              itemCount: 5,
            ),
            SliverToBoxAdapter(
              child: Container(
                color: Colors.cyan,
                height: 100,
                child: const Center(
                  child: Text('Another sliver child',
                      style: TextStyle(fontSize: 24)),
                ),
              ),
            )
          ],
        ),
        SliverToBoxAdapter(
          child: Container(
            height: 1000,
            decoration: const BoxDecoration(color: Colors.greenAccent),
            child: const Center(
              child: Text('Hello World!', style: TextStyle(fontSize: 24)),
            ),
          ),
        ),
      ],
    );
  }
}

@rMozes
Copy link

rMozes commented Jul 2, 2023

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:

import 'package:flutter/material.dart';

void main() => runApp(const SliverMainAxisGroupExampleApp());

class SliverMainAxisGroupExampleApp extends StatelessWidget {
  const SliverMainAxisGroupExampleApp({super.key});

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      home: Scaffold(
        appBar: AppBar(title: const Text('SliverMainAxisGroup Sample')),
        body: const SliverMainAxisGroupExample(),
      ),
    );
  }
}

class Delegate extends SliverPersistentHeaderDelegate {
  final Color backgroundColor;
  final String _title;

  Delegate(this.backgroundColor, this._title);

  @override
  Widget build(BuildContext context, double shrinkOffset, bool overlapsContent) {
    return Container(
      color: backgroundColor,
      child: Center(
        child: Text(
          _title,
          style: const TextStyle(
            color: Colors.black,
            fontSize: 25,
          ),
        ),
      ),
    );
  }

  @override
  double get maxExtent => 50;

  @override
  double get minExtent => 50;

  @override
  bool shouldRebuild(SliverPersistentHeaderDelegate oldDelegate) {
    return true;
  }
}

class SliverMainAxisGroupExample extends StatelessWidget {
  const SliverMainAxisGroupExample({super.key});

  @override
  Widget build(BuildContext context) {
    return CustomScrollView(
      slivers: <Widget>[
        SliverMainAxisGroup(
          slivers: <Widget>[
            SliverPersistentHeader(
              pinned: true,
              floating: false,
              delegate: Delegate(Colors.white, 'Test'),
          ),
            SliverList.builder(
              itemBuilder: (BuildContext context, int index) {
                return Container(
                  color: index.isEven ? Colors.amber[300] : Colors.blue[300],
                  height: 100.0,
                  child: Center(
                    child: Text(
                      'Item $index',
                      style: const TextStyle(fontSize: 24),
                    ),
                  ),
                );
              },
              itemCount: 5,
            ),
            SliverPersistentHeader(
                pinned: true,
                floating: false,
                delegate: Delegate(Colors.white, 'Test2'),
            ),
            SliverList.builder(
              itemBuilder: (BuildContext context, int index) {
                return Container(
                  color: index.isEven ? Colors.amber[300] : Colors.blue[300],
                  height: 100.0,
                  child: Center(
                    child: Text(
                      'Item $index',
                      style: const TextStyle(fontSize: 24),
                    ),
                  ),
                );
              },
              itemCount: 5,
            ),
            SliverToBoxAdapter(
              child: Container(
                color: Colors.cyan,
                height: 100,
                child: const Center(
                  child: Text('Another sliver child',
                      style: TextStyle(fontSize: 24)),
                ),
              ),
            )
          ],
        ),
        SliverToBoxAdapter(
          child: Container(
            height: 1000,
            decoration: const BoxDecoration(color: Colors.greenAccent),
            child: const Center(
              child: Text('Hello World!', style: TextStyle(fontSize: 24)),
            ),
          ),
        ),
      ],
    );
  }
}

@Areopagitics probably you should wrap each header in different SliverMainAxisGroup in order to make it work

@Areopagitics
Copy link

Areopagitics commented Jul 3, 2023

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

import 'package:flutter/material.dart';

void main() => runApp(const SliverMainAxisGroupExampleApp());

class SliverMainAxisGroupExampleApp extends StatelessWidget {
  const SliverMainAxisGroupExampleApp({super.key});

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      home: Scaffold(
        appBar: AppBar(title: const Text('SliverMainAxisGroup Sample')),
        body: const SliverMainAxisGroupExample(),
      ),
    );
  }
}

class Delegate extends SliverPersistentHeaderDelegate {
  final Color backgroundColor;
  final String _title;

  Delegate(this.backgroundColor, this._title);

  @override
  Widget build(BuildContext context, double shrinkOffset, bool overlapsContent) {
    return Container(
      color: backgroundColor,
      child: Center(
        child: Text(
          _title,
          style: const TextStyle(
            color: Colors.black,
            fontSize: 25,
          ),
        ),
      ),
    );
  }

  @override
  double get maxExtent => 50;

  @override
  double get minExtent => 50;

  @override
  bool shouldRebuild(SliverPersistentHeaderDelegate oldDelegate) {
    return true;
  }
}

class SliverMainAxisGroupExample extends StatelessWidget {
  const SliverMainAxisGroupExample({super.key});

  @override
  Widget build(BuildContext context) {
    return CustomScrollView(
      slivers: <Widget>[
        SliverMainAxisGroup(
          slivers: <Widget>[
            SliverPersistentHeader(
              pinned: true,
              floating: false,
              delegate: Delegate(Colors.white, 'Test'),
          ),
            SliverList.builder(
              itemBuilder: (BuildContext context, int index) {
                return Container(
                  color: index.isEven ? Colors.amber[300] : Colors.blue[300],
                  height: 100.0,
                  child: Center(
                    child: Text(
                      'Item $index',
                      style: const TextStyle(fontSize: 24),
                    ),
                  ),
                );
              },
              itemCount: 5,
            ),
          ],
        ),
       SliverMainAxisGroup(
          slivers: <Widget>[
            SliverPersistentHeader(
              pinned: true,
              floating: false,
              delegate: Delegate(Colors.white, 'Test 2'),
          ),
            SliverList.builder(
              itemBuilder: (BuildContext context, int index) {
                return Container(
                  color: index.isEven ? Colors.amber[300] : Colors.blue[300],
                  height: 100.0,
                  child: Center(
                    child: Text(
                      'Item $index',
                      style: const TextStyle(fontSize: 24),
                    ),
                  ),
                );
              },
              itemCount: 5,
            ),
          ],
        ),
      ],
    );
  }
}

@Areopagitics
Copy link

I've been testing SliverMainAxisGroup and it would be a nice feature if a Sliverlist inside a SliverMainAxisGroup could have a center key in CustomScrollView. For now only the immediate child of the CustomScrollView will work with its center key feature.

@Piinks
Copy link
Contributor

Piinks commented Jul 26, 2023

probably you should wrap each header in different SliverMainAxisGroup in order to make it work

This is correct.

Regarding the behavior of center, that is the intended behavior and is not specific to SliverMainAxisGroup.

The center must be the key of one of the slivers built by buildSlivers.

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.

@thkim1011 thkim1011 deleted the tae/sliver-main-axis-group branch July 26, 2023 20:44
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
auto-submit bot pushed a commit that referenced this pull request Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt. d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A Flex (Column/Row) like widget for slivers

5 participants