From fe2fc8daae6c355f18f7b6ac53dfac2d79e6efce Mon Sep 17 00:00:00 2001 From: Renzo Olivares Date: Fri, 12 Aug 2022 19:06:07 -0700 Subject: [PATCH] =?UTF-8?q?Single=20tap=20on=20the=20previous=20selection?= =?UTF-8?q?=20should=20toggle=20the=20toolbar=20on=20iOS=E2=80=A6=20(#1089?= =?UTF-8?q?13)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../flutter/lib/src/cupertino/text_field.dart | 1 - .../flutter/lib/src/material/text_field.dart | 1 - .../lib/src/widgets/editable_text.dart | 4 +- .../lib/src/widgets/text_selection.dart | 26 +++++- .../test/cupertino/text_field_test.dart | 89 +++++++++++++++++- .../test/material/text_field_test.dart | 91 ++++++++++++++++++- .../test/widgets/text_selection_test.dart | 36 +++++++- 7 files changed, 232 insertions(+), 16 deletions(-) diff --git a/packages/flutter/lib/src/cupertino/text_field.dart b/packages/flutter/lib/src/cupertino/text_field.dart index 4070b5394ec..c155223edc3 100644 --- a/packages/flutter/lib/src/cupertino/text_field.dart +++ b/packages/flutter/lib/src/cupertino/text_field.dart @@ -102,7 +102,6 @@ class _CupertinoTextFieldSelectionGestureDetectorBuilder extends TextSelectionGe @override void onSingleTapUp(TapUpDetails details) { - editableText.hideToolbar(); // Because TextSelectionGestureDetector listens to taps that happen on // widgets in front of it, tapping the clear button will also trigger // this handler. If the clear button widget recognizes the up event, diff --git a/packages/flutter/lib/src/material/text_field.dart b/packages/flutter/lib/src/material/text_field.dart index 5f21bba04dd..1a3630864c7 100644 --- a/packages/flutter/lib/src/material/text_field.dart +++ b/packages/flutter/lib/src/material/text_field.dart @@ -88,7 +88,6 @@ class _TextFieldSelectionGestureDetectorBuilder extends TextSelectionGestureDete @override void onSingleTapUp(TapUpDetails details) { - editableText.hideToolbar(); super.onSingleTapUp(details); _state._requestKeyboard(); _state.widget.onTap?.call(); diff --git a/packages/flutter/lib/src/widgets/editable_text.dart b/packages/flutter/lib/src/widgets/editable_text.dart index 067f76320b8..0d9241b2599 100644 --- a/packages/flutter/lib/src/widgets/editable_text.dart +++ b/packages/flutter/lib/src/widgets/editable_text.dart @@ -3192,10 +3192,10 @@ class EditableTextState extends State with AutomaticKeepAliveClien } /// Toggles the visibility of the toolbar. - void toggleToolbar() { + void toggleToolbar([bool hideHandles = true]) { assert(_selectionOverlay != null); if (_selectionOverlay!.toolbarIsVisible) { - hideToolbar(); + hideToolbar(hideHandles); } else { showToolbar(); } diff --git a/packages/flutter/lib/src/widgets/text_selection.dart b/packages/flutter/lib/src/widgets/text_selection.dart index 5319625900a..96076ae4570 100644 --- a/packages/flutter/lib/src/widgets/text_selection.dart +++ b/packages/flutter/lib/src/widgets/text_selection.dart @@ -1591,6 +1591,19 @@ class TextSelectionGestureDetectorBuilder { && renderEditable.selection!.end >= textPosition.offset; } + bool _tapWasOnSelection(Offset position) { + if (renderEditable.selection == null) { + return false; + } + + final TextPosition textPosition = renderEditable.getPositionForPoint( + position, + ); + + return renderEditable.selection!.start < textPosition.offset + && renderEditable.selection!.end > textPosition.offset; + } + // Expand the selection to the given global position. // // Either base or extent will be moved to the last tapped position, whichever @@ -1821,6 +1834,7 @@ class TextSelectionGestureDetectorBuilder { case TargetPlatform.linux: case TargetPlatform.macOS: case TargetPlatform.windows: + editableText.hideToolbar(); // On desktop platforms the selection is set on tap down. if (_isShiftTapping) { _isShiftTapping = false; @@ -1828,6 +1842,7 @@ class TextSelectionGestureDetectorBuilder { break; case TargetPlatform.android: case TargetPlatform.fuchsia: + editableText.hideToolbar(); if (isShiftPressedValid) { _isShiftTapping = true; _extendSelection(details.globalPosition, SelectionChangedCause.tap); @@ -1861,7 +1876,16 @@ class TextSelectionGestureDetectorBuilder { case PointerDeviceKind.touch: case PointerDeviceKind.unknown: // On iOS/iPadOS a touch tap places the cursor at the edge of the word. - renderEditable.selectWordEdge(cause: SelectionChangedCause.tap); + final TextSelection previousSelection = editableText.textEditingValue.selection; + // If the tap was within the previous selection, then the selection should stay the same. + if (!_tapWasOnSelection(details.globalPosition)) { + renderEditable.selectWordEdge(cause: SelectionChangedCause.tap); + } + if (previousSelection == editableText.textEditingValue.selection && renderEditable.hasFocus) { + editableText.toggleToolbar(false); + } else { + editableText.hideToolbar(false); + } break; } break; diff --git a/packages/flutter/test/cupertino/text_field_test.dart b/packages/flutter/test/cupertino/text_field_test.dart index 51a9ccef4ae..0c5f5749a5c 100644 --- a/packages/flutter/test/cupertino/text_field_test.dart +++ b/packages/flutter/test/cupertino/text_field_test.dart @@ -1752,10 +1752,86 @@ void main() { expect(controller.selection.isCollapsed, isTrue); expect(controller.selection.baseOffset, isTargetPlatformMobile ? 7 : 6); - // No toolbar. - expect(find.byType(CupertinoButton), findsNothing); + // Toolbar shows on mobile. + expect(find.byType(CupertinoButton), isContextMenuProvidedByPlatform ? findsNothing : isTargetPlatformMobile ? findsNWidgets(2) : findsNothing); }, variant: const TargetPlatformVariant({ TargetPlatform.iOS, TargetPlatform.macOS })); + testWidgets( + 'Tapping on a non-collapsed selection toggles the toolbar and retains the selection', + (WidgetTester tester) async { + final TextEditingController controller = TextEditingController( + text: 'Atwater Peel Sherbrooke Bonaventure', + ); + // On iOS/iPadOS, during a tap we select the edge of the word closest to the tap. + await tester.pumpWidget( + CupertinoApp( + home: Center( + child: CupertinoTextField( + controller: controller, + ), + ), + ), + ); + + final Offset vPos = textOffsetToPosition(tester, 29); // Index of 'Bonav|enture'. + final Offset ePos = textOffsetToPosition(tester, 35) + const Offset(7.0, 0.0); // Index of 'Bonaventure|' + Offset(7.0,0), which taps slightly to the right of the end of the text. + final Offset wPos = textOffsetToPosition(tester, 3); // Index of 'Atw|ater'. + + // This tap just puts the cursor somewhere different than where the double + // tap will occur to test that the double tap moves the existing cursor first. + await tester.tapAt(wPos); + await tester.pump(const Duration(milliseconds: 500)); + + await tester.tapAt(vPos); + await tester.pump(const Duration(milliseconds: 50)); + // First tap moved the cursor. + expect(controller.selection.isCollapsed, true); + expect( + controller.selection.baseOffset, + 35, + ); + await tester.tapAt(vPos); + await tester.pumpAndSettle(const Duration(milliseconds: 500)); + + // Second tap selects the word around the cursor. + expect( + controller.selection, + const TextSelection(baseOffset: 24, extentOffset: 35), + ); + + // Selected text shows 3 toolbar buttons. + expect(find.byType(CupertinoButton), isContextMenuProvidedByPlatform ? findsNothing : findsNWidgets(3)); + + // Tap the selected word to hide the toolbar and retain the selection. + await tester.tapAt(vPos); + await tester.pumpAndSettle(); + expect( + controller.selection, + const TextSelection(baseOffset: 24, extentOffset: 35), + ); + expect(find.byType(CupertinoButton), findsNothing); + + // Tap the selected word to show the toolbar and retain the selection. + await tester.tapAt(vPos); + await tester.pumpAndSettle(); + expect( + controller.selection, + const TextSelection(baseOffset: 24, extentOffset: 35), + ); + expect(find.byType(CupertinoButton), isContextMenuProvidedByPlatform ? findsNothing : findsNWidgets(3)); + + // Tap past the selected word to move the cursor and hide the toolbar. + await tester.tapAt(ePos); + await tester.pumpAndSettle(); + expect( + controller.selection, + const TextSelection.collapsed(offset: 35), + ); + expect(find.byType(CupertinoButton), findsNothing); + }, + variant: const TargetPlatformVariant({ TargetPlatform.iOS }), + ); + testWidgets( 'double tap selects word and first tap of double tap moves cursor', (WidgetTester tester) async { @@ -2701,11 +2777,13 @@ void main() { // Double tap selecting the same word somewhere else is fine. await tester.tapAt(textFieldStart + const Offset(100.0, 5.0)); await tester.pump(const Duration(milliseconds: 50)); - // First tap moved the cursor. + // First tap hides the toolbar, and retains the selection. expect( controller.selection, - const TextSelection.collapsed(offset: 7, affinity: TextAffinity.upstream), + const TextSelection(baseOffset: 0, extentOffset: 7), ); + expect(find.byType(CupertinoButton), findsNothing); + // Second tap shows the toolbar, and retains the selection. await tester.tapAt(textFieldStart + const Offset(100.0, 5.0)); await tester.pumpAndSettle(); expect( @@ -2716,11 +2794,12 @@ void main() { await tester.tapAt(textFieldStart + const Offset(150.0, 5.0)); await tester.pump(const Duration(milliseconds: 50)); - // First tap moved the cursor. + // First tap moved the cursor and hides the toolbar. expect( controller.selection, const TextSelection.collapsed(offset: 8), ); + expect(find.byType(CupertinoButton), findsNothing); await tester.tapAt(textFieldStart + const Offset(150.0, 5.0)); await tester.pumpAndSettle(); expect( diff --git a/packages/flutter/test/material/text_field_test.dart b/packages/flutter/test/material/text_field_test.dart index f5c570b1bf2..e778c3daedf 100644 --- a/packages/flutter/test/material/text_field_test.dart +++ b/packages/flutter/test/material/text_field_test.dart @@ -7410,12 +7410,90 @@ void main() { expect(controller.selection.isCollapsed, isTrue); expect(controller.selection.baseOffset, isTargetPlatformMobile ? 7 : 6); - // No toolbar. - expect(find.byType(CupertinoButton), findsNothing); + // Toolbar shows on iOS. + expect(find.byType(CupertinoButton), isContextMenuProvidedByPlatform ? findsNothing : isTargetPlatformMobile ? findsNWidgets(2) : findsNothing); }, variant: const TargetPlatformVariant({ TargetPlatform.iOS, TargetPlatform.macOS }), ); + testWidgets( + 'Tapping on a non-collapsed selection toggles the toolbar and retains the selection', + (WidgetTester tester) async { + final TextEditingController controller = TextEditingController( + text: 'Atwater Peel Sherbrooke Bonaventure', + ); + // On iOS/iPadOS, during a tap we select the edge of the word closest to the tap. + await tester.pumpWidget( + MaterialApp( + home: Material( + child: Center( + child: TextField( + controller: controller, + ), + ), + ), + ), + ); + + final Offset vPos = textOffsetToPosition(tester, 29); // Index of 'Bonav|enture'. + final Offset ePos = textOffsetToPosition(tester, 35) + const Offset(7.0, 0.0); // Index of 'Bonaventure|' + Offset(7.0,0), which taps slightly to the right of the end of the text. + final Offset wPos = textOffsetToPosition(tester, 3); // Index of 'Atw|ater'. + + // This tap just puts the cursor somewhere different than where the double + // tap will occur to test that the double tap moves the existing cursor first. + await tester.tapAt(wPos); + await tester.pump(const Duration(milliseconds: 500)); + + await tester.tapAt(vPos); + await tester.pump(const Duration(milliseconds: 50)); + // First tap moved the cursor. + expect(controller.selection.isCollapsed, true); + expect( + controller.selection.baseOffset, + 35, + ); + await tester.tapAt(vPos); + await tester.pumpAndSettle(const Duration(milliseconds: 500)); + + // Second tap selects the word around the cursor. + expect( + controller.selection, + const TextSelection(baseOffset: 24, extentOffset: 35), + ); + + // Selected text shows 3 toolbar buttons. + expect(find.byType(CupertinoButton), isContextMenuProvidedByPlatform ? findsNothing : findsNWidgets(3)); + + // Tap the selected word to hide the toolbar and retain the selection. + await tester.tapAt(vPos); + await tester.pumpAndSettle(); + expect( + controller.selection, + const TextSelection(baseOffset: 24, extentOffset: 35), + ); + expect(find.byType(CupertinoButton), findsNothing); + + // Tap the selected word to show the toolbar and retain the selection. + await tester.tapAt(vPos); + await tester.pumpAndSettle(); + expect( + controller.selection, + const TextSelection(baseOffset: 24, extentOffset: 35), + ); + expect(find.byType(CupertinoButton), isContextMenuProvidedByPlatform ? findsNothing : findsNWidgets(3)); + + // Tap past the selected word to move the cursor and hide the toolbar. + await tester.tapAt(ePos); + await tester.pumpAndSettle(); + expect( + controller.selection, + const TextSelection.collapsed(offset: 35), + ); + expect(find.byType(CupertinoButton), findsNothing); + }, + variant: const TargetPlatformVariant({ TargetPlatform.iOS }), + ); + testWidgets( 'double tap selects word and first tap of double tap moves cursor', (WidgetTester tester) async { @@ -8804,11 +8882,13 @@ void main() { // Double tap selecting the same word somewhere else is fine. await tester.tapAt(textfieldStart + const Offset(100.0, 9.0)); await tester.pump(const Duration(milliseconds: 50)); - // First tap moved the cursor. + // First tap hides the toolbar and retains the selection. expect( controller.selection, - const TextSelection.collapsed(offset: 7, affinity: TextAffinity.upstream), + const TextSelection(baseOffset: 0, extentOffset: 7), ); + expect(find.byType(CupertinoButton), findsNothing); + // Second tap shows the toolbar and retains the selection. await tester.tapAt(textfieldStart + const Offset(100.0, 9.0)); await tester.pumpAndSettle(); expect( @@ -8819,11 +8899,12 @@ void main() { await tester.tapAt(textfieldStart + const Offset(150.0, 9.0)); await tester.pump(const Duration(milliseconds: 50)); - // First tap moved the cursor. + // First tap moved the cursor and hides the toolbar. expect( controller.selection, const TextSelection.collapsed(offset: 8), ); + expect(find.byType(CupertinoButton), findsNothing); await tester.tapAt(textfieldStart + const Offset(150.0, 9.0)); await tester.pumpAndSettle(); expect( diff --git a/packages/flutter/test/widgets/text_selection_test.dart b/packages/flutter/test/widgets/text_selection_test.dart index 09980cd9626..746f6c7a6ce 100644 --- a/packages/flutter/test/widgets/text_selection_test.dart +++ b/packages/flutter/test/widgets/text_selection_test.dart @@ -569,6 +569,38 @@ void main() { } }, variant: TargetPlatformVariant.all()); + testWidgets('test TextSelectionGestureDetectorBuilder toggles toolbar on single tap on previous selection iOS', (WidgetTester tester) async { + await pumpTextSelectionGestureDetectorBuilder(tester); + + final FakeEditableTextState state = tester.state(find.byType(FakeEditableText)); + final FakeRenderEditable renderEditable = tester.renderObject(find.byType(FakeEditable)); + expect(state.showToolbarCalled, isFalse); + expect(state.toggleToolbarCalled, isFalse); + renderEditable.selection = const TextSelection(baseOffset: 2, extentOffset: 6); + renderEditable.hasFocus = true; + + final TestGesture gesture = await tester.startGesture( + const Offset(25.0, 200.0), + pointer: 0, + ); + await gesture.up(); + await tester.pumpAndSettle(); + + switch (defaultTargetPlatform) { + case TargetPlatform.iOS: + expect(renderEditable.selectWordEdgeCalled, isFalse); + expect(state.toggleToolbarCalled, isTrue); + break; + case TargetPlatform.macOS: + case TargetPlatform.android: + case TargetPlatform.fuchsia: + case TargetPlatform.linux: + case TargetPlatform.windows: + expect(renderEditable.selectPositionAtCalled, isTrue); + break; + } + }, variant: TargetPlatformVariant.all()); + testWidgets('test TextSelectionGestureDetectorBuilder double tap', (WidgetTester tester) async { await pumpTextSelectionGestureDetectorBuilder(tester); final TestGesture gesture = await tester.startGesture( @@ -1333,6 +1365,7 @@ class FakeEditableText extends EditableText { class FakeEditableTextState extends EditableTextState { final GlobalKey _editableKey = GlobalKey(); bool showToolbarCalled = false; + bool toggleToolbarCalled = false; @override RenderEditable get renderEditable => _editableKey.currentContext!.findRenderObject()! as RenderEditable; @@ -1344,7 +1377,8 @@ class FakeEditableTextState extends EditableTextState { } @override - void toggleToolbar() { + void toggleToolbar([bool hideHandles = true]) { + toggleToolbarCalled = true; return; }