Skip to content

SliverFillRemaining accounts for child size when hasScrollBody is false - #35810

Merged
Piinks merged 23 commits into
flutter:masterfrom
Piinks:sliverFillRemainingChild
Jul 19, 2019
Merged

Piinks merged 23 commits into
flutter:masterfrom
Piinks:sliverFillRemainingChild

Conversation

@Piinks

@Piinks Piinks commented Jul 9, 2019 •

Copy link
Copy Markdown
Contributor

Description

SliverFillRemaining stipulates that it will size its child to fill the remaining space in the viewport in the main axis.
Its actual default behavior has been to fill the entire viewport in the main axis, which is desirable behavior for applications such as NestedScrollView, but is not the expected behavior for most cases.

#33627 added the flag hasScrollBody, which allows developers to specify if the child is a scrollable, like in a NestedScrollView, or not. However, this implementation did not take into account cases when the child's size should be used instead of the calculated extent of the sliver.
This PR modifies the original implementation of #33627 to include this consideration.
When hasScrollBody is false, the child's size will be compared to that of the calculated extent to choose the appropriate size.

Also in this PR is an additional flag - fillOverscroll. This give developers even more power over the layout of this widget, as they can specify what the behavior should be for scroll physics that utilize an overscroll feature, such as with the default scroll behavior on iOS platforms. fillOverscroll defaults to false, meaning the child widget will maintain its size during overscroll. When set to true, it will cause the child's size to stretch and fill the overscroll area.

An executable study app is available here that illustrates use cases and behaviors: gist

Related Issues

Fixes #35758
Related to #35753, will follow-up with documentation and diagrams.
Related to #17444 (#33627)

Tests

SliverFillRemaining - hasScrollBody

  • does not extend past viewport when false
  • scrolls beyond viewport by default
  • child without size is sized by extent when false
  • child with size is sized by extent when false
  • extent is overridden by child with larger size when false
  • extent is overridden by child size if precedingScrollExtent > viewportMainAxisExtent when false

iOS/similar scroll physics when hasScrollBody: false & fillOverscroll: true

  • child without size is sized by extent and overscroll
  • child with size is overridden and sized by extent and overscroll
  • extent is overridden by child size and overscroll if precedingScrollExtent > viewportMainAxisExtent

Android/other scroll physics when hasScrollBody: false, ignores fillOverscroll: true

  • child without size is sized by extent, fillOverscroll is ignored
  • child with size is overridden and sized by extent, fillOverscroll is ignored
  • extent is overridden by child size if precedingScrollExtent > viewportMainAxisExtent, fillOverscroll is ignored

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.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

  • No, this is not a breaking change.

@Piinks Piinks added framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. work in progress; do not review labels Jul 9, 2019
@Piinks Piinks self-assigned this Jul 9, 2019
@Piinks

Piinks commented Jul 9, 2019

Copy link
Copy Markdown
Contributor Author

Will incorporate sample code & visuals to better illustrate the use cases of this feature. ref: #35753

@Piinks Piinks changed the title SliverFillRemaining accounts for child size when hasScrollBody is false WIP SliverFillRemaining accounts for child size when hasScrollBody is false Jul 9, 2019
@codecov

This comment has been minimized.

@codecov

This comment has been minimized.

@codecov

This comment has been minimized.

@codecov

This comment has been minimized.

@googlebot

Copy link
Copy Markdown

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

@googlebot

Copy link
Copy Markdown

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@Piinks
Piinks marked this pull request as ready for review July 17, 2019 20:35
@Piinks Piinks changed the title WIP SliverFillRemaining accounts for child size when hasScrollBody is false SliverFillRemaining accounts for child size when hasScrollBody is false Jul 17, 2019
}
}

assert(extent.isFinite);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's add a test covering this, and ideally have a message here that explains what can be done if anything (e.g. wrapping the child in something else or changing the child to some other type?).

@Piinks Piinks Jul 18, 2019 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

(post-offline chat) I haven't been able to trigger this assert in a test, as it is currently covered by the child's layout method. I will leave this assertion in place thought to guard against future changes that may result in triggering this assert.
I have added a message to explain why this assertion would fail.

@dnfield dnfield left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@Piinks
Piinks merged commit 38e41f5 into flutter:master Jul 19, 2019
/// defer to the child's size rather than overriding it.
bool hasScrollBody;

/// Indicates whether the child should stretch to fill the overscroll area

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: the first paragraph of a doc comment should be a single sentence summary.

johnsonmh pushed a commit to johnsonmh/flutter that referenced this pull request Jul 30, 2019
…se (flutter#35810)

Fixes the hasScrollBody flag not accounting for child size. Adds the ability to specify over-scroll behavior.
@Piinks
Piinks deleted the sliverFillRemainingChild branch November 22, 2019 00:53
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Aug 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

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.

SliverFillRemaining with hasScrollBody: false "hides" child behind scrollable

4 participants