Skip to content

Commit

Permalink
Fix the breaking of multi-code-unit characters in obscure mode (#123366)
Browse files Browse the repository at this point in the history 
This PR changes the character boundary behavior of obscured fields to be based on code points instead of code units.

So it used to be possible to traverse and delete obscured text inside of code points (and breaking a code point like that would cause a crash):

![output2](https://github.com/flutter/flutter/assets/389558/674c89a4-c47d-4cdc-a402-4cadb5d2f73b)

But now moving the cursor and deleting is based on code points:

![output1](https://github.com/flutter/flutter/assets/389558/e46301f7-b5af-48d2-812a-0ad649f1383b)

### Native behavior

Native iOS deletes part of the emoji, native Mac deletes the whole emoji.  See flutter/flutter#122381 (comment).  So it's unclear what the desired behavior should actually be.

### Resources

Fixes flutter/flutter#122381

I thought this might not fix the case where a broken emoji is directly pasted into the field, but it seems to work by trying this:  �������

CC @LongCatIsLooong
  • Loading branch information
@justinmc
justinmc authored May 23, 2023
1 parent ffbeb72 commit 4341ed4
Show file tree
Hide file tree
Showing 5 changed files with 509 additions and 28 deletions.
45 changes: 31 additions & 14 deletions packages/flutter/lib/src/painting/text_painter.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,11 +170,11 @@ class WordBoundary extends TextBoundary {
// single code point that represents a supplementary character.
static int _codePointFromSurrogates(int highSurrogate, int lowSurrogate) {
assert(
TextPainter._isHighSurrogate(highSurrogate),
TextPainter.isHighSurrogate(highSurrogate),
'U+${highSurrogate.toRadixString(16).toUpperCase().padLeft(4, "0")}) is not a high surrogate.',
);
assert(
TextPainter._isLowSurrogate(lowSurrogate),
TextPainter.isLowSurrogate(lowSurrogate),
'U+${lowSurrogate.toRadixString(16).toUpperCase().padLeft(4, "0")}) is not a low surrogate.',
);
const int base = 0x010000 - (0xD800 << 10) - 0xDC00;
Expand Down Expand Up @@ -991,17 +991,34 @@ class TextPainter {
canvas.drawParagraph(_paragraph!, offset);
}

// Returns true iff the given value is a valid UTF-16 high surrogate. The value
// must be a UTF-16 code unit, meaning it must be in the range 0x0000-0xFFFF.
//
// See also:
// * https://en.wikipedia.org/wiki/UTF-16#Code_points_from_U+010000_to_U+10FFFF
static bool _isHighSurrogate(int value) {
// Returns true if value falls in the valid range of the UTF16 encoding.
static bool _isUTF16(int value) {
return value >= 0x0 && value <= 0xFFFFF;
}

/// Returns true iff the given value is a valid UTF-16 high (first) surrogate.
/// The value must be a UTF-16 code unit, meaning it must be in the range
/// 0x0000-0xFFFF.
///
/// See also:
/// * https://en.wikipedia.org/wiki/UTF-16#Code_points_from_U+010000_to_U+10FFFF
/// * [isLowSurrogate], which checks the same thing for low (second)
/// surrogates.
static bool isHighSurrogate(int value) {
assert(_isUTF16(value));
return value & 0xFC00 == 0xD800;
}

// Whether the given UTF-16 code unit is a low (second) surrogate.
static bool _isLowSurrogate(int value) {
/// Returns true iff the given value is a valid UTF-16 low (second) surrogate.
/// The value must be a UTF-16 code unit, meaning it must be in the range
/// 0x0000-0xFFFF.
///
/// See also:
/// * https://en.wikipedia.org/wiki/UTF-16#Code_points_from_U+010000_to_U+10FFFF
/// * [isHighSurrogate], which checks the same thing for high (first)
/// surrogates.
static bool isLowSurrogate(int value) {
assert(_isUTF16(value));
return value & 0xFC00 == 0xDC00;
}

Expand All @@ -1021,7 +1038,7 @@ class TextPainter {
return null;
}
// TODO(goderbauer): doesn't handle extended grapheme clusters with more than one Unicode scalar value (https://github.com/flutter/flutter/issues/13404).
return _isHighSurrogate(nextCodeUnit) ? offset + 2 : offset + 1;
return isHighSurrogate(nextCodeUnit) ? offset + 2 : offset + 1;
}

/// Returns the closest offset before `offset` at which the input cursor can
Expand All @@ -1032,7 +1049,7 @@ class TextPainter {
return null;
}
// TODO(goderbauer): doesn't handle extended grapheme clusters with more than one Unicode scalar value (https://github.com/flutter/flutter/issues/13404).
return _isLowSurrogate(prevCodeUnit) ? offset - 2 : offset - 1;
return isLowSurrogate(prevCodeUnit) ? offset - 2 : offset - 1;
}

// Unicode value for a zero width joiner character.
Expand All @@ -1052,7 +1069,7 @@ class TextPainter {
const int NEWLINE_CODE_UNIT = 10;

// Check for multi-code-unit glyphs such as emojis or zero width joiner.
final bool needsSearch = _isHighSurrogate(prevCodeUnit) || _isLowSurrogate(prevCodeUnit) || _text!.codeUnitAt(offset) == _zwjUtf16 || _isUnicodeDirectionality(prevCodeUnit);
final bool needsSearch = isHighSurrogate(prevCodeUnit) || isLowSurrogate(prevCodeUnit) || _text!.codeUnitAt(offset) == _zwjUtf16 || _isUnicodeDirectionality(prevCodeUnit);
int graphemeClusterLength = needsSearch ? 2 : 1;
List<TextBox> boxes = <TextBox>[];
while (boxes.isEmpty) {
Expand Down Expand Up @@ -1103,7 +1120,7 @@ class TextPainter {
final int nextCodeUnit = plainText.codeUnitAt(min(offset, plainTextLength - 1));

// Check for multi-code-unit glyphs such as emojis or zero width joiner
final bool needsSearch = _isHighSurrogate(nextCodeUnit) || _isLowSurrogate(nextCodeUnit) || nextCodeUnit == _zwjUtf16 || _isUnicodeDirectionality(nextCodeUnit);
final bool needsSearch = isHighSurrogate(nextCodeUnit) || isLowSurrogate(nextCodeUnit) || nextCodeUnit == _zwjUtf16 || _isUnicodeDirectionality(nextCodeUnit);
int graphemeClusterLength = needsSearch ? 2 : 1;
List<TextBox> boxes = <TextBox>[];
while (boxes.isEmpty) {
Expand Down
10 changes: 8 additions & 2 deletions packages/flutter/lib/src/services/text_boundary.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ abstract class TextBoundary {
/// `position`, or null if no boundaries can be found.
///
/// The return value, if not null, is usually less than or equal to `position`.
///
/// The range of the return value is given by the closed interval
/// `[0, string.length]`.
int? getLeadingTextBoundaryAt(int position) {
if (position < 0) {
return null;
Expand All @@ -39,10 +42,13 @@ abstract class TextBoundary {
return start >= 0 ? start : null;
}

/// Returns the offset of the closest text boundaries after the given `position`,
/// or null if there is no boundaries can be found after `position`.
/// Returns the offset of the closest text boundary after the given
/// `position`, or null if there is no boundary can be found after `position`.
///
/// The return value, if not null, is usually greater than `position`.
///
/// The range of the return value is given by the closed interval
/// `[0, string.length]`.
int? getTrailingTextBoundaryAt(int position) {
final int end = getTextBoundaryAt(max(0, position)).end;
return end >= 0 ? end : null;
Expand Down
75 changes: 65 additions & 10 deletions packages/flutter/lib/src/widgets/editable_text.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4246,7 +4246,7 @@ class EditableTextState extends State<EditableText> with AutomaticKeepAliveClien

// --------------------------- Text Editing Actions ---------------------------

TextBoundary _characterBoundary() => widget.obscureText ? _CodeUnitBoundary(_value.text) : CharacterBoundary(_value.text);
TextBoundary _characterBoundary() => widget.obscureText ? _CodePointBoundary(_value.text) : CharacterBoundary(_value.text);
TextBoundary _nextWordBoundary() => widget.obscureText ? _documentBoundary() : renderEditable.wordBoundaries.moveByWordBoundary;
TextBoundary _linebreak() => widget.obscureText ? _documentBoundary() : LineBoundary(renderEditable);
TextBoundary _paragraphBoundary() => ParagraphBoundary(_value.text);
Expand Down Expand Up @@ -5076,21 +5076,76 @@ class _ScribblePlaceholder extends WidgetSpan {
}
}

/// A text boundary that uses code units as logical boundaries.
/// A text boundary that uses code points as logical boundaries.
///
/// This text boundary treats every character in input string as an utf-16 code
/// unit. This can be useful when handling text without any grapheme cluster,
/// e.g. password input in [EditableText]. If you are handling text that may
/// include grapheme clusters, consider using [CharacterBoundary].
class _CodeUnitBoundary extends TextBoundary {
const _CodeUnitBoundary(this._text);
/// A code point represents a single character. This may be smaller than what is
/// represented by a user-perceived character, or grapheme. For example, a
/// single grapheme (in this case a Unicode extended grapheme cluster) like
/// "👨‍👩‍👦" consists of five code points: the man emoji, a zero
/// width joiner, the woman emoji, another zero width joiner, and the boy emoji.
/// The [String] has a length of eight because each emoji consists of two code
/// units.
///
/// Code units are the units by which Dart's String class is measured, which is
/// encoded in UTF-16.
///
/// See also:
///
/// * [String.runes], which deals with code points like this class.
/// * [String.characters], which deals with graphemes.
/// * [CharacterBoundary], which is a [TextBoundary] like this class, but whose
/// boundaries are graphemes instead of code points.
class _CodePointBoundary extends TextBoundary {
const _CodePointBoundary(this._text);

final String _text;

// Returns true if the given position falls in the center of a surrogate pair.
bool _breaksSurrogatePair(int position) {
assert(position > 0 && position < _text.length && _text.length > 1);
return TextPainter.isHighSurrogate(_text.codeUnitAt(position - 1))
&& TextPainter.isLowSurrogate(_text.codeUnitAt(position));
}

@override
int getLeadingTextBoundaryAt(int position) => position.clamp(0, _text.length); // ignore_clamp_double_lint
int? getLeadingTextBoundaryAt(int position) {
if (_text.isEmpty || position < 0) {
return null;
}
if (position == 0) {
return 0;
}
if (position >= _text.length) {
return _text.length;
}
if (_text.length <= 1) {
return position;
}

return _breaksSurrogatePair(position)
? position - 1
: position;
}

@override
int getTrailingTextBoundaryAt(int position) => (position + 1).clamp(0, _text.length); // ignore_clamp_double_lint
int? getTrailingTextBoundaryAt(int position) {
if (_text.isEmpty || position >= _text.length) {
return null;
}
if (position < 0) {
return 0;
}
if (position == _text.length - 1) {
return _text.length;
}
if (_text.length <= 1) {
return position;
}

return _breaksSurrogatePair(position + 1)
? position + 2
: position + 1;
}
}

// ------------------------------- Text Actions -------------------------------
Expand Down
5 changes: 3 additions & 2 deletions packages/flutter/test/widgets/editable_text_shortcuts_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -444,14 +444,15 @@ void main() {
await tester.pumpWidget(buildEditableText(obscured: true));
await sendKeyCombination(tester, const SingleActivator(trigger));

// Both emojis that were partially selected are deleted entirely.
expect(
controller.text,
'👨‍👩‍👦👨‍👩‍👦',
'‍👩‍👦👨‍👩‍👦',
);

expect(
controller.selection,
const TextSelection.collapsed(offset: 1),
const TextSelection.collapsed(offset: 0),
);
}, variant: TargetPlatformVariant.all(excluding: <TargetPlatform>{ TargetPlatform.iOS }));
});
Expand Down
Loading

0 comments on commit 4341ed4

Please sign in to comment.