Fixing few related editing issues with LTR/RTL text#188503
Merged
Merged
Conversation
1. GetBoxesForRange in RTL 2. GetPositionForGlyph in RTL
It might need some extra work for complex cases
Still wrong end pos for RTL text with Paragraph LTR
github-actions
Bot
added
engine
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors web paragraph layout, query, and painting logic, including strut style height calculations, text position offset resolution, and glyph info retrieval, alongside adding new paragraph tests. The reviewer feedback highlights several critical issues: the use of solo: true in tests which skips other test runs, potential runtime exceptions in layout.dart due to force-unwrapping nullable font sizes and potential division by zero, incorrect glyph retrieval at the end of text, performance regressions from commenting out assert wrappers around debug logging, and various leftover debug prints, typos, and commented-out code blocks.
Comment on lines
+916
to
+918
| final ui.GlyphInfo? result = getGlyphInfoAt( | ||
| position.offset + (position.affinity == ui.TextAffinity.downstream ? 0 : -1), | ||
| ); |
Contributor
There was a problem hiding this comment.
If position.offset is equal to text.length (which happens when clicking at the end of the text) and position.affinity is downstream, position.offset + 0 evaluates to text.length. Since there is no glyph at index text.length, getGlyphInfoAt will return null. We should adjust the offset to text.length - 1 in this case to correctly retrieve the last glyph's info.
final int targetOffset = position.offset == text.length
? text.length - 1
: position.offset + (position.affinity == ui.TextAffinity.downstream ? 0 : -1);
final ui.GlyphInfo? result = getGlyphInfoAt(targetOffset);
Contributor
Author
There was a problem hiding this comment.
It is an expected behavior.
Contributor
There was a problem hiding this comment.
Can we double-check that this is an expected behavior? I think getClosestGlyphInfoForOffset is supposed to return the closest glyph, in this case the closest glyph is the last one, not null.
Answer:
The code in SkParagraph goes like this:
const PositionWithAffinity res = this->getGlyphPositionAtCoordinate(dx, dy);
SkASSERT(res.position != 0 || res.affinity != Affinity::kUpstream);
const size_t utf16Offset = res.position + (res.affinity == Affinity::kDownstream ? 0 : -1);
...
return this->getGlyphClusterAt(fUTF8IndexForUTF16Index[utf16Offset], glyphInfo);
and fixing get_position tests
Contributor
Author
|
/gemini review |
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors text layout, painting, and query logic in the Flutter Web engine, including updates to box height styles, block positioning, glyph info retrieval, and debug logging. However, two significant issues were identified in the review: first, resolving an offset to the right of a non-last line incorrectly uses the last line's blocks, causing an assertion failure; second, simplifying the codepoint position in getLineBoundary to ignore affinity introduces a regression for upstream positions at line breaks.
mdebbar
reviewed
Jun 25, 2026
Comment on lines
+916
to
+918
| final ui.GlyphInfo? result = getGlyphInfoAt( | ||
| position.offset + (position.affinity == ui.TextAffinity.downstream ? 0 : -1), | ||
| ); |
Contributor
There was a problem hiding this comment.
Can we double-check that this is an expected behavior? I think getClosestGlyphInfoForOffset is supposed to return the closest glyph, in this case the closest glyph is the last one, not null.
Answer:
The code in SkParagraph goes like this:
const PositionWithAffinity res = this->getGlyphPositionAtCoordinate(dx, dy);
SkASSERT(res.position != 0 || res.affinity != Affinity::kUpstream);
const size_t utf16Offset = res.position + (res.affinity == Affinity::kDownstream ? 0 : -1);
...
return this->getGlyphClusterAt(fUTF8IndexForUTF16Index[utf16Offset], glyphInfo);
mdebbar
approved these changes
Jun 26, 2026
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. 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. |
This was referenced Jun 29, 2026
auto-submit Bot
pushed a commit
to flutter/packages
that referenced
this pull request
Jul 7, 2026
…#12081) Manual roll Flutter from 0c80830e465b to ca9f874f5284 (119 revisions) Manual roll requested by [email protected] flutter/flutter@0c80830...ca9f874 2026-06-30 [email protected] [tool] Don't require a Flutter compile task when staging jniLibs (flutter/flutter#188805) 2026-06-30 [email protected] [Impeller] Fix potential overflow when allocating buffers at the next power of two size (flutter/flutter#188742) 2026-06-30 [email protected] [Framework/Tool] Decouple preview theme imports (flutter/flutter#188176) 2026-06-30 [email protected] Add import of `dart:_js_interop_wasm` in sdk rewriter tool (flutter/flutter#188620) 2026-06-30 [email protected] Detach LLDB and print stack trace on process stop (flutter/flutter#188576) 2026-06-30 [email protected] [Linux] Fix FlCompositorOpenGL.pixels comment (flutter/flutter#188754) 2026-06-30 [email protected] [flutter_tools] Fix crash in flutter create when pubspec.yaml is empty (flutter/flutter#188385) 2026-06-30 [email protected] Roll Packages from 656ccaa to 274ed3e (23 revisions) (flutter/flutter#188792) 2026-06-30 [email protected] In AndroidImageGenerator, check that the destination pixel buffer has sufficient capacity for the decoded data (flutter/flutter#188752) 2026-06-30 [email protected] Roll pub packages (flutter/flutter#188773) 2026-06-30 [email protected] [Impeller] Add a flat VertexAttributeFormat for vertex inputs (flutter/flutter#188684) 2026-06-30 [email protected] Roll pub packages (flutter/flutter#188764) 2026-06-30 [email protected] Roll Dart SDK from 0cb483880b6b to e1bdb9ce3327 (2 revisions) (flutter/flutter#188763) 2026-06-29 [email protected] Handle 'no permissions' adb device state (flutter/flutter#187248) 2026-06-29 [email protected] [windows]: adjusts uniform buffers to hit hlsl optimization (flutter/flutter#188538) 2026-06-29 [email protected] Roll Skia from bfb7860cb9c7 to 71947c4110b0 (9 revisions) (flutter/flutter#188747) 2026-06-29 [email protected] ci: extract wait-for-engine-build logic into a reusable composite action (flutter/flutter#188748) 2026-06-29 [email protected] Provide guided migration logs when iOS app crashes on simulator (flutter/flutter#188736) 2026-06-29 [email protected] Revert "[flutter_tools] Track asset transformer dependencies for hot reload" (flutter/flutter#188751) 2026-06-29 [email protected] Migrate ABI splits to new AGP dsl (flutter/flutter#188369) 2026-06-29 [email protected] Add Impeller+OpenGLES startup benchmark for mokey (flutter/flutter#188495) 2026-06-29 [email protected] Increase macOS minimum supported version from 10.15 to 12 to support Xcode 27 (flutter/flutter#188520) 2026-06-29 [email protected] Moves test ownership validation to flutter/flutter (flutter/flutter#188655) 2026-06-29 [email protected] Add --flavor support for Windows desktop builds (flutter/flutter#187034) 2026-06-29 [email protected] Android_hardware_smoke_test: Enable pixel exact local file comparator to read goldens from flutter asset URI (flutter/flutter#188587) 2026-06-29 [email protected] [flutter_tools] Use DeviceHub.app for iOS simulator path on Xcode 27+ (flutter/flutter#187910) 2026-06-29 [email protected] Roll Skia from 111e7582d081 to bfb7860cb9c7 (2 revisions) (flutter/flutter#188731) 2026-06-29 [email protected] Use `revert` label instead of `revert_wf` (flutter/flutter#188639) 2026-06-29 [email protected] Properly await Dart Development Service shutdown with timeout (flutter/flutter#188387) 2026-06-29 [email protected] [flutter_tools] Track asset transformer dependencies for hot reload (flutter/flutter#187947) 2026-06-29 [email protected] [Tool] Tolerate malformed UTF-8 in process streaming decoders (flutter/flutter#188453) 2026-06-29 [email protected] [flutter_tools] Use new ddc modules in test (flutter/flutter#188240) 2026-06-29 [email protected] Roll Packages from c1f7d92 to 656ccaa (12 revisions) (flutter/flutter#188728) 2026-06-29 [email protected] Remove unused fields (flutter/flutter#188705) 2026-06-29 [email protected] Clear text input handler widget on view dispose (flutter/flutter#188701) 2026-06-29 [email protected] Free compositor in view renderer finalize to avoid use-after-free (flutter/flutter#188702) 2026-06-29 [email protected] [linux] Use GWeakRef in mock signal handler test helper (flutter/flutter#188700) 2026-06-29 [email protected] Fixing few related editing issues with LTR/RTL text (flutter/flutter#188503) 2026-06-29 [email protected] [Flutter GPU] Add Texture.fromImage to wrap a ui.Image texture (flutter/flutter#188605) 2026-06-29 [email protected] [Flutter GPU] Honor the enable argument in RenderPass.setDepthWriteEnable (flutter/flutter#188715) 2026-06-29 [email protected] Roll Skia from ba1942d8c3e1 to 111e7582d081 (1 revision) (flutter/flutter#188721) 2026-06-29 [email protected] Roll Skia from 587d8befe1ee to ba1942d8c3e1 (6 revisions) (flutter/flutter#188717) 2026-06-29 [email protected] Remove some refs to package:intl (flutter/flutter#188504) 2026-06-29 [email protected] [VPAT] Update a11y assessment app FAB example to announce value change when it's updated. (flutter/flutter#188466) ...
kalyujniy
pushed a commit
to brickit-app/camera
that referenced
this pull request
Jul 8, 2026
…flutter#12081) Manual roll Flutter from 0c80830e465b to ca9f874f5284 (119 revisions) Manual roll requested by [email protected] flutter/flutter@0c80830...ca9f874 2026-06-30 [email protected] [tool] Don't require a Flutter compile task when staging jniLibs (flutter/flutter#188805) 2026-06-30 [email protected] [Impeller] Fix potential overflow when allocating buffers at the next power of two size (flutter/flutter#188742) 2026-06-30 [email protected] [Framework/Tool] Decouple preview theme imports (flutter/flutter#188176) 2026-06-30 [email protected] Add import of `dart:_js_interop_wasm` in sdk rewriter tool (flutter/flutter#188620) 2026-06-30 [email protected] Detach LLDB and print stack trace on process stop (flutter/flutter#188576) 2026-06-30 [email protected] [Linux] Fix FlCompositorOpenGL.pixels comment (flutter/flutter#188754) 2026-06-30 [email protected] [flutter_tools] Fix crash in flutter create when pubspec.yaml is empty (flutter/flutter#188385) 2026-06-30 [email protected] Roll Packages from 656ccaa to 274ed3e (23 revisions) (flutter/flutter#188792) 2026-06-30 [email protected] In AndroidImageGenerator, check that the destination pixel buffer has sufficient capacity for the decoded data (flutter/flutter#188752) 2026-06-30 [email protected] Roll pub packages (flutter/flutter#188773) 2026-06-30 [email protected] [Impeller] Add a flat VertexAttributeFormat for vertex inputs (flutter/flutter#188684) 2026-06-30 [email protected] Roll pub packages (flutter/flutter#188764) 2026-06-30 [email protected] Roll Dart SDK from 0cb483880b6b to e1bdb9ce3327 (2 revisions) (flutter/flutter#188763) 2026-06-29 [email protected] Handle 'no permissions' adb device state (flutter/flutter#187248) 2026-06-29 [email protected] [windows]: adjusts uniform buffers to hit hlsl optimization (flutter/flutter#188538) 2026-06-29 [email protected] Roll Skia from bfb7860cb9c7 to 71947c4110b0 (9 revisions) (flutter/flutter#188747) 2026-06-29 [email protected] ci: extract wait-for-engine-build logic into a reusable composite action (flutter/flutter#188748) 2026-06-29 [email protected] Provide guided migration logs when iOS app crashes on simulator (flutter/flutter#188736) 2026-06-29 [email protected] Revert "[flutter_tools] Track asset transformer dependencies for hot reload" (flutter/flutter#188751) 2026-06-29 [email protected] Migrate ABI splits to new AGP dsl (flutter/flutter#188369) 2026-06-29 [email protected] Add Impeller+OpenGLES startup benchmark for mokey (flutter/flutter#188495) 2026-06-29 [email protected] Increase macOS minimum supported version from 10.15 to 12 to support Xcode 27 (flutter/flutter#188520) 2026-06-29 [email protected] Moves test ownership validation to flutter/flutter (flutter/flutter#188655) 2026-06-29 [email protected] Add --flavor support for Windows desktop builds (flutter/flutter#187034) 2026-06-29 [email protected] Android_hardware_smoke_test: Enable pixel exact local file comparator to read goldens from flutter asset URI (flutter/flutter#188587) 2026-06-29 [email protected] [flutter_tools] Use DeviceHub.app for iOS simulator path on Xcode 27+ (flutter/flutter#187910) 2026-06-29 [email protected] Roll Skia from 111e7582d081 to bfb7860cb9c7 (2 revisions) (flutter/flutter#188731) 2026-06-29 [email protected] Use `revert` label instead of `revert_wf` (flutter/flutter#188639) 2026-06-29 [email protected] Properly await Dart Development Service shutdown with timeout (flutter/flutter#188387) 2026-06-29 [email protected] [flutter_tools] Track asset transformer dependencies for hot reload (flutter/flutter#187947) 2026-06-29 [email protected] [Tool] Tolerate malformed UTF-8 in process streaming decoders (flutter/flutter#188453) 2026-06-29 [email protected] [flutter_tools] Use new ddc modules in test (flutter/flutter#188240) 2026-06-29 [email protected] Roll Packages from c1f7d92 to 656ccaa (12 revisions) (flutter/flutter#188728) 2026-06-29 [email protected] Remove unused fields (flutter/flutter#188705) 2026-06-29 [email protected] Clear text input handler widget on view dispose (flutter/flutter#188701) 2026-06-29 [email protected] Free compositor in view renderer finalize to avoid use-after-free (flutter/flutter#188702) 2026-06-29 [email protected] [linux] Use GWeakRef in mock signal handler test helper (flutter/flutter#188700) 2026-06-29 [email protected] Fixing few related editing issues with LTR/RTL text (flutter/flutter#188503) 2026-06-29 [email protected] [Flutter GPU] Add Texture.fromImage to wrap a ui.Image texture (flutter/flutter#188605) 2026-06-29 [email protected] [Flutter GPU] Honor the enable argument in RenderPass.setDepthWriteEnable (flutter/flutter#188715) 2026-06-29 [email protected] Roll Skia from ba1942d8c3e1 to 111e7582d081 (1 revision) (flutter/flutter#188721) 2026-06-29 [email protected] Roll Skia from 587d8befe1ee to ba1942d8c3e1 (6 revisions) (flutter/flutter#188717) 2026-06-29 [email protected] Remove some refs to package:intl (flutter/flutter#188504) 2026-06-29 [email protected] [VPAT] Update a11y assessment app FAB example to announce value change when it's updated. (flutter/flutter#188466) ...
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #188314
Fixes #188315
Fixes #188316
Fixes #188318
Fixes #188319