Skip to content

Commit

Permalink
Make TextSpan hit testing precise. (flutter#139717)
Browse files Browse the repository at this point in the history
Fixes flutter#131435, flutter#104594, flutter#43400
Needs flutter/engine#48774 (to fix the web test failure).

Currently the method we use for text span hit testing `TextPainter.getPositionForOffset` always returns the closest `TextPosition`, even when the given offset is far away from the text. 

The new TextPaintes method tells you the layout bounds (`width =  letterspacing / 2 + x_advance + letterspacing / 2`, `height = font ascent + font descent`) of a character, the PR changes the hit testing implementation such that a TextSpan is only considered hit if the point-down event landed in one of it's character's layout bounds.

Potential issues:

1. In theory since the text is baseline aligned, we should use the max ascent and max descent of each character to calculate the height of the text span's hit-test region, in case some characters in the span have to fall back to a different font, but that will be slower and it typically doesn't make a huge difference. 

This is a breaking change. It also introduces a new finder and a new method `WidgetTester.tapOnText`: `await tester.tapOnText('string to match')` for ease of migration.
  • Loading branch information
LongCatIsLooong authored Dec 20, 2023
1 parent e86b825 commit ea5b972
Show file tree
Hide file tree
Showing 13 changed files with 747 additions and 40 deletions.
1 change: 1 addition & 0 deletions packages/flutter/lib/src/painting/basic_types.dart
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export 'dart:ui' show
FontStyle,
FontVariation,
FontWeight,
GlyphInfo,
ImageShader,
Locale,
MaskFilter,
Expand Down
21 changes: 20 additions & 1 deletion packages/flutter/lib/src/painting/text_painter.dart
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import 'dart:math' show max, min;
import 'dart:ui' as ui show
BoxHeightStyle,
BoxWidthStyle,
GlyphInfo,
LineMetrics,
Paragraph,
ParagraphBuilder,
Expand All @@ -24,6 +25,7 @@ import 'strut_style.dart';
import 'text_scaler.dart';
import 'text_span.dart';

export 'dart:ui' show LineMetrics;
export 'package:flutter/services.dart' show TextRange, TextSelection;

/// The default font size if none is specified.
Expand Down Expand Up @@ -1493,7 +1495,24 @@ class TextPainter {
: boxes.map((TextBox box) => _shiftTextBox(box, offset)).toList(growable: false);
}

/// Returns the position within the text for the given pixel offset.
/// Returns the [GlyphInfo] of the glyph closest to the given `offset` in the
/// paragraph coordinate system, or null if the text is empty, or is entirely
/// clipped or ellipsized away.
///
/// This method first finds the line closest to `offset.dy`, and then returns
/// the [GlyphInfo] of the closest glyph(s) within that line.
ui.GlyphInfo? getClosestGlyphForOffset(Offset offset) {
assert(_debugAssertTextLayoutIsValid);
assert(!_debugNeedsRelayout);
final _TextPainterLayoutCacheWithOffset cachedLayout = _layoutCache!;
final ui.GlyphInfo? rawGlyphInfo = cachedLayout.paragraph.getClosestGlyphInfoForOffset(offset - cachedLayout.paintOffset);
if (rawGlyphInfo == null || cachedLayout.paintOffset == Offset.zero) {
return rawGlyphInfo;
}
return ui.GlyphInfo(rawGlyphInfo.graphemeClusterLayoutBounds.shift(cachedLayout.paintOffset), rawGlyphInfo.graphemeClusterCodeUnitRange, rawGlyphInfo.writingDirection);
}

/// Returns the closest position within the text for the given pixel offset.
TextPosition getPositionForOffset(Offset offset) {
assert(_debugAssertTextLayoutIsValid);
assert(!_debugNeedsRelayout);
Expand Down
8 changes: 5 additions & 3 deletions packages/flutter/lib/src/painting/text_span.dart
Original file line number Diff line number Diff line change
Expand Up @@ -343,18 +343,20 @@ class TextSpan extends InlineSpan implements HitTestTarget, MouseTrackerAnnotati
/// Returns the text span that contains the given position in the text.
@override
InlineSpan? getSpanForPositionVisitor(TextPosition position, Accumulator offset) {
if (text == null) {
final String? text = this.text;
if (text == null || text.isEmpty) {
return null;
}
final TextAffinity affinity = position.affinity;
final int targetOffset = position.offset;
final int endOffset = offset.value + text!.length;
final int endOffset = offset.value + text.length;

if (offset.value == targetOffset && affinity == TextAffinity.downstream ||
offset.value < targetOffset && targetOffset < endOffset ||
endOffset == targetOffset && affinity == TextAffinity.upstream) {
return this;
}
offset.increment(text!.length);
offset.increment(text.length);
return null;
}

Expand Down
12 changes: 10 additions & 2 deletions packages/flutter/lib/src/rendering/editable.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1941,8 +1941,16 @@ class RenderEditable extends RenderBox with RelayoutWhenSystemFontsChangeMixin,
@protected
bool hitTestChildren(BoxHitTestResult result, { required Offset position }) {
final Offset effectivePosition = position - _paintOffset;
final InlineSpan? textSpan = _textPainter.text;
switch (textSpan?.getSpanForPosition(_textPainter.getPositionForOffset(effectivePosition))) {
final GlyphInfo? glyph = _textPainter.getClosestGlyphForOffset(effectivePosition);
// The hit-test can't fall through the horizontal gaps between visually
// adjacent characters on the same line, even with a large letter-spacing or
// text justification, as graphemeClusterLayoutBounds.width is the advance
// width to the next character, so there's no gap between their
// graphemeClusterLayoutBounds rects.
final InlineSpan? spanHit = glyph != null && glyph.graphemeClusterLayoutBounds.contains(effectivePosition)
? _textPainter.text!.getSpanForPosition(TextPosition(offset: glyph.graphemeClusterCodeUnitRange.start))
: null;
switch (spanHit) {
case final HitTestTarget span:
result.add(HitTestEntry(span));
return true;
Expand Down
14 changes: 12 additions & 2 deletions packages/flutter/lib/src/rendering/paragraph.dart
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,7 @@ class RenderParagraph extends RenderBox with ContainerRenderObjectMixin<RenderBo
}

static final String _placeholderCharacter = String.fromCharCode(PlaceholderSpan.placeholderCodeUnit);

final TextPainter _textPainter;

List<AttributedString>? _cachedAttributedLabels;
Expand Down Expand Up @@ -730,9 +731,18 @@ class RenderParagraph extends RenderBox with ContainerRenderObjectMixin<RenderBo
bool hitTestSelf(Offset position) => true;

@override
@protected
bool hitTestChildren(BoxHitTestResult result, { required Offset position }) {
final TextPosition textPosition = _textPainter.getPositionForOffset(position);
switch (_textPainter.text!.getSpanForPosition(textPosition)) {
final GlyphInfo? glyph = _textPainter.getClosestGlyphForOffset(position);
// The hit-test can't fall through the horizontal gaps between visually
// adjacent characters on the same line, even with a large letter-spacing or
// text justification, as graphemeClusterLayoutBounds.width is the advance
// width to the next character, so there's no gap between their
// graphemeClusterLayoutBounds rects.
final InlineSpan? spanHit = glyph != null && glyph.graphemeClusterLayoutBounds.contains(position)
? _textPainter.text!.getSpanForPosition(TextPosition(offset: glyph.graphemeClusterCodeUnitRange.start))
: null;
switch (spanHit) {
case final HitTestTarget span:
result.add(HitTestEntry(span));
return true;
Expand Down
18 changes: 18 additions & 0 deletions packages/flutter/test/painting/text_span_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,24 @@ void main() {
expect(textSpan2.compareTo(textSpan2), RenderComparison.identical);
});

test('GetSpanForPosition', () {
const TextSpan textSpan = TextSpan(
text: '',
children: <InlineSpan>[
TextSpan(text: '', children: <InlineSpan>[
TextSpan(text: 'a'),
]),
TextSpan(text: 'b'),
TextSpan(text: 'c'),
],
);

expect((textSpan.getSpanForPosition(const TextPosition(offset: 0)) as TextSpan?)?.text, 'a');
expect((textSpan.getSpanForPosition(const TextPosition(offset: 1)) as TextSpan?)?.text, 'b');
expect((textSpan.getSpanForPosition(const TextPosition(offset: 2)) as TextSpan?)?.text, 'c');
expect((textSpan.getSpanForPosition(const TextPosition(offset: 3)) as TextSpan?)?.text, isNull);
});

test('GetSpanForPosition with WidgetSpan', () {
const TextSpan textSpan = TextSpan(
text: 'a',
Expand Down
118 changes: 112 additions & 6 deletions packages/flutter/test/rendering/editable_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ import 'package:flutter_test/flutter_test.dart';

import 'rendering_tester.dart';

double _caretMarginOf(RenderEditable renderEditable) {
return renderEditable.cursorWidth + 1.0;
}

void _applyParentData(List<RenderBox> inlineRenderBoxes, InlineSpan span) {
int index = 0;
RenderBox? previousBox;
Expand Down Expand Up @@ -1184,8 +1188,107 @@ void main() {
});

group('hit testing', () {
final TextSelectionDelegate delegate = _FakeEditableTextState();

test('Basic TextSpan Hit testing', () {
final TextSpan textSpanA = TextSpan(text: 'A' * 10);
const TextSpan textSpanBC = TextSpan(text: 'BC', style: TextStyle(letterSpacing: 26.0));

final TextSpan text = TextSpan(
text: '',
style: const TextStyle(fontSize: 10.0),
children: <InlineSpan>[textSpanA, textSpanBC],
);

final RenderEditable renderEditable = RenderEditable(
text: text,
maxLines: null,
startHandleLayerLink: LayerLink(),
endHandleLayerLink: LayerLink(),
textDirection: TextDirection.ltr,
offset: ViewportOffset.fixed(0.0),
textSelectionDelegate: delegate,
selection: const TextSelection.collapsed(offset: 0),
);
layout(renderEditable, constraints: BoxConstraints.tightFor(width: 100.0 + _caretMarginOf(renderEditable)));

BoxHitTestResult result;

// Hit-testing the first line
// First A
expect(renderEditable.hitTest(result = BoxHitTestResult(), position: const Offset(5.0, 5.0)), isTrue);
expect(result.path.map((HitTestEntry<HitTestTarget> entry) => entry.target).whereType<TextSpan>(), <TextSpan>[textSpanA]);
// The last A.
expect(renderEditable.hitTest(result = BoxHitTestResult(), position: const Offset(95.0, 5.0)), isTrue);
expect(result.path.map((HitTestEntry<HitTestTarget> entry) => entry.target).whereType<TextSpan>(), <TextSpan>[textSpanA]);
// Far away from the line.
expect(renderEditable.hitTest(result = BoxHitTestResult(), position: const Offset(200.0, 5.0)), isFalse);
expect(result.path.map((HitTestEntry<HitTestTarget> entry) => entry.target).whereType<TextSpan>(), <TextSpan>[]);

// Hit-testing the second line
// Tapping on B (startX = letter-spacing / 2 = 13.0).
expect(renderEditable.hitTest(result = BoxHitTestResult(), position: const Offset(18.0, 15.0)), isTrue);
expect(result.path.map((HitTestEntry<HitTestTarget> entry) => entry.target).whereType<TextSpan>(), <TextSpan>[textSpanBC]);

// Between B and C, with large letter-spacing.
expect(renderEditable.hitTest(result = BoxHitTestResult(), position: const Offset(31.0, 15.0)), isTrue);
expect(result.path.map((HitTestEntry<HitTestTarget> entry) => entry.target).whereType<TextSpan>(), <TextSpan>[textSpanBC]);

// On C.
expect(renderEditable.hitTest(result = BoxHitTestResult(), position: const Offset(54.0, 15.0)), isTrue);
expect(result.path.map((HitTestEntry<HitTestTarget> entry) => entry.target).whereType<TextSpan>(), <TextSpan>[textSpanBC]);

// After C.
expect(renderEditable.hitTest(result = BoxHitTestResult(), position: const Offset(100.0, 15.0)), isTrue);
expect(result.path.map((HitTestEntry<HitTestTarget> entry) => entry.target).whereType<TextSpan>(), <TextSpan>[]);

// Not even remotely close.
expect(renderEditable.hitTest(result = BoxHitTestResult(), position: const Offset(9999.0, 9999.0)), isFalse);
expect(result.path.map((HitTestEntry<HitTestTarget> entry) => entry.target).whereType<TextSpan>(), <TextSpan>[]);
});

test('TextSpan Hit testing with text justification', () {
const TextSpan textSpanA = TextSpan(text: 'A '); // The space is a word break.
const TextSpan textSpanB = TextSpan(text: 'B\u200B'); // The zero-width space is used as a line break.
final TextSpan textSpanC = TextSpan(text: 'C' * 10); // The third span starts a new line since it's too long for the first line.

// The text should look like:
// A B
// CCCCCCCCCC
final TextSpan text = TextSpan(
text: '',
style: const TextStyle(fontSize: 10.0),
children: <InlineSpan>[textSpanA, textSpanB, textSpanC],
);
final RenderEditable renderEditable = RenderEditable(
text: text,
maxLines: null,
startHandleLayerLink: LayerLink(),
endHandleLayerLink: LayerLink(),
textDirection: TextDirection.ltr,
textAlign: TextAlign.justify,
offset: ViewportOffset.fixed(0.0),
textSelectionDelegate: delegate,
selection: const TextSelection.collapsed(offset: 0),
);

layout(renderEditable, constraints: BoxConstraints.tightFor(width: 100.0 + _caretMarginOf(renderEditable)));
BoxHitTestResult result;

// Tapping on A.
expect(renderEditable.hitTest(result = BoxHitTestResult(), position: const Offset(5.0, 5.0)), isTrue);
expect(result.path.map((HitTestEntry<HitTestTarget> entry) => entry.target).whereType<TextSpan>(), <TextSpan>[textSpanA]);

// Between A and B.
expect(renderEditable.hitTest(result = BoxHitTestResult(), position: const Offset(50.0, 5.0)), isTrue);
expect(result.path.map((HitTestEntry<HitTestTarget> entry) => entry.target).whereType<TextSpan>(), <TextSpan>[textSpanA]);

// On B.
expect(renderEditable.hitTest(result = BoxHitTestResult(), position: const Offset(95.0, 5.0)), isTrue);
expect(result.path.map((HitTestEntry<HitTestTarget> entry) => entry.target).whereType<TextSpan>(), <TextSpan>[textSpanB]);
});

test('hits correct TextSpan when not scrolled', () {
final TextSelectionDelegate delegate = _FakeEditableTextState();
final RenderEditable editable = RenderEditable(
text: const TextSpan(
style: TextStyle(height: 1.0, fontSize: 10.0),
Expand Down Expand Up @@ -1692,7 +1795,8 @@ void main() {
// Prepare for painting after layout.
pumpFrame(phase: EnginePhase.compositingBits);
BoxHitTestResult result = BoxHitTestResult();
editable.hitTest(result, position: Offset.zero);
// The WidgetSpans have a height of 14.0, so "test" has a y offset of 4.0.
editable.hitTest(result, position: const Offset(1.0, 5.0));
// We expect two hit test entries in the path because the RenderEditable
// will add itself as well.
expect(result.path, hasLength(2));
Expand All @@ -1702,7 +1806,7 @@ void main() {
// Only testing the RenderEditable entry here once, not anymore below.
expect(result.path.last.target, isA<RenderEditable>());
result = BoxHitTestResult();
editable.hitTest(result, position: const Offset(15.0, 0.0));
editable.hitTest(result, position: const Offset(15.0, 5.0));
expect(result.path, hasLength(2));
target = result.path.first.target;
expect(target, isA<TextSpan>());
Expand Down Expand Up @@ -1775,7 +1879,8 @@ void main() {
// Prepare for painting after layout.
pumpFrame(phase: EnginePhase.compositingBits);
BoxHitTestResult result = BoxHitTestResult();
editable.hitTest(result, position: Offset.zero);
// The WidgetSpans have a height of 14.0, so "test" has a y offset of 4.0.
editable.hitTest(result, position: const Offset(0.0, 4.0));
// We expect two hit test entries in the path because the RenderEditable
// will add itself as well.
expect(result.path, hasLength(2));
Expand All @@ -1785,13 +1890,14 @@ void main() {
// Only testing the RenderEditable entry here once, not anymore below.
expect(result.path.last.target, isA<RenderEditable>());
result = BoxHitTestResult();
editable.hitTest(result, position: const Offset(15.0, 0.0));
editable.hitTest(result, position: const Offset(15.0, 4.0));
expect(result.path, hasLength(2));
target = result.path.first.target;
expect(target, isA<TextSpan>());
expect((target as TextSpan).text, text);

result = BoxHitTestResult();
// "test" is 40 pixel wide.
editable.hitTest(result, position: const Offset(41.0, 0.0));
expect(result.path, hasLength(3));
target = result.path.first.target;
Expand All @@ -1814,7 +1920,7 @@ void main() {

result = BoxHitTestResult();
editable.hitTest(result, position: const Offset(5.0, 15.0));
expect(result.path, hasLength(2));
expect(result.path, hasLength(1)); // Only the RenderEditable.
}, skip: isBrowser); // https://github.com/flutter/flutter/issues/61020
});

Expand Down
Loading

0 comments on commit ea5b972

Please sign in to comment.