Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fixes TextInputFormatter gets wrong old value of a selection #75541

Merged
merged 4 commits into from Feb 19, 2021
Merged

fixes TextInputFormatter gets wrong old value of a selection #75541

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4913dd8
refactor text selection overlay and selection update
chunhtai Feb 6, 2021
96283ab
addressing comments
chunhtai Feb 18, 2021
b00c26c
update comments
chunhtai Feb 18, 2021
8868819
update
chunhtai Feb 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/flutter/lib/src/cupertino/text_field.dart
View file
Open in desktop
Expand Up @@ -102,6 +102,7 @@ class _CupertinoTextFieldSelectionGestureDetectorBuilder extends TextSelectionGe

@override
void onSingleTapUp(TapUpDetails details) {
editableText.hideToolbar();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have this in Material Textfield but not cupertinotextfield, This is to deal with the case where there are toolbar open and user tap the text outside of the current selection. The expect behavior is the tool bar will be closed and the collapsed selection moved to a new place with the toolbar remaining closed.

The workflow is as the following:

onSingleTapUp(hide toolbar) -> EditableTextOnselectionChange(update toolbar) -> composing changes(hide toolbar)

In theory, if we don't add this hide toolbar, you should see the selection overlay gets moved to the new location with the toolbar open. the toolbar will then be hidden after a frame delay.

This works prior to this pr in a weird way. When user tap and the toolbar will be rebuild with the new selection, but that will only show up after one frame delay. right before the frame end, the selection change again probably due to composing changes. This will hide the toolbar before it builds.

In short, we have a selection overlay built and destroyed in the same frame.

My PR optimizes the selection change that it won't need to rebuild the selection overlay every time. This breaks because the overlay will get updated immediately before the hide toolbar remove the overlay on next frame. Visually, you will see the selection change to the new place with the toolbar still open, and the toolbar will close after one frame.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also part of the reason why i think we should move the selection overlay out of editable text.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm on board with all of this, I think we're moving in the right direction. Currently, our behavior for showing/hiding the text selection toolbar is incorrect in a bunch of cases already (see #48434 (comment)). Once we've been able to refactor more I hope we can get it all right.

// Because TextSelectionGestureDetector listens to taps that happen on
// widgets in front of it, tapping the clear button will also trigger
// this handler. If the clear button widget recognizes the up event,
Expand Down
8 changes: 6 additions & 2 deletions packages/flutter/lib/src/material/selectable_text.dart
View file
Open in desktop
Expand Up @@ -494,17 +494,21 @@ class _SelectableTextState extends State<SelectableText> with AutomaticKeepAlive
});
}

TextSelection? _lastSeenTextSelection;

void _handleSelectionChanged(TextSelection selection, SelectionChangedCause? cause) {
final bool willShowSelectionHandles = _shouldShowSelectionHandles(cause);
if (willShowSelectionHandles != _showSelectionHandles) {
setState(() {
_showSelectionHandles = willShowSelectionHandles;
});
}

if (widget.onSelectionChanged != null) {
// TODO(chunhtai): The selection may be the same. We should remove this
// check once this is fixed https://github.com/flutter/flutter/issues/76349.
if (widget.onSelectionChanged != null && _lastSeenTextSelection != selection) {
widget.onSelectionChanged!(selection, cause);
}
_lastSeenTextSelection = selection;

switch (Theme.of(context).platform) {
case TargetPlatform.iOS:
Expand Down
151 changes: 59 additions & 92 deletions packages/flutter/lib/src/rendering/editable.dart
View file
Open in desktop
Expand Up @@ -31,39 +31,13 @@ const Radius _kFloatingCaretRadius = Radius.circular(1.0);
/// (including the cursor location).
///
/// Used by [RenderEditable.onSelectionChanged].
@Deprecated(
'Signature of a deprecated class method, '
'textSelectionDelegate.userUpdateTextEditingValue. '
'This feature was deprecated after v1.26.0-17.2.pre.'
)
typedef SelectionChangedHandler = void Function(TextSelection selection, RenderEditable renderObject, SelectionChangedCause cause);

/// Indicates what triggered the change in selected text (including changes to
/// the cursor location).
enum SelectionChangedCause {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a dependency loop or something if you keep this here and import it from text_input.dart?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that will be a layer violation, text_input.dart is in service layer

/// The user tapped on the text and that caused the selection (or the location
/// of the cursor) to change.
tap,

/// The user tapped twice in quick succession on the text and that caused
/// the selection (or the location of the cursor) to change.
doubleTap,

/// The user long-pressed the text and that caused the selection (or the
/// location of the cursor) to change.
longPress,

/// The user force-pressed the text and that caused the selection (or the
/// location of the cursor) to change.
forcePress,

/// The user used the keyboard to change the selection or the location of the
/// cursor.
///
/// Keyboard-triggered selection changes may be caused by the IME as well as
/// by accessibility tools (e.g. TalkBack on Android).
keyboard,

/// The user used the mouse to change the selection by dragging over a piece
/// of text.
drag,
}

/// Signature for the callback that reports when the caret location changes.
///
/// Used by [RenderEditable.onCaretChanged].
Expand Down Expand Up @@ -158,10 +132,6 @@ bool _isWhitespace(int codeUnit) {
/// If, when the render object paints, the caret is found to have changed
/// location, [onCaretChanged] is called.
///
/// The user may interact with the render object by tapping or long-pressing.
/// When the user does so, the selection is updated, and [onSelectionChanged] is
/// called.
///
/// Keyboard handling, IME handling, scrolling, toggling the [showCursor] value
/// to actually blink the cursor, and other features not mentioned above are the
/// responsibility of higher layers and not handled by this object.
Expand Down Expand Up @@ -198,6 +168,10 @@ class RenderEditable extends RenderBox with RelayoutWhenSystemFontsChangeMixin {
double textScaleFactor = 1.0,
TextSelection? selection,
required ViewportOffset offset,
@Deprecated(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need another @Deprecated down on the onSelectionChanged property as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will onSelectionChanged continue to function as it did before until we remove this deprecation? It looks like it may not be called anymore after this PR, or at least the behavior may change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good now 👍

'Uses the textSelectionDelegate.userUpdateTextEditingValue instead. '
'This feature was deprecated after v1.26.0-17.2.pre.'
)
this.onSelectionChanged,
this.onCaretChanged,
this.ignorePointer = false,
Expand Down Expand Up @@ -401,6 +375,10 @@ class RenderEditable extends RenderBox with RelayoutWhenSystemFontsChangeMixin {
/// Called when the selection changes.
///
/// If this is null, then selection changes will be ignored.
@Deprecated(
'Uses the textSelectionDelegate.userUpdateTextEditingValue instead. '
'This feature was deprecated after v1.26.0-17.2.pre.'
)
SelectionChangedHandler? onSelectionChanged;

double? _textLayoutLastMaxWidth;
Expand Down Expand Up @@ -579,7 +557,19 @@ class RenderEditable extends RenderBox with RelayoutWhenSystemFontsChangeMixin {
// down in a multiline text field when selecting using the keyboard.
bool _wasSelectingVerticallyWithKeyboard = false;

// Call through to onSelectionChanged.
void _setTextEditingValue(TextEditingValue newValue, SelectionChangedCause cause) {
textSelectionDelegate.textEditingValue = newValue;
textSelectionDelegate.userUpdateTextEditingValue(newValue, cause);
}

void _setSelection(TextSelection nextSelection, SelectionChangedCause cause) {
_handleSelectionChange(nextSelection, cause);
_setTextEditingValue(
textSelectionDelegate.textEditingValue.copyWith(selection: nextSelection),
cause,
);
}

void _handleSelectionChange(
TextSelection nextSelection,
SelectionChangedCause cause,
Expand Down Expand Up @@ -642,7 +632,7 @@ class RenderEditable extends RenderBox with RelayoutWhenSystemFontsChangeMixin {
return;
}

if (keyEvent is! RawKeyDownEvent || onSelectionChanged == null)
if (keyEvent is! RawKeyDownEvent)
return;
final Set<LogicalKeyboardKey> keysPressed = LogicalKeyboardKey.collapseSynonyms(RawKeyboard.instance.keysPressed);
final LogicalKeyboardKey key = keyEvent.logicalKey;
Expand Down Expand Up @@ -908,12 +898,10 @@ class RenderEditable extends RenderBox with RelayoutWhenSystemFontsChangeMixin {
newSelection = TextSelection.fromPosition(TextPosition(offset: newOffset));
}

_handleSelectionChange(
_setSelection(
newSelection,
SelectionChangedCause.keyboard,
);
// Update the text selection delegate so that the engine knows what we did.
textSelectionDelegate.textEditingValue = textSelectionDelegate.textEditingValue.copyWith(selection: newSelection);
}

// Handles shortcut functionality including cut, copy, paste and select all
Expand Down Expand Up @@ -961,13 +949,10 @@ class RenderEditable extends RenderBox with RelayoutWhenSystemFontsChangeMixin {
);
}
if (value != null) {
if (textSelectionDelegate.textEditingValue.selection != value.selection) {
_handleSelectionChange(
value.selection,
SelectionChangedCause.keyboard,
);
}
textSelectionDelegate.textEditingValue = value;
_setTextEditingValue(
value,
SelectionChangedCause.keyboard,
);
}
}

Expand All @@ -994,15 +979,12 @@ class RenderEditable extends RenderBox with RelayoutWhenSystemFontsChangeMixin {
}
}
final TextSelection newSelection = TextSelection.collapsed(offset: cursorPosition);
if (selection != newSelection) {
_handleSelectionChange(
newSelection,
SelectionChangedCause.keyboard,
);
}
textSelectionDelegate.textEditingValue = TextEditingValue(
text: textBefore + textAfter,
selection: newSelection,
_setTextEditingValue(
TextEditingValue(
text: textBefore + textAfter,
selection: newSelection,
),
SelectionChangedCause.keyboard,
);
}

Expand Down Expand Up @@ -1530,7 +1512,7 @@ class RenderEditable extends RenderBox with RelayoutWhenSystemFontsChangeMixin {
// callbacks are invoked, in which case the callbacks will crash...

void _handleSetSelection(TextSelection selection) {
_handleSelectionChange(selection, SelectionChangedCause.keyboard);
_setSelection(selection, SelectionChangedCause.keyboard);
}

void _handleMoveCursorForwardByCharacter(bool extentSelection) {
Expand All @@ -1539,8 +1521,9 @@ class RenderEditable extends RenderBox with RelayoutWhenSystemFontsChangeMixin {
if (extentOffset == null)
return;
final int baseOffset = !extentSelection ? extentOffset : selection!.baseOffset;
_handleSelectionChange(
TextSelection(baseOffset: baseOffset, extentOffset: extentOffset), SelectionChangedCause.keyboard,
_setSelection(
TextSelection(baseOffset: baseOffset, extentOffset: extentOffset),
SelectionChangedCause.keyboard,
);
}

Expand All @@ -1550,8 +1533,9 @@ class RenderEditable extends RenderBox with RelayoutWhenSystemFontsChangeMixin {
if (extentOffset == null)
return;
final int baseOffset = !extentSelection ? extentOffset : selection!.baseOffset;
_handleSelectionChange(
TextSelection(baseOffset: baseOffset, extentOffset: extentOffset), SelectionChangedCause.keyboard,
_setSelection(
TextSelection(baseOffset: baseOffset, extentOffset: extentOffset),
SelectionChangedCause.keyboard
);
}

Expand All @@ -1562,7 +1546,7 @@ class RenderEditable extends RenderBox with RelayoutWhenSystemFontsChangeMixin {
if (nextWord == null)
return;
final int baseOffset = extentSelection ? selection!.baseOffset : nextWord.start;
_handleSelectionChange(
_setSelection(
TextSelection(
baseOffset: baseOffset,
extentOffset: nextWord.start,
Expand All @@ -1578,12 +1562,12 @@ class RenderEditable extends RenderBox with RelayoutWhenSystemFontsChangeMixin {
if (previousWord == null)
return;
final int baseOffset = extentSelection ? selection!.baseOffset : previousWord.start;
_handleSelectionChange(
_setSelection(
TextSelection(
baseOffset: baseOffset,
extentOffset: previousWord.start,
),
SelectionChangedCause.keyboard,
SelectionChangedCause.keyboard
);
}

Expand Down Expand Up @@ -1894,7 +1878,7 @@ class RenderEditable extends RenderBox with RelayoutWhenSystemFontsChangeMixin {
textSpan.recognizer?.addPointer(event);
}

if (!ignorePointer && onSelectionChanged != null) {
if (!ignorePointer) {
// Propagates the pointer event to selection handlers.
_tap.addPointer(event);
_longPress.addPointer(event);
Expand Down Expand Up @@ -1992,9 +1976,6 @@ class RenderEditable extends RenderBox with RelayoutWhenSystemFontsChangeMixin {
assert(cause != null);
assert(from != null);
_layoutText(minWidth: constraints.minWidth, maxWidth: constraints.maxWidth);
if (onSelectionChanged == null) {
return;
}
final TextPosition fromPosition = _textPainter.getPositionForOffset(globalToLocal(from - _paintOffset));
final TextPosition? toPosition = to == null
? null
Expand All @@ -2008,8 +1989,7 @@ class RenderEditable extends RenderBox with RelayoutWhenSystemFontsChangeMixin {
extentOffset: extentOffset,
affinity: fromPosition.affinity,
);
// Call [onSelectionChanged] only when the selection actually changed.
_handleSelectionChange(newSelection, cause);
_setSelection(newSelection, cause);
}

/// Select a word around the location of the last tap down.
Expand All @@ -2029,22 +2009,16 @@ class RenderEditable extends RenderBox with RelayoutWhenSystemFontsChangeMixin {
assert(cause != null);
assert(from != null);
_layoutText(minWidth: constraints.minWidth, maxWidth: constraints.maxWidth);
if (onSelectionChanged == null) {
return;
}
final TextPosition firstPosition = _textPainter.getPositionForOffset(globalToLocal(from - _paintOffset));
final TextSelection firstWord = _selectWordAtOffset(firstPosition);
final TextSelection lastWord = to == null ?
firstWord : _selectWordAtOffset(_textPainter.getPositionForOffset(globalToLocal(to - _paintOffset)));

_handleSelectionChange(
TextSelection(
baseOffset: firstWord.base.offset,
extentOffset: lastWord.extent.offset,
affinity: firstWord.affinity,
),
cause,
final TextSelection newSelection = TextSelection(
baseOffset: firstWord.base.offset,
extentOffset: lastWord.extent.offset,
affinity: firstWord.affinity,
);
_setSelection(newSelection, cause);
}

/// Move the selection to the beginning or end of a word.
Expand All @@ -2054,22 +2028,15 @@ class RenderEditable extends RenderBox with RelayoutWhenSystemFontsChangeMixin {
assert(cause != null);
_layoutText(minWidth: constraints.minWidth, maxWidth: constraints.maxWidth);
assert(_lastTapDownPosition != null);
if (onSelectionChanged == null) {
return;
}
final TextPosition position = _textPainter.getPositionForOffset(globalToLocal(_lastTapDownPosition! - _paintOffset));
final TextRange word = _textPainter.getWordBoundary(position);
late TextSelection newSelection;
if (position.offset - word.start <= 1) {
_handleSelectionChange(
TextSelection.collapsed(offset: word.start, affinity: TextAffinity.downstream),
cause,
);
newSelection = TextSelection.collapsed(offset: word.start, affinity: TextAffinity.downstream);
} else {
_handleSelectionChange(
TextSelection.collapsed(offset: word.end, affinity: TextAffinity.upstream),
cause,
);
newSelection = TextSelection.collapsed(offset: word.end, affinity: TextAffinity.upstream);
}
_setSelection(newSelection, cause);
}

TextSelection _selectWordAtOffset(TextPosition position) {
Expand Down