Skip to content

Commit cd0082f

Browse files
authored
Fix: DraggableScrollableSheet may not close if snapping is enabled (flutter#165557)
<!-- Thanks for filing a pull request! Reviewers are typically assigned within a week of filing a request. To learn more about code review, see our documentation on Tree Hygiene: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md --> fixes flutter#140701 This PR fixes an issue where a DraggableScrollableSheet with `snap` set to true and `shouldCloseOnMinExtent` set to true may not close when dragged downward. The issue was caused by round-off errors accumulated by `addPixelDelta` method, which could lead to `extent.currentSize` not matching the `extent.minSize` exactly when the bottom is reached. I added logic to correct it when the snapping ballistic animation is complete. <details> <summary>Sample code</summary> ```Dart import 'package:flutter/material.dart'; void main() { runApp(MyApp()); } class MyApp extends StatelessWidget { const MyApp({super.key}); @OverRide Widget build(BuildContext context) { return MaterialApp( home: HomePage(), ); } } class HomePage extends StatelessWidget { const HomePage({super.key}); @OverRide Widget build(BuildContext context) { return Scaffold( appBar: AppBar( title: Text("DraggableScrollableSheet Test"), ), body: Center( child: ElevatedButton( child: Text("Open"), onPressed: () { showModalBottomSheet( context: context, showDragHandle: true, isScrollControlled: true, builder: (context) => _buildBottomSheet(), ); }, ), ), ); } Widget _buildBottomSheet() { return NotificationListener<DraggableScrollableNotification>( onNotification: (notification) { print(notification.extent); return false; }, child: DraggableScrollableSheet( expand: false, snap: true, shouldCloseOnMinExtent: true, maxChildSize: 0.9, minChildSize: 0.25, builder: (context, scrollController) { return ListView.builder( controller: scrollController, itemCount: 100, itemBuilder: (context, index) { return ListTile( title: Text("Item $index"), ); }, ); }, ), ); } } ``` </details> | Before applying fix | After | | --- | --- | | * Occurs with probability <video src="https://github.com/user-attachments/assets/ffd2d097-3ed5-4775-90d5-950092d49591"> | <video src="https://github.com/user-attachments/assets/0f20cb81-3444-40a3-a84d-ed4bff15887e"> | ## 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 cc556ae commit cd0082f

File tree

2 files changed

+42
-4
lines changed

2 files changed

+42
-4
lines changed

packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ library;
1515

1616
import 'dart:math' as math;
1717

18+
import 'package:collection/collection.dart';
1819
import 'package:flutter/foundation.dart';
1920
import 'package:flutter/gestures.dart';
2021

@@ -909,14 +910,18 @@ class _DraggableScrollableSheetScrollPosition extends ScrollPositionWithSingleCo
909910
}
910911
}
911912

912-
bool get _isAtSnapSize {
913-
return extent.snapSizes.any((double snapSize) {
913+
// Checks if the sheet's current size is close to a snap size, returning the
914+
// snap size if so; returns null otherwise.
915+
double? _getCurrentSnapSize() {
916+
return extent.snapSizes.firstWhereOrNull((double snapSize) {
914917
return (extent.currentSize - snapSize).abs() <=
915918
extent.pixelsToSize(physics.toleranceFor(this).distance);
916919
});
917920
}
918921

919-
bool get _shouldSnap => extent.snap && extent.hasDragged && !_isAtSnapSize;
922+
bool _isAtSnapSize() => _getCurrentSnapSize() != null;
923+
924+
bool _shouldSnap() => extent.snap && extent.hasDragged && !_isAtSnapSize();
920925

921926
@override
922927
void dispose() {
@@ -929,7 +934,7 @@ class _DraggableScrollableSheetScrollPosition extends ScrollPositionWithSingleCo
929934

930935
@override
931936
void goBallistic(double velocity) {
932-
if ((velocity == 0.0 && !_shouldSnap) ||
937+
if ((velocity == 0.0 && !_shouldSnap()) ||
933938
(velocity < 0.0 && listShouldScroll) ||
934939
(velocity > 0.0 && extent.isAtMax)) {
935940
super.goBallistic(velocity);
@@ -981,6 +986,13 @@ class _DraggableScrollableSheetScrollPosition extends ScrollPositionWithSingleCo
981986
super.goBallistic(velocity);
982987
ballisticController.stop();
983988
} else if (ballisticController.isCompleted) {
989+
// Update the extent value after the snap animation completes to
990+
// avoid rounding errors that could prevent the sheet from closing when
991+
// it reaches minSize.
992+
final double? snapSize = _getCurrentSnapSize();
993+
if (snapSize != null) {
994+
extent.updateSize(snapSize, context.notificationContext!);
995+
}
984996
super.goBallistic(0);
985997
}
986998
}

packages/flutter/test/widgets/draggable_scrollable_sheet_test.dart

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1892,4 +1892,30 @@ void main() {
18921892
expect(receivedNotification!.shouldCloseOnMinExtent, isFalse);
18931893
controller.dispose();
18941894
});
1895+
1896+
// Regression test for https://github.com/flutter/flutter/issues/140701
1897+
testWidgets('DraggableScrollableSheet snaps exactly to minChildSize', (
1898+
WidgetTester tester,
1899+
) async {
1900+
double? lastExtent;
1901+
1902+
await tester.pumpWidget(
1903+
boilerplateWidget(
1904+
null,
1905+
snap: true,
1906+
onDraggableScrollableNotification: (DraggableScrollableNotification notification) {
1907+
lastExtent = notification.extent;
1908+
return false;
1909+
},
1910+
),
1911+
);
1912+
1913+
// One of the conditions for reproducing the round-off error.
1914+
await tester.fling(find.text('Item 1'), const Offset(0, 100), 2000);
1915+
await tester.pumpFrames(
1916+
tester.widget(find.byType(Directionality)),
1917+
const Duration(milliseconds: 500),
1918+
);
1919+
expect(lastExtent, .25);
1920+
});
18951921
}

0 commit comments

Comments
 (0)