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

gnprice · 2023-03-12T01:53:59Z

Fixes #50563.
Fixes #98516.
Fixes #118403.
Fixes #122477.

It turns out that Paragraph.getBoxesForRange will not accept a negative index for the start of the range; if you pass one, it will return an empty list. We'd been doing that in TextPainter._getRectFromUpstream; this was causing #50563, #98516, and #118403. (Arguably this makes them duplicates of each other, but they looked quite distinct until being debugged: they involve complex emoji, zalgo text, and Indic script respectively.)

Fix that by clamping the start to zero, because what we really intended here in that case is the whole text up to the given end offset.

Then it turns out that we were sometimes reaching this line with a negative end index too (when the selection is empty on reaching RenderEditable._updateSelectionExtentsVisibility.) That was giving an empty list of boxes, but if we clamp the start but not the end, we'd get back boxes for the whole text instead. Avoid that by short-circuiting that case at the top of the function, like we do when the offset is out of bounds in the other direction.

Finally, when we get back more than one box from getBoxesForRange, we were looking at the wrong end of the list: if we're looking upstream, we want the end of the last preceding box, not the first one. Similarly when looking downstream we want the start of the first following box, not the last one. This was causing #122477, which I discovered while studying this code to diagnose #50563. Fix that too.

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.

…rics Fixes flutter#50563. Fixes flutter#98516. Fixes flutter#118403. Fixes flutter#122477. It turns out that `Paragraph.getBoxesForRange` will not accept a negative index for the start of the range; if you pass one, it will return an empty list. We'd been doing that in `TextPainter._getRectFromUpstream`; this was causing flutter#50563, flutter#98516, and flutter#118403. (Arguably this makes them duplicates of each other, but they looked quite distinct until being debugged: they involve complex emoji, zalgo text, and Indic script respectively.) Fix that by clamping the start to zero, because what we really intended here in that case is the whole text up to the given end offset. Then it turns out that we were sometimes reaching this line with a negative end index too (when the selection is empty on reaching `RenderEditable._updateSelectionExtentsVisibility`.) That was giving an empty list of boxes, but if we clamp the start but not the end, we'd get back boxes for the whole text instead. Avoid that by short-circuiting that case at the top of the function, like we do when the offset is out of bounds in the other direction. Finally, when we get back more than one box from `getBoxesForRange`, we were looking at the wrong end of the list: if we're looking upstream, we want the end of the *last* preceding box, not the first one. Similarly when looking downstream we want the start of the first following box, not the last one. This was causing flutter#122477, which I discovered while studying this code to diagnose flutter#50563. Fix that too.

gnprice · 2023-03-12T02:13:16Z

There are also several issues in the tracker that sound potentially related to the issues this fixes but involve RTL text:

or WidgetSpans:

WidgetSpan wrong caret position #107432

I haven't yet investigated whether this fixes any of those issues.

gnprice · 2023-03-19T22:12:10Z

Resolved conflicts after #120834.

LongCatIsLooong

Thanks for looking into this! LGTM overall but I think it might be worth looking at the callers of getBoxesForRange and fixing them as well.

On the other hand currently we do expose getBoxesForRange as a public API (also there're some other APIs that provide similar functionalities) but it's not clear to me how they should behave when given an OOB index. I'll create an issue for that.

LongCatIsLooong · 2023-03-20T15:39:37Z

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

  _CaretMetrics? _getMetricsFromUpstream(int offset) {
    final int plainTextLength = plainText.length;
-    if (plainTextLength == 0 || offset > plainTextLength) {
+    if (plainTextLength == 0 || offset > plainTextLength || offset < 0) {


I think the framework is not supposed to query the caret location at a negative offset (-1 is used to indicates the caret is placed at an invalid location and thus should be invisible, for historical reasons). Maybe this should be an assert, and the caller should sanitize the input before calling getOffsetForCaret?

Sure, I can take a look at doing that. I think it was only after I wrote this PR that I found #95978 (comment) and #79615 and understood the context of that -1 better.

gnprice · 2023-03-20T17:50:46Z

LGTM overall but I think it might be worth looking at the callers of getBoxesForRange and fixing them as well.

Makes sense. I just took a quick look, and there's only two other call sites in the tree: one a benchmark that never gives it OOB indexes, and one in [TextPainter.getBoxesForSelection]:

  List<TextBox> getBoxesForSelection(
    TextSelection selection, /* … */) {
    assert(_debugAssertTextLayoutIsValid);
    return _paragraph!.getBoxesForRange(
      selection.start,
      selection.end,

So that looks like it can currently pass OOB indexes, though only in a harmless way: it'll be (-1, -1), and as I found above, a negative start index produces an empty list.

Better would be for that to be explicit in a check before calling getBoxesForRange, though:

    if (!selection.isValid) {
      return <TextBox>[];
    }
    return _paragraph!.getBoxesForRange(
      selection.start,
      selection.end,

I can add that to the PR.

gnprice · 2023-03-21T03:20:01Z

Revised. Thanks for the review!

I took the liberty of adding a couple of TODO comments referring to you (@LongCatIsLooong). They come about because I ran into this existing TODO comment:

    // TODO(LongCatIsLooong): skip painting the caret when the selection is
    // (-1, -1).
    if (selection == null || !selection.isCollapsed) {
      return;

From the Git history, I see that's from #79495 , for which a previous fix was reverted because fixing it was a breaking change.

exaby73 · 2023-03-21T06:30:45Z

cc: @exaby73 for later reference #120049 (comment)

LongCatIsLooong

LGTM

LongCatIsLooong · 2023-03-23T21:14:09Z

packages/flutter/test/painting/text_painter_test.dart

    bool.fromEnvironment('FLUTTER_WEB_USE_SKIA');

 void main() {
+  void checkCaretOffsetsLtrAt(String text, List<int> boundaries) {


nit: maybe move these helper functions out of the scope of main? Or create a group for the caret tests and the helper functions?

Sure. Moved them out of the scope of main, and then also took the occasion to expand on the docs a bit more.

LongCatIsLooong · 2023-03-23T22:41:17Z

packages/flutter/test/painting/text_painter_test.dart

+      ..text = text
+      ..layout(maxWidth: 10000);
+    final int length = text.toPlainText().length;
+    final List<double> result =


should be on the same line?

LongCatIsLooong · 2023-03-23T22:44:21Z

packages/flutter/test/painting/text_painter_test.dart

+    final List<double> prefixWidths = <double>[];
+    for (final int boundary in boundaries) {
+      painter.text = TextSpan(text: text.substring(0, boundary));
+      painter.layout(maxWidth: 10000);


nit: I think you could just omit maxWidth here? The default value is infinity iirc.

Ah indeed, thanks for the catch.

gnprice · 2023-03-24T05:47:50Z

Thanks for the review! Pushed a version with those tweaks to the tests.

tgucio · 2023-03-24T09:16:46Z

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

        continue;
      }
-      final TextBox box = boxes.first;
+      final TextBox box = boxes.last;


This makes me wonder: does it work the same way in LTR and RTL? I could image in RTL text boxes come in display order, not logical order.

Interesting question. I'd be surprised if that was the behavior, but I can't immediately rule it out — in particular the dartdoc on getBoxesForRange doesn't say anything about the order of the boxes in the list.

... OK, and I wrote a test for this, and indeed the boxes seem to be in display order! Thanks for the catch.

I'll revise so that the change doesn't break anything that was working before. I expect it still won't work reliably in the presence of bidi, but fixing that may require bigger changes to this code.

Done; PTAL.

I also filed #123424 to track that this logic doesn't seem like it can work in the bidi case. That's an issue this PR doesn't fix, and I think is best left out of scope because it will probably require bigger changes.

tgucio

LGTM

justinmc

LGTM 👍

I think this can merge, but there's a simple merge conflict in the test.

justinmc · 2023-03-27T17:00:34Z

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


  /// Returns a list of rects that bound the given selection.
  ///
+  /// The [selection] must be a valid range (with [TextSelection.isValid] true.)


Nit: Period outside the parenthesis: true).

justinmc · 2023-03-27T17:04:56Z

packages/flutter/test/painting/text_painter_test.dart

+/// 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 .


Nit: Remove the space between the end of the GitHub link and the period.

I tend to avoid putting periods (or commas, etc.) just after a URL, because then it's ambiguous whether they're part of the URL.

But here in dartdoc, there's a <…> syntax for enclosing URLs; switched to that and cut the space. (That syntax is unambiguous because > cannot appear in a URL.)

justinmc · 2023-03-27T17:06:05Z

packages/flutter/test/painting/text_painter_test.dart

+///
+/// If you have a [String] instead of a nontrivial [TextSpan],
+/// consider using [checkCaretOffsetsLtr] instead.
+List<double> caretOffsetsForTextSpan(TextDirection textDirection, TextSpan text) {


Nit: Should these functions be private or do you expect them to be used elsewhere?

Because this file has a main function and will never be imported from, the private/public distinction doesn't mean as much as usual. I think the convention I've mostly seen in test files is to leave off the _ prefix, even for test fixtures that are firmly private in intention.

Searching, it's a bit more mixed than my impression had been, but I think still more without the _ prefix than with it: git grep -P '^(?!import\b|library\b|void main\b|//)\w.*' 'test/*_test.dart'

Given that and because these read a bit better to me without the _ prefix, I'm inclined to leave it off.

Sounds good, I've also seen both in other tests.

gnprice

Thanks for the review! Merged (i.e. resolved conflicts) and fixed nits.

gnprice · 2023-03-27T17:51:49Z

packages/flutter/test/painting/text_painter_test.dart

+/// 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 .


I tend to avoid putting periods (or commas, etc.) just after a URL, because then it's ambiguous whether they're part of the URL.

But here in dartdoc, there's a <…> syntax for enclosing URLs; switched to that and cut the space. (That syntax is unambiguous because > cannot appear in a URL.)

gnprice · 2023-03-27T18:04:11Z

packages/flutter/test/painting/text_painter_test.dart

+///
+/// If you have a [String] instead of a nontrivial [TextSpan],
+/// consider using [checkCaretOffsetsLtr] instead.
+List<double> caretOffsetsForTextSpan(TextDirection textDirection, TextSpan text) {


Because this file has a main function and will never be imported from, the private/public distinction doesn't mean as much as usual. I think the convention I've mostly seen in test files is to leave off the _ prefix, even for test fixtures that are firmly private in intention.

Searching, it's a bit more mixed than my impression had been, but I think still more without the _ prefix than with it: git grep -P '^(?!import\b|library\b|void main\b|//)\w.*' 'test/*_test.dart'

Given that and because these read a bit better to me without the _ prefix, I'm inclined to leave it off.

gnprice · 2023-03-28T02:30:28Z

Re-running customer tests after flutter/tests#239 .

…aret metrics (flutter/flutter#122480)

RomanIvn · 2023-04-03T10:50:24Z

Issues persist when using multiline text

TextField(
keyboardType: TextInputType.multiline,
maxLines: null,
textDirection: TextDirection.rtl,
textAlign: TextAlign.right,
)

gnprice · 2023-04-03T15:58:35Z

@RomanIvn Please file a new issue for the problem you're seeing. That will be a more effective place to address it than in a merged PR's thread. Do include all the information in the issue template, including details of what behavior you expected and what behavior you see instead.

Since you mention TextDirection.rtl, take a look also at these still-open issues and see if one of them describes what you're seeing:

There are also several issues in the tracker that sound potentially related to the issues this fixes but involve RTL text:

Arrow keys in RTL move the wrong way #78660

Incorrect cursor position in RTL TextField when moving between letters #110416

Text field cursor position in RTL goes to the penultimate character #118566

RTL textField Cursor movement has a problem with the last position and Emojies and text selection #120049

TextField cursor stands in a wrong position while using persian words #120604

or WidgetSpans:

WidgetSpan wrong caret position #107432

I haven't yet investigated whether this fixes any of those issues.

…aret metrics (flutter/flutter#122480)

flutter-dashboard bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. labels Mar 12, 2023

This was referenced Mar 12, 2023

Caret can fail to move on keyboard left/right arrow in Indic script #122478

Closed

Cursor always at the front when first typing some emojis #50563

Closed

Incorrect horizontal caret movement with Unicode combining characters #98516

Closed

HansMuller requested a review from LongCatIsLooong March 17, 2023 22:04

Merge remote-tracking branch 'origin/main' into HEAD

28c2fb9

LongCatIsLooong reviewed Mar 20, 2023

View reviewed changes

justinmc mentioned this pull request Mar 20, 2023

RTL textField Cursor movement has a problem with the last position and Emojies and text selection #120049

Open

gnprice added 3 commits March 20, 2023 19:29

Merge remote-tracking branch 'origin/main' into dev-textinput

1a0c5ce

Require valid selection in getBoxesForSelection

7939c6f

Require caret position be valid

ab0c5df

justinmc requested review from Renzo-Olivares and justinmc March 23, 2023 22:29

LongCatIsLooong approved these changes Mar 23, 2023

View reviewed changes

gnprice added 4 commits March 23, 2023 21:50

Merge remote-tracking branch 'origin'

8b1856e

Small nit fixes

0b37d5a

Move helper functions out of scope of main

88c4d5c

Clean up test helpers' names a bit, and write a bit more docs

d16b371

tgucio reviewed Mar 24, 2023

View reviewed changes

justinmc mentioned this pull request Mar 24, 2023

Fix caret misplaced for emoji with modifier scalar. #123136

Closed

8 tasks

Fix choice of box in RTL case, with a test

9136591

gnprice mentioned this pull request Mar 24, 2023

Caret misplaced in bidi text #123424

Closed

gnprice mentioned this pull request Mar 25, 2023

Google testing fails to report status to GitHub #123289

Closed

tgucio approved these changes Mar 25, 2023

View reviewed changes

justinmc approved these changes Mar 27, 2023

View reviewed changes

gnprice added 2 commits March 27, 2023 10:41

Merge remote-tracking branch 'origin'

24ee7fb

fix nits

72b9b09

gnprice commented Mar 27, 2023

View reviewed changes

gnprice added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 28, 2023

auto-submit bot merged commit 4811119 into flutter:master Mar 28, 2023

gnprice deleted the pr-caret-offset branch March 28, 2023 04:06

engine-flutter-autoroll mentioned this pull request Mar 28, 2023

Roll Flutter from 4a1002e2e255 to c0ddbf129e5f (29 revisions) flutter/packages#3567

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2023

4811119 Fix out-of-bounds and reversed TextBox queries in computing c…

9263a15

…aret metrics (flutter/flutter#122480)

engine-flutter-autoroll mentioned this pull request Mar 28, 2023

Roll Flutter from 4a1002e2e255 to f528f9f56ce8 (31 revisions) flutter/packages#3568

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2023

4811119 Fix out-of-bounds and reversed TextBox queries in computing c…

bb6b050

…aret metrics (flutter/flutter#122480)

engine-flutter-autoroll mentioned this pull request Mar 28, 2023

Roll Flutter from 4a1002e2e255 to 8a6cb26f1acf (32 revisions) flutter/packages#3569

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2023

4811119 Fix out-of-bounds and reversed TextBox queries in computing c…

7b427b8

…aret metrics (flutter/flutter#122480)

engine-flutter-autoroll mentioned this pull request Mar 28, 2023

Roll Flutter from 4a1002e2e255 to 12b36de8f7ac (33 revisions) flutter/packages#3570

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2023

4811119 Fix out-of-bounds and reversed TextBox queries in computing c…

f50406a

…aret metrics (flutter/flutter#122480)

reidbaker mentioned this pull request Mar 28, 2023

i123643 print java version gradle #123644

Merged

8 tasks

HansMuller mentioned this pull request Mar 31, 2023

Flutter Framework Team: Notable Commits #121415

Closed

reidbaker mentioned this pull request Apr 1, 2023

Add Java-Gradle-AGP validation to flutter analyze #123916

Merged

8 tasks

M144-Coder mentioned this pull request Apr 1, 2023

Incorrect cursor position in RTL TextField when moving between letters #110416

Closed

reidbaker mentioned this pull request Apr 5, 2023

Update getGradleVersion to ignore commented out lines #124260

Merged

8 tasks

justinmc mentioned this pull request Apr 14, 2023

Cursor is wrongly placed for some Thai text #122683

Closed

gnprice mentioned this pull request Apr 17, 2023

compose: Prototype upload-file UI zulip/zulip-flutter#63

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023

4811119 Fix out-of-bounds and reversed TextBox queries in computing c…

0c6b373

…aret metrics (flutter/flutter#122480)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023

4811119 Fix out-of-bounds and reversed TextBox queries in computing c…

68daa19

…aret metrics (flutter/flutter#122480)

gnprice mentioned this pull request Sep 8, 2023

[Windows] The cursor can not be moved by left/right keys when using Pinyin input method #123749

Closed

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

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

Uh oh!

Conversation

gnprice commented Mar 12, 2023

Pre-launch Checklist

Uh oh!

gnprice commented Mar 12, 2023

Uh oh!

gnprice commented Mar 19, 2023

Uh oh!

LongCatIsLooong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice commented Mar 20, 2023

Uh oh!

gnprice commented Mar 21, 2023

Uh oh!

exaby73 commented Mar 21, 2023

Uh oh!

LongCatIsLooong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice commented Mar 24, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgucio left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice commented Mar 28, 2023

Uh oh!

RomanIvn commented Apr 3, 2023

Uh oh!

gnprice commented Apr 3, 2023

Uh oh!

Reviewers

Assignees

Labels

gnprice left a comment •

edited

Loading