firka/flutter
firka/flutter
4
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Fix out-of-bounds and reversed TextBox queries in computing caret metrics (#122480)

Browse Source 
Fix out-of-bounds and reversed TextBox queries in computing caret metrics
This commit is contained in:
Greg Price 2023-03-27 20:16:20 -07:00 committed by GitHub
parent 017aed4c5e
commit 48111197ac
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 235 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

40
packages/flutter/lib/src/painting/text_painter.dart
View File

@ -1044,6 +1044,7 @@ class TextPainter {
// Get the caret metrics (in logical pixels) based off the near edge of the
// character upstream from the given string offset.
_CaretMetrics? _getMetricsFromUpstream(int offset) {
assert(offset >= 0);
final int plainTextLength = plainText.length;
if (plainTextLength == 0 || offset > plainTextLength) {
return null;
@ -1061,7 +1062,7 @@ class TextPainter {
final int prevRuneOffset = offset - graphemeClusterLength;
// Use BoxHeightStyle.strut to ensure that the caret's height fits within
// the line's height and is consistent throughout the line.
boxes = _paragraph!.getBoxesForRange(prevRuneOffset, offset, boxHeightStyle: ui.BoxHeightStyle.strut);
boxes = _paragraph!.getBoxesForRange(max(0, prevRuneOffset), offset, boxHeightStyle: ui.BoxHeightStyle.strut);
// When the range does not include a full cluster, no boxes will be returned.
if (boxes.isEmpty) {
// When we are at the beginning of the line, a non-surrogate position will
@ -1079,7 +1080,12 @@ class TextPainter {
graphemeClusterLength *= 2;
continue;
}
final TextBox box = boxes.first;
// Try to identify the box nearest the offset. This logic works when
// there's just one box, and when all boxes have the same direction.
// It may not work in bidi text: https://github.com/flutter/flutter/issues/123424
final TextBox box = boxes.last.direction == TextDirection.ltr
? boxes.last : boxes.first;
return prevCodeUnit == NEWLINE_CODE_UNIT
? _EmptyLineCaretMetrics(lineVerticalOffset: box.bottom)
@ -1087,11 +1093,13 @@ class TextPainter {
}
return null;
}
// Get the caret metrics (in logical pixels) based off the near edge of the
// character downstream from the given string offset.
_CaretMetrics? _getMetricsFromDownstream(int offset) {
assert(offset >= 0);
final int plainTextLength = plainText.length;
if (plainTextLength == 0 || offset < 0) {
if (plainTextLength == 0) {
return null;
}
// We cap the offset at the final index of plain text.
@ -1123,7 +1131,13 @@ class TextPainter {
graphemeClusterLength *= 2;
continue;
}
final TextBox box = boxes.last;
// Try to identify the box nearest the offset. This logic works when
// there's just one box, and when all boxes have the same direction.
// It may not work in bidi text: https://github.com/flutter/flutter/issues/123424
final TextBox box = boxes.first.direction == TextDirection.ltr
? boxes.first : boxes.last;
return _LineCaretMetrics(offset: Offset(box.start, box.top), writingDirection: box.direction, fullHeight: box.bottom - box.top);
}
return null;
@ -1159,7 +1173,13 @@ class TextPainter {
///
/// Valid only after [layout] has been called.
Offset getOffsetForCaret(TextPosition position, Rect caretPrototype) {
final _CaretMetrics caretMetrics = _computeCaretMetrics(position);
final _CaretMetrics caretMetrics;
if (position.offset < 0) {
// TODO(LongCatIsLooong): make this case impossible; see https://github.com/flutter/flutter/issues/79495
caretMetrics = const _EmptyLineCaretMetrics(lineVerticalOffset: 0);
} else {
caretMetrics = _computeCaretMetrics(position);
}
if (caretMetrics is _EmptyLineCaretMetrics) {
final double paintOffsetAlignment = _computePaintOffsetFraction(textAlign, textDirection!);
@ -1192,6 +1212,10 @@ class TextPainter {
///
/// Valid only after [layout] has been called.
double? getFullHeightForCaret(TextPosition position, Rect caretPrototype) {
if (position.offset < 0) {
// TODO(LongCatIsLooong): make this case impossible; see https://github.com/flutter/flutter/issues/79495
return null;
}
final _CaretMetrics caretMetrics = _computeCaretMetrics(position);
return caretMetrics is _LineCaretMetrics ? caretMetrics.fullHeight : null;
}
@ -1232,10 +1256,11 @@ class TextPainter {
/// Returns a list of rects that bound the given selection.
///
/// The [selection] must be a valid range (with [TextSelection.isValid] true).
///
/// The [boxHeightStyle] and [boxWidthStyle] arguments may be used to select
/// the shape of the [TextBox]s. These properties default to
/// [ui.BoxHeightStyle.tight] and [ui.BoxWidthStyle.tight] respectively and
/// must not be null.
/// [ui.BoxHeightStyle.tight] and [ui.BoxWidthStyle.tight] respectively.
///
/// A given selection might have more than one rect if this text painter
/// contains bidirectional text because logically contiguous text might not be
@ -1253,6 +1278,7 @@ class TextPainter {
ui.BoxWidthStyle boxWidthStyle = ui.BoxWidthStyle.tight,
}) {
assert(_debugAssertTextLayoutIsValid);
assert(selection.isValid);
return _paragraph!.getBoxesForRange(
selection.start,
selection.end,

8
packages/flutter/lib/src/rendering/editable.dart
View File

@ -720,6 +720,11 @@ class RenderEditable extends RenderBox with RelayoutWhenSystemFontsChangeMixin,
void _updateSelectionExtentsVisibility(Offset effectiveOffset) {
assert(selection != null);
if (!selection!.isValid) {
_selectionStartInViewport.value = false;
_selectionEndInViewport.value = false;
return;
}
final Rect visibleRegion = Offset.zero & size;
final Offset startOffset = _textPainter.getOffsetForCaret(
@ -3010,8 +3015,7 @@ class _FloatingCursorPainter extends RenderEditablePainter {
final TextSelection? selection = renderEditable.selection;
// TODO(LongCatIsLooong): skip painting the caret when the selection is
// (-1, -1).
// TODO(LongCatIsLooong): skip painting caret when selection is (-1, -1): https://github.com/flutter/flutter/issues/79495
if (selection == null || !selection.isCollapsed) {
return;
}

196
packages/flutter/test/painting/text_painter_test.dart
View File

@ -8,12 +8,121 @@ import 'package:flutter/foundation.dart';
import 'package:flutter/widgets.dart';
import 'package:flutter_test/flutter_test.dart';
void _checkCaretOffsetsLtrAt(String text, List<int> boundaries) {
expect(boundaries.first, 0);
expect(boundaries.last, text.length);
final TextPainter painter = TextPainter()
..textDirection = TextDirection.ltr;
// Lay out the string up to each boundary, and record the width.
final List<double> prefixWidths = <double>[];
for (final int boundary in boundaries) {
painter.text = TextSpan(text: text.substring(0, boundary));
painter.layout();
prefixWidths.add(painter.width);
}
// The painter has the full text laid out. Check the caret offsets.
double caretOffset(int offset) {
final TextPosition position = ui.TextPosition(offset: offset);
return painter.getOffsetForCaret(position, ui.Rect.zero).dx;
}
expect(boundaries.map(caretOffset).toList(), prefixWidths);
double lastOffset = caretOffset(0);
for (int i = 1; i <= text.length; i++) {
final double offset = caretOffset(i);
expect(offset, greaterThanOrEqualTo(lastOffset));
lastOffset = offset;
}
painter.dispose();
}
/// Check the caret offsets are accurate for the given single line of LTR text.
///
/// This lays out the given text as a single line with [TextDirection.ltr]
/// and checks the following invariants, which should always hold if the text
/// is made up of LTR characters:
/// * The caret offsets go monotonically from 0.0 to the width of the text.
/// * At each character (that is, grapheme cluster) boundary, the caret
/// offset equals the width that the text up to that point would have
/// if laid out on its own.
///
/// If you have a [TextSpan] instead of a plain [String],
/// see [caretOffsetsForTextSpan].
void checkCaretOffsetsLtr(String text) {
final List<int> characterBoundaries = <int>[];
final CharacterRange range = CharacterRange.at(text, 0);
while (true) {
characterBoundaries.add(range.current.length);
if (range.stringAfterLength <= 0) {
break;
}
range.expandNext();
}
_checkCaretOffsetsLtrAt(text, characterBoundaries);
}
/// Check the caret offsets are accurate for the given single line of LTR text,
/// ignoring character boundaries within each given cluster.
///
/// This concatenates [clusters] into a string and then performs the same
/// checks as [checkCaretOffsetsLtr], except that instead of checking the
/// offset-equals-prefix-width invariant at every character boundary,
/// it does so only at the boundaries between the elements of [clusters].
///
/// The elements of [clusters] should be composed of whole characters: each
/// element should be a valid character range in the concatenated string.
///
/// Consider using [checkCaretOffsetsLtr] instead of this function. If that
/// doesn't pass, you may have an instance of <https://github.com/flutter/flutter/issues/122478>.
void checkCaretOffsetsLtrFromPieces(List<String> clusters) {
final StringBuffer buffer = StringBuffer();
final List<int> boundaries = <int>[];
boundaries.add(buffer.length);
for (final String cluster in clusters) {
buffer.write(cluster);
boundaries.add(buffer.length);
}
_checkCaretOffsetsLtrAt(buffer.toString(), boundaries);
}
/// Compute the caret offsets for the given single line of text, a [TextSpan].
///
/// This lays out the given text as a single line with the given [textDirection]
/// and returns a full list of caret offsets, one at each code unit boundary.
///
/// This also checks that the offset at the very start or very end, if the text
/// direction is RTL or LTR respectively, equals the line's width.
///
/// If you have a [String] instead of a nontrivial [TextSpan],
/// consider using [checkCaretOffsetsLtr] instead.
List<double> caretOffsetsForTextSpan(TextDirection textDirection, TextSpan text) {
final TextPainter painter = TextPainter()
..textDirection = textDirection
..text = text
..layout();
final int length = text.toPlainText().length;
final List<double> result = List<double>.generate(length + 1, (int offset) {
final TextPosition position = ui.TextPosition(offset: offset);
return painter.getOffsetForCaret(position, ui.Rect.zero).dx;
});
switch (textDirection) {
case TextDirection.ltr: expect(result[length], painter.width);
case TextDirection.rtl: expect(result[0], painter.width);
}
painter.dispose();
return result;
}
void main() {
test('TextPainter caret test', () {
final TextPainter painter = TextPainter()
..textDirection = TextDirection.ltr;
String text = 'A';
checkCaretOffsetsLtr(text);
painter.text = TextSpan(text: text);
painter.layout();
@ -28,6 +137,7 @@ void main() {
// Check that getOffsetForCaret handles a character that is encoded as a
// surrogate pair.
text = 'A\u{1F600}';
checkCaretOffsetsLtr(text);
painter.text = TextSpan(text: text);
painter.layout();
caretOffset = painter.getOffsetForCaret(ui.TextPosition(offset: text.length), ui.Rect.zero);
@ -87,6 +197,8 @@ void main() {
// Format: '👩‍<zwj>👩‍<zwj>👦👩‍<zwj>👩‍<zwj>👧‍<zwj>👧👏<modifier>'
// One three-person family, one four-person family, one clapping hands (medium skin tone).
const String text = '👩‍👩‍👦👩‍👩‍👧‍👧👏🏽';
checkCaretOffsetsLtr(text);
painter.text = const TextSpan(text: text);
painter.layout(maxWidth: 10000);
@ -147,6 +259,90 @@ void main() {
painter.dispose();
}, skip: isBrowser && !isCanvasKit); // https://github.com/flutter/flutter/issues/56308
test('TextPainter caret emoji tests: single, long emoji', () {
// Regression test for https://github.com/flutter/flutter/issues/50563
checkCaretOffsetsLtr('👩‍🚀');
checkCaretOffsetsLtr('👩‍❤️‍💋‍👩');
checkCaretOffsetsLtr('👨‍👩‍👦‍👦');
checkCaretOffsetsLtr('👨🏾‍🤝‍👨🏻');
checkCaretOffsetsLtr('👨‍👦');
checkCaretOffsetsLtr('👩‍👦');
checkCaretOffsetsLtr('🏌🏿‍♀️');
checkCaretOffsetsLtr('🏊‍♀️');
checkCaretOffsetsLtr('🏄🏻‍♂️');
// These actually worked even before #50563 was fixed (because
// their lengths in code units are powers of 2, namely 4 and 8).
checkCaretOffsetsLtr('🇺🇳');
checkCaretOffsetsLtr('👩‍❤️‍👨');
}, skip: isBrowser && !isCanvasKit); // https://github.com/flutter/flutter/issues/56308
test('TextPainter caret emoji test: letters, then 1 emoji of 5 code units', () {
// Regression test for https://github.com/flutter/flutter/issues/50563
checkCaretOffsetsLtr('a👩‍🚀');
checkCaretOffsetsLtr('ab👩‍🚀');
checkCaretOffsetsLtr('abc👩‍🚀');
checkCaretOffsetsLtr('abcd👩‍🚀');
}, skip: isBrowser && !isCanvasKit); // https://github.com/flutter/flutter/issues/56308
test('TextPainter caret zalgo test', () {
// Regression test for https://github.com/flutter/flutter/issues/98516
checkCaretOffsetsLtr('Z͉̳̺ͥͬ̾a̴͕̲̒̒͌̋ͪl̨͎̰̘͉̟ͤ̀̈̚͜g͕͔̤͖̟̒͝ͅo̵̡̡̼͚̐ͯ̅ͪ̆ͣ̚');
}, skip: isBrowser && !isCanvasKit); // https://github.com/flutter/flutter/issues/56308
test('TextPainter caret Devanagari test', () {
// Regression test for https://github.com/flutter/flutter/issues/118403
checkCaretOffsetsLtrFromPieces(
<String>['प्रा', 'प्त', ' ', 'व', 'र्ण', 'न', ' ', 'प्र', 'व्रु', 'ति']);
}, skip: isBrowser && !isCanvasKit); // https://github.com/flutter/flutter/issues/56308
test('TextPainter caret Devanagari test, full strength', () {
// Regression test for https://github.com/flutter/flutter/issues/118403
checkCaretOffsetsLtr('प्राप्त वर्णन प्रव्रुति');
}, skip: true); // https://github.com/flutter/flutter/issues/122478
test('TextPainter caret emoji test LTR: letters next to emoji, as separate TextBoxes', () {
// Regression test for https://github.com/flutter/flutter/issues/122477
// The trigger for this bug was to have SkParagraph report separate
// TextBoxes for the emoji and for the characters next to it.
// In normal usage on a real device, this can happen by simply typing
// letters and then an emoji, presumably because they get different fonts.
// In these tests, our single test font covers both letters and emoji,
// so we provoke the same effect by adding styles.
expect(caretOffsetsForTextSpan(
TextDirection.ltr,
const TextSpan(children: <TextSpan>[
TextSpan(text: '👩‍🚀', style: TextStyle()),
TextSpan(text: ' words', style: TextStyle(fontWeight: FontWeight.bold)),
])),
<double>[0, 28, 28, 28, 28, 28, 42, 56, 70, 84, 98, 112]);
expect(caretOffsetsForTextSpan(
TextDirection.ltr,
const TextSpan(children: <TextSpan>[
TextSpan(text: 'words ', style: TextStyle(fontWeight: FontWeight.bold)),
TextSpan(text: '👩‍🚀', style: TextStyle()),
])),
<double>[0, 14, 28, 42, 56, 70, 84, 84, 84, 84, 84, 112]);
}, skip: isBrowser && !isCanvasKit); // https://github.com/flutter/flutter/issues/56308
test('TextPainter caret emoji test RTL: letters next to emoji, as separate TextBoxes', () {
// Regression test for https://github.com/flutter/flutter/issues/122477
expect(caretOffsetsForTextSpan(
TextDirection.rtl,
const TextSpan(children: <TextSpan>[
TextSpan(text: '👩‍🚀', style: TextStyle()),
TextSpan(text: ' מילים', style: TextStyle(fontWeight: FontWeight.bold)),
])),
<double>[112, 84, 84, 84, 84, 84, 70, 56, 42, 28, 14, 0]);
expect(caretOffsetsForTextSpan(
TextDirection.rtl,
const TextSpan(children: <TextSpan>[
TextSpan(text: 'מילים ', style: TextStyle(fontWeight: FontWeight.bold)),
TextSpan(text: '👩‍🚀', style: TextStyle()),
])),
<double>[112, 98, 84, 70, 56, 42, 28, 28, 28, 28, 28, 0]);
}, skip: isBrowser && !isCanvasKit); // https://github.com/flutter/flutter/issues/56308
test('TextPainter caret center space test', () {
final TextPainter painter = TextPainter()
..textDirection = TextDirection.ltr;