-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Fix NestedScrollView inner position logic
#157756
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.
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
Conversation
bleroux
left a comment
There was a problem hiding this 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.
examples/api/test/widgets/nested_scroll_view/nested_scroll_view.0_test.dart
Outdated
Show resolved
Hide resolved
|
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? Text version of the gif:
Codeimport '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),
),
);
}
} |
|
oh no |
|
Well, I have good news: I copy-pasted your code sample and ran it against my branch, and it's working! 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';
|
Yep, already changed it. But TY anyway :)
This particular issue may be not related directly to the PR, I'm not fully familiar with the context, sorry for this. 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. |
|
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 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). |
Co-authored-by: Bruno Leroux <[email protected]>
ecbe23e to
2732799
Compare
Quick update from my side: |
|
No problem, thanks for letting me know! |
bleroux
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Great PR 👍
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


closes #40740
Demo source code
(click to expand)(this is a slightly more concise version of the code sample from #40740)
This bug can be traced to a return statement inside
_NestedScrollPosition:Thanks to some quirks of floating-point arithmetic,
applyClampedDragUpdatewould sometimes return a tiny non-zero value, which ends up ruining one of the scroll coordinator's equality checks.flutter/packages/flutter/lib/src/widgets/nested_scroll_view.dart
Lines 658 to 664 in 8990ed6