Fix Text selection handler disappear when dragged to new words by xu-baolin · Pull Request #108808 · flutter/flutter

xu-baolin · 2022-08-02T12:39:11Z

Issue 1 - Tap the word the handler appear randomly

1, Tap on the middle of 'Hello'
2, Local value update to

TextEditingValue(text: ┤Hello World├, selection: TextSelection.collapsed(offset: 2, affinity: TextAffinity.upstream, isDirectional: false), composing: TextRange(start: -1, end: -1))

3, Send the new local value to remote, and then, the auto correction will update the composing back to framework

TextEditingValue(text: ┤Hello World├, selection: TextSelection.collapsed(offset: 2, affinity: TextAffinity.downstream, isDirectional: false), composing: TextRange(start: 0, end: 2))

But the affinity will mutate randomly. I'm not sure if this is a problem with the engine, but I understand that in this scenario, affinity is meaningless.

If the affinity was mutate, the handle will be hide by _TextFieldState._handleSelectionChanged because the cause is keyboard.

Issue 2 - The handler can not drag to a new word

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

Lines 2148 to 2149 in 2d771d4

    
           hideToolbar(); 
        
           _currentPromptRectRange = null;

This is because we remove the handler overlay during drag because the engine update the composing during drag.

I found that we can remove the line, the handler's visibility will be updated by _TextFieldState._handleSelectionChanged CC @LongCatIsLooong

flutter-dashboard · 2022-08-02T12:39:15Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

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

xu-baolin · 2022-08-02T12:41:27Z

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

I know this a little hack.
If we can figure out why the engine change the affinity maybe we have a better solution.
CC @LongCatIsLooong

The engine doesn't know/care about text affinity I believe. It always sets it to downstream.

I have correct the value when receive the remote value. WDYT?

xu-baolin · 2022-08-03T03:31:17Z

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

Could you double check deleting this is safety? @LongCatIsLooong THX : )

In UIKit/Android apps, the text edit menu should go away once the user starts typing. Would this change that behavior?

Will add a test to cover this.

Ah _handleSelectionChanged is called in _formatAndSave too, that seems to be a messy callpath. Do you mind adding a comment to explain why we're not hiding the handles here (or link the issue)?

xu-baolin · 2022-08-09T02:40:33Z

@LongCatIsLooong @justinmc Hi, guys, I think this is ready for review :)

LongCatIsLooong · 2022-08-09T16:48:19Z

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

Do you still want to revert the affinity change if the user actually typed something?

As you point out that the input server never know the actually affinity and can not change the affinity by key board.
Currently we can only change the affinity by tap the text string and change the selection.
Additional, the affinity is only valid when line breaks cause by render wrapping, not explicit newline characters.

Ah sorry I misspoke, the engine doesn't care but the framework does (for rendering the caret at the right place).

As far as I can tell on iOS/Android when the user types using a virtual/hardware keyboard the affinity is always set to TextAffinity.downstream. TextAffinity.upstream is only used when the selection is change by certain gestures when the caret position is located at a soft-wrap line boundary, or bidi boundary. For example when the text field needs to show

aa| bb

instead of

aa |bb

Absolutely correct, so we can override the affinity from engine, right?
Otherwise, the only affinity change will trigger selection change and dismiss the handler by mistake.

Say the line can only accommodate 2 characters, and the text editing state was initially
(text: 'a', textSelection.collapsed(1, affinity: upstream)), when the user types another a the affinity should be changed to downstream, so the framework knows the caret should be rendered at the start of line 2?

Could you explain why? The documentation says that affinity only makes sense when there is ambiguity in the visual location, and is used to paint the caret. Am I miss something?

When the user types something, or changes the selection using a keyboard, the caret typically shouldn't be painted using the upstream affinity. In other words it should be

aa |bb

instead of

aa| bb

I almost understand what you mean.
Another case, if the aa| is composed, and I wanna to type aaa| with composed, it should be painted using upstream affinity?

That I'm not sure. On Android it's behaving like it's upstream. But I vaguely remember there's no cursor affinity on Android.

LongCatIsLooong · 2022-08-09T16:51:51Z

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

In UIKit/Android apps, the text edit menu should go away once the user starts typing. Would this change that behavior?

justinmc

Thanks for the PR! I was thinking this might have needed a tricky engine fix, I'm happy if that's not true.

Can you check if this fixes/improves #84849?

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

packages/flutter/test/material/text_field_test.dart

xu-baolin · 2022-08-15T03:16:26Z

Can you check if this fixes/improves #84849?

Yes, this indeed fixes #84849 too.

alexmercerind · 2022-08-15T08:42:28Z

Can you check if this fixes/improves #84849?

Yes, this indeed fixes #84849 too.

Thanks a lot for your work!

justinmc

LGTM 👍 Thanks again!

I tried running this locally and it does indeed appear to fix both of those issues!

justinmc · 2022-08-15T16:26:43Z

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

+        value.selection.affinity != _value.selection.affinity) {
+      return true;
+    }
+    return false;


Could you get rid of the if and just return the condition directly?

flutter/flutter#108808)

flutter-dashboard bot added a: text input Entering text in a text field or keyboard related problems f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Aug 2, 2022

xu-baolin requested a review from LongCatIsLooong August 2, 2022 12:39

xu-baolin commented Aug 2, 2022

View reviewed changes

xu-baolin commented Aug 3, 2022

View reviewed changes

LongCatIsLooong reviewed Aug 9, 2022

View reviewed changes

xu-baolin force-pushed the xbl000802 branch from 59f454d to 53c0881 Compare August 10, 2022 03:38

xu-baolin requested a review from LongCatIsLooong August 10, 2022 03:40

justinmc mentioned this pull request Aug 12, 2022

Input connection reinitiated when handles are dragged #84849

Closed

justinmc reviewed Aug 12, 2022

View reviewed changes

packages/flutter/lib/src/widgets/editable_text.dart Outdated Show resolved Hide resolved

packages/flutter/test/material/text_field_test.dart Outdated Show resolved Hide resolved

packages/flutter/test/material/text_field_test.dart Outdated Show resolved Hide resolved

xu-baolin added 7 commits August 15, 2022 10:58

++

012c865

++

a229150

++

2cf735b

add test

c963599

review feedback

03c54aa

fix test

ab95190

code review feedback

b36d3c8

code review feedback

d9bbdc9

xu-baolin force-pushed the xbl000802 branch from b13789f to d9bbdc9 Compare August 15, 2022 03:39

xu-baolin requested a review from justinmc August 15, 2022 03:43

feedback

3f25d2c

justinmc approved these changes Aug 15, 2022

View reviewed changes

xu-baolin mentioned this pull request Aug 16, 2022

TextAffinity has a mutation when changing the selection, #65813

Closed

feedback

4a97251

xu-baolin added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 16, 2022

auto-submit bot merged commit 44f87da into flutter:master Aug 16, 2022

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2022

44f87da Fix Text selection handler disappear when dragged to new words (

f5402ed

flutter/flutter#108808)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 17, 2022

44f87da Fix Text selection handler disappear when dragged to new words (

6c897f0

flutter/flutter#108808)

Conversation

xu-baolin commented Aug 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue 1 - Tap the word the handler appear randomly

Issue 2 - The handler can not drag to a new word

Uh oh!

flutter-dashboard bot commented Aug 2, 2022

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!

xu-baolin commented Aug 9, 2022

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 Aug 12, 2022 • 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!

xu-baolin Aug 15, 2022 • 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!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xu-baolin commented Aug 15, 2022

Uh oh!

alexmercerind commented Aug 15, 2022

Uh oh!

justinmc left a comment

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

xu-baolin commented Aug 2, 2022 •

edited

Loading

LongCatIsLooong Aug 12, 2022 •

edited

Loading

xu-baolin Aug 15, 2022 •

edited

Loading