From d620ec9274edf794c57ea6a15d41b0968740a99d Mon Sep 17 00:00:00 2001 From: Renzo Olivares Date: Mon, 9 Dec 2024 17:25:58 -0800 Subject: [PATCH] fix: SelectableRegion should only finalize selection after changing (#159698) There are some cases where selection behavior varies on a given platform by the pointer device, for example dragging to select changes on mobile platforms depending on whether a mouse or a touch is used. When a mouse is used a user can drag to select normally, when a touch is used the selection does not change on mobile platforms when dragging. Before this change at the end of a touch drag users would still be notified the selection was finalized even though nothing changed and they had not previously received a `changing` notification. After this change the selection is no longer finalized unless the `SelectableRegionSelectionStatus` was previously in a `changing` state. ## 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. --------- Co-authored-by: Renzo Olivares --- .../lib/src/widgets/selectable_region.dart | 73 ++++++++--------- .../test/widgets/selectable_region_test.dart | 81 +++++++++++++++++++ 2 files changed, 115 insertions(+), 39 deletions(-) diff --git a/packages/flutter/lib/src/widgets/selectable_region.dart b/packages/flutter/lib/src/widgets/selectable_region.dart index ee31e303cb..a6bac976d5 100644 --- a/packages/flutter/lib/src/widgets/selectable_region.dart +++ b/packages/flutter/lib/src/widgets/selectable_region.dart @@ -491,7 +491,7 @@ class SelectableRegionState extends State with TextSelectionDe // the Flutter application. clearSelection(); _selectionStatusNotifier.value = SelectableRegionSelectionStatus.changing; - _selectionStatusNotifier.value = SelectableRegionSelectionStatus.finalized; + _finalizeSelectableRegionStatus(); } } if (kIsWeb) { @@ -541,6 +541,14 @@ class SelectableRegionState extends State with TextSelectionDe } } + void _finalizeSelectableRegionStatus() { + if (_selectionStatusNotifier.value != SelectableRegionSelectionStatus.changing) { + // Don't finalize the selection again if it is not currently changing. + return; + } + _selectionStatusNotifier.value = SelectableRegionSelectionStatus.finalized; + } + // Converts the details.consecutiveTapCount from a TapAndDrag*Details object, // which can grow to be infinitely large, to a value between 1 and the supported // max consecutive tap count. The value that the raw count is converted to is @@ -808,20 +816,22 @@ class SelectableRegionState extends State with TextSelectionDe } void _handleMouseDragEnd(TapDragEndDetails details) { - final bool isPointerPrecise = _lastPointerDeviceKind != null && _lastPointerDeviceKind == PointerDeviceKind.mouse; + assert(_lastPointerDeviceKind != null); + final bool isPointerPrecise = _isPrecisePointerDevice(_lastPointerDeviceKind!); + // On mobile platforms like android, fuchsia, and iOS, a drag gesture will + // only show the selection overlay when the drag has finished and the pointer + // device kind is not precise, for example at the end of a double tap + drag + // to select on native iOS. + final bool shouldShowSelectionOverlayOnMobile = !isPointerPrecise; switch (defaultTargetPlatform) { case TargetPlatform.android: case TargetPlatform.fuchsia: - if (!isPointerPrecise) { - // On Android, a drag gesture will only show the selection overlay when - // the drag has finished and the pointer device kind is not precise. + if (shouldShowSelectionOverlayOnMobile) { _showHandles(); _showToolbar(); } case TargetPlatform.iOS: - if (!isPointerPrecise) { - // On iOS, a drag gesture will only show the selection toolbar when - // the drag has finished and the pointer device kind is not precise. + if (shouldShowSelectionOverlayOnMobile) { _showToolbar(); } case TargetPlatform.macOS: @@ -832,7 +842,7 @@ class SelectableRegionState extends State with TextSelectionDe } _finalizeSelection(); _updateSelectedContentIfNeeded(); - _selectionStatusNotifier.value = SelectableRegionSelectionStatus.finalized; + _finalizeSelectableRegionStatus(); } void _handleMouseTapUp(TapDragUpDetails details) { @@ -856,12 +866,10 @@ class SelectableRegionState extends State with TextSelectionDe hideToolbar(); _collapseSelectionAt(offset: details.globalPosition); _selectionStatusNotifier.value = SelectableRegionSelectionStatus.changing; - _selectionStatusNotifier.value = SelectableRegionSelectionStatus.finalized; case TargetPlatform.macOS: case TargetPlatform.linux: case TargetPlatform.windows: // On desktop platforms the selection is set on tap down. - _selectionStatusNotifier.value = SelectableRegionSelectionStatus.finalized; } case 2: final bool isPointerPrecise = _isPrecisePointerDevice(details.kind); @@ -878,7 +886,7 @@ class SelectableRegionState extends State with TextSelectionDe if (!isPointerPrecise) { if (kIsWeb) { // Double tap on iOS web only triggers when a drag begins after the double tap. - return; + break; } // On iOS, a double tap will only show the selection toolbar after // the following tap up when the pointer device kind is not precise. @@ -891,24 +899,8 @@ class SelectableRegionState extends State with TextSelectionDe // on a double click. break; } - _selectionStatusNotifier.value = SelectableRegionSelectionStatus.finalized; - case 3: - switch (defaultTargetPlatform) { - case TargetPlatform.android: - case TargetPlatform.fuchsia: - case TargetPlatform.iOS: - if (_isPrecisePointerDevice(details.kind)) { - // Triple tap on static text is only supported on mobile - // platforms using a precise pointer device, so we should - // only update the SelectableRegionSelectionStatus in that case. - _selectionStatusNotifier.value = SelectableRegionSelectionStatus.finalized; - } - case TargetPlatform.macOS: - case TargetPlatform.linux: - case TargetPlatform.windows: - _selectionStatusNotifier.value = SelectableRegionSelectionStatus.finalized; - } } + _finalizeSelectableRegionStatus(); _updateSelectedContentIfNeeded(); } @@ -946,7 +938,7 @@ class SelectableRegionState extends State with TextSelectionDe void _handleTouchLongPressEnd(LongPressEndDetails details) { _finalizeSelection(); _updateSelectedContentIfNeeded(); - _selectionStatusNotifier.value = SelectableRegionSelectionStatus.finalized; + _finalizeSelectableRegionStatus(); _showToolbar(); if (defaultTargetPlatform == TargetPlatform.android) { _showHandles(); @@ -1007,7 +999,7 @@ class SelectableRegionState extends State with TextSelectionDe } } _selectionStatusNotifier.value = SelectableRegionSelectionStatus.changing; - _selectionStatusNotifier.value = SelectableRegionSelectionStatus.finalized; + _finalizeSelectableRegionStatus(); // Restore _lastSecondaryTapDownPosition since it may be cleared if a user // accesses contextMenuAnchors. _lastSecondaryTapDownPosition = details.globalPosition; @@ -1064,7 +1056,7 @@ class SelectableRegionState extends State with TextSelectionDe } _finalizeSelection(); _updateSelectedContentIfNeeded(); - _selectionStatusNotifier.value = SelectableRegionSelectionStatus.finalized; + _finalizeSelectableRegionStatus(); } void _stopSelectionEndEdgeUpdate() { @@ -1545,7 +1537,7 @@ class SelectableRegionState extends State with TextSelectionDe ); _updateSelectedContentIfNeeded(); _selectionStatusNotifier.value = SelectableRegionSelectionStatus.changing; - _selectionStatusNotifier.value = SelectableRegionSelectionStatus.finalized; + _finalizeSelectableRegionStatus(); } double? _directionalHorizontalBaseline; @@ -1569,7 +1561,7 @@ class SelectableRegionState extends State with TextSelectionDe ); _updateSelectedContentIfNeeded(); _selectionStatusNotifier.value = SelectableRegionSelectionStatus.changing; - _selectionStatusNotifier.value = SelectableRegionSelectionStatus.finalized; + _finalizeSelectableRegionStatus(); } // [TextSelectionDelegate] overrides. @@ -1602,7 +1594,7 @@ class SelectableRegionState extends State with TextSelectionDe case TargetPlatform.fuchsia: clearSelection(); _selectionStatusNotifier.value = SelectableRegionSelectionStatus.changing; - _selectionStatusNotifier.value = SelectableRegionSelectionStatus.finalized; + _finalizeSelectableRegionStatus(); case TargetPlatform.iOS: hideToolbar(false); case TargetPlatform.linux: @@ -1633,7 +1625,7 @@ class SelectableRegionState extends State with TextSelectionDe case TargetPlatform.fuchsia: clearSelection(); _selectionStatusNotifier.value = SelectableRegionSelectionStatus.changing; - _selectionStatusNotifier.value = SelectableRegionSelectionStatus.finalized; + _finalizeSelectableRegionStatus(); case TargetPlatform.iOS: hideToolbar(false); case TargetPlatform.linux: @@ -1735,7 +1727,7 @@ class SelectableRegionState extends State with TextSelectionDe } _updateSelectedContentIfNeeded(); _selectionStatusNotifier.value = SelectableRegionSelectionStatus.changing; - _selectionStatusNotifier.value = SelectableRegionSelectionStatus.finalized; + _finalizeSelectableRegionStatus(); } @Deprecated( @@ -1747,7 +1739,7 @@ class SelectableRegionState extends State with TextSelectionDe _copy(); clearSelection(); _selectionStatusNotifier.value = SelectableRegionSelectionStatus.changing; - _selectionStatusNotifier.value = SelectableRegionSelectionStatus.finalized; + _finalizeSelectableRegionStatus(); } @Deprecated( @@ -3246,7 +3238,10 @@ final class _SelectableRegionSelectionStatusNotifier extends ChangeNotifier impl /// Listeners are notified even if the value did not change. @protected set value(SelectableRegionSelectionStatus newStatus) { - _selectableRegionSelectionStatus = newStatus; + assert(newStatus == SelectableRegionSelectionStatus.finalized && value == SelectableRegionSelectionStatus.changing + || newStatus == SelectableRegionSelectionStatus.changing, + 'Attempting to finalize the selection when it is already finalized.'); + _selectableRegionSelectionStatus = newStatus; notifyListeners(); } } diff --git a/packages/flutter/test/widgets/selectable_region_test.dart b/packages/flutter/test/widgets/selectable_region_test.dart index 130528e1f6..4028215c6e 100644 --- a/packages/flutter/test/widgets/selectable_region_test.dart +++ b/packages/flutter/test/widgets/selectable_region_test.dart @@ -1431,6 +1431,87 @@ void main() { await gesture.up(); }, variant: TargetPlatformVariant.mobile()); + testWidgets('mouse drag finalizes the selection', (WidgetTester tester) async { + SelectableRegionSelectionStatus? selectionStatus; + final GlobalKey textKey = GlobalKey(); + await tester.pumpWidget( + MaterialApp( + home: SelectableRegion( + selectionControls: materialTextSelectionControls, + child: Center( + child: Text( + key: textKey, + 'How are you', + ), + ), + ), + ), + ); + await tester.pumpAndSettle(); + expect(textKey.currentContext, isNotNull); + final ValueListenable? selectionStatusNotifier = SelectableRegionSelectionStatusScope.maybeOf(textKey.currentContext!); + void onSelectionStatusChange() { + selectionStatus = selectionStatusNotifier?.value; + } + selectionStatusNotifier?.addListener(onSelectionStatusChange); + addTearDown(() { + selectionStatusNotifier?.removeListener(onSelectionStatusChange); + }); + final RenderParagraph paragraph = tester.renderObject(find.descendant(of: find.text('How are you'), matching: find.byType(RichText))); + final TestGesture gesture = await tester.startGesture(textOffsetToPosition(paragraph, 2), kind: PointerDeviceKind.mouse); + addTearDown(gesture.removePointer); + await tester.pump(); + + await gesture.moveTo(textOffsetToPosition(paragraph, 4)); + await tester.pump(); + expect(selectionStatus, SelectableRegionSelectionStatus.changing); + await gesture.up(); + await tester.pump(); + + expect(paragraph.selections.length, 1); + expect(selectionStatus, SelectableRegionSelectionStatus.finalized); + }, variant: TargetPlatformVariant.all()); + + testWidgets('touch drag does not finalize selection on mobile platforms', (WidgetTester tester) async { + SelectableRegionSelectionStatus? selectionStatus; + final GlobalKey textKey = GlobalKey(); + await tester.pumpWidget( + MaterialApp( + home: SelectableRegion( + selectionControls: materialTextSelectionControls, + child: Center( + child: Text( + key: textKey, + 'How are you', + ), + ), + ), + ), + ); + await tester.pumpAndSettle(); + expect(textKey.currentContext, isNotNull); + final ValueListenable? selectionStatusNotifier = SelectableRegionSelectionStatusScope.maybeOf(textKey.currentContext!); + void onSelectionStatusChange() { + selectionStatus = selectionStatusNotifier?.value; + } + selectionStatusNotifier?.addListener(onSelectionStatusChange); + addTearDown(() { + selectionStatusNotifier?.removeListener(onSelectionStatusChange); + }); + final RenderParagraph paragraph = tester.renderObject(find.descendant(of: find.text('How are you'), matching: find.byType(RichText))); + final TestGesture gesture = await tester.startGesture(textOffsetToPosition(paragraph, 2)); + addTearDown(gesture.removePointer); + await tester.pump(); + + await gesture.moveTo(textOffsetToPosition(paragraph, 4)); + await tester.pump(); + await gesture.up(); + await tester.pump(); + + expect(paragraph.selections.length, 0); + expect(selectionStatus, isNull); + }, variant: TargetPlatformVariant.mobile()); + testWidgets('mouse can select word-by-word on double click drag', (WidgetTester tester) async { await tester.pumpWidget( MaterialApp(