Skip to content

RenderViewport max layout cycles should depend on number of slivers #144104

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 13, 2024
Merged

RenderViewport max layout cycles should depend on number of slivers #144104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bd2d3df
RenderViewport max layout cycles should depend on number of children
knopp Feb 25, 2024
ec2cfad
formatting
knopp Feb 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions packages/flutter/lib/src/rendering/viewport.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1385,7 +1385,7 @@ class RenderViewport extends RenderViewportBase<SliverPhysicalContainerParentDat
return constraints.biggest;
}

static const int _maxLayoutCycles = 10;
static const int _maxLayoutCyclesPerChild = 10;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a little lower now that it is per child? Before a viewport with 3 sliver children would allow 10 passes, now it would be 30.

Copy link
Member Author

@knopp knopp Feb 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think any reasonable sliver would need more than a handful of passes, the problem is that you can have nested slivers in MultiSliver or even SliverMainAxisGroup (but I don't think that one properly propagates scrollOffsetCorrection?) and we have no good way to count correction count per actual source sliver. For that we would probably need changes to SliverGeometry.

So lowering the mainLayoutCycles could potentially be breaking change for someone that has nested slivers.

More importantly I do think that the check is meant to detect pathological cases where the layout is stuck in a loop of corrections, and as such it does matter if it is 10 passes or 30? It will throw an exception anyway so it is not something that happens during regular layout process.

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM, thanks for explaining your thinking here. :)


// Out-of-band data computed during layout.
late double _minScrollExtent;
Expand Down Expand Up @@ -1419,6 +1419,7 @@ class RenderViewport extends RenderViewportBase<SliverPhysicalContainerParentDat
};

final double centerOffsetAdjustment = center!.centerOffsetAdjustment;
final int maxLayoutCycles = _maxLayoutCyclesPerChild * childCount;

double correction;
int count = 0;
Expand All @@ -1435,9 +1436,9 @@ class RenderViewport extends RenderViewportBase<SliverPhysicalContainerParentDat
}
}
count += 1;
} while (count < _maxLayoutCycles);
} while (count < maxLayoutCycles);
assert(() {
if (count >= _maxLayoutCycles) {
if (count >= maxLayoutCycles) {
assert(count != 1);
throw FlutterError(
'A RenderViewport exceeded its maximum number of layout cycles.\n'
Expand Down
81 changes: 81 additions & 0 deletions packages/flutter/test/rendering/viewport_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2370,4 +2370,85 @@ void main() {
);
errors.clear();
});

testWidgets('RenderViewport maxLayoutCycles depends on the number of children',
(WidgetTester tester) async {
Future<void> expectFlutterError({
required Widget widget,
required WidgetTester tester,
}) async {
final List<FlutterErrorDetails> errors = <FlutterErrorDetails>[];
final FlutterExceptionHandler? oldHandler = FlutterError.onError;
FlutterError.onError = (FlutterErrorDetails error) => errors.add(error);
try {
await tester.pumpWidget(widget);
} finally {
FlutterError.onError = oldHandler;
}
expect(errors, isNotEmpty);
expect(errors.first.exception, isFlutterError);
}

Widget buildWidget({required int sliverCount, required int correctionsCount}) {
return Directionality(
textDirection: TextDirection.ltr,
child: CustomScrollView(
slivers: List<Widget>.generate(
sliverCount,
(_) => _ScrollOffsetCorrectionSliver(correctionsCount: correctionsCount)),
),
);
}

// 5 correction per child will pass.
await tester.pumpWidget(buildWidget(sliverCount: 30, correctionsCount: 5));

// 15 correction per child will throw exception.
await expectFlutterError(
widget: buildWidget(sliverCount: 1, correctionsCount: 15),
tester: tester,
);
});
}

// Simple sliver that applies N scroll offset corrections.
class _RenderScrollOffsetCorrectionSliver extends RenderSliver {
int _correctionCount = 0;
@override
void performLayout() {
if (_correctionCount > 0) {
--_correctionCount;
geometry = const SliverGeometry(scrollOffsetCorrection: 1.0);
return;
}
const double extent = 5;
final double paintedChildSize = calculatePaintOffset(constraints, from: 0.0, to: extent);
final double cacheExtent = calculateCacheOffset(constraints, from: 0.0, to: extent);

geometry = SliverGeometry(
scrollExtent: extent,
paintExtent: paintedChildSize,
maxPaintExtent: extent,
cacheExtent: cacheExtent
);
}
}

class _ScrollOffsetCorrectionSliver extends SingleChildRenderObjectWidget {
const _ScrollOffsetCorrectionSliver({required this.correctionsCount});
final int correctionsCount;

@override
_RenderScrollOffsetCorrectionSliver createRenderObject(BuildContext context) {
final _RenderScrollOffsetCorrectionSliver sliver = _RenderScrollOffsetCorrectionSliver();
sliver._correctionCount = correctionsCount;
return sliver;
}

@override
void updateRenderObject(BuildContext context, covariant _RenderScrollOffsetCorrectionSliver renderObject) {
super.updateRenderObject(context, renderObject);
renderObject.markNeedsLayout();
renderObject._correctionCount = correctionsCount;
}
}