Unverified Commit 4341ed42 authored by Justin McCandless's avatar Justin McCandless Committed by GitHub

Fix the breaking of multi-code-unit characters in obscure mode (#123366)

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 https://github.com/flutter/flutter/issues/122381#issuecomment-1482042620.  So it's unclear what the desired behavior should actually be.

### Resources

Fixes https://github.com/flutter/flutter/issues/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
parent ffbeb723
...@@ -170,11 +170,11 @@ class WordBoundary extends TextBoundary { ...@@ -170,11 +170,11 @@ class WordBoundary extends TextBoundary {
// single code point that represents a supplementary character. // single code point that represents a supplementary character.
static int _codePointFromSurrogates(int highSurrogate, int lowSurrogate) { static int _codePointFromSurrogates(int highSurrogate, int lowSurrogate) {
assert( assert(
TextPainter._isHighSurrogate(highSurrogate), TextPainter.isHighSurrogate(highSurrogate),
'U+${highSurrogate.toRadixString(16).toUpperCase().padLeft(4, "0")}) is not a high surrogate.', 'U+${highSurrogate.toRadixString(16).toUpperCase().padLeft(4, "0")}) is not a high surrogate.',
); );
assert( assert(
TextPainter._isLowSurrogate(lowSurrogate), TextPainter.isLowSurrogate(lowSurrogate),
'U+${lowSurrogate.toRadixString(16).toUpperCase().padLeft(4, "0")}) is not a low surrogate.', 'U+${lowSurrogate.toRadixString(16).toUpperCase().padLeft(4, "0")}) is not a low surrogate.',
); );
const int base = 0x010000 - (0xD800 << 10) - 0xDC00; const int base = 0x010000 - (0xD800 << 10) - 0xDC00;
...@@ -991,17 +991,34 @@ class TextPainter { ...@@ -991,17 +991,34 @@ class TextPainter {
canvas.drawParagraph(_paragraph!, offset); canvas.drawParagraph(_paragraph!, offset);
} }
// Returns true iff the given value is a valid UTF-16 high surrogate. The value // Returns true if value falls in the valid range of the UTF16 encoding.
// must be a UTF-16 code unit, meaning it must be in the range 0x0000-0xFFFF. static bool _isUTF16(int value) {
// return value >= 0x0 && value <= 0xFFFFF;
// See also: }
// * https://en.wikipedia.org/wiki/UTF-16#Code_points_from_U+010000_to_U+10FFFF
static bool _isHighSurrogate(int value) { /// 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; return value & 0xFC00 == 0xD800;
} }
// Whether the given UTF-16 code unit is a low (second) surrogate. /// Returns true iff the given value is a valid UTF-16 low (second) surrogate.
static bool _isLowSurrogate(int value) { /// 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; return value & 0xFC00 == 0xDC00;
} }
...@@ -1021,7 +1038,7 @@ class TextPainter { ...@@ -1021,7 +1038,7 @@ class TextPainter {
return null; return null;
} }
// TODO(goderbauer): doesn't handle extended grapheme clusters with more than one Unicode scalar value (https://github.com/flutter/flutter/issues/13404). // 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 /// Returns the closest offset before `offset` at which the input cursor can
...@@ -1032,7 +1049,7 @@ class TextPainter { ...@@ -1032,7 +1049,7 @@ class TextPainter {
return null; return null;
} }
// TODO(goderbauer): doesn't handle extended grapheme clusters with more than one Unicode scalar value (https://github.com/flutter/flutter/issues/13404). // 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. // Unicode value for a zero width joiner character.
...@@ -1052,7 +1069,7 @@ class TextPainter { ...@@ -1052,7 +1069,7 @@ class TextPainter {
const int NEWLINE_CODE_UNIT = 10; const int NEWLINE_CODE_UNIT = 10;
// Check for multi-code-unit glyphs such as emojis or zero width joiner. // 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; int graphemeClusterLength = needsSearch ? 2 : 1;
List<TextBox> boxes = <TextBox>[]; List<TextBox> boxes = <TextBox>[];
while (boxes.isEmpty) { while (boxes.isEmpty) {
...@@ -1103,7 +1120,7 @@ class TextPainter { ...@@ -1103,7 +1120,7 @@ class TextPainter {
final int nextCodeUnit = plainText.codeUnitAt(min(offset, plainTextLength - 1)); final int nextCodeUnit = plainText.codeUnitAt(min(offset, plainTextLength - 1));
// Check for multi-code-unit glyphs such as emojis or zero width joiner // 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; int graphemeClusterLength = needsSearch ? 2 : 1;
List<TextBox> boxes = <TextBox>[]; List<TextBox> boxes = <TextBox>[];
while (boxes.isEmpty) { while (boxes.isEmpty) {
......
...@@ -31,6 +31,9 @@ abstract class TextBoundary { ...@@ -31,6 +31,9 @@ abstract class TextBoundary {
/// `position`, or null if no boundaries can be found. /// `position`, or null if no boundaries can be found.
/// ///
/// The return value, if not null, is usually less than or equal to `position`. /// 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) { int? getLeadingTextBoundaryAt(int position) {
if (position < 0) { if (position < 0) {
return null; return null;
...@@ -39,10 +42,13 @@ abstract class TextBoundary { ...@@ -39,10 +42,13 @@ abstract class TextBoundary {
return start >= 0 ? start : null; return start >= 0 ? start : null;
} }
/// Returns the offset of the closest text boundaries after the given `position`, /// Returns the offset of the closest text boundary after the given
/// or null if there is no boundaries can be found after `position`. /// `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 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) { int? getTrailingTextBoundaryAt(int position) {
final int end = getTextBoundaryAt(max(0, position)).end; final int end = getTextBoundaryAt(max(0, position)).end;
return end >= 0 ? end : null; return end >= 0 ? end : null;
......
...@@ -4246,7 +4246,7 @@ class EditableTextState extends State<EditableText> with AutomaticKeepAliveClien ...@@ -4246,7 +4246,7 @@ class EditableTextState extends State<EditableText> with AutomaticKeepAliveClien
// --------------------------- Text Editing Actions --------------------------- // --------------------------- 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 _nextWordBoundary() => widget.obscureText ? _documentBoundary() : renderEditable.wordBoundaries.moveByWordBoundary;
TextBoundary _linebreak() => widget.obscureText ? _documentBoundary() : LineBoundary(renderEditable); TextBoundary _linebreak() => widget.obscureText ? _documentBoundary() : LineBoundary(renderEditable);
TextBoundary _paragraphBoundary() => ParagraphBoundary(_value.text); TextBoundary _paragraphBoundary() => ParagraphBoundary(_value.text);
...@@ -5076,21 +5076,76 @@ class _ScribblePlaceholder extends WidgetSpan { ...@@ -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 /// A code point represents a single character. This may be smaller than what is
/// unit. This can be useful when handling text without any grapheme cluster, /// represented by a user-perceived character, or grapheme. For example, a
/// e.g. password input in [EditableText]. If you are handling text that may /// single grapheme (in this case a Unicode extended grapheme cluster) like
/// include grapheme clusters, consider using [CharacterBoundary]. /// "👨‍👩‍👦" consists of five code points: the man emoji, a zero
class _CodeUnitBoundary extends TextBoundary { /// width joiner, the woman emoji, another zero width joiner, and the boy emoji.
const _CodeUnitBoundary(this._text); /// 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; 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 @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 @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 ------------------------------- // ------------------------------- Text Actions -------------------------------
......
...@@ -444,14 +444,15 @@ void main() { ...@@ -444,14 +444,15 @@ void main() {
await tester.pumpWidget(buildEditableText(obscured: true)); await tester.pumpWidget(buildEditableText(obscured: true));
await sendKeyCombination(tester, const SingleActivator(trigger)); await sendKeyCombination(tester, const SingleActivator(trigger));
// Both emojis that were partially selected are deleted entirely.
expect( expect(
controller.text, controller.text,
'👨‍👩‍👦👨‍👩‍👦', '‍👩‍👦👨‍👩‍👦',
); );
expect( expect(
controller.selection, controller.selection,
const TextSelection.collapsed(offset: 1), const TextSelection.collapsed(offset: 0),
); );
}, variant: TargetPlatformVariant.all(excluding: <TargetPlatform>{ TargetPlatform.iOS })); }, variant: TargetPlatformVariant.all(excluding: <TargetPlatform>{ TargetPlatform.iOS }));
}); });
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment