Fix SliverMainAxisGroup layout in reverse #145572
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Piinks
added
framework
HansMuller
reviewed
Mar 22, 2024
| throw FlutterError( | ||
| 'Unreachable sliver found, you may have a sliver behind ' | ||
| 'a sliver with infinite extent. ' | ||
| 'Unreachable sliver found, you may have a sliver following ' |
| context.paintChild(child, offset + childParentData.paintOffset); | ||
| context.paintChild(child, childOffset); | ||
| } | ||
| child = childBefore(child); |
Contributor
There was a problem hiding this comment.
I don't completely understand this. Are we cycling backwards through every sliver group child, even though at some point they will no longer be child.geometry!.visible? It seems like a short-circuit would be useful although maybe no short-circuit is possible?
Contributor
Author
There was a problem hiding this comment.
We paint them in reverse because the group supports overlapping widgets like pinned app bars. Paint order stacks, so if we painted from first to last, a pinned app bar would be covered by the slivers it is meant to overlap. We only paint slivers that are visible to be efficient.
Contributor
There was a problem hiding this comment.
I think what you're saying is that a short-circuit exit from the loop isn't possible because the visibility of one child doesn't imply anything about previous children?
jellynoone
reviewed
Mar 22, 2024
| addExtent = true; | ||
| } | ||
|
|
||
| RenderSliver? child = lastChild; |
Contributor
There was a problem hiding this comment.
@Piinks Thank you for working on this!
Note, I'm not well versed with slivers so I might be wrong here, but wouldn't this paint children in reverse order?
Wouldn't a layout like this be rendered incorrectly then? i.e. the blue box would be rendered beneath the brown box due to a paint transform being applied.
SliverMainAxisGroup(slivers: [
const SliverToBoxAdapter(
child: ColoredBox(
color: Colors.brown,
child: SizedBox(height: 100),
),
),
SliverToBoxAdapter(
child: Transform.translate(
offset: const Offset(0, -50),
child: const ColoredBox(
color: Colors.blue,
child: SizedBox(height: 100),
),
),
),
]);However, it seems slivers in general are painted in reverse.
I tested the following code on stable and I'd assume all to render the same but they don't.
PageThroughList renders as expected, while MainAxisGroupPage and PageThroughSlivers render the blue box below the brown one.
import 'package:flutter/material.dart';
void main() {
runApp(const MainApp());
}
class MainApp extends StatelessWidget {
const MainApp({super.key});
@override
Widget build(BuildContext context) {
return const MaterialApp(
home: PageThroughSlivers(),
);
}
}
const _brown = ColoredBox(
color: Colors.brown,
child: SizedBox(height: 100),
);
final _blue = Transform.translate(
offset: const Offset(0, -50),
child: const ColoredBox(
color: Color(0xcc0000ff),
child: SizedBox(height: 100),
),
);
class MainAxisGroupPage extends StatelessWidget {
const MainAxisGroupPage({super.key});
@override
Widget build(BuildContext context) {
return Scaffold(
body: CustomScrollView(slivers: [
SliverMainAxisGroup(slivers: [
const SliverToBoxAdapter(child: _brown),
SliverToBoxAdapter(child: _blue),
]),
]),
);
}
}
class PageThroughSlivers extends StatelessWidget {
const PageThroughSlivers({super.key});
@override
Widget build(BuildContext context) {
return Scaffold(
body: CustomScrollView(slivers: [
const SliverToBoxAdapter(child: _brown),
SliverToBoxAdapter(child: _blue),
]),
);
}
}
class PageThroughList extends StatelessWidget {
const PageThroughList({super.key});
@override
Widget build(BuildContext context) {
return Scaffold(
body: CustomScrollView(slivers: [
SliverList.list(children: [
_brown,
_blue,
]),
]),
);
}
}Should I file another issue or is this working as expected?
Contributor
Author
There was a problem hiding this comment.
Hi @jellynoone can you file a separate issue for this? SliverMainAxisGroup has always painted from last to first so this PR is not changing that behavior.
Contributor
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Apr 1, 2024
auto-submit bot
pushed a commit
to flutter/packages
that referenced
this pull request
Apr 2, 2024
Manual roll requested by [email protected] flutter/flutter@9d32f07...7fa932b 2024-04-01 [email protected] Roll Flutter Engine from e6f19409b613 to ea93c5d91b12 (3 revisions) (flutter/flutter#146100) 2024-04-01 [email protected] Refactor realm_checker (flutter/flutter#145905) 2024-04-01 [email protected] Add info strings to code blocks. (flutter/flutter#146085) 2024-04-01 [email protected] Add test for animated_container.0.dart API example. (flutter/flutter#145995) 2024-04-01 [email protected] Roll Flutter Engine from 8dff6b833fe2 to e6f19409b613 (2 revisions) (flutter/flutter#146093) 2024-04-01 [email protected] Roll Flutter Engine from d33666d90916 to 8dff6b833fe2 (3 revisions) (flutter/flutter#146087) 2024-04-01 [email protected] Fix SliverMainAxisGroup layout in reverse (flutter/flutter#145572) 2024-04-01 [email protected] Roll Flutter Engine from dd4f5cd5c9d5 to d33666d90916 (3 revisions) (flutter/flutter#146083) 2024-04-01 [email protected] Roll Packages from 51faaa1 to d5aff19 (3 revisions) (flutter/flutter#146081) 2024-04-01 [email protected] Fixes some gesture recognizers are not disposed. (flutter/flutter#146072) 2024-04-01 [email protected] Flutter Gradle Plugin: add versionName and versionCode to FlutterExtension (flutter/flutter#146044) 2024-04-01 [email protected] Roll Flutter Engine from bf348cd73d49 to dd4f5cd5c9d5 (1 revision) (flutter/flutter#146071) 2024-04-01 [email protected] Roll Flutter Engine from 984a78b04671 to bf348cd73d49 (1 revision) (flutter/flutter#146065) 2024-04-01 [email protected] Deprecate `ButtonBar`, `ButtonBarThemeData`, and `ThemeData.buttonBarTheme` (flutter/flutter#145523) 2024-04-01 [email protected] Add `DataColumn.headingRowAlignment ` for `DataTable` (flutter/flutter#144006) 2024-04-01 [email protected] Roll Flutter Engine from e9d35f8bfbe2 to 984a78b04671 (2 revisions) (flutter/flutter#146062) 2024-04-01 [email protected] Roll Flutter Engine from 4f6b832c8e33 to e9d35f8bfbe2 (1 revision) (flutter/flutter#146060) 2024-03-31 [email protected] Roll Flutter Engine from 9689390986b7 to 4f6b832c8e33 (1 revision) (flutter/flutter#146055) 2024-03-31 [email protected] Roll Flutter Engine from 34081fea4d59 to 9689390986b7 (1 revision) (flutter/flutter#146053) 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],[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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
TecHaxter
pushed a commit
to TecHaxter/flutter_packages
that referenced
this pull request
May 22, 2024
) Manual roll requested by [email protected] flutter/flutter@9d32f07...7fa932b 2024-04-01 [email protected] Roll Flutter Engine from e6f19409b613 to ea93c5d91b12 (3 revisions) (flutter/flutter#146100) 2024-04-01 [email protected] Refactor realm_checker (flutter/flutter#145905) 2024-04-01 [email protected] Add info strings to code blocks. (flutter/flutter#146085) 2024-04-01 [email protected] Add test for animated_container.0.dart API example. (flutter/flutter#145995) 2024-04-01 [email protected] Roll Flutter Engine from 8dff6b833fe2 to e6f19409b613 (2 revisions) (flutter/flutter#146093) 2024-04-01 [email protected] Roll Flutter Engine from d33666d90916 to 8dff6b833fe2 (3 revisions) (flutter/flutter#146087) 2024-04-01 [email protected] Fix SliverMainAxisGroup layout in reverse (flutter/flutter#145572) 2024-04-01 [email protected] Roll Flutter Engine from dd4f5cd5c9d5 to d33666d90916 (3 revisions) (flutter/flutter#146083) 2024-04-01 [email protected] Roll Packages from 51faaa1 to d5aff19 (3 revisions) (flutter/flutter#146081) 2024-04-01 [email protected] Fixes some gesture recognizers are not disposed. (flutter/flutter#146072) 2024-04-01 [email protected] Flutter Gradle Plugin: add versionName and versionCode to FlutterExtension (flutter/flutter#146044) 2024-04-01 [email protected] Roll Flutter Engine from bf348cd73d49 to dd4f5cd5c9d5 (1 revision) (flutter/flutter#146071) 2024-04-01 [email protected] Roll Flutter Engine from 984a78b04671 to bf348cd73d49 (1 revision) (flutter/flutter#146065) 2024-04-01 [email protected] Deprecate `ButtonBar`, `ButtonBarThemeData`, and `ThemeData.buttonBarTheme` (flutter/flutter#145523) 2024-04-01 [email protected] Add `DataColumn.headingRowAlignment ` for `DataTable` (flutter/flutter#144006) 2024-04-01 [email protected] Roll Flutter Engine from e9d35f8bfbe2 to 984a78b04671 (2 revisions) (flutter/flutter#146062) 2024-04-01 [email protected] Roll Flutter Engine from 4f6b832c8e33 to e9d35f8bfbe2 (1 revision) (flutter/flutter#146060) 2024-03-31 [email protected] Roll Flutter Engine from 9689390986b7 to 4f6b832c8e33 (1 revision) (flutter/flutter#146055) 2024-03-31 [email protected] Roll Flutter Engine from 34081fea4d59 to 9689390986b7 (1 revision) (flutter/flutter#146053) 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],[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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #145068
The original tests for SliverMainAxisGroup did not actually check where the child was painted (#126596).
Followed the same pattern in RenderSliverMultiBoxAdaptor:
flutter/packages/flutter/lib/src/rendering/sliver_multi_box_adaptor.dart
Line 666 in 11c034f
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.