Use glyph boundaries for horizontal character traversal by tgucio · Pull Request #178258 · flutter/flutter

tgucio · 2025-11-10T21:41:41Z

This PR implements horizontal character traversal (using arrow keys) in text fields and paragraphs using actual glyph information from text layout, as opposed to "guessing" character boundaries from the underlying text. This allows for correct horizontal caret traversal of RTL text as well as correctly positioning the caret between characters in complex scripts (e.g. Indic) that rely on text shaping.

Before:

After:

Fixes #34610
Fixes #78660
Maybe fixes #120049

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].
I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
All existing and new tests are passing.

gemini-code-assist

Code Review

This pull request introduces a GlyphBoundary to enable correct caret traversal in text with mixed right-to-left and left-to-right scripts, which is a great improvement. The implementation is mostly solid, but the core logic in _moveBeyondGlyphBoundary within editable_text.dart has some critical issues. There are TODO comments indicating failing tests and untested edge cases that must be addressed. The logic for handling LTR/RTL transitions is also very complex, potentially buggy, and has formatting issues that violate the repository's style guide. I've left detailed comments on these points.

packages/flutter/lib/src/widgets/editable_text.dart

gemini-code-assist · 2025-11-10T21:44:24Z

packages/flutter/lib/src/widgets/editable_text.dart

+      // LTR/RTL or RTL/LTR boundary: resort to searching for the next glyph.
+      if (boundary.isNormalized) {
+        // LTR->RTL: start skipping at the next boundary.
+        do {
+          boundary = nextBoundary;
+          offset = math.max(0, nextOffset);
+          nextOffset = forward ? boundary.start : boundary.end - 1;
+          nextBoundary = textBoundary.getTextBoundaryAt(nextOffset);
+        } while (nextOffset < _value.text.length
+            && nextOffset >= 0
+            && nextBoundary.isValid
+            && nextBoundary.isNormalized == boundary.isNormalized);
+          offset = math.max(0, boundary.start);
+          affinity = forward ? TextAffinity.upstream : TextAffinity.downstream;
+      } else {
+        // RTL->LTR: start skipping at the current boundary.
+        nextBoundary = boundary;
+        do {
+          boundary = nextBoundary;
+          offset = math.max(0, nextOffset);
+          nextOffset = forward ? boundary.start : boundary.end - 1;
+          nextBoundary = textBoundary.getTextBoundaryAt(nextOffset);
+        } while (nextOffset < _value.text.length
+            && nextOffset >= 0
+            && nextBoundary.isValid
+            && nextBoundary.isNormalized == boundary.isNormalized);
+          offset = math.max(0, nextBoundary.end);
+          affinity = forward ? TextAffinity.downstream : TextAffinity.upstream;
+      }
+    }


The logic for handling LTR/RTL boundary transitions is quite complex and hard to follow. For instance, the offset variable is updated inside the do-while loops but this value seems to be unused, as it's immediately overwritten after the loop. This could be a potential source of bugs.

Additionally, the indentation for the lines after the do-while loops (lines 5330-5331 and 5344-5345) seems incorrect. They should be indented to be within their respective if/else blocks. This appears to be a violation of the Flutter Style Guide which requires all code to be formatted with dart format.¹

Given the complexity and the existing TODOs about failing tests, this section might benefit from simplification or more extensive comments explaining the state transitions. Could you refactor this to improve clarity and correctness, perhaps by breaking it down into smaller helper methods and fixing the formatting?

Style Guide References

Footnotes

All Dart code is formatted using dart format. This is enforced by CI. ↩

LongCatIsLooong · 2025-11-25T22:29:41Z

Sorry for the delay.

TextPainter is where the caret positioning logic lives so you may want to take look at its getOffsetForCaret implementation instead of doing this in RenderEditable. IIRC, TextPainter currently does not expose information about glyph clusters (the PR seems to be using graphemeClusterRange, that's the text range of the grapheme cluster instead of the glyph cluster, that's actually what TextPainter currently uses to calculate the caret position).

I havent looked into how bidi traversal is implemented in this PR. However with SKParagraph not exposing the bidi regions in the text with the associated bidi levels, I suspect there isn't an easy algorithm that can handle nested bidi regions, by just looking at the writing direction of each grapheme cluster. To implement it correctly I think you would have to implement the unicode bidi algorithm in dart (and unfortunately I don't think we have access to the unicode character properties) to get the bidi regions and then compute the traversal order.

tgucio · 2025-11-26T08:59:47Z

@LongCatIsLooong the implementation in this PR is actually based on ui.Paragraph.getGlyphInfoAt which exposes glyph direction. The logic to get boundaries is implemented in GlyphBoundary as an extension of TextBoundary to keep the current design in text_painter.dart. The idea is to use actual rendered glyph information instead of relying on the Characters package to "guess" where the glyphs should start/end.

flutter-dashboard · 2025-12-16T09:01:27Z

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

justinmc

@tgucio I really appreciate this ambitious PR. I left some quick thoughts but I'm interested in whether @LongCatIsLooong thinks this is feasible or not.

justinmc · 2025-12-29T18:56:39Z

packages/flutter/lib/src/widgets/editable_text.dart

+    }
+
+    TextAffinity affinity = TextAffinity.downstream;
+    TextRange boundary = textBoundary.getTextBoundaryAt(offset);


Does TextBoundary.getTextBoundaryAt possibly require iterating the entire string? I'm thinking about what the performance implications of this PR could be. I assume it will be negligible in most cases, but we should think it through.

That's correct, in the worst case (single text direction change), this would traverse the string towards either end. But I think the current WordBoundary implementation does so too.

justinmc · 2025-12-29T18:58:05Z

packages/flutter/test/widgets/editable_text_test.dart

We should probably include a high level test that actually puts a bidirectional string into an EditableText and traverses it with arrow keys.

Agree, this needs a couple of new tests here and there.

justinmc · 2025-12-29T18:58:57Z

packages/flutter/lib/src/widgets/editable_text.dart

        this,
        _characterBoundary,
-        _moveBeyondTextBoundary,
+        _moveBeyondGlyphBoundary,


Are there any other places that would benefit from _moveBeyondGlyphBoundary?

Good question there. Perhaps Paragraph for changing text selection?

justinmc · 2025-12-29T18:59:40Z

packages/flutter/lib/src/widgets/editable_text.dart

+  TextPosition _moveBeyondGlyphBoundary(
+    TextPosition extent,
+    bool forward,
+    TextBoundary textBoundary,
+  ) {


Nit: If you accept _value as a parameter, could this be static? Just thinking it might make this complex method easier to understand if so.

justinmc · 2026-01-29T21:49:42Z

CC @LongCatIsLooong to follow up from triage, thanks.

LongCatIsLooong · 2026-01-29T23:42:43Z

Oh I only realized this is implementing visual order traversal after seeing the before and after gifs (I think @Renzo-Olivares or @loic-sharma might be interested). But still, I don't think SkParagraph currently exposes enough information for us to implement visual order traversal properly. Here's an example that I think we won't be able to handle without the bidi region information from SkParagraph (or maybe just ICU but SkParagraph needs that info for layout and already maintains the info for each character IIRC):
aBcDe. This can be render as aBcDe or aDcBe based on whether BcD are in the same bidi region. So we don't know if the next character visually after a is B or D without the bidi region info. Technically you can still get which character is visually after a using the hit-test method (getClosestGlyphAtPosition or something with a similar name) but that's going to be very slow and painful.

Renzo-Olivares · 2026-02-19T21:30:58Z

Hi @tgucio, thank you for your contribution here. I've been meaning to review this pull request recently since it aligns with the RTL work I wanted to work on this quarter but I haven't had the bandwidth yet. Will try to get to this soon, appreciate the patience!

Renzo-Olivares · 2026-03-17T23:38:05Z

Had a few initial thoughts.

From testing on macOS web (chrome/safari/firefox), chrome seems to prefer logical order traversal for text selection with keyboard shortcuts. Safari and Firefox surprisingly differs in that it uses visual order for collapsed selection but logical order for uncollapsed selection with keyboard shortcuts.

On native, SwiftUI on macOS, visual order traversal was used, except with uncollapsed selections the selection could not span a mixed region. An interesting find is that in the Notes app selection can span a mixed region.

On windows web (chrome)/and native (Notepad/Word) it also seems to prefer logic order traversal.

Still need to try on a linux machine (my guess is logical).

Because this behavior varies per platform i'm leaning towards guarding this with a platform check so only macOS native and Safari get this behavior. Generally I think this would be a good addition to the framework as it improves our fidelity with Apple platforms.

Renzo-Olivares · 2026-03-17T23:49:15Z

I noticed some issues with uncollapsed selections when selecting near or across mixed regions, is it expected that the selection jumps from the beginning of "c" to encompass the entire rtl region? I think if we were going on visual order I would expect every shift + right to select a single character to the right. Is a goal of this PR to also have continuous selections between mixed regions? If not then I think a reasonable first step would be to just enable this glyph traversal for collapsed selections.

Screen.Recording.2026-03-17.at.4.44.17.PM.mov

Renzo-Olivares · 2026-03-18T00:05:23Z

Thank you for your patience on this one and for the amazing contributions!

Initial implementation

699ef6c

github-actions 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 Nov 10, 2025

tgucio requested review from LongCatIsLooong and justinmc November 10, 2025 21:43

gemini-code-assist bot reviewed Nov 10, 2025

View reviewed changes

tgucio changed the title ~~Initial implementation~~ Use glyph boundaries for horizontal character traversal Nov 11, 2025

tgucio added 2 commits November 11, 2025 16:57

dart format

fd8a9a2

Format paragraph.dart

31c124f

darshankawar mentioned this pull request Nov 24, 2025

TextField cursor splits Arabic characters when editing (RTL caret placed inside glyph) #178945

Open

justinmc reviewed Dec 29, 2025

View reviewed changes

Renzo-Olivares self-requested a review January 15, 2026 21:59

Conversation

tgucio commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

gemini-code-assist bot Nov 10, 2025

Choose a reason for hiding this comment

Style Guide References

Footnotes

Uh oh!

LongCatIsLooong commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgucio commented Nov 26, 2025

Uh oh!

flutter-dashboard bot commented Dec 16, 2025

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

tgucio Jan 30, 2026

Choose a reason for hiding this comment

Uh oh!

justinmc Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

tgucio Jan 30, 2026

Choose a reason for hiding this comment

Uh oh!

justinmc Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

tgucio Jan 30, 2026

Choose a reason for hiding this comment

Uh oh!

justinmc Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

justinmc commented Jan 29, 2026

Uh oh!

LongCatIsLooong commented Jan 29, 2026

Uh oh!

Renzo-Olivares commented Feb 19, 2026

Uh oh!

Renzo-Olivares commented Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Renzo-Olivares commented Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Renzo-Olivares commented Mar 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tgucio commented Nov 10, 2025 •

edited

Loading

LongCatIsLooong commented Nov 25, 2025 •

edited

Loading

Renzo-Olivares commented Mar 17, 2026 •

edited

Loading

Renzo-Olivares commented Mar 17, 2026 •

edited

Loading