Skip to content

Commit 4811119

Browse files
authored
Fix out-of-bounds and reversed TextBox queries in computing caret metrics (flutter#122480)
Fix out-of-bounds and reversed TextBox queries in computing caret metrics
1 parent 017aed4 commit 4811119

File tree

3 files changed

+235
-9
lines changed

3 files changed

+235
-9
lines changed

packages/flutter/lib/src/painting/text_painter.dart

Lines changed: 33 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1044,6 +1044,7 @@ class TextPainter {
10441044
// Get the caret metrics (in logical pixels) based off the near edge of the
10451045
// character upstream from the given string offset.
10461046
_CaretMetrics? _getMetricsFromUpstream(int offset) {
1047+
assert(offset >= 0);
10471048
final int plainTextLength = plainText.length;
10481049
if (plainTextLength == 0 || offset > plainTextLength) {
10491050
return null;
@@ -1061,7 +1062,7 @@ class TextPainter {
10611062
final int prevRuneOffset = offset - graphemeClusterLength;
10621063
// Use BoxHeightStyle.strut to ensure that the caret's height fits within
10631064
// the line's height and is consistent throughout the line.
1064-
boxes = _paragraph!.getBoxesForRange(prevRuneOffset, offset, boxHeightStyle: ui.BoxHeightStyle.strut);
1065+
boxes = _paragraph!.getBoxesForRange(max(0, prevRuneOffset), offset, boxHeightStyle: ui.BoxHeightStyle.strut);
10651066
// When the range does not include a full cluster, no boxes will be returned.
10661067
if (boxes.isEmpty) {
10671068
// When we are at the beginning of the line, a non-surrogate position will
@@ -1079,19 +1080,26 @@ class TextPainter {
10791080
graphemeClusterLength *= 2;
10801081
continue;
10811082
}
1082-
final TextBox box = boxes.first;
1083+
1084+
// Try to identify the box nearest the offset. This logic works when
1085+
// there's just one box, and when all boxes have the same direction.
1086+
// It may not work in bidi text: https://github.com/flutter/flutter/issues/123424
1087+
final TextBox box = boxes.last.direction == TextDirection.ltr
1088+
? boxes.last : boxes.first;
10831089

10841090
return prevCodeUnit == NEWLINE_CODE_UNIT
10851091
? _EmptyLineCaretMetrics(lineVerticalOffset: box.bottom)
10861092
: _LineCaretMetrics(offset: Offset(box.end, box.top), writingDirection: box.direction, fullHeight: box.bottom - box.top);
10871093
}
10881094
return null;
10891095
}
1096+
10901097
// Get the caret metrics (in logical pixels) based off the near edge of the
10911098
// character downstream from the given string offset.
10921099
_CaretMetrics? _getMetricsFromDownstream(int offset) {
1100+
assert(offset >= 0);
10931101
final int plainTextLength = plainText.length;
1094-
if (plainTextLength == 0 || offset < 0) {
1102+
if (plainTextLength == 0) {
10951103
return null;
10961104
}
10971105
// We cap the offset at the final index of plain text.
@@ -1123,7 +1131,13 @@ class TextPainter {
11231131
graphemeClusterLength *= 2;
11241132
continue;
11251133
}
1126-
final TextBox box = boxes.last;
1134+
1135+
// Try to identify the box nearest the offset. This logic works when
1136+
// there's just one box, and when all boxes have the same direction.
1137+
// It may not work in bidi text: https://github.com/flutter/flutter/issues/123424
1138+
final TextBox box = boxes.first.direction == TextDirection.ltr
1139+
? boxes.first : boxes.last;
1140+
11271141
return _LineCaretMetrics(offset: Offset(box.start, box.top), writingDirection: box.direction, fullHeight: box.bottom - box.top);
11281142
}
11291143
return null;
@@ -1159,7 +1173,13 @@ class TextPainter {
11591173
///
11601174
/// Valid only after [layout] has been called.
11611175
Offset getOffsetForCaret(TextPosition position, Rect caretPrototype) {
1162-
final _CaretMetrics caretMetrics = _computeCaretMetrics(position);
1176+
final _CaretMetrics caretMetrics;
1177+
if (position.offset < 0) {
1178+
// TODO(LongCatIsLooong): make this case impossible; see https://github.com/flutter/flutter/issues/79495
1179+
caretMetrics = const _EmptyLineCaretMetrics(lineVerticalOffset: 0);
1180+
} else {
1181+
caretMetrics = _computeCaretMetrics(position);
1182+
}
11631183

11641184
if (caretMetrics is _EmptyLineCaretMetrics) {
11651185
final double paintOffsetAlignment = _computePaintOffsetFraction(textAlign, textDirection!);
@@ -1192,6 +1212,10 @@ class TextPainter {
11921212
///
11931213
/// Valid only after [layout] has been called.
11941214
double? getFullHeightForCaret(TextPosition position, Rect caretPrototype) {
1215+
if (position.offset < 0) {
1216+
// TODO(LongCatIsLooong): make this case impossible; see https://github.com/flutter/flutter/issues/79495
1217+
return null;
1218+
}
11951219
final _CaretMetrics caretMetrics = _computeCaretMetrics(position);
11961220
return caretMetrics is _LineCaretMetrics ? caretMetrics.fullHeight : null;
11971221
}
@@ -1232,10 +1256,11 @@ class TextPainter {
12321256

12331257
/// Returns a list of rects that bound the given selection.
12341258
///
1259+
/// The [selection] must be a valid range (with [TextSelection.isValid] true).
1260+
///
12351261
/// The [boxHeightStyle] and [boxWidthStyle] arguments may be used to select
12361262
/// the shape of the [TextBox]s. These properties default to
1237-
/// [ui.BoxHeightStyle.tight] and [ui.BoxWidthStyle.tight] respectively and
1238-
/// must not be null.
1263+
/// [ui.BoxHeightStyle.tight] and [ui.BoxWidthStyle.tight] respectively.
12391264
///
12401265
/// A given selection might have more than one rect if this text painter
12411266
/// contains bidirectional text because logically contiguous text might not be
@@ -1253,6 +1278,7 @@ class TextPainter {
12531278
ui.BoxWidthStyle boxWidthStyle = ui.BoxWidthStyle.tight,
12541279
}) {
12551280
assert(_debugAssertTextLayoutIsValid);
1281+
assert(selection.isValid);
12561282
return _paragraph!.getBoxesForRange(
12571283
selection.start,
12581284
selection.end,

packages/flutter/lib/src/rendering/editable.dart

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -720,6 +720,11 @@ class RenderEditable extends RenderBox with RelayoutWhenSystemFontsChangeMixin,
720720

721721
void _updateSelectionExtentsVisibility(Offset effectiveOffset) {
722722
assert(selection != null);
723+
if (!selection!.isValid) {
724+
_selectionStartInViewport.value = false;
725+
_selectionEndInViewport.value = false;
726+
return;
727+
}
723728
final Rect visibleRegion = Offset.zero & size;
724729

725730
final Offset startOffset = _textPainter.getOffsetForCaret(
@@ -3010,8 +3015,7 @@ class _FloatingCursorPainter extends RenderEditablePainter {
30103015

30113016
final TextSelection? selection = renderEditable.selection;
30123017

3013-
// TODO(LongCatIsLooong): skip painting the caret when the selection is
3014-
// (-1, -1).
3018+
// TODO(LongCatIsLooong): skip painting caret when selection is (-1, -1): https://github.com/flutter/flutter/issues/79495
30153019
if (selection == null || !selection.isCollapsed) {
30163020
return;
30173021
}

packages/flutter/test/painting/text_painter_test.dart

Lines changed: 196 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,12 +8,121 @@ import 'package:flutter/foundation.dart';
88
import 'package:flutter/widgets.dart';
99
import 'package:flutter_test/flutter_test.dart';
1010

11+
void _checkCaretOffsetsLtrAt(String text, List<int> boundaries) {
12+
expect(boundaries.first, 0);
13+
expect(boundaries.last, text.length);
14+
15+
final TextPainter painter = TextPainter()
16+
..textDirection = TextDirection.ltr;
17+
18+
// Lay out the string up to each boundary, and record the width.
19+
final List<double> prefixWidths = <double>[];
20+
for (final int boundary in boundaries) {
21+
painter.text = TextSpan(text: text.substring(0, boundary));
22+
painter.layout();
23+
prefixWidths.add(painter.width);
24+
}
25+
26+
// The painter has the full text laid out. Check the caret offsets.
27+
double caretOffset(int offset) {
28+
final TextPosition position = ui.TextPosition(offset: offset);
29+
return painter.getOffsetForCaret(position, ui.Rect.zero).dx;
30+
}
31+
expect(boundaries.map(caretOffset).toList(), prefixWidths);
32+
double lastOffset = caretOffset(0);
33+
for (int i = 1; i <= text.length; i++) {
34+
final double offset = caretOffset(i);
35+
expect(offset, greaterThanOrEqualTo(lastOffset));
36+
lastOffset = offset;
37+
}
38+
painter.dispose();
39+
}
40+
41+
/// Check the caret offsets are accurate for the given single line of LTR text.
42+
///
43+
/// This lays out the given text as a single line with [TextDirection.ltr]
44+
/// and checks the following invariants, which should always hold if the text
45+
/// is made up of LTR characters:
46+
/// * The caret offsets go monotonically from 0.0 to the width of the text.
47+
/// * At each character (that is, grapheme cluster) boundary, the caret
48+
/// offset equals the width that the text up to that point would have
49+
/// if laid out on its own.
50+
///
51+
/// If you have a [TextSpan] instead of a plain [String],
52+
/// see [caretOffsetsForTextSpan].
53+
void checkCaretOffsetsLtr(String text) {
54+
final List<int> characterBoundaries = <int>[];
55+
final CharacterRange range = CharacterRange.at(text, 0);
56+
while (true) {
57+
characterBoundaries.add(range.current.length);
58+
if (range.stringAfterLength <= 0) {
59+
break;
60+
}
61+
range.expandNext();
62+
}
63+
_checkCaretOffsetsLtrAt(text, characterBoundaries);
64+
}
65+
66+
/// Check the caret offsets are accurate for the given single line of LTR text,
67+
/// ignoring character boundaries within each given cluster.
68+
///
69+
/// This concatenates [clusters] into a string and then performs the same
70+
/// checks as [checkCaretOffsetsLtr], except that instead of checking the
71+
/// offset-equals-prefix-width invariant at every character boundary,
72+
/// it does so only at the boundaries between the elements of [clusters].
73+
///
74+
/// The elements of [clusters] should be composed of whole characters: each
75+
/// element should be a valid character range in the concatenated string.
76+
///
77+
/// Consider using [checkCaretOffsetsLtr] instead of this function. If that
78+
/// doesn't pass, you may have an instance of <https://github.com/flutter/flutter/issues/122478>.
79+
void checkCaretOffsetsLtrFromPieces(List<String> clusters) {
80+
final StringBuffer buffer = StringBuffer();
81+
final List<int> boundaries = <int>[];
82+
boundaries.add(buffer.length);
83+
for (final String cluster in clusters) {
84+
buffer.write(cluster);
85+
boundaries.add(buffer.length);
86+
}
87+
_checkCaretOffsetsLtrAt(buffer.toString(), boundaries);
88+
}
89+
90+
/// Compute the caret offsets for the given single line of text, a [TextSpan].
91+
///
92+
/// This lays out the given text as a single line with the given [textDirection]
93+
/// and returns a full list of caret offsets, one at each code unit boundary.
94+
///
95+
/// This also checks that the offset at the very start or very end, if the text
96+
/// direction is RTL or LTR respectively, equals the line's width.
97+
///
98+
/// If you have a [String] instead of a nontrivial [TextSpan],
99+
/// consider using [checkCaretOffsetsLtr] instead.
100+
List<double> caretOffsetsForTextSpan(TextDirection textDirection, TextSpan text) {
101+
final TextPainter painter = TextPainter()
102+
..textDirection = textDirection
103+
..text = text
104+
..layout();
105+
final int length = text.toPlainText().length;
106+
final List<double> result = List<double>.generate(length + 1, (int offset) {
107+
final TextPosition position = ui.TextPosition(offset: offset);
108+
return painter.getOffsetForCaret(position, ui.Rect.zero).dx;
109+
});
110+
switch (textDirection) {
111+
case TextDirection.ltr: expect(result[length], painter.width);
112+
case TextDirection.rtl: expect(result[0], painter.width);
113+
}
114+
painter.dispose();
115+
return result;
116+
}
117+
11118
void main() {
12119
test('TextPainter caret test', () {
13120
final TextPainter painter = TextPainter()
14121
..textDirection = TextDirection.ltr;
15122

16123
String text = 'A';
124+
checkCaretOffsetsLtr(text);
125+
17126
painter.text = TextSpan(text: text);
18127
painter.layout();
19128

@@ -28,6 +137,7 @@ void main() {
28137
// Check that getOffsetForCaret handles a character that is encoded as a
29138
// surrogate pair.
30139
text = 'A\u{1F600}';
140+
checkCaretOffsetsLtr(text);
31141
painter.text = TextSpan(text: text);
32142
painter.layout();
33143
caretOffset = painter.getOffsetForCaret(ui.TextPosition(offset: text.length), ui.Rect.zero);
@@ -87,6 +197,8 @@ void main() {
87197
// Format: '👩‍<zwj>👩‍<zwj>👦👩‍<zwj>👩‍<zwj>👧‍<zwj>👧👏<modifier>'
88198
// One three-person family, one four-person family, one clapping hands (medium skin tone).
89199
const String text = '👩‍👩‍👦👩‍👩‍👧‍👧👏🏽';
200+
checkCaretOffsetsLtr(text);
201+
90202
painter.text = const TextSpan(text: text);
91203
painter.layout(maxWidth: 10000);
92204

@@ -147,6 +259,90 @@ void main() {
147259
painter.dispose();
148260
}, skip: isBrowser && !isCanvasKit); // https://github.com/flutter/flutter/issues/56308
149261

262+
test('TextPainter caret emoji tests: single, long emoji', () {
263+
// Regression test for https://github.com/flutter/flutter/issues/50563
264+
checkCaretOffsetsLtr('👩‍🚀');
265+
checkCaretOffsetsLtr('👩‍❤️‍💋‍👩');
266+
checkCaretOffsetsLtr('👨‍👩‍👦‍👦');
267+
checkCaretOffsetsLtr('👨🏾‍🤝‍👨🏻');
268+
checkCaretOffsetsLtr('👨‍👦');
269+
checkCaretOffsetsLtr('👩‍👦');
270+
checkCaretOffsetsLtr('🏌🏿‍♀️');
271+
checkCaretOffsetsLtr('🏊‍♀️');
272+
checkCaretOffsetsLtr('🏄🏻‍♂️');
273+
274+
// These actually worked even before #50563 was fixed (because
275+
// their lengths in code units are powers of 2, namely 4 and 8).
276+
checkCaretOffsetsLtr('🇺🇳');
277+
checkCaretOffsetsLtr('👩‍❤️‍👨');
278+
}, skip: isBrowser && !isCanvasKit); // https://github.com/flutter/flutter/issues/56308
279+
280+
test('TextPainter caret emoji test: letters, then 1 emoji of 5 code units', () {
281+
// Regression test for https://github.com/flutter/flutter/issues/50563
282+
checkCaretOffsetsLtr('a👩‍🚀');
283+
checkCaretOffsetsLtr('ab👩‍🚀');
284+
checkCaretOffsetsLtr('abc👩‍🚀');
285+
checkCaretOffsetsLtr('abcd👩‍🚀');
286+
}, skip: isBrowser && !isCanvasKit); // https://github.com/flutter/flutter/issues/56308
287+
288+
test('TextPainter caret zalgo test', () {
289+
// Regression test for https://github.com/flutter/flutter/issues/98516
290+
checkCaretOffsetsLtr('Z͉̳̺ͥͬ̾a̴͕̲̒̒͌̋ͪl̨͎̰̘͉̟ͤ̀̈̚͜g͕͔̤͖̟̒͝ͅo̵̡̡̼͚̐ͯ̅ͪ̆ͣ̚');
291+
}, skip: isBrowser && !isCanvasKit); // https://github.com/flutter/flutter/issues/56308
292+
293+
test('TextPainter caret Devanagari test', () {
294+
// Regression test for https://github.com/flutter/flutter/issues/118403
295+
checkCaretOffsetsLtrFromPieces(
296+
<String>['प्रा', 'प्त', ' ', 'व', 'र्ण', 'न', ' ', 'प्र', 'व्रु', 'ति']);
297+
}, skip: isBrowser && !isCanvasKit); // https://github.com/flutter/flutter/issues/56308
298+
299+
test('TextPainter caret Devanagari test, full strength', () {
300+
// Regression test for https://github.com/flutter/flutter/issues/118403
301+
checkCaretOffsetsLtr('प्राप्त वर्णन प्रव्रुति');
302+
}, skip: true); // https://github.com/flutter/flutter/issues/122478
303+
304+
test('TextPainter caret emoji test LTR: letters next to emoji, as separate TextBoxes', () {
305+
// Regression test for https://github.com/flutter/flutter/issues/122477
306+
// The trigger for this bug was to have SkParagraph report separate
307+
// TextBoxes for the emoji and for the characters next to it.
308+
// In normal usage on a real device, this can happen by simply typing
309+
// letters and then an emoji, presumably because they get different fonts.
310+
// In these tests, our single test font covers both letters and emoji,
311+
// so we provoke the same effect by adding styles.
312+
expect(caretOffsetsForTextSpan(
313+
TextDirection.ltr,
314+
const TextSpan(children: <TextSpan>[
315+
TextSpan(text: '👩‍🚀', style: TextStyle()),
316+
TextSpan(text: ' words', style: TextStyle(fontWeight: FontWeight.bold)),
317+
])),
318+
<double>[0, 28, 28, 28, 28, 28, 42, 56, 70, 84, 98, 112]);
319+
expect(caretOffsetsForTextSpan(
320+
TextDirection.ltr,
321+
const TextSpan(children: <TextSpan>[
322+
TextSpan(text: 'words ', style: TextStyle(fontWeight: FontWeight.bold)),
323+
TextSpan(text: '👩‍🚀', style: TextStyle()),
324+
])),
325+
<double>[0, 14, 28, 42, 56, 70, 84, 84, 84, 84, 84, 112]);
326+
}, skip: isBrowser && !isCanvasKit); // https://github.com/flutter/flutter/issues/56308
327+
328+
test('TextPainter caret emoji test RTL: letters next to emoji, as separate TextBoxes', () {
329+
// Regression test for https://github.com/flutter/flutter/issues/122477
330+
expect(caretOffsetsForTextSpan(
331+
TextDirection.rtl,
332+
const TextSpan(children: <TextSpan>[
333+
TextSpan(text: '👩‍🚀', style: TextStyle()),
334+
TextSpan(text: ' מילים', style: TextStyle(fontWeight: FontWeight.bold)),
335+
])),
336+
<double>[112, 84, 84, 84, 84, 84, 70, 56, 42, 28, 14, 0]);
337+
expect(caretOffsetsForTextSpan(
338+
TextDirection.rtl,
339+
const TextSpan(children: <TextSpan>[
340+
TextSpan(text: 'מילים ', style: TextStyle(fontWeight: FontWeight.bold)),
341+
TextSpan(text: '👩‍🚀', style: TextStyle()),
342+
])),
343+
<double>[112, 98, 84, 70, 56, 42, 28, 28, 28, 28, 28, 0]);
344+
}, skip: isBrowser && !isCanvasKit); // https://github.com/flutter/flutter/issues/56308
345+
150346
test('TextPainter caret center space test', () {
151347
final TextPainter painter = TextPainter()
152348
..textDirection = TextDirection.ltr;

0 commit comments

Comments
 (0)