From eba38c4b778fb7a56802ca5bc712ccfc52fed973 Mon Sep 17 00:00:00 2001 From: Renzo Olivares Date: Wed, 24 Jan 2024 16:59:06 -0800 Subject: [PATCH] Fix text selection edge scrolling when inside a horizontal scrollable (#140250) Fixes #129590 * Consider `AxisDirection` when calculating scroll offset used in determining TextSelection during a drag/long press drag. Previously it seems that we were assuming the direction was always vertical https://github.com/flutter/flutter/blob/30cc83198544582b858e48c7bb9d761ecdb3d944/packages/flutter/lib/src/widgets/text_selection.dart#L2842-L2844 . * SelectableText now considers RenderEditable offset changes and Scrollable offset changes when calculating the TextSelection during a long press drag. --- .../lib/src/material/selectable_text.dart | 69 ++++- .../lib/src/widgets/text_selection.dart | 20 +- .../test/widgets/selectable_text_test.dart | 269 +++++++++++++++--- 3 files changed, 296 insertions(+), 62 deletions(-) diff --git a/packages/flutter/lib/src/material/selectable_text.dart b/packages/flutter/lib/src/material/selectable_text.dart index e97441e7a80..f19c264787a 100644 --- a/packages/flutter/lib/src/material/selectable_text.dart +++ b/packages/flutter/lib/src/material/selectable_text.dart @@ -59,6 +59,31 @@ class _SelectableTextSelectionGestureDetectorBuilder extends TextSelectionGestur final _SelectableTextState _state; + /// The viewport offset pixels of any [Scrollable] containing the + /// [RenderEditable] at the last drag start. + double _dragStartScrollOffset = 0.0; + + /// The viewport offset pixels of the [RenderEditable] at the last drag start. + double _dragStartViewportOffset = 0.0; + + double get _scrollPosition { + final ScrollableState? scrollableState = + delegate.editableTextKey.currentContext == null + ? null + : Scrollable.maybeOf(delegate.editableTextKey.currentContext!); + return scrollableState == null + ? 0.0 + : scrollableState.position.pixels; + } + + AxisDirection? get _scrollDirection { + final ScrollableState? scrollableState = + delegate.editableTextKey.currentContext == null + ? null + : Scrollable.maybeOf(delegate.editableTextKey.currentContext!); + return scrollableState?.axisDirection; + } + @override void onForcePressStart(ForcePressDetails details) { super.onForcePressStart(details); @@ -73,14 +98,36 @@ class _SelectableTextSelectionGestureDetectorBuilder extends TextSelectionGestur } @override - void onSingleLongTapMoveUpdate(LongPressMoveUpdateDetails details) { - if (delegate.selectionEnabled) { - renderEditable.selectWordsInRange( - from: details.globalPosition - details.offsetFromOrigin, - to: details.globalPosition, - cause: SelectionChangedCause.longPress, - ); + void onSingleLongTapStart(LongPressStartDetails details) { + if (!delegate.selectionEnabled) { + return; } + renderEditable.selectWord(cause: SelectionChangedCause.longPress); + Feedback.forLongPress(_state.context); + _dragStartViewportOffset = renderEditable.offset.pixels; + _dragStartScrollOffset = _scrollPosition; + } + + @override + void onSingleLongTapMoveUpdate(LongPressMoveUpdateDetails details) { + if (!delegate.selectionEnabled) { + return; + } + // Adjust the drag start offset for possible viewport offset changes. + final Offset editableOffset = renderEditable.maxLines == 1 + ? Offset(renderEditable.offset.pixels - _dragStartViewportOffset, 0.0) + : Offset(0.0, renderEditable.offset.pixels - _dragStartViewportOffset); + final double effectiveScrollPosition = _scrollPosition - _dragStartScrollOffset; + final bool scrollingOnVerticalAxis = _scrollDirection == AxisDirection.up || _scrollDirection == AxisDirection.down; + final Offset scrollableOffset = Offset( + !scrollingOnVerticalAxis ? effectiveScrollPosition : 0.0, + scrollingOnVerticalAxis ? effectiveScrollPosition : 0.0, + ); + renderEditable.selectWordsInRange( + from: details.globalPosition - details.offsetFromOrigin - editableOffset - scrollableOffset, + to: details.globalPosition, + cause: SelectionChangedCause.longPress, + ); } @override @@ -100,14 +147,6 @@ class _SelectableTextSelectionGestureDetectorBuilder extends TextSelectionGestur } _state.widget.onTap?.call(); } - - @override - void onSingleLongTapStart(LongPressStartDetails details) { - if (delegate.selectionEnabled) { - renderEditable.selectWord(cause: SelectionChangedCause.longPress); - Feedback.forLongPress(_state.context); - } - } } /// A run of selectable text with a single style. diff --git a/packages/flutter/lib/src/widgets/text_selection.dart b/packages/flutter/lib/src/widgets/text_selection.dart index bb751a639c1..e83f266ce4f 100644 --- a/packages/flutter/lib/src/widgets/text_selection.dart +++ b/packages/flutter/lib/src/widgets/text_selection.dart @@ -2131,6 +2131,14 @@ class TextSelectionGestureDetectorBuilder { : scrollableState.position.pixels; } + AxisDirection? get _scrollDirection { + final ScrollableState? scrollableState = + delegate.editableTextKey.currentContext == null + ? null + : Scrollable.maybeOf(delegate.editableTextKey.currentContext!); + return scrollableState?.axisDirection; + } + // For a shift + tap + drag gesture, the TextSelection at the point of the // tap. Mac uses this value to reset to the original selection when an // inversion of the base and offset happens. @@ -2498,9 +2506,11 @@ class TextSelectionGestureDetectorBuilder { final Offset editableOffset = renderEditable.maxLines == 1 ? Offset(renderEditable.offset.pixels - _dragStartViewportOffset, 0.0) : Offset(0.0, renderEditable.offset.pixels - _dragStartViewportOffset); + final double effectiveScrollPosition = _scrollPosition - _dragStartScrollOffset; + final bool scrollingOnVerticalAxis = _scrollDirection == AxisDirection.up || _scrollDirection == AxisDirection.down; final Offset scrollableOffset = Offset( - 0.0, - _scrollPosition - _dragStartScrollOffset, + !scrollingOnVerticalAxis ? effectiveScrollPosition : 0.0, + scrollingOnVerticalAxis ? effectiveScrollPosition : 0.0, ); switch (defaultTargetPlatform) { case TargetPlatform.iOS: @@ -2839,9 +2849,11 @@ class TextSelectionGestureDetectorBuilder { final Offset editableOffset = renderEditable.maxLines == 1 ? Offset(renderEditable.offset.pixels - _dragStartViewportOffset, 0.0) : Offset(0.0, renderEditable.offset.pixels - _dragStartViewportOffset); + final double effectiveScrollPosition = _scrollPosition - _dragStartScrollOffset; + final bool scrollingOnVerticalAxis = _scrollDirection == AxisDirection.up || _scrollDirection == AxisDirection.down; final Offset scrollableOffset = Offset( - 0.0, - _scrollPosition - _dragStartScrollOffset, + !scrollingOnVerticalAxis ? effectiveScrollPosition : 0.0, + scrollingOnVerticalAxis ? effectiveScrollPosition : 0.0, ); final Offset dragStartGlobalPosition = details.globalPosition - details.offsetFromOrigin; diff --git a/packages/flutter/test/widgets/selectable_text_test.dart b/packages/flutter/test/widgets/selectable_text_test.dart index a63c4d02472..b1e1d1772e0 100644 --- a/packages/flutter/test/widgets/selectable_text_test.dart +++ b/packages/flutter/test/widgets/selectable_text_test.dart @@ -3689,36 +3689,32 @@ void main() { variant: const TargetPlatformVariant({ TargetPlatform.macOS }), ); - testWidgets('long press drag can edge scroll', (WidgetTester tester) async { + testWidgets('long press drag can edge scroll when inside a scrollable', (WidgetTester tester) async { + // This is a regression test for https://github.com/flutter/flutter/issues/129590. await tester.pumpWidget( - const MaterialApp( + MaterialApp( home: Material( child: Center( - child: SelectableText( - 'Atwater Peel Sherbrooke Bonaventure Angrignon Peel Côte-des-Neiges', - maxLines: 1, + child: SizedBox( + width: 300.0, + child: SingleChildScrollView( + scrollDirection: Axis.horizontal, + child: SelectableText( + 'Atwater Peel Sherbrooke Bonaventure Angrignon Peel Côte-des-Neiges ' * 2, + maxLines: 1, + ), + ), ), ), ), ), ); - final RenderEditable renderEditable = findRenderEditable(tester); - - List lastCharEndpoint = renderEditable.getEndpointsForSelection( - const TextSelection.collapsed(offset: 66), // Last character's position. - ); - - expect(lastCharEndpoint.length, 1); - // Just testing the test and making sure that the last character is off - // the right side of the screen. - expect(lastCharEndpoint[0].point.dx, 924.0); - final Offset selectableTextStart = tester.getTopLeft(find.byType(SelectableText)); final TestGesture gesture = - await tester.startGesture(selectableTextStart + const Offset(300, 5)); - await tester.pump(const Duration(milliseconds: 500)); + await tester.startGesture(selectableTextStart + const Offset(200.0, 0.0)); + await tester.pump(kLongPressTimeout); final EditableText editableTextWidget = tester.widget(find.byType(EditableText).first); final TextEditingController controller = editableTextWidget.controller; @@ -3727,43 +3723,196 @@ void main() { controller.selection, const TextSelection(baseOffset: 13, extentOffset: 23), ); - expect(find.byType(CupertinoButton), findsNothing); - await gesture.moveBy(const Offset(600, 0)); + await gesture.moveBy(const Offset(100, 0)); // To the edge of the screen basically. await tester.pump(); expect( controller.selection, const TextSelection( baseOffset: 13, - extentOffset: 66, + extentOffset: 23, ), ); // Keep moving out. - await gesture.moveBy(const Offset(1, 0)); + await gesture.moveBy(const Offset(100, 0)); await tester.pump(); expect( controller.selection, const TextSelection( baseOffset: 13, - extentOffset: 66, + extentOffset: 35, ), ); - await gesture.moveBy(const Offset(1, 0)); + await gesture.moveBy(const Offset(1600, 0)); await tester.pump(); expect( controller.selection, const TextSelection( baseOffset: 13, - extentOffset: 66, + extentOffset: 134, ), ); - expect(find.byType(CupertinoButton), findsNothing); await gesture.up(); - await tester.pump(); + await tester.pumpAndSettle(); // The selection isn't affected by the gesture lift. + expect( + controller.selection, + const TextSelection( + baseOffset: 13, + extentOffset: 134, + ), + ); + // The toolbar shows up. + if (defaultTargetPlatform == TargetPlatform.iOS) { + expectCupertinoSelectionToolbar(); + } else { + expectMaterialSelectionToolbar(); + } + + final RenderEditable renderEditable = findRenderEditable(tester); + final List endpoints = globalize( + renderEditable.getEndpointsForSelection(controller.selection), + renderEditable, + ); + expect(endpoints.isNotEmpty, isTrue); + expect(endpoints.length, 2); + expect(endpoints[0].point.dx, isNegative); + expect(endpoints[1].point.dx, isPositive); + }, + // TODO(Renzo-Olivares): Add in TargetPlatform.android in the line below when + // we fix edge scrolling in a Scrollable https://github.com/flutter/flutter/issues/64059. + variant: const TargetPlatformVariant({ TargetPlatform.iOS }), + ); + + testWidgets('Desktop mouse drag can edge scroll when inside a horizontal scrollable', (WidgetTester tester) async { + // This is a regression test for https://github.com/flutter/flutter/issues/129590. + await tester.pumpWidget( + MaterialApp( + home: Material( + child: Center( + child: SizedBox( + width: 300.0, + child: SingleChildScrollView( + scrollDirection: Axis.horizontal, + child: SelectableText( + 'Atwater Peel Sherbrooke Bonaventure Angrignon Peel Côte-des-Neiges ' * 2, + maxLines: 1, + ), + ), + ), + ), + ), + ), + ); + + final Offset selectableTextStart = tester.getTopLeft(find.byType(SelectableText)); + + final TestGesture gesture = + await tester.startGesture(selectableTextStart + const Offset(200.0, 0.0)); + await tester.pump(); + + final EditableText editableTextWidget = tester.widget(find.byType(EditableText).first); + final TextEditingController controller = editableTextWidget.controller; + + await gesture.moveBy(const Offset(100, 0)); + // To the edge of the screen basically. + await tester.pump(); + expect( + controller.selection, + const TextSelection( + baseOffset: 14, + extentOffset: 21, + ), + ); + // Keep moving out. + await gesture.moveBy(const Offset(100, 0)); + await tester.pump(); + expect( + controller.selection, + const TextSelection( + baseOffset: 14, + extentOffset: 28, + ), + ); + await gesture.moveBy(const Offset(1600, 0)); + await tester.pump(); + expect( + controller.selection, + const TextSelection( + baseOffset: 14, + extentOffset: 134, + ), + ); + + await gesture.up(); + await tester.pumpAndSettle(); + + // The selection isn't affected by the gesture lift. + expect( + controller.selection, + const TextSelection( + baseOffset: 14, + extentOffset: 134, + ), + ); + + final RenderEditable renderEditable = findRenderEditable(tester); + final List endpoints = globalize( + renderEditable.getEndpointsForSelection(controller.selection), + renderEditable, + ); + expect(endpoints.isNotEmpty, isTrue); + expect(endpoints.length, 2); + expect(endpoints[0].point.dx, isNegative); + expect(endpoints[1].point.dx, isPositive); + }, + variant: TargetPlatformVariant.desktop(), + ); + + testWidgets('long press drag can edge scroll', (WidgetTester tester) async { + await tester.pumpWidget( + MaterialApp( + home: Material( + child: Center( + child: SelectableText( + 'Atwater Peel Sherbrooke Bonaventure Angrignon Peel Côte-des-Neiges ' * 2, + maxLines: 1, + ), + ), + ), + ), + ); + + final Offset selectableTextStart = tester.getTopLeft(find.byType(SelectableText)); + + final TestGesture gesture = + await tester.startGesture(selectableTextStart + const Offset(300, 5)); + await tester.pump(kLongPressTimeout); + + final EditableText editableTextWidget = tester.widget(find.byType(EditableText).first); + final TextEditingController controller = editableTextWidget.controller; + + expect( + controller.selection, + const TextSelection(baseOffset: 13, extentOffset: 23), + ); + + await gesture.moveBy(const Offset(300, 0)); + // To the edge of the screen basically. + await tester.pump(); + expect( + controller.selection, + const TextSelection( + baseOffset: 13, + extentOffset: 45, + ), + ); + // Keep moving out. + await gesture.moveBy(const Offset(300, 0)); + await tester.pump(); expect( controller.selection, const TextSelection( @@ -3771,26 +3920,60 @@ void main() { extentOffset: 66, ), ); - // The toolbar shows up with one button (copy). - expect(find.byType(CupertinoButton), findsNWidgets(1)); - - lastCharEndpoint = renderEditable.getEndpointsForSelection( - const TextSelection.collapsed(offset: 66), // Last character's position. + await gesture.moveBy(const Offset(400, 0)); + await tester.pump(); + expect( + controller.selection, + const TextSelection( + baseOffset: 13, + extentOffset: 102, + ), ); - expect(lastCharEndpoint.length, 1); - // The last character is now on screen near the right edge. - expect(lastCharEndpoint[0].point.dx, moreOrLessEquals(798, epsilon: 1)); - - final List firstCharEndpoint = renderEditable.getEndpointsForSelection( - const TextSelection.collapsed(offset: 0), // First character's position. + await gesture.moveBy(const Offset(700, 0)); + await tester.pump(); + expect( + controller.selection, + const TextSelection( + baseOffset: 13, + extentOffset: 134, + ), ); - expect(firstCharEndpoint.length, 1); - // The first character is now offscreen to the left. - expect(firstCharEndpoint[0].point.dx, moreOrLessEquals(-125, epsilon: 1)); + + await gesture.up(); + await tester.pumpAndSettle(); + + // The selection isn't affected by the gesture lift. + expect( + controller.selection, + const TextSelection( + baseOffset: 13, + extentOffset: 134, + ), + ); + // The toolbar shows up. + if (defaultTargetPlatform == TargetPlatform.iOS) { + expectCupertinoSelectionToolbar(); + } else { + expectMaterialSelectionToolbar(); + } + + // Find the selection handle fade transition after the start handle has been + // hidden because it is out of view. + final List transitionsAfter = find.descendant( + of: find.byWidgetPredicate((Widget w) => '${w.runtimeType}' == '_SelectionHandleOverlay'), + matching: find.byType(FadeTransition), + ).evaluate().map((Element e) => e.widget).cast().toList(); + + expect(transitionsAfter.length, 2); + + final FadeTransition startHandleAfter = transitionsAfter[0]; + final FadeTransition endHandleAfter = transitionsAfter[1]; + + expect(startHandleAfter.opacity.value, 0.0); + expect(endHandleAfter.opacity.value, 1.0); }, - variant: const TargetPlatformVariant({ TargetPlatform.iOS, TargetPlatform.macOS }), - skip: true, // https://github.com/flutter/flutter/issues/64059 + variant: const TargetPlatformVariant({ TargetPlatform.iOS, TargetPlatform.android }), ); testWidgets(