From c80366a160ad605eb20b2d2cdb04b667e1bab517 Mon Sep 17 00:00:00 2001 From: Mouad Debbar Date: Thu, 21 Mar 2019 13:25:29 -0700 Subject: [PATCH] Avoid flickering while dragging to select text (#29563) --- .../flutter/lib/src/cupertino/text_field.dart | 7 + .../flutter/lib/src/rendering/editable.dart | 16 +-- .../test/cupertino/text_field_test.dart | 103 +++++++++++++ .../test/material/text_field_test.dart | 136 ++++++++++++++++-- .../flutter/test/rendering/editable_test.dart | 55 +++++++ 5 files changed, 297 insertions(+), 20 deletions(-) diff --git a/packages/flutter/lib/src/cupertino/text_field.dart b/packages/flutter/lib/src/cupertino/text_field.dart index 4f2a3b4d75..2ab7e752d3 100644 --- a/packages/flutter/lib/src/cupertino/text_field.dart +++ b/packages/flutter/lib/src/cupertino/text_field.dart @@ -1,6 +1,7 @@ // Copyright 2018 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +import 'package:flutter/gestures.dart'; import 'package:flutter/rendering.dart'; import 'package:flutter/services.dart'; import 'package:flutter/widgets.dart'; @@ -180,12 +181,14 @@ class CupertinoTextField extends StatefulWidget { this.cursorColor, this.keyboardAppearance, this.scrollPadding = const EdgeInsets.all(20.0), + this.dragStartBehavior = DragStartBehavior.start, }) : assert(textAlign != null), assert(autofocus != null), assert(obscureText != null), assert(autocorrect != null), assert(maxLengthEnforced != null), assert(scrollPadding != null), + assert(dragStartBehavior != null), assert(maxLines == null || maxLines > 0), assert(minLines == null || minLines > 0), assert( @@ -404,6 +407,9 @@ class CupertinoTextField extends StatefulWidget { /// {@macro flutter.widgets.editableText.scrollPadding} final EdgeInsets scrollPadding; + /// {@macro flutter.widgets.scrollable.dragStartBehavior} + final DragStartBehavior dragStartBehavior; + @override _CupertinoTextFieldState createState() => _CupertinoTextFieldState(); @@ -703,6 +709,7 @@ class _CupertinoTextFieldState extends State with AutomaticK backgroundCursorColor: CupertinoColors.inactiveGray, scrollPadding: widget.scrollPadding, keyboardAppearance: keyboardAppearance, + dragStartBehavior: widget.dragStartBehavior, ), ), ); diff --git a/packages/flutter/lib/src/rendering/editable.dart b/packages/flutter/lib/src/rendering/editable.dart index 793a1762ea..0c50fbfc22 100644 --- a/packages/flutter/lib/src/rendering/editable.dart +++ b/packages/flutter/lib/src/rendering/editable.dart @@ -1384,15 +1384,15 @@ class RenderEditable extends RenderBox { extentOffset = math.max(fromPosition.offset, toPosition.offset); } - onSelectionChanged( - TextSelection( - baseOffset: baseOffset, - extentOffset: extentOffset, - affinity: fromPosition.affinity, - ), - this, - cause, + final TextSelection newSelection = TextSelection( + baseOffset: baseOffset, + extentOffset: extentOffset, + affinity: fromPosition.affinity, ); + // Call [onSelectionChanged] only when the selection actually changed. + if (newSelection != _selection) { + onSelectionChanged(newSelection, this, cause); + } } } diff --git a/packages/flutter/test/cupertino/text_field_test.dart b/packages/flutter/test/cupertino/text_field_test.dart index aa199c1ac9..b059ab0f2b 100644 --- a/packages/flutter/test/cupertino/text_field_test.dart +++ b/packages/flutter/test/cupertino/text_field_test.dart @@ -8,6 +8,7 @@ import 'package:flutter/cupertino.dart'; import 'package:flutter/rendering.dart'; import 'package:flutter/services.dart'; import 'package:flutter/foundation.dart'; +import 'package:flutter/gestures.dart' show DragStartBehavior; import 'package:flutter_test/flutter_test.dart'; class MockClipboard { @@ -30,6 +31,45 @@ void main() { final MockClipboard mockClipboard = MockClipboard(); SystemChannels.platform.setMockMethodCallHandler(mockClipboard.handleMethodCall); + // Returns the first RenderEditable. + RenderEditable findRenderEditable(WidgetTester tester) { + final RenderObject root = tester.renderObject(find.byType(EditableText)); + expect(root, isNotNull); + + RenderEditable renderEditable; + void recursiveFinder(RenderObject child) { + if (child is RenderEditable) { + renderEditable = child; + return; + } + child.visitChildren(recursiveFinder); + } + root.visitChildren(recursiveFinder); + expect(renderEditable, isNotNull); + return renderEditable; + } + + List globalize(Iterable points, RenderBox box) { + return points.map((TextSelectionPoint point) { + return TextSelectionPoint( + box.localToGlobal(point.point), + point.direction, + ); + }).toList(); + } + + Offset textOffsetToPosition(WidgetTester tester, int offset) { + final RenderEditable renderEditable = findRenderEditable(tester); + final List endpoints = globalize( + renderEditable.getEndpointsForSelection( + TextSelection.collapsed(offset: offset), + ), + renderEditable, + ); + expect(endpoints.length, 1); + return endpoints[0].point + const Offset(0.0, -2.0); + } + testWidgets( 'takes available space horizontally and takes intrinsic space vertically no-strut', (WidgetTester tester) async { @@ -1688,6 +1728,69 @@ void main() { expect(find.byType(CupertinoButton), findsNothing); }); + testWidgets('Cannot drag one handle past the other', (WidgetTester tester) async { + final TextEditingController controller = TextEditingController( + text: 'abc def ghi', + ); + + await tester.pumpWidget( + CupertinoApp( + home: Center( + child: CupertinoTextField( + dragStartBehavior: DragStartBehavior.down, + controller: controller, + style: const TextStyle( + fontFamily: 'Ahem', + fontSize: 10.0, + ), + ), + ), + ), + ); + + // Double tap on 'e' to select 'def'. + final Offset ePos = textOffsetToPosition(tester, 5); + await tester.tapAt(ePos, pointer: 7); + await tester.pump(const Duration(milliseconds: 50)); + expect(controller.selection.isCollapsed, isTrue); + expect(controller.selection.baseOffset, 4); + await tester.tapAt(ePos, pointer: 7); + await tester.pump(const Duration(milliseconds: 50)); + expect(controller.selection.baseOffset, 4); + expect(controller.selection.extentOffset, 7); + + final RenderEditable renderEditable = findRenderEditable(tester); + final List endpoints = globalize( + renderEditable.getEndpointsForSelection(controller.selection), + renderEditable, + ); + expect(endpoints.length, 2); + + // Drag the right handle until there's only 1 char selected. + // We use a small offset because the endpoint is on the very corner + // of the handle. + final Offset handlePos = endpoints[1].point; + Offset newHandlePos = textOffsetToPosition(tester, 5); // Position of 'e'. + final TestGesture gesture = await tester.startGesture(handlePos, pointer: 7); + await tester.pump(); + await gesture.moveTo(newHandlePos); + await tester.pump(); + + expect(controller.selection.baseOffset, 4); + expect(controller.selection.extentOffset, 5); + + newHandlePos = textOffsetToPosition(tester, 2); // Position of 'c'. + await gesture.moveTo(newHandlePos); + await tester.pump(); + await gesture.up(); + await tester.pump(); + + expect(controller.selection.baseOffset, 4); + // The selection doesn't move beyond the left handle. There's always at + // least 1 char selected. + expect(controller.selection.extentOffset, 5); + }); + testWidgets( 'text field respects theme', (WidgetTester tester) async { diff --git a/packages/flutter/test/material/text_field_test.dart b/packages/flutter/test/material/text_field_test.dart index a1fed5660e..eef9e727a7 100644 --- a/packages/flutter/test/material/text_field_test.dart +++ b/packages/flutter/test/material/text_field_test.dart @@ -645,6 +645,58 @@ void main() { expect(controller.selection.extentOffset, testValue.indexOf('g')); }); + testWidgets('Continuous dragging does not cause flickering', (WidgetTester tester) async { + int selectionChangedCount = 0; + const String testValue = 'abc def ghi'; + final TextEditingController controller = TextEditingController(text: testValue); + + controller.addListener(() { + selectionChangedCount++; + }); + + await tester.pumpWidget( + MaterialApp( + home: Material( + child: TextField( + dragStartBehavior: DragStartBehavior.down, + controller: controller, + style: const TextStyle(fontFamily: 'Ahem', fontSize: 10.0), + ), + ), + ), + ); + + final Offset cPos = textOffsetToPosition(tester, 2); // Index of 'c'. + final Offset gPos = textOffsetToPosition(tester, 8); // Index of 'g'. + final Offset hPos = textOffsetToPosition(tester, 9); // Index of 'h'. + + // Drag from 'c' to 'g'. + final TestGesture gesture = await tester.startGesture(cPos, kind: PointerDeviceKind.mouse); + await tester.pump(); + await gesture.moveTo(gPos); + await tester.pumpAndSettle(); + + expect(selectionChangedCount, isNonZero); + selectionChangedCount = 0; + expect(controller.selection.baseOffset, 2); + expect(controller.selection.extentOffset, 8); + + // Tiny movement shouldn't cause text selection to change. + await gesture.moveTo(gPos + const Offset(4.0, 0.0)); + await tester.pumpAndSettle(); + expect(selectionChangedCount, 0); + + // Now a text selection change will occur after a significant movement. + await gesture.moveTo(hPos); + await tester.pump(); + await gesture.up(); + await tester.pumpAndSettle(); + + expect(selectionChangedCount, 1); + expect(controller.selection.baseOffset, 2); + expect(controller.selection.extentOffset, 9); + }); + testWidgets('Dragging in opposite direction also works', (WidgetTester tester) async { final TextEditingController controller = TextEditingController(); @@ -712,12 +764,10 @@ void main() { final TextEditingController controller = TextEditingController(); await tester.pumpWidget( - MaterialApp( - home: Material( - child: TextField( - dragStartBehavior: DragStartBehavior.down, - controller: controller, - ), + overlay( + child: TextField( + dragStartBehavior: DragStartBehavior.down, + controller: controller, ), ), ); @@ -735,6 +785,8 @@ void main() { await tester.pump(const Duration(milliseconds: 200)); // skip past the frame where the opacity is zero final TextSelection selection = controller.selection; + expect(selection.baseOffset, 4); + expect(selection.extentOffset, 7); final RenderEditable renderEditable = findRenderEditable(tester); final List endpoints = globalize( @@ -747,7 +799,7 @@ void main() { // We use a small offset because the endpoint is on the very corner // of the handle. Offset handlePos = endpoints[1].point + const Offset(1.0, 1.0); - Offset newHandlePos = textOffsetToPosition(tester, selection.extentOffset+2); + Offset newHandlePos = textOffsetToPosition(tester, 9); // Position of 'h'. gesture = await tester.startGesture(handlePos, pointer: 7); await tester.pump(); await gesture.moveTo(newHandlePos); @@ -755,12 +807,12 @@ void main() { await gesture.up(); await tester.pump(); - expect(controller.selection.baseOffset, selection.baseOffset); - expect(controller.selection.extentOffset, selection.extentOffset); + expect(controller.selection.baseOffset, 4); + expect(controller.selection.extentOffset, 9); // Drag the left handle 2 letters to the left. handlePos = endpoints[0].point + const Offset(-1.0, 1.0); - newHandlePos = textOffsetToPosition(tester, selection.baseOffset-2); + newHandlePos = textOffsetToPosition(tester, 2); // Position of 'c'. gesture = await tester.startGesture(handlePos, pointer: 7); await tester.pump(); await gesture.moveTo(newHandlePos); @@ -768,8 +820,68 @@ void main() { await gesture.up(); await tester.pump(); - expect(controller.selection.baseOffset, selection.baseOffset); - expect(controller.selection.extentOffset, selection.extentOffset); + expect(controller.selection.baseOffset, 2); + expect(controller.selection.extentOffset, 9); + }); + + testWidgets('Cannot drag one handle past the other', (WidgetTester tester) async { + final TextEditingController controller = TextEditingController(); + + await tester.pumpWidget( + overlay( + child: TextField( + dragStartBehavior: DragStartBehavior.down, + controller: controller, + ), + ), + ); + + const String testValue = 'abc def ghi'; + await tester.enterText(find.byType(TextField), testValue); + await skipPastScrollingAnimation(tester); + + // Long press the 'e' to select 'def'. + final Offset ePos = textOffsetToPosition(tester, 5); // Position of 'e'. + TestGesture gesture = await tester.startGesture(ePos, pointer: 7); + await tester.pump(const Duration(seconds: 2)); + await gesture.up(); + await tester.pump(); + await tester.pump(const Duration(milliseconds: 200)); // skip past the frame where the opacity is zero + + final TextSelection selection = controller.selection; + expect(selection.baseOffset, 4); + expect(selection.extentOffset, 7); + + final RenderEditable renderEditable = findRenderEditable(tester); + final List endpoints = globalize( + renderEditable.getEndpointsForSelection(selection), + renderEditable, + ); + expect(endpoints.length, 2); + + // Drag the right handle until there's only 1 char selected. + // We use a small offset because the endpoint is on the very corner + // of the handle. + final Offset handlePos = endpoints[1].point + const Offset(1.0, 1.0); + Offset newHandlePos = textOffsetToPosition(tester, 5); // Position of 'e'. + gesture = await tester.startGesture(handlePos, pointer: 7); + await tester.pump(); + await gesture.moveTo(newHandlePos); + await tester.pump(); + + expect(controller.selection.baseOffset, 4); + expect(controller.selection.extentOffset, 5); + + newHandlePos = textOffsetToPosition(tester, 2); // Position of 'c'. + await gesture.moveTo(newHandlePos); + await tester.pump(); + await gesture.up(); + await tester.pump(); + + expect(controller.selection.baseOffset, 4); + // The selection doesn't move beyond the left handle. There's always at + // least 1 char selected. + expect(controller.selection.extentOffset, 5); }); testWidgets('Can use selection toolbar', (WidgetTester tester) async { diff --git a/packages/flutter/test/rendering/editable_test.dart b/packages/flutter/test/rendering/editable_test.dart index a797bdb577..63b6368d92 100644 --- a/packages/flutter/test/rendering/editable_test.dart +++ b/packages/flutter/test/rendering/editable_test.dart @@ -367,4 +367,59 @@ void main() { expect(currentSelection.baseOffset, 1); expect(currentSelection.extentOffset, 3); }); + + test('selection does not flicker as user is dragging', () { + int selectionChangedCount = 0; + TextSelection updatedSelection; + final TextSelectionDelegate delegate = FakeEditableTextState(); + const TextSpan text = TextSpan( + text: 'abc def ghi', + style: TextStyle( + height: 1.0, fontSize: 10.0, fontFamily: 'Ahem', + ), + ); + + final RenderEditable editable1 = RenderEditable( + textSelectionDelegate: delegate, + textDirection: TextDirection.ltr, + offset: ViewportOffset.zero(), + selection: const TextSelection(baseOffset: 3, extentOffset: 4), + onSelectionChanged: (TextSelection selection, RenderEditable renderObject, SelectionChangedCause cause) { + selectionChangedCount++; + updatedSelection = selection; + }, + text: text, + ); + + layout(editable1); + + // Shouldn't cause a selection change. + editable1.selectPositionAt(from: const Offset(30, 2), to: const Offset(42, 2), cause: SelectionChangedCause.drag); + pumpFrame(); + + expect(updatedSelection, isNull); + expect(selectionChangedCount, 0); + + final RenderEditable editable2 = RenderEditable( + textSelectionDelegate: delegate, + textDirection: TextDirection.ltr, + offset: ViewportOffset.zero(), + selection: const TextSelection(baseOffset: 3, extentOffset: 4), + onSelectionChanged: (TextSelection selection, RenderEditable renderObject, SelectionChangedCause cause) { + selectionChangedCount++; + updatedSelection = selection; + }, + text: text, + ); + + layout(editable2); + + // Now this should cause a selection change. + editable2.selectPositionAt(from: const Offset(30, 2), to: const Offset(48, 2), cause: SelectionChangedCause.drag); + pumpFrame(); + + expect(updatedSelection.baseOffset, 3); + expect(updatedSelection.extentOffset, 5); + expect(selectionChangedCount, 1); + }); }