iOS selection handles are invisible #31332

justinmc · 2019-04-19T23:09:42Z

The selection handles in iOS don't show up anymore. It seems the cursor is being reported by the engine as being up out of the text input by ~-0.1, which with the recent PR to hide handles when they're out of the input, the handles are always hidden.

It seems to be caused by rounding in the engine and not something that can be reasonably fixed there, so my current POC solution just does a more fuzzy check on where the cursor is.

TODO

Test so this doesn't happen again.
Write a serious comment explaining the hackiness. Point to place in the engine specifically.

by fuzzily checking contains

justinmc · 2019-04-22T22:44:32Z

packages/flutter/lib/src/rendering/editable.dart

@GaryQian Is there anything else more specific that I can refer people to for the slightly off engine bug that I asked you about on Friday?

By the way, despite all of the tests that I added to this, I was never able to reproduce that engine bug in the tests. Any idea how I might do that if it's possible?

There's a few places we do height rounding for text: https://github.com/flutter/engine/blob/master/third_party/txt/src/txt/paragraph.cc#L921
and https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/painting/text_painter.dart#L294 come to mind. The combination of these produces integer height values.

Is this the bug you are asking about? Maybe because we are rounding in both the engine and the framework, one of those rounds is eating up your "slightly off".

GetRectsForRange produces the box by using the rounded baseline, but then adds on the unrounded ascent and descent, which may cause inconsistencies between the reported height and the getRectsForRange positions.

justinmc · 2019-04-22T22:44:38Z

packages/flutter/lib/src/rendering/editable.dart

Any better way to do this?

Instead of a fuzzy check, it may be better to instead increase the size of the clipping zone by measuring the height of the actual glyphs. With the current API, should be able to be achieved with calling GetBoxesForRange() with BoxHeightStyle.max on the first and last glyph. This should result in the true size of the paragraph, except that this requires layout first. The problem with this metric is that a single line height does not convert to multi line height with a simple int multiplication even if all lines are identical.

Another thing that could cause this is that since we now use strut on TextFields, it is now possible for text to extend past the provided space due to different fonts being fallen back on.

Overall, it does seem like a fuzzy check is a hack and adds slop to the way everything works, I'd prefer a more formal and rigid solution if it is achievable. Feel free to come chat about other approaches to this.

GaryQian · 2019-04-22T23:43:28Z

I commented on my reservations on a fuzzy check, but this may end up being the only reasonable way to fix the bug without doing an overhaul of the legacy rounding system currently in place, which would be a massive breaking change that would basically break all golden tests in existence.

HansMuller · 2019-04-23T16:17:06Z

Lets fix this problem with a fuzzy check now. It would be nice to be able tighten up the logic in the future.

… error

justinmc · 2019-04-23T16:44:34Z

@GaryQian @HansMuller I improved the fuzzy check and the docs and created an issue for cleaning this up in the future so we don't have to do an approximation: #31495

HansMuller

Looks good, just some small time feedback.

HansMuller · 2019-04-23T16:20:08Z

packages/flutter/lib/src/rendering/editable.dart

+  // Returns a bool indicating if the offset is within factor of being contained
+  // within the rect.
+  static bool _containsFuzzy(Rect rect, Offset offset, [double factor = 0.1]) {
+    return rect.contains(offset)


This expression could just be rect.inflate(factor).contains(offset). Suggest renaming factor => slop (here and elsewhere). We actually use "slop" in other similar names.

HansMuller · 2019-04-23T16:26:01Z

packages/flutter/lib/src/rendering/editable.dart

+    // Check if the caret is visible with an approximation because of an error
+    // in the engine that always reports that the caret extends above the input
+    // by a small amount. See paragraph.cc's implementation of GetRectsForRange.
+    const double kApproximateFactor = 0.1;


For const variables, don't use the "k" prefix. In this case (sadly) the variable name that would be consistent with existing names would be visibleRegionSlop.

Can you also describe in the comment above how you arrived at this 0.1 value? _applyFloatingPointHack seem to describe rounding to the nearest pixel. Is this related?

HansMuller · 2019-04-23T16:26:42Z

packages/flutter/test/material/text_field_test.dart

+    );
+
+    final EditableTextState state =
+        tester.state<EditableTextState>(find.byType(EditableText));


It's OK to just put this variable and its value on one line; otherwise 2-space indent (here and below).

HansMuller · 2019-04-23T16:29:54Z

packages/flutter/test/widgets/editable_text_test.dart

+
+  testWidgets('iOS text selection handle visibility', (WidgetTester tester) async {
+    debugDefaultTargetPlatformOverride = TargetPlatform.iOS;
+    final GlobalKey<EditableTextState> editableTextKey =


Is this key really needed? Could always use find.byType(EditableText)?

HansMuller · 2019-04-23T16:32:05Z

packages/flutter/test/widgets/editable_text_test.dart

+      rightVisibleBefore = rightVisible;
+
+      // Check that the handles' positions are correct (clamped within the
+      // viewport but not stuck).


What does "stuck" mean here?

I copied this from the material test. I think it means not in a single position every time even when the selected text moves. I'll update both comments.

Sounds good!

HansMuller · 2019-04-23T16:35:49Z

packages/flutter/test/widgets/editable_text_test.dart

  });
+
+  testWidgets('iOS text selection handle visibility', (WidgetTester tester) async {
+    debugDefaultTargetPlatformOverride = TargetPlatform.iOS;


Include a comment here:

// Regression test for https://github.com/flutter/flutter/issues/31287

HansMuller · 2019-04-23T16:39:38Z

packages/flutter/test/widgets/editable_text_test.dart

+
+    Future<void> verifyVisibility(
+      bool leftVisible,
+      Symbol leftPosition,


We usually don't use Symbols for this kind of thing. Better to just define some local integer constants.

More copied code, but I'll quickly update it to an enum. Let me know if that doesn't work.

HansMuller · 2019-04-23T16:40:39Z

packages/flutter/test/widgets/editable_text_test.dart

+
+      final Size viewport = renderEditable.size;
+
+      void testPosition(double pos, Symbol expected) {


Use a switch here since expected is just a substitute for a local enum.

HansMuller · 2019-04-23T16:47:15Z

packages/flutter/test/widgets/editable_text_test.dart

+      final Size viewport = renderEditable.size;
+
+      void testPosition(double pos, Symbol expected) {
+        if (expected == #left)


These symbols might be a little easier to understand if they were called "leftViewportEdge" or "withinViewport" etc

HansMuller · 2019-04-23T16:49:47Z

packages/flutter/test/widgets/editable_text_test.dart

+    bool leftVisibleBefore = false;
+    bool rightVisibleBefore = false;
+
+    Future<void> verifyVisibility(


This might be slightly easier to read if the parameter pairs were reversed:

verifyVisibility(leftViewportEdge, true, rightViewportEdge, true)

xster

LGTM. Thanks for fixing.

xster · 2019-04-23T21:49:04Z

packages/flutter/lib/src/rendering/editable.dart

    );
-
-    _selectionStartInViewport.value = visibleRegion.contains(startOffset + effectiveOffset);
+    // Check if the caret is visible with an approximation because a difference


Can you add a TODO with a tracking bug to fix the root cause?

Will do. Here's the issue for the record: #31495

s/caret/handle right? I think the code in here all reference the blinking cursor as caret and the edges of a selection as handles.

xster · 2019-04-23T21:50:57Z

packages/flutter/lib/src/rendering/editable.dart

+    // Check if the caret is visible with an approximation because of an error
+    // in the engine that always reports that the caret extends above the input
+    // by a small amount. See paragraph.cc's implementation of GetRectsForRange.
+    const double kApproximateFactor = 0.1;


Can you also describe in the comment above how you arrived at this 0.1 value? _applyFloatingPointHack seem to describe rounding to the nearest pixel. Is this related?

xster · 2019-04-23T21:57:46Z

packages/flutter/test/material/text_field_test.dart

+    final List<Widget> transitions =
+      find.byType(FadeTransition).evaluate().map((Element e) => e.widget).toList();
+    expect(transitions.length, 3);
+    final FadeTransition left = transitions[1];


Add a comment for why 3 and what are the first and second ones expected to be.

xster · 2019-04-23T21:58:49Z

packages/flutter/test/material/text_field_test.dart

+    expect(right.opacity.value, equals(1.0));
+  });
+
+  testWidgets('iOS shows selection carets', (WidgetTester tester) async {


Can you add this test to cupertino/text_field_test.dart too?

xster · 2019-04-23T21:59:19Z

packages/flutter/test/widgets/editable_text_test.dart

+    final GlobalKey<EditableTextState> editableTextKey =
+        GlobalKey<EditableTextState>();
+
+    const String testText = 'XXXXX          XXXXX';


Add a comment for what the spaces are for

xster · 2019-04-23T22:00:25Z

packages/flutter/test/widgets/editable_text_test.dart

+    const String testText = 'XXXXX          XXXXX';
+    final TextEditingController controller = TextEditingController(text: testText);
+
+    final Widget widget = MaterialApp(


You don't seem to be using this reference later. Maybe just inline this into the pumpWidget?

xster · 2019-04-23T22:10:51Z

packages/flutter/test/widgets/editable_text_test.dart

+    bool rightVisibleBefore = false;
+
+    Future<void> verifyVisibility(
+      bool leftVisible,


maybe call this expectedLeftVisible etc?

Might be worth checking this in RTL too in which case it's really expectedStartVisible

I just added a very simple RTL test. I tried copying one of these big tests and making it RTL, but I've been fighting with it all day to get it to pass. I'm seeing lots of weird behavior that I don't see when I manually test the app myself. I think it's better to just have a simple one than for me to hack in a copy of these that I don't understand.

LongCatIsLooong · 2019-04-24T01:15:14Z

packages/flutter/test/widgets/editable_text_test.dart

I think const can not be used before type declarations?

Whoops, good catch.

justinmc · 2019-04-24T15:25:23Z

@GaryQian Can we be certain that the rounding error will always be smaller than -0.1? That's what I see experimentally, but I want to add some details in my comment if we know why #31332 (comment)

Update: I've changed the fudge factor to be 0.5 because I saw that once when using EditableText directly. Maybe that makes more sense if it's a whole number rounding thing...

justinmc · 2019-04-24T22:47:34Z

Ready for re-review @HansMuller @xster

GaryQian · 2019-04-24T23:10:19Z

Yes, the 0.5 should cover the rounding error. in practice, it is usually < 0.1, but I do think it can be up to 0.5, as the rounding is to closest int.

xster

LGTM

xster · 2019-04-24T23:24:22Z

packages/flutter/lib/src/rendering/editable.dart

    );
-
-    _selectionStartInViewport.value = visibleRegion.contains(startOffset + effectiveOffset);
+    // Check if the caret is visible with an approximation because a difference


s/caret/handle right? I think the code in here all reference the blinking cursor as caret and the edges of a selection as handles.

xster · 2019-04-24T23:25:34Z

packages/flutter/test/cupertino/text_field_test.dart

    expect(editableText.cursorColor, const Color(0xFFF44336));
  });
+
+  testWidgets('iOS shows selection carets', (WidgetTester tester) async {


'Shows' might be a bit optimistic :D since you're not fundamentally checking that it's visible.
Maybe iOS selection handles aren't faded away by default

xster · 2019-04-24T23:25:55Z

packages/flutter/test/material/text_field_test.dart

    expect(topLeft.dx, equals(383)); // Should be same as equivalent in 'Caret center position'
  });
+
+  testWidgets('shows selection carets', (WidgetTester tester) async {


Check the verbiage of caret vs handle everywhere

HansMuller

LGTM

HansMuller · 2019-04-24T23:24:37Z

packages/flutter/lib/src/rendering/editable.dart

+    // Check if the caret is visible with an approximation because a difference
+    // between rounded and unrounded values causes the caret to be reported as
+    // having a slightly (< 0.5) negative y offset. This rounding happens in
+    // paragraph.cc's layout and TextPainer's _applyFloatingPointHack. Ideally,


TextPainer => TextPainter

Whoops 🎨

justinmc · 2019-04-25T18:19:36Z

@GaryQian Thanks, that's what it looked like to me. I always saw <0.1 until I copied a weird case from the tests into an app where we render an EditableText directly and it was ~0.5.

Fix a bug where text selection handles were invisible in iOS

justinmc self-assigned this Apr 19, 2019

googlebot added the cla: yes label Apr 19, 2019

goderbauer added framework flutter/packages/flutter repository. See also f: labels. platform-ios iOS applications specifically labels Apr 22, 2019

justinmc added the work in progress; do not review label Apr 22, 2019

Work around engine reporting that caret is above input by a small factor

ad48e16

by fuzzily checking contains

justinmc force-pushed the invisible-ios-handles branch from 1d69883 to ad48e16 Compare April 22, 2019 17:41

Test iOS selection handles and their slight differences

f3d8881

justinmc removed the work in progress; do not review label Apr 22, 2019

justinmc changed the title ~~WIP iOS selection handles are invisible~~ iOS selection handles are invisible Apr 22, 2019

justinmc commented Apr 22, 2019

View reviewed changes

justinmc requested review from GaryQian and HansMuller April 22, 2019 22:44

Test for simple existence of text selection handles on ios and android

777c755

justinmc force-pushed the invisible-ios-handles branch from 25f6fe8 to 777c755 Compare April 22, 2019 22:46

A much simpler fuzzy check and a clarified comment about the rounding…

420eacf

… error

justinmc mentioned this pull request Apr 23, 2019

Improve fuzzy check for text selection handles #31495

Open

HansMuller reviewed Apr 23, 2019

View reviewed changes

xster mentioned this pull request Apr 23, 2019

iOS Textfield caret handles do not appear when text is selected in latest beta release (1.4.9) #31287

Closed

xster reviewed Apr 23, 2019

View reviewed changes

LongCatIsLooong reviewed Apr 24, 2019

View reviewed changes

Code review fixes round 1

36389e3

justinmc force-pushed the invisible-ios-handles branch from e3bf76a to 36389e3 Compare April 24, 2019 01:21

justinmc force-pushed the invisible-ios-handles branch 2 times, most recently from 62a9168 to 590678f Compare April 24, 2019 16:59

Code review round 2

a6514b9

justinmc force-pushed the invisible-ios-handles branch from 590678f to a6514b9 Compare April 24, 2019 17:03

Simple RTL test

ea0cf87

justinmc force-pushed the invisible-ios-handles branch from f4d32fd to ea0cf87 Compare April 24, 2019 22:15

xster reviewed Apr 24, 2019

View reviewed changes

HansMuller approved these changes Apr 24, 2019

View reviewed changes

Code review round 3

405446e

justinmc force-pushed the invisible-ios-handles branch from 3307a9f to 405446e Compare April 25, 2019 18:24

justinmc merged commit a21a1f4 into flutter:master Apr 25, 2019

justinmc deleted the invisible-ios-handles branch April 25, 2019 19:32

GaryQian mentioned this pull request Apr 30, 2019

iOS CupertinoTextField gallery text jitters with selection handles #31817

Closed

Hixie pushed a commit to Hixie/flutter that referenced this pull request Apr 30, 2019

iOS selection handles are invisible (flutter#31332)

d6b395b

Fix a bug where text selection handles were invisible in iOS

Hixie added a commit that referenced this pull request Apr 30, 2019

iOS selection handles are invisible (#31332) (#31862)

09cbc34

Fix a bug where text selection handles were invisible in iOS

justinmc mentioned this pull request May 10, 2019

TextField's Text Selection Buttons are Disappeared when text starts with Chinese char #32332

Closed

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


		final Size viewport = renderEditable.size;

		void testPosition(double pos, Symbol expected) {

iOS selection handles are invisible #31332

iOS selection handles are invisible #31332

Uh oh!

Conversation

justinmc commented Apr 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

GaryQian Apr 22, 2019 • 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!

GaryQian commented Apr 22, 2019

Uh oh!

HansMuller commented Apr 23, 2019

Uh oh!

justinmc commented Apr 23, 2019

Uh oh!

HansMuller 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!

xster 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

justinmc commented Apr 19, 2019 •

edited

Loading

GaryQian Apr 22, 2019 •

edited

Loading

justinmc commented Apr 24, 2019 •

edited

Loading

GaryQian commented Apr 24, 2019 •

edited

Loading