Fix SliverMainAxisGroup scrollOffsetCorrection #174369

manu-sncf · 2025-08-25T13:37:34Z

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the gemini-code-assist bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.

gemini-code-assist

Code Review

This pull request fixes an issue where SliverMainAxisGroup did not correctly handle scrollOffsetCorrection from its children. The change adds logic to RenderSliverMainAxisGroup to check for and propagate this correction, which is the correct approach. A comprehensive regression test has been added to verify the fix in a scenario with a dynamically changing list. My review focuses on improving the new test's maintainability by addressing code duplication and inconsistency.

gemini-code-assist · 2025-08-25T13:38:55Z

packages/flutter/test/widgets/sliver_main_axis_group_test.dart

+  testWidgets(
+    'With SliverList can handle inaccurate scroll offset due to changes in children list',
+    (WidgetTester tester) async {
+      bool skip = true;
+      Widget buildItem(BuildContext context, int index) {
+        return !skip || index.isEven
+            ? Card(
+                child: ListTile(title: Text('item$index', style: const TextStyle(fontSize: 80))),
+              )
+            : Container();
+      }
+
+      await tester.pumpWidget(
+        MaterialApp(
+          theme: ThemeData(useMaterial3: false),
+          home: Scaffold(
+            body: CustomScrollView(
+              slivers: <Widget>[
+                SliverMainAxisGroup(
+                  slivers: <Widget>[
+                    SliverList(delegate: SliverChildBuilderDelegate(buildItem, childCount: 30)),
+                  ],
+                ),
+              ],
+            ),
+          ),
+        ),
+      );
+      // Only even items 0~12 are on the screen.
+      expect(find.text('item0'), findsOneWidget);
+      expect(find.text('item12'), findsOneWidget);
+      expect(find.text('item14'), findsNothing);
+
+      await tester.drag(find.byType(CustomScrollView), const Offset(0.0, -750.0));
+      await tester.pump();
+      // Only even items 16~28 are on the screen.
+      expect(find.text('item15'), findsNothing);
+      expect(find.text('item16'), findsOneWidget);
+      expect(find.text('item28'), findsOneWidget);
+
+      skip = false;
+      await tester.pumpWidget(
+        MaterialApp(
+          home: Scaffold(
+            body: CustomScrollView(
+              slivers: <Widget>[
+                SliverMainAxisGroup(
+                  slivers: <Widget>[
+                    SliverList(delegate: SliverChildBuilderDelegate(buildItem, childCount: 30)),
+                  ],
+                ),
+              ],
+            ),
+          ),
+        ),
+      );
+
+      // Only items 12~19 are on the screen.
+      expect(find.text('item11'), findsNothing);
+      expect(find.text('item12'), findsOneWidget);
+      expect(find.text('item19'), findsOneWidget);
+      expect(find.text('item20'), findsNothing);
+
+      await tester.drag(find.byType(CustomScrollView), const Offset(0.0, 250.0));
+      await tester.pump();
+
+      // Only items 10~16 are on the screen.
+      expect(find.text('item9'), findsNothing);
+      expect(find.text('item10'), findsOneWidget);
+      expect(find.text('item16'), findsOneWidget);
+      expect(find.text('item17'), findsNothing);
+
+      // The inaccurate scroll offset should reach zero at this point
+      await tester.drag(find.byType(CustomScrollView), const Offset(0.0, 250.0));
+      await tester.pump();
+
+      // Only items 7~13 are on the screen.
+      expect(find.text('item6'), findsNothing);
+      expect(find.text('item7'), findsOneWidget);
+      expect(find.text('item13'), findsOneWidget);
+      expect(find.text('item14'), findsNothing);
+
+      // It will be corrected as we scroll, so we have to drag multiple times.
+      await tester.drag(find.byType(CustomScrollView), const Offset(0.0, 250.0));
+      await tester.pump();
+      await tester.drag(find.byType(CustomScrollView), const Offset(0.0, 250.0));
+      await tester.pump();
+      await tester.drag(find.byType(CustomScrollView), const Offset(0.0, 250.0));
+      await tester.pump();
+
+      // Only items 0~6 are on the screen.
+      expect(find.text('item0'), findsOneWidget);
+      expect(find.text('item6'), findsOneWidget);
+      expect(find.text('item7'), findsNothing);
+    },
+  );


The MaterialApp widget is constructed twice in this test, and the two instances are inconsistent as the second one (line 1243) is missing the theme property. This could lead to subtle differences in widget sizes, making the test less reliable.¹

To improve readability, reduce duplication, and ensure consistency, consider extracting the widget creation into a local helper function.

For example:

Widget buildFrame() { return MaterialApp( theme: ThemeData(useMaterial3: false), home: Scaffold( body: CustomScrollView( slivers: <Widget>[ SliverMainAxisGroup( slivers: <Widget>[ SliverList(delegate: SliverChildBuilderDelegate(buildItem, childCount: 30)), ], ), ], ), ), ); } await tester.pumpWidget(buildFrame()); // ... skip = false; await tester.pumpWidget(buildFrame());

Style Guide References

Footnotes

Code should be readable and written correctly. Duplicated code with inconsistencies makes it harder to read, maintain, and can introduce bugs. (link) ↩

That's not a bad point. Are the two pumpWidgets sections of the test self contained too? Maybe you could split it into two separate tests.

Both calls of tester.pumpWidget in same test are necessary to update the widgets tree with skip=true/false that trigger a SliverGeometry.scrollOffsetCorrection and thus reproduce the issue.

@justinmc, , can I resolve this ? As mentioned before, both calls to tester.pumpWidget are necessary in this test. The second call uses the skip variable to modify the SliverList state, which is precisely what triggers the SliverGeometry.scrollOffsetCorrection we need to verify.

justinmc · 2025-08-26T22:09:20Z

@Piinks Tagged you for review since this is about Slivers. I also plan to take a look.

justinmc · 2025-08-26T23:05:47Z

packages/flutter/test/widgets/sliver_main_axis_group_test.dart

+      // Only even items 0~12 are on the screen.
+      expect(find.text('item0'), findsOneWidget);
+      expect(find.text('item12'), findsOneWidget);
+      expect(find.text('item14'), findsNothing);


Nit: You could loop through 0-12 if you think it would be valuable to check everything.

Hi @justinmc, thanks for your review. FYI, this test is a duplicate of "slivers_test.dart" -> "SliverList can handle inaccurate scroll offset due to changes in children list".

I modified the test with a loop, as you suggested.

Is it ok for you @justinmc ?

justinmc · 2025-08-26T23:09:03Z

packages/flutter/test/widgets/sliver_main_axis_group_test.dart

+  testWidgets(
+    'With SliverList can handle inaccurate scroll offset due to changes in children list',
+    (WidgetTester tester) async {
+      bool skip = true;
+      Widget buildItem(BuildContext context, int index) {
+        return !skip || index.isEven
+            ? Card(
+                child: ListTile(title: Text('item$index', style: const TextStyle(fontSize: 80))),
+              )
+            : Container();
+      }
+
+      await tester.pumpWidget(
+        MaterialApp(
+          theme: ThemeData(useMaterial3: false),
+          home: Scaffold(
+            body: CustomScrollView(
+              slivers: <Widget>[
+                SliverMainAxisGroup(
+                  slivers: <Widget>[
+                    SliverList(delegate: SliverChildBuilderDelegate(buildItem, childCount: 30)),
+                  ],
+                ),
+              ],
+            ),
+          ),
+        ),
+      );
+      // Only even items 0~12 are on the screen.
+      expect(find.text('item0'), findsOneWidget);
+      expect(find.text('item12'), findsOneWidget);
+      expect(find.text('item14'), findsNothing);
+
+      await tester.drag(find.byType(CustomScrollView), const Offset(0.0, -750.0));
+      await tester.pump();
+      // Only even items 16~28 are on the screen.
+      expect(find.text('item15'), findsNothing);
+      expect(find.text('item16'), findsOneWidget);
+      expect(find.text('item28'), findsOneWidget);
+
+      skip = false;
+      await tester.pumpWidget(
+        MaterialApp(
+          home: Scaffold(
+            body: CustomScrollView(
+              slivers: <Widget>[
+                SliverMainAxisGroup(
+                  slivers: <Widget>[
+                    SliverList(delegate: SliverChildBuilderDelegate(buildItem, childCount: 30)),
+                  ],
+                ),
+              ],
+            ),
+          ),
+        ),
+      );
+
+      // Only items 12~19 are on the screen.
+      expect(find.text('item11'), findsNothing);
+      expect(find.text('item12'), findsOneWidget);
+      expect(find.text('item19'), findsOneWidget);
+      expect(find.text('item20'), findsNothing);
+
+      await tester.drag(find.byType(CustomScrollView), const Offset(0.0, 250.0));
+      await tester.pump();
+
+      // Only items 10~16 are on the screen.
+      expect(find.text('item9'), findsNothing);
+      expect(find.text('item10'), findsOneWidget);
+      expect(find.text('item16'), findsOneWidget);
+      expect(find.text('item17'), findsNothing);
+
+      // The inaccurate scroll offset should reach zero at this point
+      await tester.drag(find.byType(CustomScrollView), const Offset(0.0, 250.0));
+      await tester.pump();
+
+      // Only items 7~13 are on the screen.
+      expect(find.text('item6'), findsNothing);
+      expect(find.text('item7'), findsOneWidget);
+      expect(find.text('item13'), findsOneWidget);
+      expect(find.text('item14'), findsNothing);
+
+      // It will be corrected as we scroll, so we have to drag multiple times.
+      await tester.drag(find.byType(CustomScrollView), const Offset(0.0, 250.0));
+      await tester.pump();
+      await tester.drag(find.byType(CustomScrollView), const Offset(0.0, 250.0));
+      await tester.pump();
+      await tester.drag(find.byType(CustomScrollView), const Offset(0.0, 250.0));
+      await tester.pump();
+
+      // Only items 0~6 are on the screen.
+      expect(find.text('item0'), findsOneWidget);
+      expect(find.text('item6'), findsOneWidget);
+      expect(find.text('item7'), findsNothing);
+    },
+  );


That's not a bad point. Are the two pumpWidgets sections of the test self contained too? Maybe you could split it into two separate tests.

justinmc · 2025-08-26T23:09:12Z

packages/flutter/test/widgets/sliver_main_axis_group_test.dart

@@ -1197,6 +1197,103 @@ void main() {
      expect(tester.getBottomRight(find.byKey(key)), offset - const Offset(0.0, 20.0));
    },
  );
+
+  testWidgets(
+    'With SliverList can handle inaccurate scroll offset due to changes in children list',


I confirmed that this test fails on master.

justinmc · 2025-08-26T23:09:42Z

packages/flutter/lib/src/rendering/sliver_group.dart

+      if (childLayoutGeometry case SliverGeometry(
+        :final double scrollOffsetCorrection,
+      ) when scrollOffsetCorrection != 0) {
+        geometry = SliverGeometry(scrollOffsetCorrection: scrollOffsetCorrection);


I'll defer to @Piinks on whether this is the right way to solve this.

Piinks · 2025-08-28T22:57:29Z

packages/flutter/lib/src/rendering/sliver_group.dart

+      if (childLayoutGeometry case SliverGeometry(
+        :final double scrollOffsetCorrection,
+      ) when scrollOffsetCorrection != 0) {


Can you write this differently? We have not adopted this pattern in the framework because it can be hard for readability in some cases.

Piinks

@yiiim may also be interested in this, they have made a lot of changes here recently.

yiiim

LGTM for the Sliver changes. For tests and formatting issues, I will follow the guidance of @justinmc and @Piinks

Piinks

This LGTM, thank you for the contribution!

Piinks · 2025-09-05T20:33:11Z

There are some merge conflicts that need to be resolved here, and then this should be good to go!

…tter#174369)

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. labels Aug 25, 2025

gemini-code-assist bot reviewed Aug 25, 2025

View reviewed changes

justinmc requested a review from Piinks August 26, 2025 22:08

justinmc reviewed Aug 26, 2025

View reviewed changes

Piinks reviewed Aug 28, 2025

View reviewed changes

yiiim approved these changes Aug 29, 2025

View reviewed changes

Piinks approved these changes Sep 5, 2025

View reviewed changes

manu-sncf added 3 commits September 5, 2025 22:37

Fix SliverMainAxisGroup scrollOffsetCorrection

aa9de8a

Review - check all items

b0f6c72

Review - no pattern matching

482c7ea

manu-sncf force-pushed the scroll_offset_correction branch from 0dec903 to 482c7ea Compare September 5, 2025 20:38

Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 5, 2025

auto-submit bot added this pull request to the merge queue Sep 5, 2025

Merged via the queue into flutter:master with commit 242043f Sep 5, 2025
78 checks passed

flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 5, 2025

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 6, 2025

242043fca Fix SliverMainAxisGroup scrollOffsetCorrection (flutter/flu…

6b750db

…tter#174369)

engine-flutter-autoroll mentioned this pull request Sep 6, 2025

Roll Flutter from 87d5b753196c to ad3cd078d192 (13 revisions) flutter/packages#9966

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 6, 2025

242043fca Fix SliverMainAxisGroup scrollOffsetCorrection (flutter/flu…

6834dc5

…tter#174369)

engine-flutter-autoroll mentioned this pull request Sep 6, 2025

Roll Flutter from 87d5b753196c to ad3cd078d192 (13 revisions) flutter/packages#9967

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 7, 2025

242043fca Fix SliverMainAxisGroup scrollOffsetCorrection (flutter/flu…

3d721b0

…tter#174369)

engine-flutter-autoroll mentioned this pull request Sep 7, 2025

Roll Flutter from 87d5b753196c to 06ab279c02bb (15 revisions) flutter/packages#9969

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 7, 2025

242043fca Fix SliverMainAxisGroup scrollOffsetCorrection (flutter/flu…

7e092da

…tter#174369)

engine-flutter-autoroll mentioned this pull request Sep 7, 2025

Roll Flutter from 87d5b753196c to 06ab279c02bb (15 revisions) flutter/packages#9970

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 7, 2025

242043fca Fix SliverMainAxisGroup scrollOffsetCorrection (flutter/flu…

a2ec25f

…tter#174369)

engine-flutter-autoroll mentioned this pull request Sep 7, 2025

Roll Flutter from 87d5b753196c to 0081ee50dc3f (16 revisions) flutter/packages#9971

Closed

Fix SliverMainAxisGroup scrollOffsetCorrection #174369

Fix SliverMainAxisGroup scrollOffsetCorrection #174369

Conversation

manu-sncf commented Aug 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Aug 25, 2025

Choose a reason for hiding this comment

Style Guide References

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmc commented Aug 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manu-sncf Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

yiiim left a comment

Choose a reason for hiding this comment

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Piinks commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

manu-sncf commented Aug 25, 2025 •

edited

Loading

manu-sncf Aug 27, 2025 •

edited

Loading

Piinks commented Sep 5, 2025 •

edited

Loading