Make `TextSpan` hit testing precise. #139717

LongCatIsLooong · 2023-12-07T03:45:25Z

Fixes #131435, #104594, #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:

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.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

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

goderbauer · 2023-12-11T21:35:33Z

packages/flutter_test/lib/src/controller.dart

@@ -3,6 +3,7 @@
 // found in the LICENSE file.

 import 'package:clock/clock.dart';
+import 'package:collection/collection.dart';


Currently, flutter_test doesn't directly depend on package:collection (it's just a transitive dependency). We'd need to update the pubspec to reflect this. What is this using from package:collection? There have been some efforts (albeit currently stalled) to remove pkg:collection as a dependency from flutter, this would move us in the opposite direction.

I used first where or null. Turned that into a for loop.

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

goderbauer · 2023-12-11T21:41:45Z

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

-  /// 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 glyph is not in the visible
+  /// range.


for my own understanding (and maybe to clarify in the docs): What is the "visible range"?

Ah I copied the documentation from another method and that doesn't make sense in this context.
This method returns null if the text is empty, or is entirely clipped or ellipsized. I'll fix that in the engine PR.

goderbauer · 2023-12-11T21:43:50Z

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

+  ///
+  /// This method can be used to implement per-glyph hit-testing. The returned
+  /// [GlyphInfo] can help determine whether the given `offset` directly hits a
+  /// glyph in the paragraph.


Maybe clarify in the doc: What do I need to do with the GlyphInfo to figure out whether the glyph was directly hit or not? (e.g. check that offset is inside graphemeClusterLayoutBounds)

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

goderbauer · 2023-12-11T22:33:11Z

packages/flutter_test/lib/src/controller.dart

+  /// This method currently only works on static text widgets ([Text] or
+  /// [RichText] for example) that use [RenderParagraph] under the hood. It
+  /// currently does not support finding substrings in [TextField]s or


Remove the "currently"s in these two sentences?

"does not support" => will it throw if it find the substring in TextFields? Will it just not see that text at all? Maybe be a little more specific what "not supported" means.

Right now it skips RenderEditables because it's not need for my migration. Removed the paragraph because the limitation is the finder not this method.

goderbauer · 2023-12-11T22:35:58Z

packages/flutter_test/lib/src/controller.dart

+  /// throws a [FlutterError].
+  ///
+  /// If the target substring contains more than one hit-testable [InlineSpan]s,
+  /// [tapOnText] taps on one of them, but does not guarantee which.


Huh? The first paragraph said it would hit the first one. This one says it doesn't guarantee which one. Can you unify these two statements? Also, if the "no guarantee" statement is the correct one: Why can we not guarantee which one gets hit? If we cannot define which one gets hit, should we instead throw in this case and make that illegal? Is it at least deterministic which gets hit?

I don't want users to rely on this, the real order can be a bit hard to reason about because of bidi reordering and fallback fonts being used (the getBoxesForSelection method may return different TextBoxes, and combine adjacent TextBoxes into one).

Updated the first paragraph.

goderbauer · 2023-12-11T22:37:21Z

packages/flutter_test/lib/src/controller.dart

+  /// [RichText] for example) that use [RenderParagraph] under the hood. It
+  /// currently does not support finding substrings in [TextField]s or
+  /// [SelectableText].
+  Future<void> tapOnText(finders.FinderBase<finders.TextRangeContext> textRangeFinder, {int? pointer, int buttons = kPrimaryButton }) {


Document the other parameters

packages/flutter_test/lib/src/controller.dart

Needed for flutter/flutter#139717 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

goderbauer

LGTM

goderbauer · 2023-12-13T23:04:46Z

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

@@ -1492,8 +1494,24 @@ class TextPainter {
      ? boxes
      : boxes.map((TextBox box) => _shiftTextBox(box, offset)).toList(growable: false);
  }
+  /// Returns the [GlyphInfo] of the glyph closest to the given `offset` in the


nit: add blank line before this line?

goderbauer · 2023-12-13T23:05:43Z

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

@@ -1492,8 +1494,24 @@ class TextPainter {
      ? boxes
      : boxes.map((TextBox box) => _shiftTextBox(box, offset)).toList(growable: false);
  }
+  /// Returns the [GlyphInfo] of the glyph closest to the given `offset` in the
+  /// paragraph coordinate system, or null if if the text is empty, or is


remove one of the "if"s?

goderbauer · 2023-12-13T23:10:06Z

packages/flutter/test/widgets/slivers_padding_test.dart

+      // This expect always fails.
+      expect(target, anyOf(isA<TextSpan>(), isA<RenderParagraph>()));


Why not use fail then to make it explicit that this is a failure case and provide a failure message?

https://main-api.flutter.dev/flutter/package-matcher_expect/fail.html

goderbauer · 2023-12-13T23:45:09Z

packages/flutter_test/lib/src/controller.dart

+
+/// Helper function that returns 3 special Offsets within the given `box`, to
+/// try performing hit-testing at.
+Iterable<Offset> _testOffsetsForEachTextBox(TextBox box) {


Could we just hit test the center, similar to https://main-api.flutter.dev/flutter/flutter_test/WidgetController/tap.html?

If we do this, let's also mention it in the docs.

I think it's a bit hard to explain how the glyphs are grouped into boxes, if we want to mention hit testing the center of TextBoxes? If the specified font doesn't have glyphs for some characters in the substring then these glyphs will usually be in separate TextBoxes. The 3rd paragraph says the substring must not be obstructed or offscreen, does that work?

goderbauer · 2023-12-13T23:49:01Z

packages/flutter_test/lib/src/finders.dart

+  /// If the `skipOffstage` argument is true (the default), then this skips
+  /// static text inside widgets that are [Offstage], or that are from inactive
+  /// [Route]s.
+  ///


Maybe document how overlapping substrings are handled.

chunhtai

LGTM, just some questions

chunhtai · 2023-12-14T20:39:36Z

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

+    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) {


Can this be replaced by getClosestGlyphForOffset?

I believe so.

chunhtai · 2023-12-14T20:40:48Z

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

-    final InlineSpan? textSpan = _textPainter.text;
-    switch (textSpan?.getSpanForPosition(_textPainter.getPositionForOffset(effectivePosition))) {
+    final GlyphInfo? glyph = _textPainter.getClosestGlyphForOffset(effectivePosition);
+    // This works even with large letter-spacing and text justification, as


reading this i am not sure what works mean. Do you mean it will miss when click in between character? or it would still hit the span. I think it is the latter, but would be good to be more clear

It would still hit the span (same behavior as getSpanForPosition ). I'll update the comment.

chunhtai · 2023-12-14T20:41:23Z

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

-    switch (textSpan?.getSpanForPosition(_textPainter.getPositionForOffset(effectivePosition))) {
+    final GlyphInfo? glyph = _textPainter.getClosestGlyphForOffset(effectivePosition);
+    // This works even with large letter-spacing and text justification, as
+    // graphemeClusterLayoutBounds.width is the x layout advance to the next


I can guess what x layout advance mean, but is it a term that commonly used? If not, we should probably explain what this mean

In https://freetype.org/freetype2/docs/glyphs/glyphs-3.html it's called advance width. I can change to that if you prefer it over x layout advance?

TIL, yes I think using advance width may be more clear

flutter/flutter@0eb7881...da0cd69 2023-12-20 git@reb0.org Revert automated changes made to deprecated settings.gradle (plugins.each) (flutter/flutter#140037) 2023-12-20 xilaizhang@google.com [github actions] change minimal example workflow to be manually dispatched (flutter/flutter#140435) 2023-12-20 31859944+LongCatIsLooong@users.noreply.github.com Make `TextSpan` hit testing precise. (flutter/flutter#139717) 2023-12-20 xilaizhang@google.com [github actions] add minimal workflow to test token (flutter/flutter#140363) 2023-12-20 engine-flutter-autoroll@skia.org Roll Flutter Engine from 5279873a8635 to c70f0a495ace (2 revisions) (flutter/flutter#140431) 2023-12-20 engine-flutter-autoroll@skia.org Roll Flutter Engine from df1593e96a6b to 5279873a8635 (2 revisions) (flutter/flutter#140428) 2023-12-20 christopherfujino@gmail.com [flutter_tools] handle FileSystemException trying to delete temp directory from core_devices.dart (flutter/flutter#140415) 2023-12-19 zanderso@users.noreply.github.com Move hybrid_android_views_integration_test back to Moto G4 (flutter/flutter#140421) 2023-12-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3b156c8ce9bd to df1593e96a6b (2 revisions) (flutter/flutter#140422) 2023-12-19 72284940+feduke-nukem@users.noreply.github.com Added onEnd callback into AnimatedSize (flutter/flutter#139859) 2023-12-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3f45f9db4471 to 3b156c8ce9bd (1 revision) (flutter/flutter#140413) 2023-12-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from 187334c39b44 to 3f45f9db4471 (3 revisions) (flutter/flutter#140412) 2023-12-19 15619084+vashworth@users.noreply.github.com Remove workarounds for `plugin_lint_mac` needed for older version of Cocoapods (flutter/flutter#140395) 2023-12-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from b87a782ce3a3 to 187334c39b44 (2 revisions) (flutter/flutter#140398) 2023-12-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from 1d5a141917fa to b87a782ce3a3 (1 revision) (flutter/flutter#140390) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC bmparr@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

This reverts commit ea5b972.

Reverts #139717 Initiated by: LongCatIsLooong This change reverts the following previous change: Original Description: Fixes #131435, #104594, #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.

Extracted from #139717 as-is. Landing this change first so we can avoid doing a g3fix.

…r#5729) flutter/flutter@0eb7881...da0cd69 2023-12-20 git@reb0.org Revert automated changes made to deprecated settings.gradle (plugins.each) (flutter/flutter#140037) 2023-12-20 xilaizhang@google.com [github actions] change minimal example workflow to be manually dispatched (flutter/flutter#140435) 2023-12-20 31859944+LongCatIsLooong@users.noreply.github.com Make `TextSpan` hit testing precise. (flutter/flutter#139717) 2023-12-20 xilaizhang@google.com [github actions] add minimal workflow to test token (flutter/flutter#140363) 2023-12-20 engine-flutter-autoroll@skia.org Roll Flutter Engine from 5279873a8635 to c70f0a495ace (2 revisions) (flutter/flutter#140431) 2023-12-20 engine-flutter-autoroll@skia.org Roll Flutter Engine from df1593e96a6b to 5279873a8635 (2 revisions) (flutter/flutter#140428) 2023-12-20 christopherfujino@gmail.com [flutter_tools] handle FileSystemException trying to delete temp directory from core_devices.dart (flutter/flutter#140415) 2023-12-19 zanderso@users.noreply.github.com Move hybrid_android_views_integration_test back to Moto G4 (flutter/flutter#140421) 2023-12-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3b156c8ce9bd to df1593e96a6b (2 revisions) (flutter/flutter#140422) 2023-12-19 72284940+feduke-nukem@users.noreply.github.com Added onEnd callback into AnimatedSize (flutter/flutter#139859) 2023-12-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3f45f9db4471 to 3b156c8ce9bd (1 revision) (flutter/flutter#140413) 2023-12-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from 187334c39b44 to 3f45f9db4471 (3 revisions) (flutter/flutter#140412) 2023-12-19 15619084+vashworth@users.noreply.github.com Remove workarounds for `plugin_lint_mac` needed for older version of Cocoapods (flutter/flutter#140395) 2023-12-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from b87a782ce3a3 to 187334c39b44 (2 revisions) (flutter/flutter#140398) 2023-12-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from 1d5a141917fa to b87a782ce3a3 (1 revision) (flutter/flutter#140390) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC bmparr@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

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.

Reverts flutter#139717 Initiated by: LongCatIsLooong This change reverts the following previous change: Original Description: 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.

Extracted from flutter#139717 as-is. Landing this change first so we can avoid doing a g3fix.