iOS text selection #11224

xster · 2017-07-14T03:07:00Z

Refactored some text selection stuff from material to widget.

Not in this PR:

Selection magnifier (EditableText should show a loupe on iOS when changing selection #9507).
The highlight color is a bit off. Needs some refactoring.

collinjackson

This is a fair bit of code in an area that I'm not knowledgeable about so a second reviewer might be useful. Most of my comments are stylistic.

Can we test this?

collinjackson · 2017-07-14T13:21:19Z

packages/flutter/lib/src/cupertino/button.dart

  final double pressedOpacity;

+  /// The radius of the rounded corners of the button when it has a background
+  /// color. Defaults to 8 logical pixels.


Put the second sentence in a separate paragraph.

Perhaps this should be a BorderRadius instead of a double to make it more configurable.

collinjackson · 2017-07-14T13:22:03Z

packages/flutter/lib/src/cupertino/button.dart

-                borderRadius: const BorderRadius.all(const Radius.circular(8.0)),
+                borderRadius: widget.borderRadius == null
+                    ? null
+                    : new BorderRadius.all(new Radius.circular(widget.borderRadius)),


If you're going to do it this way, I think you can use the BorderRadius.circular constructor here

collinjackson · 2017-07-14T15:38:34Z

packages/flutter/lib/src/cupertino/text_selection.dart

+
+/// Manages a copy/paste text selection toolbar.
+class _TextSelectionToolbar extends StatelessWidget {
+    const _TextSelectionToolbar(


Even though this widget is private, I would use named parameters here because you have several parameters of the same type.

collinjackson · 2017-07-14T15:39:47Z

packages/flutter/lib/src/cupertino/text_selection.dart

+
+  @override
+  bool shouldRepaint(_TextSelectionToolbarNotchPainter oldPainter) {
+    return false;


Possible candidate for fat-arrow syntax

collinjackson · 2017-07-14T15:40:25Z

packages/flutter/lib/src/cupertino/text_selection.dart

+  void paint(Canvas canvas, Size size) {
+    final Paint paint = new Paint()..color = _selectionToolbarBackgroundBlack;
+    final Path triangle = new Path();
+    triangle.lineTo(_selectionToolbarTriangleSize.width / 2, 0.0);


I would use the .. cascade operator here and below.

Ah nice. It's a lot cleaner. Thanks!

collinjackson · 2017-07-14T16:05:00Z

packages/flutter/lib/src/widgets/text_selection.dart

  Size get handleSize;
+
+  void handleCut(TextSelectionDelegate delegate) {
+    Clipboard.setData(new ClipboardData(


You use delegate.textEditingValue here a lot, I think you should assign it to a local final.

collinjackson · 2017-07-14T16:06:11Z

packages/flutter/lib/src/material/text_field.dart

  final ValueChanged<String> onSubmitted;

-  /// Optional input validation and formatting overrides. Formatters are run 
+  /// Optional input validation and formatting overrides. Formatters are run


You didn't introduce this, but since you touched this line, can you move the second sentence into its own paragraph?

collinjackson · 2017-07-14T16:08:19Z

packages/flutter/lib/src/cupertino/text_selection.dart

+          child: new Row(mainAxisSize: MainAxisSize.min, children: items),
+        ),
+        // TODO(xster): position the triangle based on the layout delegate.
+        // And avoid letting the triangle line up with any dividers.


Should this get an issue as well?

collinjackson · 2017-07-14T16:16:07Z

packages/flutter/lib/src/cupertino/text_selection.dart

+
+const Color _selectionToolbarBackgroundBlack = const Color(0xFF2E2E2E);
+const Color _selectionToolbarDividerGray = const Color(0xFFB9B9B9);
+const Color _selectionHandlesBlue = const Color(0xFF146DDE);


nit: Having a description of the color in the name of color constants seems a bit odd to me. "Color" instead of "Black/Gray/Blue" would concentrate the knowledge of what these colors actually are to a single place instead of everywhere the constants are used.

You can see this in other places in Flutter, e.g. _kActiveTickColor, _kDisabledBackground

collinjackson · 2017-07-14T16:18:24Z

packages/flutter/lib/src/cupertino/text_selection.dart

+// Padding around the line at the edge of the text selection that has 0 width and
+// the height of the text font.
+const double _selectionHandlesPadding = 18.0;
+const double _kToolbarScreenPadding = 8.0; // pixels


nit: I found this comment a bit confusing since all padding is pixels and was wondering if it is trying to specify logical vs physical? I realized it's probably copy-pasted from previous code, but the comment isn't on the line above. May be better to remove it.

Cleaned up a bit here and it material

Hixie · 2017-07-17T23:39:41Z

packages/flutter/lib/src/cupertino/button.dart

looks like several of these arguments should be asserted-non-null and have a comment in the docs for the constructor that says they must not be null (see other widgets for examples of the style for these docs).

Added a few more null handling instead

Hixie · 2017-07-17T23:40:31Z

packages/flutter/lib/src/cupertino/text_selection.dart

0.5 doesn't correspond to one physical pixel, does it?

isn't it for ios?

iPhone 6 Plus has a device pixel ratio of 3. iPhone 3 has a device pixel ratio of 1. But also, all the iOS code has to work on Android devices and Fuchsia and whatever else.

Double checked. It is indeed exactly one physical pixel on all devices. Changed from constant to read the device pixel ratio.

Hixie · 2017-07-17T23:41:58Z

packages/flutter/lib/src/cupertino/text_selection.dart

as written this will never match.

oops, sorry, missed some refactor cleanup

Hixie · 2017-07-17T23:44:40Z

packages/flutter/lib/src/cupertino/text_selection.dart

2 -> 2.0 to avoid an int-to-double conversion

Hixie · 2017-07-17T23:45:20Z

packages/flutter/lib/src/cupertino/text_selection.dart

I don't understand what "Also flipped for iOS" means.

reworded a bit

collinjackson · 2017-07-21T14:18:01Z

Posse wanted to check if there's still work to be done here or if this can be merged.

@alardizabal

xster · 2017-07-21T14:56:51Z

It's done, just waiting for an LGTM

mogol · 2017-07-22T15:53:12Z

packages/flutter/lib/src/widgets/text_selection.dart

+              child: widget.selectionControls.buildHandle(
+                context,
+                type,
+                widget.renderObject.size.height / widget.renderObject.maxLines,


Hi,
Seems this changes cause exception in case when TextField was created with unlimited lines new TextField(maxLines: null).
Steps:

Create TextField with maxLines = null

Enter some text, tap Done button

Tap on TextField again

➜ mobileapp git:(develop) ✗ flutter --version Flutter • channel master • https://github.com/flutter/flutter.git Framework • revision a92a62706c (16 hours ago) • 2017-07-21 16:57:33 -0700 Engine • revision 5fcfb995bb Tools • Dart 1.25.0-dev.7.0

The following NoSuchMethodError was thrown building _TextSelectionHandleOverlay(dirty; state: _TextSelectionHandleOverlayState#201d1()): The method 'toDouble' was called on null. Receiver: null Tried calling: toDouble() When the exception was thrown, this was the stack: #0 Object._noSuchMethod (dart:core-patch/object_patch.dart:42) #1 Object.noSuchMethod (dart:core-patch/object_patch.dart:46) #2 double./ (dart:core-patch/double.dart:39) #3 _TextSelectionHandleOverlayState.build (package:flutter/src/widgets/text_selection.dart:482:49) #4 StatefulElement.build (package:flutter/src/widgets/framework.dart:3632:27) #5 ComponentElement.performRebuild (package:flutter/src/widgets/framework.dart:3542:15) #6 Element.rebuild (package:flutter/src/widgets/framework.dart:3443:5) #7 ComponentElement._firstBuild (package:flutter/src/widgets/framework.dart:3522:5) #8 StatefulElement._firstBuild (package:flutter/src/widgets/framework.dart:3660:22) #9 ComponentElement.mount (package:flutter/src/widgets/framework.dart:3517:5) #10 Element.inflateWidget (package:flutter/src/widgets/framework.dart:2901:14) #11 Element.updateChild (package:flutter/src/widgets/framework.dart:2704:12) #12 SingleChildRenderObjectElement.mount (package:flutter/src/widgets/framework.dart:4526:14) #13 Element.inflateWidget (package:flutter/src/widgets/framework.dart:2901:14) #14 Element.updateChild (package:flutter/src/widgets/framework.dart:2704:12) #15 ComponentElement.performRebuild (package:flutter/src/widgets/framework.dart:3554:16) #16 Element.rebuild (package:flutter/src/widgets/framework.dart:3443:5) #17 ComponentElement._firstBuild (package:flutter/src/widgets/framework.dart:3522:5) #18 StatefulElement._firstBuild (package:flutter/src/widgets/framework.dart:3660:22) #19 ComponentElement.mount (package:flutter/src/widgets/framework.dart:3517:5) #20 Element.inflateWidget (package:flutter/src/widgets/framework.dart:2901:14) #21 Element.updateChild (package:flutter/src/widgets/framework.dart:2704:12) #22 ComponentElement.performRebuild (package:flutter/src/widgets/framework.dart:3554:16) #23 Element.rebuild (package:flutter/src/widgets/framework.dart:3443:5) #24 ComponentElement._firstBuild (package:flutter/src/widgets/framework.dart:3522:5) #25 StatefulElement._firstBuild (package:flutter/src/widgets/framework.dart:3660:22) #26 ComponentElement.mount (package:flutter/src/widgets/framework.dart:3517:5) #27 Element.inflateWidget (package:flutter/src/widgets/framework.dart:2901:14) #28 Element.updateChild (package:flutter/src/widgets/framework.dart:2704:12) #29 RenderObjectElement.updateChildren (package:flutter/src/widgets/framework.dart:4317:32) #30 MultiChildRenderObjectElement.update (package:flutter/src/widgets/framework.dart:4641:17) #31 Element.updateChild (package:flutter/src/widgets/framework.dart:2693:15) #32 _TheatreElement.update (package:flutter/src/widgets/overlay.dart:505:16) #33 Element.updateChild (package:flutter/src/widgets/framework.dart:2693:15) #34 ComponentElement.performRebuild (package:flutter/src/widgets/framework.dart:3554:16) #35 Element.rebuild (package:flutter/src/widgets/framework.dart:3443:5) #36 BuildOwner.buildScope (package:flutter/src/widgets/framework.dart:2279:33) #37 BindingBase&SchedulerBinding&GestureBinding&ServicesBinding&RendererBinding&WidgetsBinding.drawFrame (package:flutter/src/widgets/binding.dart:503:20) #38 BindingBase&SchedulerBinding&GestureBinding&ServicesBinding&RendererBinding._handlePersistentFrameCallback (package:flutter/src/rendering/binding.dart:189:5) #39 BindingBase&SchedulerBinding._invokeFrameCallback (package:flutter/src/scheduler/binding.dart:688:15) #40 BindingBase&SchedulerBinding.handleDrawFrame (package:flutter/src/scheduler/binding.dart:636:9) #41 _invoke (file:///Users/herman/Projects/github/flutter/bin/cache/pkg/sky_engine/lib/ui/hooks.dart:86) #42 _drawFrame (file:///Users/herman/Projects/github/flutter/bin/cache/pkg/sky_engine/lib/ui/hooks.dart:75)

Thanks so much for tracking this down. Got a patch in #11353

xster added 3 commits July 13, 2017 18:27

Implement selection toolbar

ef672d5

iOS selection handles

b11234f

some clean up

9b7be89

xster requested a review from collinjackson July 14, 2017 03:07

googlebot added the cla: yes label Jul 14, 2017

xster requested a review from cbracken July 14, 2017 03:07

collinjackson reviewed Jul 14, 2017

View reviewed changes

Hixie reviewed Jul 17, 2017

View reviewed changes

xster force-pushed the platform-text-selection branch from 3b537ce to d30e07d Compare July 17, 2017 23:42

Hixie reviewed Jul 17, 2017

View reviewed changes

Partial review comments

19a277d

Hixie reviewed Jul 17, 2017

View reviewed changes

xster force-pushed the platform-text-selection branch from d30e07d to 19a277d Compare July 17, 2017 23:45

xster added 2 commits July 18, 2017 12:31

more partial review

4be57ee

Make divider opaque

e1651fe

xster force-pushed the platform-text-selection branch 2 times, most recently from 009556b to 42d2b09 Compare July 18, 2017 20:21

Some more ui tweaks and null handling

67a3037

xster force-pushed the platform-text-selection branch from 42d2b09 to 67a3037 Compare July 18, 2017 20:41

Exactly one physical pixel

c495828

collinjackson approved these changes Jul 21, 2017

View reviewed changes

xster merged commit aa096b5 into flutter:master Jul 21, 2017

xster deleted the platform-text-selection branch July 21, 2017 15:33

mogol reviewed Jul 22, 2017

View reviewed changes

xster mentioned this pull request Jul 24, 2017

Fix text selection line height #11353

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 13, 2021

iOS text selection #11224

iOS text selection #11224

Uh oh!

Conversation

xster commented Jul 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

collinjackson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

collinjackson Jul 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

collinjackson commented Jul 21, 2017

Uh oh!

xster commented Jul 21, 2017

Uh oh!

xster commented Jul 14, 2017 •

edited

Loading

collinjackson Jul 14, 2017 •

edited

Loading