Text selection menu show/hide cases #35219

justinmc · 2019-06-27T21:18:57Z

Description

There are various situations, described in the linked issues, where the text selection menu is not shown/hidden when it should be. This PR aims to get all cases right.

Related Issues

Closes #34215
Closes #33293
Probably should close #35837 too

…yness

…eing visible

justinmc · 2019-07-01T21:07:22Z

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

    }
    _requestKeyboard();
    _confirmCurrentSplash();
+    _editableTextKey.currentState.hideToolbar();


Fix 1/2: This fixes the problem where the selection menu could get stuck open. I had to add a check to hideToolbar to make sure that we're not hiding a toolbar that's already hidden, but it seems like that probably should have been there anyway.

It might be safer to turn it off at the start of the function? although I don't think we turn on toolbar in selectPosition

I think I like putting it at the top more, I'll make that change. It seems to work fine.

justinmc · 2019-07-01T21:07:24Z

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

      if (!_value.selection.isValid) {
        // Place cursor at the end if the selection is invalid when we receive focus.
-        widget.controller.selection = TextSelection.collapsed(offset: _value.text.length);
+        _handleSelectionChanged(TextSelection.collapsed(offset: _value.text.length), renderEditable, SelectionChangedCause.tap);


Fix 2/2: This fixes the case where an autofocused field couldn't ever display its selection menu. The reason was that the menu was never created.

However, this had an implication in our tests. I had to wrap a bunch of pumped test widgets in MaterialApp, because otherwise I was getting an error that there was no parent Overlay widget when it tried to create the selection menu. See the changes below.

Also, I just remembered that I used SelectionChangedCause.tap and it felt a bit hacky. This isn't isn't caused by a tap. Really, it's called when the widget is rebuilt and is automatically focused. Should I add a new value to the SelectionChangedCause enum?

I feel a bit hacky too, but I can't think of a good cause either. I wonder if it make sense to leave it null?

Good idea. I thought it couldn't be null for some reason, but it seems to work fine.

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

packages/flutter/test/material/text_field_test.dart

chunhtai · 2019-07-01T23:38:26Z

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

      if (!_value.selection.isValid) {
        // Place cursor at the end if the selection is invalid when we receive focus.
-        widget.controller.selection = TextSelection.collapsed(offset: _value.text.length);
+        _handleSelectionChanged(TextSelection.collapsed(offset: _value.text.length), renderEditable, SelectionChangedCause.tap);


I feel a bit hacky too, but I can't think of a good cause either. I wonder if it make sense to leave it null?

chunhtai · 2019-07-01T23:41:37Z

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

    }
    _requestKeyboard();
    _confirmCurrentSplash();
+    _editableTextKey.currentState.hideToolbar();


It might be safer to turn it off at the start of the function? although I don't think we turn on toolbar in selectPosition

chunhtai · 2019-07-03T17:49:35Z

Can you also add a regression test regarding #34215 ?

justinmc · 2019-07-03T18:23:39Z

Good call, I totally forgot to test those cases. I will add a couple to make sure they're covered.

justinmc · 2019-07-03T19:38:31Z

@chunhtai Updated with the tests.

chunhtai · 2019-07-09T00:10:45Z

packages/flutter/test/cupertino/text_field_test.dart

      );
    });
+
+    testWidgets(


This test is passing without the patch. Is the original issue already resolved?

After looking into this, the "single tap hides" test was passing because of strangeness with the tap location. Using a direct longPress(textFieldFinder) made the test fail on master and pass in this branch.

For the other test, "long press on autofocused", I could not get it to fail on master no matter what I tried. However, trying a sample app, the behavior looked as if the test should definitely fail. I gave up trying to make it fail and instead wrote an integration test that fails on master and passes on this branch.

I think i know what is going on.
When you use longPressAt(textOffsetToPosition(tester, 0)), it will tap at the start of the text which is also part of the selection handle. this will trigger _handleSelectionHandleTapped which toggle the toolbar off. That is why it still passing in master.

As for long press on autofocused, add an additional pump after the pump widget will fail the test as the on focus callback will update the selection from the default (-1, -1) to (0,0) but it take one frame to propagate down to renderEditable. While the long press started, the renderEditable is still has selection(-1,-1) which allow it to update its selection

Brilliant, that did in fact make it fail on master. Thanks so much. I will make that change and then remove the e2e test (since it's much slower).

justinmc · 2019-07-09T17:55:39Z

@chunhtai Updated with tests that fail on master.

justinmc · 2019-07-09T22:40:31Z

@chunhtai Updated with your fix for the test that wasn't failing on master, thanks!

chunhtai · 2019-07-10T16:36:50Z

packages/flutter/test/cupertino/text_field_test.dart

it requires a pump here

chunhtai · 2019-07-10T16:38:24Z

packages/flutter/test/material/text_field_test.dart

justinmc · 2019-07-10T18:16:24Z

@chunhtai Whoops sorry, I think I lost the pumps I added when I was switching to master to try it out. Fixed.

chunhtai

LGTM with a minor suggestion

chunhtai · 2019-07-10T18:25:13Z

packages/flutter/test/material/text_field_test.dart

nit: It might be more clear to add a comment describe why we need a pump here

chunhtai · 2019-07-10T18:25:53Z

packages/flutter/test/cupertino/text_field_test.dart

nit: same as below.

… master

- flutter#35219 tried to resolve this and autofocus, but missed the usecase where the user may empty out the field and then try to long-press to paste something. - When one taps on an empty text field, the long-press menu works, because: selection = TextSelection( baseOffset: -1, extentOffset: -1, affinity: TextAffinity.downstream, isDirectional: false, ) nextSelection = TextSelection( baseOffset: 0, extentOffset: 0, affinity: TextAffinity.downstream, isDirectional: false, ) - When one clears the text in a text field and then long-presses however, the menu doesn't appear because: selection = nextSelection = TextSelection( baseOffset: 0, extentOffset: 0, affinity: TextAffinity.downstream, isDirectional: false, ) and _handlePotentialSelectionChange() returns. - Check if baseOffset and extentOffset of current and next are both 0, and set current to -1, to replicate working behaviour.

Show and hide the text selection menu at the correct time with various gestures in the text field.

Allow paste menu to be shown in autofocused field, worried about hack…

c096398

…yness

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 Jun 27, 2019

justinmc self-assigned this Jun 27, 2019

googlebot added the cla: yes label Jun 27, 2019

Hide the toolbar when tapping

6c36f17

HansMuller added the f: material design flutter/packages/flutter/material repository. label Jun 28, 2019

justinmc added 2 commits July 1, 2019 12:41

Don't hide toolbar unless it's currently visible. Fixes handles not b…

8b349e6

…eing visible

Fix tests complaining about no Overlay

641a5c3

justinmc changed the title ~~WIP Text selection menu show/hide cases~~ Text selection menu show/hide cases Jul 1, 2019

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

justinmc requested a review from HansMuller July 1, 2019 20:59

justinmc commented Jul 1, 2019

View reviewed changes

chunhtai reviewed Jul 1, 2019

View reviewed changes

Code review fixes

63fc305

Test all fixed cases

77e75ad

justinmc force-pushed the show-selection-menu branch from 4ed393d to 77e75ad Compare July 3, 2019 19:39

chunhtai reviewed Jul 9, 2019

View reviewed changes

justinmc added 2 commits July 9, 2019 10:41

Integration test for autofocus selection menu bug

3272fe2

Test fails on master

8714840

chunhtai reviewed Jul 10, 2019

View reviewed changes

justinmc force-pushed the show-selection-menu branch from 0b208df to a22d636 Compare July 10, 2019 18:15

chunhtai approved these changes Jul 10, 2019

View reviewed changes

Test autofocus with widget test instead of e2e in a way that fails on…

147ab98

… master

justinmc force-pushed the show-selection-menu branch from a22d636 to 147ab98 Compare July 10, 2019 18:30

justinmc merged commit ff53fbe into flutter:master Jul 10, 2019

justinmc deleted the show-selection-menu branch July 10, 2019 21:08

LongCatIsLooong mentioned this pull request Jul 13, 2019

TextField toolbars (Material & Cupertino) do not show up when using nested MaterialApps #36124

Closed

johnsonmh pushed a commit to johnsonmh/flutter that referenced this pull request Jul 30, 2019

Text selection menu show/hide cases (flutter#35219)

ed63fc4

Show and hide the text selection menu at the correct time with various gestures in the text field.

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

Text selection menu show/hide cases #35219

Text selection menu show/hide cases #35219

Uh oh!

Conversation

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

Uh oh!

Description

Related Issues

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai commented Jul 3, 2019

Uh oh!

justinmc commented Jul 3, 2019

Uh oh!

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

justinmc commented Jul 9, 2019

Uh oh!

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

justinmc commented Jul 10, 2019

Uh oh!

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

justinmc commented Jun 27, 2019 •

edited

Loading

justinmc commented Jul 9, 2019 •

edited

Loading