Remove Editable.onCaretChanged callback #109114

tgucio · 2022-08-07T13:56:08Z

This PR removes the convoluted logic of Editable.onCaretChanged callback fed back to EditableText, used just so the field can be scrolled into view. Instead, the caret rect (or selection extent) is read from Editable. This also fixes a problem where after changing selection from collapsed to non-collapsed via userUpdateTextEditingValue(), EditableText would use outdated caret location when scrolling to show the caret as Editable only called the onCaretChanged when selection was collapsed.

Also addressed:

old TODO in Editable so caret painting is skipped for invalid selection
when restarting cursor blink in EditableText, both _cursorActive and _cursorTimer are checked which prevents from restarting blinking that has just been stopped.
caret/cursor class and constants in editable.dart are renamed to keep them consistent

Relevant tests passing:

% ../../bin/flutter test test/rendering/editable_gesture_test.dart \
  test/rendering/editable_test.dart \
  test/widgets/editable_text_shortcuts_test.dart \
  test/widgets/editable_text_test.dart \
  test/widgets/editable_text_show_on_screen_test.dart \
  test/widgets/editable_text_cursor_test.dart \
  test/cupertino/text_field_test.dart \
  test/cupertino/text_field_restoration_test.dart \
  test/material/text_field_helper_text_test.dart \
  test/material/text_field_test.dart \
  test/material/text_field_splash_test.dart \
  test/material/text_field_focus_test.dart \
  test/material/text_field_restoration_test.dart

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.

…g in EditableText

flutter-dashboard · 2022-08-07T13:56:10Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

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

tgucio · 2022-08-07T13:57:40Z

/cc @LongCatIsLooong

tgucio · 2022-08-10T13:59:52Z

/cc @chunhtai @justinmc in case they can take a look at reviewing

LongCatIsLooong

Thanks for looking into this! Some changes could be breaking. Let's see if we can isolate the breaking changes if there's any.

LongCatIsLooong · 2022-08-16T16:26:25Z

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

-    // TODO(LongCatIsLooong): skip painting the caret when the selection is
-    // (-1, -1).
-    if (selection == null || !selection.isCollapsed) {
+    if (selection == null || !selection.isValid || !selection.isCollapsed) {


I think the reason there's a TODO is this is a breaking change.

It still is a breaking change. It seems currently with an invalid selection the caret will painted at TextRange(0, 0).

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

justinmc

I've tried to trigger the "Google tests" check, which will help us see if this breaks Google's internal tests. I'm never sure if the PR needs to be approved or not before that will start... Let's see if it shows up.

If it's not a bad breaking change then I'm on board with this, seems cleaner to me.

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

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

justinmc · 2022-08-16T16:50:11Z

packages/flutter/test/cupertino/text_field_test.dart

@@ -3670,7 +3670,7 @@ void main() {

    // The ListView has scrolled to keep the TextField and cursor handle
    // visible.
-    expect(scrollController.offset, 26.0);
+    expect(scrollController.offset, 28.0);


Why do all these values need to change by 2 pixels?

So this got me slightly puzzled. My semi-educated guess is that this is because in _FloatingCursorPainter.paintRegularCursor() there's two things going on:

Small per-platform adjustments:

final double? caretHeight = renderEditable._textPainter.getFullHeightForCaret(textPosition, caretPrototype); [...] caretRect = Rect.fromLTWH(

then, a bit of shifting:

caretRect = caretRect.shift(renderEditable._paintOffset); final Rect integralRect = caretRect.shift(renderEditable._snapToPhysicalPixel(caretRect.topLeft)); [...] caretPaintCallback(integralRect);

It's probably because of 1. - not sure why this was only done in the paint routine and not in getLocalRectForCaret(). Maybe a topic for another PR to clean it up.

This seems to cause some internal golden images to show up differently too. Maybe replace the code in getLocalRectForCaret with the implementation here for now?

packages/flutter/test/widgets/editable_text_show_on_screen_test.dart

packages/flutter/test/widgets/editable_text_test.dart

LongCatIsLooong · 2022-08-16T18:22:55Z

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

-    // TODO(LongCatIsLooong): skip painting the caret when the selection is
-    // (-1, -1).
-    if (selection == null || !selection.isCollapsed) {
+    if (selection == null || !selection.isValid || !selection.isCollapsed) {


It still is a breaking change. It seems currently with an invalid selection the caret will painted at TextRange(0, 0).

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

LongCatIsLooong · 2022-08-16T18:29:18Z

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

-      if (_currentCaretRect == null || !_scrollController.hasClients) {
+      final RenderEditable? renderEditable =
+          _editableKey.currentContext?.findRenderObject() as RenderEditable?;
+      if (!(renderEditable?.selection?.isValid ?? false) || !_scrollController.hasClients) {


nit: if (renderEditable == null || !renderEditable.selection.isValid || ...) to promote renderEditable to non-null. Slightly easier to read too.

This now became a 3-liner but I agree it's easier to read:

if (renderEditable == null || !(renderEditable.selection?.isValid ?? false) || !_scrollController.hasClients) {

LongCatIsLooong · 2022-08-16T19:51:12Z

packages/flutter/test/cupertino/text_field_test.dart

@@ -3670,7 +3670,7 @@ void main() {

    // The ListView has scrolled to keep the TextField and cursor handle
    // visible.
-    expect(scrollController.offset, 26.0);
+    expect(scrollController.offset, 28.0);


This seems to cause some internal golden images to show up differently too. Maybe replace the code in getLocalRectForCaret with the implementation here for now?

LongCatIsLooong · 2022-09-20T22:43:20Z

Hello @tgucio, this pull request still has some internal golden test failures. Do you have plans to remove the code changes that could potentially cause the failures (see the comments above)?

Note to myself: TAP run: 467947880/OCL:467947880:BASE:475669971:1663713141112:da7081f1

tgucio · 2022-09-20T23:03:37Z

@LongCatIsLooong yes, I plan to revisit this PR this week and look at having a single method to compute the caret height for scroll and paint.

LongCatIsLooong · 2023-05-30T22:00:25Z

Just the bringIntoView behavior should be fine since there are no registered customers using that removed callback.

tgucio · 2023-06-20T11:04:23Z

@LongCatIsLooong I've submitted a PR to add this to the breaking change notes: flutter/website#8916

tgucio · 2023-06-20T13:41:18Z

BTW, the Google test is still failing - not sure if it will be updated or it's OK to merge as is.

LongCatIsLooong · 2023-06-20T17:11:17Z

There's no additional google testing failures. Feel free to merge once flutter/website#8916 gets LGTM'd. You may have to merge it manually.

The google testing failures are 3 golden image changes and they look like improvements to me so I think the images need to be updated.

Breaking change note for flutter/flutter#109114 ## Presubmit checklist - [x] This PR doesn’t contain automatically generated corrections (Grammarly or similar). - [x] This PR follows the [Google Developer Documentation Style Guidelines](https://developers.google.com/style) — for example, it doesn’t use _i.e._ or _e.g._, and it avoids _I_ and _we_ (first person). - [x] This PR uses [semantic line breaks](https://github.com/dart-lang/site-shared/blob/main/doc/writing-for-dart-and-flutter-websites.md#semantic-line-breaks) of 80 characters or fewer. --------- Co-authored-by: Shams Zakhour (ignore Sfshaza) <44418985+sfshaza2@users.noreply.github.com> Co-authored-by: Anthony Sansone <atsansone@users.noreply.github.com>

Roll Flutter from 96a2c05 to 51bef1b (37 revisions) flutter/flutter@96a2c05...51bef1b 2023-06-28 engine-flutter-autoroll@skia.org Roll Flutter Engine from e8a1c23d66ba to 241ca5c1d6be (1 revision) (flutter/flutter#129725) 2023-06-28 parlough@gmail.com Update analysis, linter, and repo links in analysis options (flutter/flutter#129686) 2023-06-28 engine-flutter-autoroll@skia.org Roll Flutter Engine from be1073aa352f to e8a1c23d66ba (1 revision) (flutter/flutter#129723) 2023-06-28 hans.muller@gmail.com Dev, examples/api, etc updated for Material 3 by default (flutter/flutter#129683) 2023-06-28 tessertaha@gmail.com Add `DatePickerTheme.inputDecorationTheme` for the DatePicker with input mode. (flutter/flutter#128950) 2023-06-28 jonahwilliams@google.com [framework] ensure flexible space bar fades when scrolling. (flutter/flutter#129527) 2023-06-28 leroux_bruno@yahoo.fr Add InputDecorator.error to allow error message customization (flutter/flutter#129275) 2023-06-28 engine-flutter-autoroll@skia.org Roll Flutter Engine from b388e852be44 to be1073aa352f (1 revision) (flutter/flutter#129712) 2023-06-28 engine-flutter-autoroll@skia.org Roll Flutter Engine from 17173994a8c2 to b388e852be44 (1 revision) (flutter/flutter#129708) 2023-06-28 xilaizhang@google.com [flutter roll] Revert "Fix `AnimatedList` & `AnimatedGrid` doesn't apply `MediaQuery` padding" (flutter/flutter#129645) 2023-06-28 engine-flutter-autoroll@skia.org Roll Flutter Engine from 2f4fc4872699 to 17173994a8c2 (1 revision) (flutter/flutter#129694) 2023-06-28 engine-flutter-autoroll@skia.org Roll Flutter Engine from a6d9d12c440f to 2f4fc4872699 (1 revision) (flutter/flutter#129691) 2023-06-28 engine-flutter-autoroll@skia.org Roll Flutter Engine from 25a5850f8b5b to a6d9d12c440f (4 revisions) (flutter/flutter#129687) 2023-06-28 engine-flutter-autoroll@skia.org Roll Flutter Engine from 7c7c45d53bec to 25a5850f8b5b (1 revision) (flutter/flutter#129682) 2023-06-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from f320b8c36fee to 7c7c45d53bec (14 revisions) (flutter/flutter#129678) 2023-06-27 32242716+ricardoamador@users.noreply.github.com Update labeler yaml (flutter/flutter#129676) 2023-06-27 32242716+ricardoamador@users.noreply.github.com Revert "Fix the matcher condition where multiple matchers are found" (flutter/flutter#129675) 2023-06-27 32242716+ricardoamador@users.noreply.github.com Revert "Labeler format to remove extra single quote" (flutter/flutter#129674) 2023-06-27 32242716+ricardoamador@users.noreply.github.com Revert "Update labeler.yml to v5.0.0-beta.1" (flutter/flutter#129673) 2023-06-27 32242716+ricardoamador@users.noreply.github.com Labeler format to remove extra single quote (flutter/flutter#129672) 2023-06-27 32242716+ricardoamador@users.noreply.github.com Fix the matcher condition where multiple matchers are found (flutter/flutter#129670) 2023-06-27 737941+loic-sharma@users.noreply.github.com Automatically migrate ClipboardData.text to non-null (flutter/flutter#129567) 2023-06-27 72562119+tgucio@users.noreply.github.com Remove Editable.onCaretChanged callback (flutter/flutter#109114) 2023-06-27 bkonyi@google.com Reland "Fix issue where DevTools would not be immediately available when using --start-paused (#126698)" (flutter/flutter#129368) 2023-06-27 15619084+vashworth@users.noreply.github.com Update Xcode to 14.3.1 (flutter/flutter#129024) 2023-06-27 109253501+pdblasi-google@users.noreply.github.com Adds `dart_fix` support to `integration_test` (flutter/flutter#129579) 2023-06-27 chillers@google.com Update labeler.yml to v5.0.0-beta.1 (flutter/flutter#129617) 2023-06-27 luccas.clezar@gmail.com iOS TextSelectionToolbar fidelity (flutter/flutter#127757) 2023-06-27 jason-simmons@users.noreply.github.com Make a paragraph test involving Chinese characters work with inconsistent host system fonts (flutter/flutter#129628) 2023-06-27 engine-flutter-autoroll@skia.org Roll Packages from 6b70804 to f89ce02 (7 revisions) (flutter/flutter#129630) 2023-06-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from 715eff211a42 to f320b8c36fee (6 revisions) (flutter/flutter#129599) 2023-06-27 jhy03261997@gmail.com Fix chinese text is not selected by long press (flutter/flutter#129320) 2023-06-26 engine-flutter-autoroll@skia.org Roll Flutter Engine from 0da06de991a9 to 715eff211a42 (4 revisions) (flutter/flutter#129593) 2023-06-26 godofredoc@google.com Fix syntax error in no-response (flutter/flutter#129588) 2023-06-26 engine-flutter-autoroll@skia.org Roll Flutter Engine from f2d70cc809cd to 0da06de991a9 (3 revisions) (flutter/flutter#129582) 2023-06-26 hans.muller@gmail.com Updated chip_test.dart tests for M3 (flutter/flutter#129570) 2023-06-26 engine-flutter-autoroll@skia.org Roll Flutter Engine from 4032a9bc964e to f2d70cc809cd (4 revisions) (flutter/flutter#129574) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC camillesimon@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose ...

This change removes an unused parameter that was recently removed in the `RenderEditable` on the `master` branch flutter/flutter#109114. Fixes #1923 ## Pre-launch Checklist - [x] I read the [Flutter Style Guide] _recently_, and have followed its advice. - [x] I signed the [CLA]. - [x] I read the [Contributors Guide]. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] All existing and new tests are passing. --------- Co-authored-by: Brett Morgan <brett.morgan@gmail.com>

tgucio added 3 commits August 7, 2022 15:48

Remove onCaretChanged callback

b9a3abb

Clean up TODO in Editable.paint()

08851ba

Check for both _cursorActive and _cursorTimer when restarting blinkin…

38ba160

…g in EditableText

flutter-dashboard 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 7, 2022

Fix viewport offsets in tests

fc7b690

tgucio added 2 commits August 7, 2022 16:02

Rename _FloatingCursorPainter to _CaretPainter

5e44989

Rename floating cursor constants

3b0ec4d

flutter-dashboard bot added f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. labels Aug 7, 2022

tgucio and others added 5 commits August 8, 2022 10:07

Comment typo

11a70dd

Merge branch 'flutter:master' into editable-text-caret

be0e344

Allow show caret when cursor not active

243a5f7

Add test

6dad569

const TextSelection

d9e91bf

tgucio changed the title ~~[WIP] Remove Editable.onCaretChanged callback~~ Remove Editable.onCaretChanged callback Aug 10, 2022

tgucio added 2 commits August 11, 2022 00:18

Handle RenderEditable unmounted

4c23ce2

Simplify condition

462a276

justinmc requested a review from LongCatIsLooong August 11, 2022 22:00

LongCatIsLooong reviewed Aug 16, 2022

View reviewed changes

justinmc reviewed Aug 16, 2022

View reviewed changes

LongCatIsLooong reviewed Aug 16, 2022

View reviewed changes

Merge branch 'flutter:master' into editable-text-caret

2198305

tgucio added 2 commits October 6, 2022 10:13

Merge branch 'flutter:master' into editable-text-caret

6ebf582

Merge branch 'flutter:master' into editable-text-caret

4fe3b06

tgucio added 2 commits May 31, 2023 12:14

Merge branch 'master' into editable-text-caret

d54c799

Merge branch 'master' into editable-text-caret

6e3bfa0

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. and removed f: material design flutter/packages/flutter/material repository. labels Jun 16, 2023

tgucio and others added 2 commits June 20, 2023 11:47

Merge branch 'master' into editable-text-caret

f32755c

Use _showBlinkingCursor

4b87d47

tgucio mentioned this pull request Jun 20, 2023

Add EditableText scroll into view breaking change note flutter/website#8916

Merged

3 tasks

tgucio merged commit ca6f23e into flutter:master Jun 27, 2023

engine-flutter-autoroll mentioned this pull request Jun 28, 2023

Roll Flutter from 96a2c0535810 to 8d837062a447 (31 revisions) flutter/packages#4334

Closed

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

ca6f23e Remove Editable.onCaretChanged callback (flutter/flutter#109114)

bee3419

engine-flutter-autoroll mentioned this pull request Jun 28, 2023

Roll Flutter from 96a2c0535810 to 32fe4d7f0a27 (33 revisions) flutter/packages#4335

Closed

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

ca6f23e Remove Editable.onCaretChanged callback (flutter/flutter#109114)

14c7b45

engine-flutter-autoroll mentioned this pull request Jun 28, 2023

Roll Flutter from 96a2c0535810 to 73bb959a6524 (35 revisions) flutter/packages#4336

Closed

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

ca6f23e Remove Editable.onCaretChanged callback (flutter/flutter#109114)

4d1196b

Renzo-Olivares mentioned this pull request Jun 28, 2023

Fix master CI for simplistic_editor flutter/samples#1926

Merged

5 tasks

engine-flutter-autoroll mentioned this pull request Jun 28, 2023

Roll Flutter from 96a2c0535810 to 51bef1b63271 (37 revisions) flutter/packages#4338

Merged

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

ca6f23e Remove Editable.onCaretChanged callback (flutter/flutter#109114)

996be68

HansMuller mentioned this pull request Jun 30, 2023

Flutter Framework Team: Notable Commits #121415

Closed

goderbauer mentioned this pull request Jul 11, 2023

Move TapAndDragGestureRecognizer code under gestures #119508

Merged

8 tasks

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

ca6f23e Remove Editable.onCaretChanged callback (flutter/flutter#109114)

f443fb1

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

ca6f23e Remove Editable.onCaretChanged callback (flutter/flutter#109114)

2f6e6f8

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

ca6f23e Remove Editable.onCaretChanged callback (flutter/flutter#109114)

179d87b

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

ca6f23e Remove Editable.onCaretChanged callback (flutter/flutter#109114)

df75614

Remove Editable.onCaretChanged callback #109114

Remove Editable.onCaretChanged callback #109114

Uh oh!

Conversation

tgucio commented Aug 7, 2022 • 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 Aug 7, 2022

Uh oh!

tgucio commented Aug 7, 2022

Uh oh!

tgucio commented Aug 10, 2022

Uh oh!

LongCatIsLooong left a comment

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!

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!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

LongCatIsLooong commented Sep 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgucio commented Sep 20, 2022

Uh oh!

LongCatIsLooong commented May 30, 2023

Uh oh!

tgucio commented Jun 20, 2023

Uh oh!

tgucio commented Jun 20, 2023

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

tgucio commented Aug 7, 2022 •

edited

Loading

justinmc left a comment •

edited

Loading

LongCatIsLooong commented Sep 20, 2022 •

edited

Loading

LongCatIsLooong commented Jun 20, 2023 •

edited

Loading