Skip to content

Commit 5d24e81

Browse files
huycozyash2moon
authored andcommitted
Fix Carousel crashes when using PageStorageKey (flutter#166817)
- Fix flutter#166067 - PageStorage is usually used to save and restore offset of scrollable widget when the widget is recreated in practice. From the crash stack trace in bug report, it happens when [oldViewportDimensions gets a null value](https://github.com/flutter/flutter/blob/3fa9b387052363e413b906da08c1b1d2d4140dfb/packages/flutter/lib/src/material/carousel.dart#L1472-L1485). Tracing it down, it’s due to [hasViewportDimension is false](https://github.com/flutter/flutter/blob/3fa9b387052363e413b906da08c1b1d2d4140dfb/packages/flutter/lib/src/widgets/scroll_position.dart#L275). There might be a very brief moment when the scroll metrics are being reconstructed, and the viewport dimensions are not yet ready, which triggers the crash. If a PageStorageKey is not specified, the scroll position would be newly created fresh each time, avoiding this restoration process and the associated null state. - The fix is simply to not force casting `oldViewportDimensions!`, instead, it falls back to `viewportDimension` which is non-null for safety. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
1 parent fe32a5a commit 5d24e81

File tree

2 files changed

+42
-1
lines changed

2 files changed

+42
-1
lines changed

packages/flutter/lib/src/material/carousel.dart

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1498,7 +1498,7 @@ class _CarouselPosition extends ScrollPositionWithSingleContext implements _Caro
14981498
// If resize from zero, we should use the _cachedItem to recover the state.
14991499
item = _cachedItem!;
15001500
} else {
1501-
item = getItemFromPixels(oldPixels, oldViewportDimensions!);
1501+
item = getItemFromPixels(oldPixels, oldViewportDimensions ?? viewportDimension);
15021502
}
15031503
final double newPixels = getPixelsFromItem(item, flexWeights, itemExtent);
15041504
// If the viewportDimension is zero, cache the item

packages/flutter/test/material/carousel_test.dart

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1433,6 +1433,47 @@ void main() {
14331433
expect(tester.takeException(), isNull);
14341434
});
14351435

1436+
// Regression test for https://github.com/flutter/flutter/issues/166067.
1437+
testWidgets('CarouselView should not crash when using PageStorageKey', (
1438+
WidgetTester tester,
1439+
) async {
1440+
await tester.pumpWidget(
1441+
MaterialApp(
1442+
home: Scaffold(
1443+
body: NestedScrollView(
1444+
headerSliverBuilder: (BuildContext context, bool innerBoxIsScrolled) {
1445+
return const <Widget>[SliverAppBar()];
1446+
},
1447+
body: CustomScrollView(
1448+
key: const PageStorageKey<String>('key1'),
1449+
slivers: <Widget>[
1450+
SliverToBoxAdapter(
1451+
child: ConstrainedBox(
1452+
constraints: const BoxConstraints(maxHeight: 50),
1453+
child: CarouselView.weighted(
1454+
flexWeights: const <int>[1, 2],
1455+
consumeMaxWeight: false,
1456+
children: List<Widget>.generate(20, (int index) {
1457+
return ColoredBox(
1458+
color: Colors.primaries[index % Colors.primaries.length].withValues(
1459+
alpha: 0.8,
1460+
),
1461+
child: const SizedBox.expand(),
1462+
);
1463+
}),
1464+
),
1465+
),
1466+
),
1467+
],
1468+
),
1469+
),
1470+
),
1471+
),
1472+
);
1473+
1474+
expect(tester.takeException(), isNull);
1475+
});
1476+
14361477
group('CarouselController.animateToItem', () {
14371478
testWidgets('CarouselView.weighted horizontal, not reversed, flexWeights [7,1]', (
14381479
WidgetTester tester,

0 commit comments

Comments
 (0)