Skip to content

Conversation

@nate-thegrate
Copy link
Contributor

closes #40740

NestedScrollController fix

Demo source code

(click to expand)

(this is a slightly more concise version of the code sample from #40740)

import 'package:flutter/material.dart';

void main() {
  runApp(
    const MaterialApp(
      debugShowCheckedModeBanner: false,
      home: Scaffold(body: NewsScreen()),
    ),
  );
}

class NewsScreen extends StatelessWidget {
  const NewsScreen({super.key});

  static const List<String> _tabs = <String>['Featured', 'Popular', 'Latest'];

  static final List<Widget> _tabViews = <Widget>[
    for (final String name in _tabs)
      SafeArea(
        top: false,
        bottom: false,
        child: Builder(builder: (BuildContext context) {
          final handle = NestedScrollView.sliverOverlapAbsorberHandleFor(context);

          return NotificationListener<ScrollNotification>(
            onNotification: (ScrollNotification notification) => true,
            child: CustomScrollView(
              key: PageStorageKey<String>(name),
              slivers: <Widget>[
                SliverOverlapInjector(handle: handle),
                SliverPadding(
                  padding: const EdgeInsets.all(8.0),
                  sliver: SliverList(
                    delegate: SliverChildBuilderDelegate(
                      childCount: 30,
                      (BuildContext context, int index) => Container(
                        margin: const EdgeInsets.only(bottom: 8),
                        width: double.infinity,
                        height: 150,
                        color: const Color(0xFFB0A4C8),
                        alignment: Alignment.center,
                        child: Text(
                          '$name $index',
                          style: const TextStyle(fontWeight: FontWeight.w600),
                        ),
                      ),
                    ),
                  ),
                ),
              ],
            ),
          );
        }),
      ),
  ];

  @override
  Widget build(BuildContext context) {
    return DefaultTabController(
      length: _tabs.length,
      child: NestedScrollView(
        headerSliverBuilder: (BuildContext context, bool innerBoxIsScrolled) => <Widget>[
          SliverOverlapAbsorber(
            handle: NestedScrollView.sliverOverlapAbsorberHandleFor(context),
            sliver: SliverSafeArea(
              top: false,
              sliver: SliverAppBar(
                title: const Text('Tab Demo'),
                floating: true,
                pinned: true,
                snap: true,
                forceElevated: innerBoxIsScrolled,
                bottom: TabBar(
                  tabs: _tabs.map((String name) => Tab(text: name)).toList(),
                ),
              ),
            ),
          ),
        ],
        body: TabBarView(children: _tabViews),
      ),
    );
  }
}


This bug can be traced to a return statement inside _NestedScrollPosition:

double applyClampedDragUpdate(double delta) {
  // ...
  return delta + offset;
}

Thanks to some quirks of floating-point arithmetic, applyClampedDragUpdate would sometimes return a tiny non-zero value, which ends up ruining one of the scroll coordinator's equality checks.

bool get canScrollBody {
final _NestedScrollPosition? outer = _outerPosition;
if (outer == null) {
return true;
}
return outer.haveDimensions && outer.extentAfter == 0.0;
}

@github-actions github-actions bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos labels Oct 28, 2024
@github-actions github-actions bot removed the a: text input Entering text in a text field or keyboard related problems label Oct 31, 2024
Copy link
Contributor

@bleroux bleroux left a comment

Choose a reason for hiding this comment

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

LGTM! I'm not a scrolling expert, so up to you to seek another reviewer.

@festelo
Copy link

festelo commented Nov 2, 2024

Hey, I tried to run the example (with some modifications for easier testing on Windows) and noticed that the scroll position resets when scrolling up, is this expected?

ezgif-4-2f4e706114

Text version of the gif:

  • Scroll to "Featured 11" (with finger or mouse wheel)
  • Switch tab to "Popular"
  • Scroll to "Popular 6"
  • Switch tab to "Featured"
  • Switch tab to "Popular"
  • Scroll up to "Popular 4", so the sliver app bar expands
  • Switch tab to "Popular"
  • Scroll position is 0
Code
import 'dart:ui';

import 'package:flutter/material.dart';

void main() {
  runApp(
    MaterialApp(
      scrollBehavior: const MaterialScrollBehavior().copyWith(
        physics: const ClampingScrollPhysics(),
        scrollbars: false,
        dragDevices: {
          PointerDeviceKind.mouse,
          PointerDeviceKind.touch,
          PointerDeviceKind.stylus,
          PointerDeviceKind.unknown
        },
      ),
      debugShowCheckedModeBanner: false,
      home: const Scaffold(
        body: NewsScreen(),
      ),
    ),
  );
}

class NewsScreen extends StatelessWidget {
  const NewsScreen({super.key});

  static const List<String> _tabs = <String>['Featured', 'Popular', 'Latest'];

  static final List<Widget> _tabViews = <Widget>[
    for (final String name in _tabs)
      SafeArea(
        top: false,
        bottom: false,
        child: Builder(builder: (BuildContext context) {
          final handle =
              NestedScrollView.sliverOverlapAbsorberHandleFor(context);

          return NotificationListener<ScrollNotification>(
            onNotification: (ScrollNotification notification) => true,
            child: CustomScrollView(
              key: PageStorageKey<String>(name),
              slivers: <Widget>[
                SliverOverlapInjector(handle: handle),
                SliverPadding(
                  padding: const EdgeInsets.all(8.0),
                  sliver: SliverList(
                    delegate: SliverChildBuilderDelegate(
                      childCount: 30,
                      (BuildContext context, int index) => Container(
                        margin: const EdgeInsets.only(bottom: 8),
                        width: double.infinity,
                        height: 150,
                        color: const Color(0xFFB0A4C8),
                        alignment: Alignment.center,
                        child: Text(
                          '$name $index',
                          style: const TextStyle(fontWeight: FontWeight.w600),
                        ),
                      ),
                    ),
                  ),
                ),
              ],
            ),
          );
        }),
      ),
  ];

  @override
  Widget build(BuildContext context) {
    return DefaultTabController(
      length: _tabs.length,
      child: NestedScrollView(
        headerSliverBuilder: (BuildContext context, bool innerBoxIsScrolled) =>
            <Widget>[
          SliverOverlapAbsorber(
            handle: NestedScrollView.sliverOverlapAbsorberHandleFor(context),
            sliver: SliverSafeArea(
              top: false,
              sliver: SliverAppBar(
                title: const Text('Tab Demo'),
                floating: true,
                pinned: true,
                snap: true,
                forceElevated: innerBoxIsScrolled,
                bottom: TabBar(
                  tabs: _tabs.map((String name) => Tab(text: name)).toList(),
                ),
              ),
            ),
          ),
        ],
        body: TabBarView(children: _tabViews),
      ),
    );
  }
}

@nate-thegrate
Copy link
Contributor Author

oh no

@nate-thegrate
Copy link
Contributor Author

Well, I have good news: I copy-pasted your code sample and ran it against my branch, and it's working!

working tab demo

I'm not exactly sure what's causing the discrepancy, but I did verify that the test I added fails on master and passes on this branch, so it's most likely something on your end.


Also, quick side note—I believe a blank line would fix your code snippet's formatting:

<details>
  <summary>Code</summary>
+                        
```dart
import 'dart:ui';

@festelo
Copy link

festelo commented Nov 2, 2024

Also, quick side note—I believe a blank line would fix your code snippet's formatting:

Yep, already changed it. But TY anyway :)
And for working on such a long-standing issue, too.

I'm not exactly sure what's causing the discrepancy, but I did verify that the test I added fails on master and passes on this branch, so it's most likely something on your end.

This particular issue may be not related directly to the PR, I'm not fully familiar with the context, sorry for this.
I did clone your branch for testing, but sure it can still be something on my end.
I can't reproduce it just by scrolling the list up, there's definitely another factor involved, but I haven't identified it yet.
However, following in precise the steps I described before always leads to this problem for me.

And I can confirm that your test passes for me too.

Can you try to run this one?

Code
  testWidgets('Maintains scroll position of inactive tab #2', (WidgetTester tester) async {
    await tester.pumpWidget(const example.NestedScrollViewExampleApp());

    final Finder finderItemAny = find.byType(ListTile, skipOffstage: false).first;
    final Finder finderItem11 = find.text('Item 11', skipOffstage: false);
    final Finder finderItem6 = find.text('Item 6', skipOffstage: false);
    final Finder finderItem4 = find.text('Item 4', skipOffstage: false);

    final Finder finderTab1 = find.text('Tab 1');
    final Finder finderTab2 = find.text('Tab 2');

    double getScrollPosition() {
      return Scrollable.of(tester.element(finderItemAny)).position.pixels;
    }

    expect(getScrollPosition(), 0.0);

    await tester.ensureVisible(finderItem11);
    await tester.pumpAndSettle();

    await tester.tap(finderTab2);
    await tester.pumpAndSettle();

    await tester.ensureVisible(finderItem6);
    await tester.pumpAndSettle();

    await tester.tap(finderTab1);
    await tester.pumpAndSettle();

    final tab1Position = getScrollPosition();

    await tester.tap(finderTab2);
    await tester.pumpAndSettle();

    // Without these two lines, the test passes
    await tester.ensureVisible(finderItem4);
    await tester.pumpAndSettle();

    await tester.tap(finderTab1);
    await tester.pumpAndSettle();

    expect(getScrollPosition(), tab1Position);
  });

If it works for you (or if it doesn't work for any other reason than being a bug) - i think we can wrap up this topic.

@nate-thegrate
Copy link
Contributor Author

Thanks for sending that over—I can confirm that the test fails on this branch (and passes without those two lines).

I believe the reason for this is: calling tester.ensureVisible(finderItem4) causes it to scroll all the way to the top, i.e. getScrollPosition() returns 0.

On this branch, scrolling one of the tabs up partway (so that the app bar becomes visible) maintains the scroll positions of each tab, but going all the way to the top of the scroll view will reset the scroll positions.

Code
  testWidgets('Maintains scroll position of inactive tab #2', (WidgetTester tester) async {
    await tester.pumpWidget(const example.NestedScrollViewExampleApp());

    final Finder finderItemAny = find.byType(ListTile, skipOffstage: false).first;
    final Finder finderItem11 = find.text('Item 11', skipOffstage: false);
    final Finder finderItem6 = find.text('Item 6', skipOffstage: false);
    final Finder finderItem4 = find.text('Item 4', skipOffstage: false);

    final Finder finderTab1 = find.text('Tab 1');
    final Finder finderTab2 = find.text('Tab 2');

    double getScrollPosition() {
      return Scrollable.of(tester.element(finderItemAny)).position.pixels;
    }

    expect(getScrollPosition(), 0.0);

    await tester.ensureVisible(finderItem11);
    await tester.pumpAndSettle();

    await tester.tap(finderTab2);
    await tester.pumpAndSettle();

    await tester.ensureVisible(finderItem6);
    await tester.pumpAndSettle();

    await tester.tap(finderTab1);
    await tester.pumpAndSettle();

    final double tab1Position = getScrollPosition();

    await tester.tap(finderTab2);
    await tester.pumpAndSettle();
    expect(getScrollPosition(), 416);

    await tester.ensureVisible(finderItem4);
    await tester.pumpAndSettle();
    expect(getScrollPosition(), 0); // passes

    // await tester.tap(finderTab1);
    // await tester.pumpAndSettle();

    // expect(getScrollPosition(), tab1Position);
  });

Overall I feel this PR is a worthwhile change, though whether this is the ideal behavior is probably subjective, and I am still puzzled by the GIF from #157756 (comment).

@bleroux bleroux self-requested a review November 2, 2024 19:23
@github-actions github-actions bot removed d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos labels Nov 4, 2024
@festelo
Copy link

festelo commented Nov 4, 2024

I am still puzzled by the GIF from #157756 (comment).

Quick update from my side:
I can no longer reproduce this bug using swipe gestures. I'm not sure why, but I'm inclined to think you were right and it was some local issue. I'm sorry for taking your time.
I'm still able to reproduce it when scrolling with the mouse wheel, although I think it might be a separate problem, I couldn't write any automatic tests that can catch it.

@nate-thegrate
Copy link
Contributor Author

No problem, thanks for letting me know!

Copy link
Contributor

@bleroux bleroux left a comment

Choose a reason for hiding this comment

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

LGTM! Great PR 👍

@nate-thegrate nate-thegrate added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 4, 2024
@auto-submit auto-submit bot merged commit 9ee00ae into flutter:master Nov 4, 2024
76 checks passed
@nate-thegrate nate-thegrate deleted the nested-scroll-view branch November 4, 2024 16:42
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 5, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 5, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 5, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 5, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 5, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Nov 5, 2024
flutter/flutter@8591d0c...29d40f7

2024-11-05 [email protected] increase subsharding for `Windows build_tests` from 8 to 9 (flutter/flutter#158146)
2024-11-05 [email protected] Reland2: Revert "Revert "Add a warning/additional handlers for parsing`synthetic-package`."" (flutter/flutter#158184)
2024-11-05 [email protected] Reland1:  "Revert "Add and plumb `useImplicitPubspecResolution` across `flutter_tools`."" (flutter/flutter#158126)
2024-11-05 [email protected] Roll Packages from 796afa3 to 7219431 (11 revisions) (flutter/flutter#158179)
2024-11-05 [email protected] Make native asset integration test more robust, thereby allowing smooth auto-update of packages via `flutter update-packages` (flutter/flutter#158170)
2024-11-05 [email protected] Readability change to `flutter.groovy`, align on null assignment, reduce unused scope for some methods, apply static where possible (flutter/flutter#157471)
2024-11-05 [email protected] Roll Flutter Engine from 7207a8fbec93 to f56401062e42 (1 revision) (flutter/flutter#158169)
2024-11-05 [email protected] Add test for `raw_scrollbar.shape.0.dart` (flutter/flutter#158094)
2024-11-05 [email protected] Roll Flutter Engine from 418609dd5b58 to 7207a8fbec93 (1 revision) (flutter/flutter#158156)
2024-11-05 [email protected] Refactor DropdownMenu tests (flutter/flutter#157913)
2024-11-05 [email protected] Roll Flutter Engine from 6271a92a376f to 418609dd5b58 (3 revisions) (flutter/flutter#158152)
2024-11-05 [email protected] Marks Linux_pixel_7pro flavors_test to be flaky (flutter/flutter#156956)
2024-11-05 [email protected] Further remove web-only considerations that are no longer necessary (flutter/flutter#158143)
2024-11-05 [email protected] Add optional parameter to FlutterTesterDevices. (flutter/flutter#158133)
2024-11-05 [email protected] Roll Flutter Engine from 75acceedca41 to 6271a92a376f (2 revisions) (flutter/flutter#158148)
2024-11-05 [email protected] Extract and restore a test that a blank native assets project still builds (flutter/flutter#158141)
2024-11-04 [email protected] Remove references to the HTML renderer in public docs. (flutter/flutter#158035)
2024-11-04 [email protected] Roll Flutter Engine from f880b56b6ede to 75acceedca41 (1 revision) (flutter/flutter#158137)
2024-11-04 [email protected] Fix `WidgetStateProperty` documentation (flutter/flutter#154298)
2024-11-04 [email protected] Roll Flutter Engine from 25c7e471e2ef to f880b56b6ede (5 revisions) (flutter/flutter#158132)
2024-11-04 [email protected] Roll Flutter Engine from 05cb5d7f7939 to 25c7e471e2ef (12 revisions) (flutter/flutter#158127)
2024-11-04 [email protected] Remove use_modular_headers! from Swift Podfiles (flutter/flutter#156257)
2024-11-04 [email protected] Disable failing native assets test (flutter/flutter#158119)
2024-11-04 [email protected] Fix `NestedScrollView` inner position logic (flutter/flutter#157756)
2024-11-04 [email protected] Add benchmarks for single-threaded Skwasm. (flutter/flutter#158027)

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] 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
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 12, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NestedScrollView's use of PrimaryScrollController breaks when there are multiple inner ScrollPositions

4 participants