Make RenderSliverGrid more accurately report overflow #104064
Conversation
flutter-dashboard
bot
added
f: scrolling
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
Piinks
left a comment
Piinks
left a comment
There was a problem hiding this comment.
I did a bit of digging in the git history. I left a comment in the issue regarding a potential test case. Let me know what you think. :)
Co-authored-by: Kate Lovett <[email protected]>
|
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
The diff reported is .01%. I can't find the pixel that is different :( |
|
Ahhh I see the difference now. This is almost certainly related to pixel snapping. Given that, I think we should wait for @jonahwilliams's changes in flutter_tester to eb adopted internally first - this will likely cause a large number of scuba diffs internally because it no longer pushes a layer in cases where it used to. I'm going to mark this as draft for now. |
|
IOW - this patch removes a composited layer in some cases because the GridView no longer always thinks it needs to push a clip layer with Clip.hardEdge, because it doesn't always think it has visual overflow. |
|
This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
(Triage) @dnfield do you have a reference to the change this PR is waiting on so we can track? |
|
Jonah's changes landed and this shoul dbe good for review now. |
|
(almost all of google golden testing now runs without pixel snapping, meaning this change won't introduce insanity in terms of golden changes.. I hope :) |
| child: GridView( | ||
| gridDelegate: const SliverGridDelegateWithFixedCrossAxisCount(crossAxisCount: 3), | ||
| children: <Widget>[ | ||
| Container(height: 200.0), |
There was a problem hiding this comment.
How do you know this isn't overflowing? is it just because 200 < default tester window height?
There was a problem hiding this comment.
Yes. This is small enough to fit into the viewport.
godofredoc
added
autosubmit
* Make RenderSliverGrid more accurately report overflow * Update packages/flutter/lib/src/rendering/sliver_grid.dart Co-authored-by: Kate Lovett <[email protected]> Co-authored-by: Kate Lovett <[email protected]>
4 participants
Fixes #104046
Existing test needed to be updated because it does not actually cause overflow - even though the container height is 2000, it's getting clipped by the individual grid square - not by the whole grid itself. Adding more squares to the grid causes overflow to happen.