Skip to content

Conversation

@chunhtai
Copy link
Contributor

…rection accidently set the child with layout offset of zero

Description

consider following example:
we have sliver list with items where each item has 10 paint extent. the screen is at item 10

-------start of screen----------
item 10 -- layout offset 100
item 11 -- layout offset 110
item 12 -- layout offset 120
item 13 -- layout offset 130
-------end of screen-----------

now, the sliver list children updated, and the each item has 20 paint extent. The RenderSliverList is smart to update the children scroll offset after the first child. So it become like this

-------start of screen----------
item 10 -- layout offset 100
item 11 -- layout offset 120
-------end of screen-----------

item 12 and 13 are not in the screen so it will be destroy.

Now, start scrolling back

-------start of screen----------
item 9 -- layout offset 80
item 11 -- layout offset 100
-------end of screen-----------

...keep scrolling

-------start of screen----------
item 5 -- layout offset 0
item 11 -- layout offset 20
-------end of screen-----------
oops, the sliver list think item 5 is the start of the list, so you cannot scroll any further.

Related Issues

Fixes #59819

Tests

I added the following tests:

see files

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 read the Tree Hygiene wiki page, which explains my responsibilities.
  • 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

Did any tests fail when you ran them? Please read Handling breaking changes.

…rection accidently set the child with layout offset of zero
@fluttergithubbot fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Jun 19, 2020
@chunhtai chunhtai requested review from Piinks and goderbauer June 19, 2020 21:11
@Piinks Piinks added the f: scrolling Viewports, list views, slivers, etc. label Jun 22, 2020
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.

NICE! 🎉

Flutter_LGTM

testWidgets(
'SliverList can handle inaccurate scroll offset due to changes in children list',
(WidgetTester tester) async {
bool skip = true;
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 one of those issue references in case it gets lost in history?

Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

},
);

testWidgets(
Copy link
Member

Choose a reason for hiding this comment

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

Add: "Regression test for " here?


assert(childScrollOffset(firstChild) > -precisionErrorTolerance);

// If the scroll offset is at the zero, we should make sure we are
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// If the scroll offset is at the zero, we should make sure we are
// If the scroll offset is at zero, we should make sure we are

assert(childScrollOffset(firstChild) > -precisionErrorTolerance);

// If the scroll offset is at the zero, we should make sure we are
// actually at the beginning of the list
Copy link
Member

Choose a reason for hiding this comment

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

nit: end with a .

@flutter-github-sync
Copy link

Google testing passed!

1 similar comment
@flutter-github-sync
Copy link

Google testing passed!

mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 30, 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.

AnimatedList/ListView missing elements on scroll

6 participants