-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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 <bruno.leroux@gmail.com>
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 andrewrkolos@gmail.com increase subsharding for `Windows build_tests` from 8 to 9 (flutter/flutter#158146) 2024-11-05 polinach@google.com Reland2: Revert "Revert "Add a warning/additional handlers for parsing`synthetic-package`."" (flutter/flutter#158184) 2024-11-05 polinach@google.com Reland1: "Revert "Add and plumb `useImplicitPubspecResolution` across `flutter_tools`."" (flutter/flutter#158126) 2024-11-05 engine-flutter-autoroll@skia.org Roll Packages from 796afa3 to 7219431 (11 revisions) (flutter/flutter#158179) 2024-11-05 kustermann@google.com Make native asset integration test more robust, thereby allowing smooth auto-update of packages via `flutter update-packages` (flutter/flutter#158170) 2024-11-05 mohellebiabdessalem@gmail.com 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 engine-flutter-autoroll@skia.org Roll Flutter Engine from 7207a8fbec93 to f56401062e42 (1 revision) (flutter/flutter#158169) 2024-11-05 32538273+ValentinVignal@users.noreply.github.com Add test for `raw_scrollbar.shape.0.dart` (flutter/flutter#158094) 2024-11-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 418609dd5b58 to 7207a8fbec93 (1 revision) (flutter/flutter#158156) 2024-11-05 bruno.leroux@gmail.com Refactor DropdownMenu tests (flutter/flutter#157913) 2024-11-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 6271a92a376f to 418609dd5b58 (3 revisions) (flutter/flutter#158152) 2024-11-05 fluttergithubbot@gmail.com Marks Linux_pixel_7pro flavors_test to be flaky (flutter/flutter#156956) 2024-11-05 matanlurey@users.noreply.github.com Further remove web-only considerations that are no longer necessary (flutter/flutter#158143) 2024-11-05 polinach@google.com Add optional parameter to FlutterTesterDevices. (flutter/flutter#158133) 2024-11-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 75acceedca41 to 6271a92a376f (2 revisions) (flutter/flutter#158148) 2024-11-05 matanlurey@users.noreply.github.com Extract and restore a test that a blank native assets project still builds (flutter/flutter#158141) 2024-11-04 matanlurey@users.noreply.github.com Remove references to the HTML renderer in public docs. (flutter/flutter#158035) 2024-11-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from f880b56b6ede to 75acceedca41 (1 revision) (flutter/flutter#158137) 2024-11-04 nate.w5687@gmail.com Fix `WidgetStateProperty` documentation (flutter/flutter#154298) 2024-11-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from 25c7e471e2ef to f880b56b6ede (5 revisions) (flutter/flutter#158132) 2024-11-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from 05cb5d7f7939 to 25c7e471e2ef (12 revisions) (flutter/flutter#158127) 2024-11-04 737941+loic-sharma@users.noreply.github.com Remove use_modular_headers! from Swift Podfiles (flutter/flutter#156257) 2024-11-04 victorsanniay@gmail.com Disable failing native assets test (flutter/flutter#158119) 2024-11-04 nate.w5687@gmail.com Fix `NestedScrollView` inner position logic (flutter/flutter#157756) 2024-11-04 jacksongardner@google.com 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 camillesimon@google.com,stuartmorgan@google.com 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