Multiline Selection Menu Position Bug #36974

justinmc · 2019-07-25T23:18:41Z

Description

The selection menu is showing up in the wrong place for multiline inputs in Material.

Related Issues

Closes #36749

Tests

Wrote a multiline input test that checks the position of the selection menu.

…hen multiline

fluttergithubbot · 2019-07-25T23:18:43Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. While there are exceptions to this rule, if this patch modifies code it is probably not an exception.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

/cc @dnfield

justinmc · 2019-07-26T19:41:51Z

@LongCatIsLooong Requesting a review from you because of your recent cleanup of the Cupertino text selection postioning code (#34095). Cupertino's behavior seems perfect after your PR, but apparently Material's was still buggy. I tried to copy your approach a bit for Material, let me know what you think or if I missed anything though.

…dded specificity

LongCatIsLooong · 2019-07-26T20:19:39Z

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

@@ -305,11 +305,12 @@ class _CupertinoTextSelectionControls extends TextSelectionControls {
    // The toolbar should appear below the TextField when there is not enough
    // space above the TextField to show it, assuming there's always enough space
    // at the bottom in this case.
-    final bool isArrowPointingDown =
-      mediaQuery.padding.top
+    final double toolbarHeightNeeded = mediaQuery.padding.top


LongCatIsLooong · 2019-07-26T20:29:55Z

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

-        : startTextSelectionPoint.point.dy - globalEditableRegion.height;
+    final double toolbarHeightNeeded = MediaQuery.of(context).padding.top
+      + _kToolbarScreenPadding
+      + _kToolbarHeight;


Is there supposed to be a non-zero content padding (between the paragraph and the bottom of the toolbar)?

It doesn't seem so looking at the code, but looking at native apps it seems like there should be. I'll add in a _kToolbarContentDistance like in iOS. It may break golden/scuba tests.

After:

Native:

LongCatIsLooong · 2019-07-26T20:47:35Z

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

+      + _kToolbarHeight;
+    final double availableHeight = globalEditableRegion.top + endpoints.first.point.dy - textLineHeight;
+    final bool fitsAbove = toolbarHeightNeeded <= availableHeight;
+    final double y = fitsAbove


The comment here

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

Lines 81 to 83 in 9be3399

/// Anchor position of the toolbar, relative to the top left of the

/// [globalEditableRegion].

final Offset position;

Makes it really confusing. I guess instead of anchor it should say bottom left? And the fact that we use bottom left instead of top left sounds strange to me too.

I think the value passed in for position is where the center of the toolbar is drawn? If I pass in 0.0 for y, it draws the toolbar so that it's vertical midpoint is on the top of the textfield.

in _TextSelectionToolbarLayout.getPositionForChild I think _TextSelectionToolbarLayout.position is used as the coordinate of the bottom left vertex of the toolbar, in global coordinate system.

Hmm I don't fully understand what that value is meant to be. Do you think I should update the comment to something specific? This is what I see when I set position to 0,0:

Ah sorry I was just ranting because it threw me off, It's not really related to this PR.

Ok got it. Yeah it's definitely confusing.

LongCatIsLooong · 2019-07-26T20:58:50Z

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

+    final bool fitsAbove = toolbarHeightNeeded <= availableHeight;
+    final double y = fitsAbove
+        ? startTextSelectionPoint.point.dy - textLineHeight
+        : startTextSelectionPoint.point.dy + _kToolbarHeight + _kToolbarContentDistanceBelow;
    final Offset preciseMidpoint = Offset(x, y);


Not really related to the issue, but I think x is already calculated in TextSelectionOverlay._buildToolbar. Instead of doing it here again maybe just use position.dx?

Yeah seems to be calculating the same exact value to me. I'll change it.

justinmc · 2019-07-31T20:39:32Z

Waiting on a goldens file update in flutter/goldens#41

LongCatIsLooong · 2019-07-31T22:32:48Z

packages/flutter/test/material/text_field_test.dart

+      final Offset firstLineToolbarTopLeft = tester.getTopLeft(find.text('SELECT ALL'));
+      final Offset firstLineTopLeft = textOffsetToPosition(tester, testValue.indexOf('a'));
+      expect(firstLineTopLeft.dy, lessThan(firstLineToolbarTopLeft.dy));
+      expect(firstLineToolbarTopLeft.dy, greaterThan(firstLineTopLeft.dy));


Is this line saying the same thing as the line above?

Yes, good catch!

LongCatIsLooong · 2019-07-31T22:46:10Z

packages/flutter/test/material/text_field_test.dart

+    await tester.tapAt(tester.getCenter(find.byType(EditableText)));
+    await tester.pump();
+    await tester.pump(const Duration(milliseconds: 200)); // skip past the frame where the opacity is zero
+    await tester.pumpAndSettle();


Why do we need to pump a third time here?

I tried it and it seems to be unnecessary, so I'll remove it.

LongCatIsLooong

LGTM!

Piinks

LGTM!

Piinks · 2019-08-01T17:16:04Z

packages/flutter/test/material/text_field_test.dart

@@ -499,7 +499,7 @@ void main() {
      find.byType(Overlay),
      matchesGoldenFile(
        'text_field_opacity_test.0.png',
-        version: 2,
+        version: 3,


Find values that look right for material selection menu above/below w…

c924a47

…hen multiline

justinmc added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. work in progress; do not review labels Jul 25, 2019

justinmc self-assigned this Jul 25, 2019

fluttergithubbot added the f: material design flutter/packages/flutter/material repository. label Jul 25, 2019

googlebot added the cla: yes label Jul 25, 2019

justinmc added 2 commits July 26, 2019 09:25

Fix position for lower lines of multiline inputs

cd24d72

Test position of selection menu in multiline layout

0c35693

justinmc changed the title ~~(WIP) Multiline Selection Menu Position Bug~~ Multiline Selection Menu Position Bug Jul 26, 2019

justinmc removed the work in progress; do not review label Jul 26, 2019

justinmc requested a review from LongCatIsLooong July 26, 2019 19:38

Expect position of toolbars at each line relative to each other for a…

9be3399

…dded specificity

justinmc force-pushed the muiltiline-selection-toolbar-position branch from 0d565af to 9be3399 Compare July 26, 2019 20:00

LongCatIsLooong reviewed Jul 26, 2019

View reviewed changes

justinmc added 3 commits July 30, 2019 08:34

Move material selection toolbar slightly up to match native

7592cd4

Remove duplicate x calculation

0229b29

Remove unused variable

1172395

justinmc mentioned this pull request Jul 31, 2019

Material Selection Menu Position flutter/goldens#41

Merged

LongCatIsLooong reviewed Jul 31, 2019

View reviewed changes

LongCatIsLooong approved these changes Jul 31, 2019

View reviewed changes

Update golden image for material text selection menu position

d3d9f23

Piinks approved these changes Aug 1, 2019

View reviewed changes

Code review fixes

ef38697

justinmc merged commit 5c48d90 into flutter:master Aug 2, 2019

justinmc deleted the muiltiline-selection-toolbar-position branch August 2, 2019 22:40

justinmc mentioned this pull request Aug 2, 2019

Copy/paste toolbar needs smarter positioning, same issue in Flutter version 1.78 #37501

Closed

justinmc mentioned this pull request Aug 12, 2019

Textfield context menu is not positioned correctly if it has more than two line. #32943

Closed

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

	/// Anchor position of the toolbar, relative to the top left of the
	/// [globalEditableRegion].
	final Offset position;

Multiline Selection Menu Position Bug #36974

Multiline Selection Menu Position Bug #36974

Uh oh!

Conversation

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

Uh oh!

Description

Related Issues

Tests

Uh oh!

fluttergithubbot commented Jul 25, 2019

Uh oh!

justinmc commented Jul 26, 2019

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!

justinmc Jul 30, 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!

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!

justinmc commented Jul 31, 2019

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!

LongCatIsLooong left a comment

Choose a reason for hiding this comment

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

justinmc commented Jul 25, 2019 •

edited

Loading

justinmc Jul 30, 2019 •

edited

Loading