Use glyph boundaries for horizontal character traversal #178258
Conversation
There was a problem hiding this comment.
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.
| // 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; | ||
| } | ||
| } |
There was a problem hiding this comment.
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.1
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. ↩
|
Sorry for the delay.
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. |
|
@LongCatIsLooong the implementation in this PR is actually based on |
|
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 Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
justinmc
left a comment
There was a problem hiding this comment.
@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.
| } | ||
|
|
||
| TextAffinity affinity = TextAffinity.downstream; | ||
| TextRange boundary = textBoundary.getTextBoundaryAt(offset); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We should probably include a high level test that actually puts a bidirectional string into an EditableText and traverses it with arrow keys.
| this, | ||
| _characterBoundary, | ||
| _moveBeyondTextBoundary, | ||
| _moveBeyondGlyphBoundary, |
There was a problem hiding this comment.
Are there any other places that would benefit from _moveBeyondGlyphBoundary?
| TextPosition _moveBeyondGlyphBoundary( | ||
| TextPosition extent, | ||
| bool forward, | ||
| TextBoundary textBoundary, | ||
| ) { |
There was a problem hiding this comment.
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.
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
///).