From 54850ddcee1853ee0e60fe8c66ea40bbda7ff7ee Mon Sep 17 00:00:00 2001 From: fujidaiti Date: Sat, 13 Jul 2024 12:00:25 +0900 Subject: [PATCH 1/3] Remove unnecessary Material widget --- package/test/scrollable/scrollable_sheet_test.dart | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/package/test/scrollable/scrollable_sheet_test.dart b/package/test/scrollable/scrollable_sheet_test.dart index d6084eec..b1db004f 100644 --- a/package/test/scrollable/scrollable_sheet_test.dart +++ b/package/test/scrollable/scrollable_sheet_test.dart @@ -101,11 +101,7 @@ void main() { controller: controller, minExtent: const Extent.pixels(200), initialExtent: const Extent.pixels(200), - child: const Material( - child: _TestSheetContent( - height: 500, - ), - ), + child: const _TestSheetContent(height: 500), ), ), ), From ab1894b1f784feb7e361ca16b1c59855479471e4 Mon Sep 17 00:00:00 2001 From: fujidaiti Date: Sat, 13 Jul 2024 15:31:14 +0900 Subject: [PATCH 2/3] Add regression test --- .../scrollable/scrollable_sheet_test.dart | 67 ++++++++++++++++++- 1 file changed, 66 insertions(+), 1 deletion(-) diff --git a/package/test/scrollable/scrollable_sheet_test.dart b/package/test/scrollable/scrollable_sheet_test.dart index b1db004f..3c47b625 100644 --- a/package/test/scrollable/scrollable_sheet_test.dart +++ b/package/test/scrollable/scrollable_sheet_test.dart @@ -38,12 +38,16 @@ class _TestSheetContent extends StatelessWidget { const _TestSheetContent({ super.key, this.height = 500, + this.itemCount = 30, // Disable the snapping effect by default in tests. this.scrollPhysics = const ClampingScrollPhysics(), + this.onTapItem, }); final double? height; + final int itemCount; final ScrollPhysics? scrollPhysics; + final void Function(int index)? onTapItem; @override Widget build(BuildContext context) { @@ -54,9 +58,10 @@ class _TestSheetContent extends StatelessWidget { child: ListView( physics: scrollPhysics, children: List.generate( - 30, + itemCount, (index) => ListTile( title: Text('Item $index'), + onTap: onTapItem != null ? () => onTapItem!(index) : null, ), ), ), @@ -135,6 +140,66 @@ void main() { ); }); + // Regression test for https://github.com/fujidaiti/smooth_sheets/issues/190 + testWidgets( + 'Press and hold scrollable view should stop momentum scrolling', + (tester) async { + const targetKey = Key('Target'); + final controller = SheetController(); + late ScrollController scrollController; + + await tester.pumpWidget( + _TestApp( + child: ScrollableSheet( + controller: controller, + child: Builder( + builder: (context) { + // TODO(fujita): Refactor this line after #116 is resolved. + scrollController = PrimaryScrollController.of(context); + return _TestSheetContent( + key: targetKey, + itemCount: 1000, + height: null, + // The items need to be clickable to cause the reported issue. + onTapItem: (index) {}, + ); + }, + ), + ), + ), + ); + + const dragDistance = 200.0; + const flingSpeed = 2000.0; + await tester.fling( + find.byKey(targetKey), + const Offset(0, -1 * dragDistance), // Fling up + flingSpeed, + ); + + final offsetAfterFling = scrollController.offset; + // Don't know why, but we need to call `pump` at least 2 times + // to forward the animation clock. + await tester.pump(); + await tester.pump(const Duration(milliseconds: 250)); + final offsetBeforePress = scrollController.offset; + expect(offsetBeforePress, greaterThan(offsetAfterFling), + reason: 'Momentum scrolling should be in progress.'); + + // Press and hold the finger on the target widget. + await tester.press(find.byKey(targetKey)); + // Wait for the momentum scrolling to stop. + await tester.pumpAndSettle(); + final offsetAfterPress = scrollController.offset; + expect( + offsetAfterPress, + equals(offsetBeforePress), + reason: 'Momentum scrolling should be stopped immediately' + 'by pressing and holding.', + ); + }, + ); + group('SheetKeyboardDismissible', () { late FocusNode focusNode; late Widget testWidget; From 00539affb4c5fec37c4c0055363eb5c30c72f255 Mon Sep 17 00:00:00 2001 From: fujidaiti Date: Sat, 13 Jul 2024 18:36:10 +0900 Subject: [PATCH 3/3] Use shouldIgnorePointer of scroll position --- .../scrollable/scrollable_sheet_activity.dart | 18 ++++++++++++------ .../scrollable/scrollable_sheet_extent.dart | 4 +--- .../sheet_content_scroll_position.dart | 6 ++++-- 3 files changed, 17 insertions(+), 11 deletions(-) diff --git a/package/lib/src/scrollable/scrollable_sheet_activity.dart b/package/lib/src/scrollable/scrollable_sheet_activity.dart index 17bc0e66..ab4c7a7e 100644 --- a/package/lib/src/scrollable/scrollable_sheet_activity.dart +++ b/package/lib/src/scrollable/scrollable_sheet_activity.dart @@ -12,6 +12,18 @@ import 'scrollable_sheet_extent.dart'; import 'sheet_content_scroll_activity.dart'; import 'sheet_content_scroll_position.dart'; +/// A [SheetActivity] that is associated with a [SheetContentScrollPosition]. +/// +/// This activity is responsible for both scrolling a scrollable content +/// in the sheet and dragging the sheet itself. +/// +/// [shouldIgnorePointer] and [SheetContentScrollPosition.shouldIgnorePointer] +/// of the associated scroll position may be synchronized, but not always. +/// For example, [BallisticScrollDrivenSheetActivity]'s [shouldIgnorePointer] +/// is always `false` while the associated scroll position sets it to `true` +/// in most cases to ensure that the pointer events, which potentially +/// interrupt the ballistic scroll animation, are not stolen by clickable +/// items in the scroll view. @internal abstract class ScrollableSheetActivity extends SheetActivity { @@ -196,7 +208,6 @@ class DragScrollDrivenSheetActivity extends ScrollableSheetActivity ..didDragEnd(details) ..goBallisticWithScrollPosition( velocity: -1 * details.velocityY, - shouldIgnorePointer: false, scrollPosition: scrollPosition, ); } @@ -215,14 +226,10 @@ class BallisticScrollDrivenSheetActivity extends ScrollableSheetActivity super.scrollPosition, { required this.simulation, required double initialPixels, - required this.shouldIgnorePointer, }) : _oldPixels = initialPixels; final Simulation simulation; - @override - final bool shouldIgnorePointer; - double _oldPixels; @override @@ -271,7 +278,6 @@ class BallisticScrollDrivenSheetActivity extends ScrollableSheetActivity void _end() { owner.goBallisticWithScrollPosition( velocity: 0, - shouldIgnorePointer: shouldIgnorePointer, scrollPosition: scrollPosition, ); } diff --git a/package/lib/src/scrollable/scrollable_sheet_extent.dart b/package/lib/src/scrollable/scrollable_sheet_extent.dart index 966e6907..88e5f78b 100644 --- a/package/lib/src/scrollable/scrollable_sheet_extent.dart +++ b/package/lib/src/scrollable/scrollable_sheet_extent.dart @@ -161,7 +161,6 @@ class ScrollableSheetExtent extends SheetExtent @override void goBallisticWithScrollPosition({ required double velocity, - required bool shouldIgnorePointer, required SheetContentScrollPosition scrollPosition, }) { assert(metrics.hasDimensions); @@ -196,13 +195,12 @@ class ScrollableSheetExtent extends SheetExtent scrollPosition, simulation: scrollSimulation, initialPixels: scrollPixelsForScrollPhysics, - shouldIgnorePointer: shouldIgnorePointer, ), ); scrollPosition.beginActivity( SheetContentBallisticScrollActivity( delegate: scrollPosition, - shouldIgnorePointer: shouldIgnorePointer, + shouldIgnorePointer: scrollPosition.shouldIgnorePointer, getVelocity: () => activity.velocity, ), ); diff --git a/package/lib/src/scrollable/sheet_content_scroll_position.dart b/package/lib/src/scrollable/sheet_content_scroll_position.dart index 0bd440e9..6671d681 100644 --- a/package/lib/src/scrollable/sheet_content_scroll_position.dart +++ b/package/lib/src/scrollable/sheet_content_scroll_position.dart @@ -32,7 +32,6 @@ abstract class SheetContentScrollPositionOwner { void goBallisticWithScrollPosition({ required double velocity, - required bool shouldIgnorePointer, required SheetContentScrollPosition scrollPosition, }); } @@ -69,6 +68,10 @@ class SheetContentScrollPosition extends ScrollPositionWithSingleContext { double _heldPreviousVelocity = 0.0; double get heldPreviousVelocity => _heldPreviousVelocity; + /// Whether the scroll view should prevent its contents from receiving + /// pointer events. + bool get shouldIgnorePointer => activity!.shouldIgnorePointer; + /// Sets the user scroll direction. /// /// This exists only to expose `updateUserScrollDirection` @@ -123,7 +126,6 @@ class SheetContentScrollPosition extends ScrollPositionWithSingleContext { if (owner != null && owner.hasPrimaryScrollPosition && !calledByOwner) { owner.goBallisticWithScrollPosition( velocity: velocity, - shouldIgnorePointer: activity?.shouldIgnorePointer ?? true, scrollPosition: this, ); return;