Sitelet https://github.com/flutter/flutter/pull/191223
Skip to content

Cache the paint offset adjusted line metrics in TextPainter - #191223

Merged
auto-submit[bot] merged 4 commits into
flutter:masterfrom
nickolasdeluca:fix/vertical-caret-run-non-left-align
Sep 25, 2026
Merged

Cache the paint offset adjusted line metrics in TextPainter#191223
auto-submit[bot] merged 4 commits into
flutter:masterfrom
nickolasdeluca:fix/vertical-caret-run-non-left-align

Conversation

@nickolasdeluca

@nickolasdeluca nickolasdeluca commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Fixes #139201

The bug

TextPainter.computeLineMetrics translates the paragraph's line metrics by paintOffset, which is only non-zero when the text is not left aligned. That translation built a brand new list on every call:

final List<ui.LineMetrics> rawMetrics = layout.lineMetrics;
return offset == Offset.zero
    ? rawMetrics
    : rawMetrics.map((ui.LineMetrics metrics) => _shiftLineMetrics(metrics, offset)).toList(growable: false);

VerticalCaretMovementRun.isValid uses the identity of that list to detect text layout changes:

final List<ui.LineMetrics> newLineMetrics = _editable._textPainter.computeLineMetrics();
// Use the implementation detail of the computeLineMetrics method to figure
// out if the current text layout has been invalidated.
if (!identical(newLineMetrics, _lineMetrics)) {
  _isValid = false;
}

So whenever the paint offset is non-zero, a vertical caret run was invalid the instant it was created. This matches @LongCatIsLooong's diagnosis in #139201 (comment).

The user visible symptom is a single line TextField / CupertinoTextField with TextAlign.center or TextAlign.right: a single line field lays its text out with an unbounded width (RenderEditable._adjustConstraints passes double.infinity as maxWidth when !_isMultiline), so contentWidth != paragraph.width and the paint offset is non-zero. Pressing the up or down arrow key then trips assert(isValid) in VerticalCaretMovementRun.moveNext / movePrevious in debug mode, and silently leaves the caret where it is in release mode.

The fix

Cache the translated list on _TextPainterLayoutCacheWithOffset, keyed on the paint offset it was computed with (paintOffset is derived from the mutable contentWidth, so it can change without a relayout). computeLineMetrics then keeps returning the same list instance for as long as the text layout and the paint offset are unchanged, which restores the identity contract VerticalCaretMovementRun depends on. It also stops allocating a fresh list of LineMetrics on every call for every non left aligned paragraph.

Relation to #160516

#160516 fixed this issue by relaxing isValid to listEquals instead. That works, but it compares every line's metrics on every arrow key press, it keeps allocating the intermediate list, and it makes a run stay valid across a genuine layout change that happens to produce equal metrics (the run's _positionCache would then be stale). This PR fixes the identity contract that was broken instead, and leaves isValid alone. That PR was closed for reasons unrelated to this issue (the RTL arrow direction work for #78660).

Tests

  • TextPainter line metrics are cached when the paint offset is not zero — asserts computeLineMetrics() returns an identical list across calls, for every TextAlign.
  • TextPainter line metrics are shifted by the paint offset — guards the translation itself, which now happens in a new place.
  • run in a single line field that is not left aligned in editable_text_shortcuts_test.dart — the end to end regression test; it throws the 'isValid': is not true assertion without this patch.

All three fail on master and pass with this change. test/painting, test/rendering/editable_test.dart, test/rendering/paragraph_test.dart, test/widgets/editable_text_test.dart, test/widgets/editable_text_shortcuts_test.dart, test/widgets/text_test.dart, test/widgets/selectable_region_test.dart, test/material/text_field_test.dart, test/material/text_form_field_test.dart, test/material/selectable_text_test.dart and test/cupertino/text_field_test.dart are green.

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.
  • All existing and new tests are passing.

@google-cla

google-cla Bot commented Aug 17, 2026

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@github-actions 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 Aug 17, 2026
gemini-code-assist[bot]
gemini-code-assist Bot reviewed Aug 17, 2026
View reviewed changes

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request caches shifted line metrics in _TextPainterLayoutCacheWithOffset to ensure that computeLineMetrics() returns the same list instance when the layout and paint offset do not change, preventing issues with VerticalCaretMovementRun in non-left-aligned text. Regression tests are also added to verify this behavior. The review feedback suggests updating the documentation comment for the lineMetrics getter to use triple-slash comments to align with the Flutter Style Guide.

Comment thread packages/flutter/lib/src/painting/text_painter.dart Outdated
@CatHood0 CatHood0 mentioned this pull request Aug 18, 2026
Open
@Renzo-Olivares
Renzo-Olivares self-requested a review August 20, 2026 20:30
Renzo-Olivares
Renzo-Olivares reviewed Aug 27, 2026
View reviewed changes

@Renzo-Olivares Renzo-Olivares left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach looks good to me, just had a few small comments.

Comment thread packages/flutter/lib/src/painting/text_painter.dart Outdated
Comment thread packages/flutter/lib/src/painting/text_painter.dart Outdated
Comment thread packages/flutter/lib/src/painting/text_painter.dart Outdated
@Renzo-Olivares Renzo-Olivares added the waiting for response The Flutter team cannot make further progress on this issue until the original reporter responds label Aug 27, 2026
@nickolasdeluca

nickolasdeluca commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review! All three addressed in 352d0ea:

  • Removed the trailing rationale sentence from the lineMetrics doc, keeping only the list identity contract that callers depend on.
  • Renamed the shadowed local to shiftedMetrics.
  • Reworded computeLineMetrics's doc to drop the paint offset reference, using your suggested wording.

@github-actions github-actions Bot removed the waiting for response The Flutter team cannot make further progress on this issue until the original reporter responds label Aug 27, 2026
Renzo-Olivares
Renzo-Olivares previously approved these changes Sep 1, 2026
View reviewed changes

@Renzo-Olivares Renzo-Olivares left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo my last comment.

Comment thread packages/flutter/lib/src/painting/text_painter.dart Outdated
@nickolasdeluca

nickolasdeluca commented Sep 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Applied the wording fix in 5301b12. The push dismissed your approval, mind re-approving? PTAL @Renzo-Olivares

Renzo-Olivares
Renzo-Olivares approved these changes Sep 1, 2026
View reviewed changes

@Renzo-Olivares Renzo-Olivares left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, this will need a secondary approval before landing. I'll find a reviewer.

@nickolasdeluca

nickolasdeluca commented Sep 21, 2026

Copy link
Copy Markdown
Contributor Author

Hi @LongCatIsLooong! Just wanted to gently follow up on the review request for #191223 whenever you get a chance. Renzo has already approved it, and I’ve addressed the previous review comments. No rush though, I just wanted to make sure it didn’t get lost in the queue. Thanks!

LongCatIsLooong
LongCatIsLooong approved these changes Sep 22, 2026
View reviewed changes

@LongCatIsLooong LongCatIsLooong left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix! LGTM.

@LongCatIsLooong LongCatIsLooong added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 22, 2026
@auto-submit auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 22, 2026
@auto-submit

auto-submit Bot commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

autosubmit label was removed for flutter/flutter/191223, because The base commit of the PR is older than 7 days and can not be merged. Please merge the latest changes from the main into this branch and resubmit the PR.

@LongCatIsLooong LongCatIsLooong added autosubmit Merge PR when tree becomes green via auto submit App CICD Run CI/CD labels Sep 22, 2026
@auto-submit auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 22, 2026
@auto-submit

auto-submit Bot commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

autosubmit label was removed for flutter/flutter/191223, because The base commit of the PR is older than 7 days and can not be merged. Please merge the latest changes from the main into this branch and resubmit the PR.

@auto-submit

auto-submit Bot commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

autosubmit label was removed for flutter/flutter/191223, because - The status or check suite ci.yaml validation has failed. Please fix the issues identified (or deflake) before re-applying this label.

@flutter-dashboard flutter-dashboard Bot removed the CICD Run CI/CD label Sep 22, 2026
@LongCatIsLooong LongCatIsLooong added the CICD Run CI/CD label Sep 22, 2026
@nickolasdeluca @Renzo-Olivares
Cache the paint offset adjusted line metrics in TextPainter
957cf05 
`TextPainter.computeLineMetrics` translates the paragraph's line metrics
by the paint offset, which is only non-zero when the text is not left
aligned. That translation allocated a brand new list on every call, and
`VerticalCaretMovementRun.isValid` uses the identity of that list to
detect text layout changes, so a vertical caret run was invalidated as
soon as it was created whenever the text was not left aligned.

The most visible symptom is a single line `TextField` or
`CupertinoTextField` with `TextAlign.center` / `TextAlign.right`: such a
field lays its text out with an unbounded width, so the paint offset is
non-zero, and pressing the up or down arrow key asserts in debug mode and
leaves the caret in place in release mode.

Cache the translated list on the layout cache instead, keyed on the paint
offset it was computed with, so `computeLineMetrics` keeps returning the
same instance while the text layout and the paint offset are unchanged.

Fixes #139201
@nickolasdeluca @Renzo-Olivares
Use /// for the lineMetrics documentation
5076965
@nickolasdeluca @Renzo-Olivares
Address review comments on the line metrics cache
7290366 
- Drop the VerticalCaretMovementRun rationale from the lineMetrics doc, it
  is review context rather than lasting documentation.
- Rename the local variable that was shadowed by the map closure.
- Do not mention the paint offset in computeLineMetrics's public doc, it is
  not part of the public API.
@nickolasdeluca @Renzo-Olivares
Clarify computeLineMetrics's caching doc
7719a07 
The method, not the returned list, is what returns the same instance.
@Renzo-Olivares
Renzo-Olivares force-pushed the fix/vertical-caret-run-non-left-align branch from 87abbc1 to 7719a07 Compare September 24, 2026 19:58
@flutter-dashboard flutter-dashboard Bot removed the CICD Run CI/CD label Sep 24, 2026
@Renzo-Olivares Renzo-Olivares added CICD Run CI/CD autosubmit Merge PR when tree becomes green via auto submit App labels Sep 24, 2026
@auto-submit
auto-submit Bot added this pull request to the merge queue Sep 25, 2026
Merged via the queue into flutter:master with commit afde83d Sep 25, 2026
30 checks passed
@flutter-dashboard flutter-dashboard Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 25, 2026
This was referenced Sep 25, 2026
Closed
Closed
Closed
@justinmc justinmc mentioned this pull request Sep 25, 2026
Open
This was referenced Sep 25, 2026
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Merged
auto-submit Bot pushed a commit to flutter/packages that referenced this pull request Sep 29, 2026
@engine-flutter-autoroll
Manual roll Flutter from 4fcd90be0045 to c5a061b18fa2 (222 revisions) (…
08e1067 
…#13060)

Manual roll Flutter from 4fcd90be0045 to c5a061b18fa2 (222 revisions)

Manual roll requested by quncheng@google.com

flutter/flutter@4fcd90b...c5a061b

2026-09-28 brackenavaron@gmail.com [cross imports] navigator_test.dart (flutter/flutter#190954)
2026-09-28 154381524+flutteractionsbot@users.noreply.github.com Revert: Reland: Only render views that need to be rendered  (flutter/flutter#193360)
2026-09-28 codefu@google.com ci(bringup): android_intent_security_test is green (flutter/flutter#193469)
2026-09-28 154381524+flutteractionsbot@users.noreply.github.com Sync CHANGELOG.md from stable (flutter/flutter#193020)
2026-09-28 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from ukukV5lEkKabtOATk... to QdgqP02_cYpRQQQNN... (flutter/flutter#193454)
2026-09-28 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#193460)
2026-09-28 victorsanniay@gmail.com Un-nest sceneBuildDuration and windowRenderDuration in web SceneBuilderRecorder (flutter/flutter#193260)
2026-09-28 zarah@google.com Fix unresolved doc comment references in dev/integration_tests and dev/benchmarks (flutter/flutter#193433)
2026-09-28 engine-flutter-autoroll@skia.org Roll Packages from e55e7ac to ba0364a (9 revisions) (flutter/flutter#193450)
2026-09-28 bkonyi@google.com [tool] Require explicit dependency injection for FlutterDevice and FlutterDevice.create (flutter/flutter#192830)
2026-09-28 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#193442)
2026-09-28 dacoharkes@google.com [flutter_tools] Include data assets from hooks when pubspec.yaml is empty (flutter/flutter#193434)
2026-09-28 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#193438)
2026-09-28 zarah@google.com Fix unresolved doc comment references in Material and Cupertino (flutter/flutter#193283)
2026-09-28 zarah@google.com Fix doc references to Material and Cupertino in the widgets library and its tests (flutter/flutter#193334)
2026-09-26 kevmoo@users.noreply.github.com wasm: enforce WasmGC opt-in capability checks and Firefox < 147 guard (flutter/flutter#193180)
2026-09-26 43054281+camsim99@users.noreply.github.com [Android] Refuse external setters of engine entrypoint and cached engine arguments via `Intent`s (flutter/flutter#190249)
2026-09-26 1961493+harryterkelsen@users.noreply.github.com [web] Unskip TextPainter.getWordBoundary test (flutter/flutter#193378)
2026-09-26 bkonyi@google.com Specify non-obvious types in pattern variable declarations (flutter/flutter#192632)
2026-09-26 30870216+gaaclarke@users.noreply.github.com Removes feedback loop from advanced filters without offscreen msaa and framebufferfetch (flutter/flutter#193306)
2026-09-26 1961493+harryterkelsen@users.noreply.github.com [web] Unskip 8 passing image tests in painting and rendering (flutter/flutter#193377)
2026-09-26 154381524+flutteractionsbot@users.noreply.github.com Revert: Allow resetting test invariants in `addTearDown` (flutter/flutter#193383)
2026-09-25 bkonyi@google.com [tool] Migrate CoverageCollector to modular dependency injection (flutter/flutter#192924)
2026-09-25 zarah@google.com Fix more unresolved doc comment references (flutter/flutter#193336)
2026-09-25 dkwingsmt@users.noreply.github.com Allow resetting test invariants in `addTearDown` (flutter/flutter#192082)
2026-09-25 jhy03261997@gmail.com [a11y] Add a semantics role for slider (flutter/flutter#193324)
2026-09-25 stuartmorgan@google.com Fix plugin tests after Pigeon plugin template changes (flutter/flutter#193302)
2026-09-25 jesswon@google.com [Android 17] Bump Standard Test Apps in flutter/flutter to AGP 9.3.1 (flutter/flutter#193263)
2026-09-25 97480502+b-luk@users.noreply.github.com SSBO-based gradients in UberSDF (flutter/flutter#192962)
2026-09-25 robert.ancell@canonical.com [Linux] Handle FlView being destroyed before rendering is complete. (flutter/flutter#193268)
2026-09-25 bkonyi@google.com [flutter_tools] Handle Windows reserved characters in test target path (flutter/flutter#191900)
2026-09-25 sneurlax@gmail.com docs(tools): nit: say hook/build.dart in CMake native assets comment (flutter/flutter#192205)
2026-09-25 kevmoo@users.noreply.github.com [web] Enable Firefox Skwasm UI CI suites, configure COI configs, and fail fast on loader rejections (flutter/flutter#193187)
2026-09-25 jesswon@google.com [Android 17] Bumped Engine Dependencies to 9.3.1 (flutter/flutter#193265)
2026-09-25 kevmoo@users.noreply.github.com [web] Omit group role on menu scrollables and assign region role to named routes (flutter/flutter#192965)
2026-09-25 engine-flutter-autoroll@skia.org Roll Skia from 8eedeed98e79 to f441ca223b2b (5 revisions) (flutter/flutter#193353)
2026-09-25 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from EbPpoJW-Lnsu-8dyZ... to ukukV5lEkKabtOATk... (flutter/flutter#193326)
2026-09-25 engine-flutter-autoroll@skia.org Roll Packages from 431ea69 to e55e7ac (6 revisions) (flutter/flutter#193351)
2026-09-25 139053348+shikharish@users.noreply.github.com Support DynamicLibrary.codeAsset in Flutter (flutter/flutter#188947)
2026-09-25 engine-flutter-autoroll@skia.org Roll Dart SDK from 75d9e87d4e3d to 0e7642b85457 (1 revision) (flutter/flutter#193346)
2026-09-25 15619084+vashworth@users.noreply.github.com Exclude iOS PRs from macOS link (flutter/flutter#193315)
2026-09-25 engine-flutter-autoroll@skia.org Roll Skia from 6357608543ec to 8eedeed98e79 (4 revisions) (flutter/flutter#193341)
2026-09-25 engine-flutter-autoroll@skia.org Roll Dart SDK from d3aedb186aab to 75d9e87d4e3d (3 revisions) (flutter/flutter#193335)
2026-09-25 nickolasdeluca@live.com Cache the paint offset adjusted line metrics in TextPainter (flutter/flutter#191223)
...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Renzo-Olivares Renzo-Olivares Renzo-Olivares approved these changes
@LongCatIsLooong LongCatIsLooong LongCatIsLooong approved these changes
+1 more reviewer
@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

a: text input Entering text in a text field or keyboard related problems CICD Run CI/CD framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

TextField with TextAlign.right throws Failed assertion: line 206 pos 12: 'isValid': is not true.

3 participants

@nickolasdeluca @Renzo-Olivares @LongCatIsLooong