Fix Caret Height On Empty Lines by LongCatIsLooong · Pull Request #120834 · flutter/flutter

LongCatIsLooong · 2023-02-15T21:16:43Z

Fixes #92080 (related: #114950)

getLocalRectForCaret now reports the real rect that will be painted.
Move caret x-coordinate clamping to RenderEditable since TextPainter doesn't know about clipping.
Caret Caching no longer depends on "caret prototype".
Fix WidgetSpan.codeUnitAtVisitor implementation

Incorporated some changes from #113268 and #118128

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.

…rCaret` now reports the real rect that will be painted. move caret x-coordinate clamping to RenderEditable since TextPainter doesn't know about clipping.

flutter-dashboard · 2023-02-16T00:23:13Z

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 package:flutter.

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

Changes reported for pull request #120834 at sha 2f4610b

Renzo-Olivares · 2023-02-16T02:33:36Z

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

@@ -1166,8 +1196,8 @@ class TextPainter {
  ///
  /// Valid only after [layout] has been called.
  double? getFullHeightForCaret(TextPosition position, Rect caretPrototype) {


Do we still need caretPrototype here? Though removing it would probably be a breaking change since this method is public.

It's no longer used. But as you pointed out it's a public API so I didn't change the signature.

Should we deprecate the parameter?

Yeah FullHeight and Offset are pretty limited. Maybe we'll have different caret painter classes and let them report the bounding box.

Renzo-Olivares · 2023-02-16T04:42:19Z

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

-class _CaretMetrics {
-  const _CaretMetrics({required this.offset, this.fullHeight});
+///
+// This should be a sealed class: A _CaretMetrics is either a _LineCaretMetrics


nit: should this also be ///? or Should the entire comment be // since this is a private class.

https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#use-for-public-quality-private-documentation

The // part is not meant for API users just a maintenance note.

Renzo-Olivares · 2023-02-16T04:43:47Z

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

+class _EmptyLineCaretMetrics implements _CaretMetrics {
+  const _EmptyLineCaretMetrics({ required this.lineVerticalOffset });
+
+  /// The y offset of the unoccupied line.


nit: Should be consistent with // vs ///

Renzo-Olivares · 2023-02-16T04:46:14Z

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

+// non-empty line are associated with a glyph within the same line.
+class _LineCaretMetrics implements _CaretMetrics {
+  const _LineCaretMetrics({required this.offset, required this.writingDirection, required this.fullHeight});
  /// The offset of the top left corner of the caret from the top left


nit: class should be consistent with // vs /// for documentation.

Renzo-Olivares · 2023-02-16T05:32:33Z

packages/flutter/lib/src/rendering/editable.dart

+    final Rect caretPrototype = _caretPrototype;
+    final Offset caretOffset = _textPainter.getOffsetForCaret(caretPosition, caretPrototype);
+    Rect caretRect = caretPrototype.shift(caretOffset + cursorOffset);
+    final double scrollableWidth = math.max(_textPainter.width + _caretMargin, size.width);


Not sure I understand why we are taking into account scrollableWidth, could explain for my sanity?

when there're trailing spaces the caret x offset may be greater than the width of the text painter.

Renzo-Olivares

LGTM, the broken tests seem unrelated probably just needs a sync up.

justinmc

LGTM 👍

One comment about maybe adding a deprecation.

justinmc · 2023-02-28T19:01:00Z

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

@@ -1166,8 +1196,8 @@ class TextPainter {
  ///
  /// Valid only after [layout] has been called.
  double? getFullHeightForCaret(TextPosition position, Rect caretPrototype) {


Should we deprecate the parameter?

justinmc · 2023-02-28T19:02:10Z

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

    // Cache the input parameters to prevent repeat work later.
    _previousCaretPosition = position;
-    _previousCaretPrototype = caretPrototype;
+    return _caretMetrics = metrics ?? const _EmptyLineCaretMetrics(lineVerticalOffset: 0);


I'm a fan of returning this change to return the value instead of setting it to an instance variable 👍

justinmc · 2023-02-28T19:04:27Z

packages/flutter/lib/src/rendering/editable.dart

-  // superior (subjectively and in terms of fidelity) in _paintCaret. We
-  // should rework this properly to once again match the platform. The constant
-  // _kCaretHeightOffset scales poorly for small font sizes.
+  // TODO(LongCatIsLooong): https://github.com/flutter/flutter/issues/120836


Thanks for creating an issue for this!

justinmc · 2023-02-28T19:07:02Z

packages/flutter/test/painting/widget_span_test.dart

+import 'package:flutter_test/flutter_test.dart';
+
+void main() {
+  test('WidgetSpan codeUnitAt', () {


Thanks for adding a test for this method!

auto-submit · 2023-03-13T21:10:17Z

auto label is removed for flutter/flutter, pr: 120834, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

LongCatIsLooong · 2023-03-13T21:19:54Z

The google testing failures are expected. Merging manually.

improve caret caching, fix caret for empty text/line, `getLocalRectFo…

be2c1c3

…rCaret` now reports the real rect that will be painted. move caret x-coordinate clamping to RenderEditable since TextPainter doesn't know about clipping.

LongCatIsLooong mentioned this pull request Feb 15, 2023

Improve Caret Fidelity #120836

Open

3 tasks

comments

2f4610b

LongCatIsLooong force-pushed the empty-text-caret branch from ebea8cf to 2f4610b Compare February 15, 2023 22:05

LongCatIsLooong requested review from Renzo-Olivares, goderbauer and justinmc February 15, 2023 22:15

flutter-dashboard bot added the will affect goldens Changes to golden files label Feb 16, 2023

Renzo-Olivares reviewed Feb 16, 2023

View reviewed changes

review

1b861b1

Renzo-Olivares approved these changes Feb 27, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/master' into empty-text-caret

a7d7438

justinmc approved these changes Feb 28, 2023

View reviewed changes

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

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 13, 2023

Merge branch 'master' into empty-text-caret

4d05f27

LongCatIsLooong merged commit 1306d7f into flutter:master Mar 13, 2023

LongCatIsLooong deleted the empty-text-caret branch March 13, 2023 22:37

guidezpl mentioned this pull request Mar 14, 2023

Textfield cursor height strange behaviour(gif) #64072

Closed

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

Roll Flutter from 9ba0d08ebc07 to ac37dd261110 (23 revisions) flutter/packages#3456

Closed

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

1306d7f Fix Caret Height On Empty Lines (flutter/flutter#120834)

015d587

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

Roll Flutter from 9ba0d08ebc07 to d8a721106108 (24 revisions) flutter/packages#3457

Merged

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

1306d7f Fix Caret Height On Empty Lines (flutter/flutter#120834)

8cbabad

HansMuller mentioned this pull request Mar 17, 2023

Flutter Framework Team: Notable Commits #121415

Closed

gnprice mentioned this pull request Mar 19, 2023

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

Merged

8 tasks

g123k mentioned this pull request Mar 25, 2023

The caret is misaligned when a text field is empty openfoodfacts/smooth-app#3806

Open

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

1306d7f Fix Caret Height On Empty Lines (flutter/flutter#120834)

651f76c

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

1306d7f Fix Caret Height On Empty Lines (flutter/flutter#120834)

d77b10a

Conversation

LongCatIsLooong commented Feb 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Feb 16, 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!

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!

Renzo-Olivares left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented Mar 13, 2023

Uh oh!

LongCatIsLooong commented Mar 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

LongCatIsLooong commented Feb 15, 2023 •

edited

Loading

justinmc left a comment •

edited

Loading