Fixed cursor blinking during selection. #141380

yiiim · 2024-01-11T15:56:40Z

Fixed the issue of cursor blinking during selection as shown below.

Screen.Recording.2024-01-11.at.23.32.07.mov

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.

Renzo-Olivares · 2024-01-11T19:58:30Z

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

+        // Stop cursor blinking and making it visible.
+        _stopCursorBlink(resetCharTicks: false);
+        _cursorBlinkOpacityController.value = 1.0;


It looks like we already call this on FloatingCursorDragState.Start, is something else resetting the cursor blink?

Yes, the cursor blinking is reset here when the selection changes. I should note this in the code.

Thanks for the explanation. I think given these details, we should probably try prevent the blinking cursor from turning itself back on while the selection is changing due to floating cursor. Maybe by updating the logic in _startOrStopCursorTimerIfNeeded.

I have made changes to the implementation, could you please take another look?

…cursor_blink

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

Renzo-Olivares · 2024-01-18T22:25:25Z

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


  void _stopCursorBlink({ bool resetCharTicks = true }) {
-    _cursorBlinkOpacityController.value = 0.0;
+    _cursorBlinkOpacityController.value = renderEditable.floatingCursorOn ? 1.0 : 0.0;


Why is this needed? Shouldn't _stopCursorBlink stop the cursor blink as expected without changes?

Even when the floating cursor is displayed, we cannot ignore the call to _stopCursorBlink in _didChangeTextEditingValue, which leads to some test failures. I haven't investigated the specific reasons yet, but I think it's feasible to always keep the cursor visible in _stopCursorBlink when the floating cursor appears. Should I investigate the reasons for the test failures or maintain the status quo?

I think I've found some clues. We need to set the value of _cursorBlinkOpacityController to stop the animation. This is to avoid re-executing _onCursorTick when the animation ends at this location:

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

Line 4004 in 5887d6c

_cursorTimer = Timer(Duration.zero, () => _cursorBlinkOpacityController.animateWith(_iosBlinkCursorSimulation).whenComplete(_onCursorTick));

.

Sorry for the delay on the review, just ran this locally and I understand what you mean. I'm okay with the code as-is then, but could you add a comment explaining why we take the floating cursor into account.

Renzo-Olivares · 2024-02-01T19:52:39Z

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

  void _stopCursorBlink({ bool resetCharTicks = true }) {
-    _cursorBlinkOpacityController.value = 0.0;
+    // If the cursor is animating, stop the animation, and we always
+    // want the cursor to be visible when the floating cursor is enabled


nit: add period at the end of this comment.

Renzo-Olivares

LGTM with small nit. Thank you for the contribution!

justinmc

LGTM 👍

Good catch, thanks for fixing this! Is there an issue anywhere about this that you're aware of?

yiiim · 2024-02-08T06:56:06Z

LGTM 👍

Good catch, thanks for fixing this! Is there an issue anywhere about this that you're aware of?

There is no related issue, this is a problem found after my other PR#138479.

Roll Flutter from 8431cae to eb5d0a4 (33 revisions) flutter/flutter@8431cae...eb5d0a4 2024-02-08 [email protected] Roll Packages from e4ea6bf to 29d8cc0 (5 revisions) (flutter/flutter#143161) 2024-02-08 [email protected] Roll Flutter Engine from 6d524588c0f9 to f53fbd8393ce (1 revision) (flutter/flutter#143160) 2024-02-08 [email protected] Roll Flutter Engine from 22e1241e9823 to 6d524588c0f9 (3 revisions) (flutter/flutter#143147) 2024-02-08 [email protected] Roll Flutter Engine from 084bd0b8f5ac to 22e1241e9823 (1 revision) (flutter/flutter#143140) 2024-02-08 [email protected] Roll Flutter Engine from 104804aa374e to 084bd0b8f5ac (1 revision) (flutter/flutter#143139) 2024-02-08 [email protected] Roll Flutter Engine from 45137ea6ba29 to 104804aa374e (1 revision) (flutter/flutter#143138) 2024-02-08 [email protected] Pass along web renderer into debugging options in the test command. (flutter/flutter#143128) 2024-02-08 [email protected] Roll Flutter Engine from 4ea7bd07328b to 45137ea6ba29 (2 revisions) (flutter/flutter#143135) 2024-02-08 [email protected] Roll Flutter Engine from 322a461dc74f to 4ea7bd07328b (2 revisions) (flutter/flutter#143131) 2024-02-07 [email protected] Add a unit test for `NavigationRail` (flutter/flutter#143108) 2024-02-07 [email protected] Roll Flutter Engine from b1ba9f329f76 to 322a461dc74f (2 revisions) (flutter/flutter#143123) 2024-02-07 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Improve build output for all platforms" (flutter/flutter#143125) 2024-02-07 [email protected] Roll Flutter Engine from e4a5acc14b44 to b1ba9f329f76 (2 revisions) (flutter/flutter#143120) 2024-02-07 [email protected] Improve build output for all platforms (flutter/flutter#128236) 2024-02-07 [email protected] Roll Flutter Engine from 19ae46afed69 to e4a5acc14b44 (7 revisions) (flutter/flutter#143112) 2024-02-07 [email protected] Fixed cursor blinking during selection. (flutter/flutter#141380) 2024-02-07 [email protected] Remove Cirrus check for web test concurrency (flutter/flutter#143046) 2024-02-07 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Enforce a policy on supported Gradle, Java, AGP, and KGP versions" (flutter/flutter#143114) 2024-02-07 [email protected] Enforce a policy on supported Gradle, Java, AGP, and KGP versions (flutter/flutter#142000) 2024-02-07 [email protected] Fix inputs and outputs for `WebReleaseBundle` (flutter/flutter#143023) 2024-02-07 [email protected] Various improvements to text-editing-related documentation. (flutter/flutter#142561) 2024-02-07 [email protected] Revert "Add `SingleChildScrollView` for `NavigationRail`" (flutter/flutter#143097) 2024-02-07 [email protected] Restore log dumps for gradle OOM crashes, and set a value for `MaxMetaspaceSize` (flutter/flutter#143085) 2024-02-07 [email protected] Move Mac_build_test flutter_gallery__transition_perf_e2e_ios out of staging (flutter/flutter#142987) 2024-02-07 [email protected] Roll Flutter Engine from fafd8e55eb4b to 19ae46afed69 (1 revision) (flutter/flutter#143094) 2024-02-07 [email protected] Add a test for the isAvailableForEnvironment logic (flutter/flutter#142251) 2024-02-07 [email protected] Add the number of mismatched pixels to golden tests output (flutter/flutter#142975) 2024-02-07 [email protected] Add indexInParent to SemanticsNode debug information (flutter/flutter#142826) 2024-02-07 [email protected] Remove unused devicelab task gradle_non_android_plugin_test (flutter/flutter#142742) 2024-02-07 [email protected] Make generate_gradle_lockfiles also write the gradle wrapper file (flutter/flutter#142329) 2024-02-07 [email protected] Roll Flutter Engine from 6807342305e4 to fafd8e55eb4b (1 revision) (flutter/flutter#143086) 2024-02-07 [email protected] Remove redundant `rootDirectoryPath` parameter in `DevFS::update` (flutter/flutter#143034) 2024-02-07 [email protected] Handle transitions to AppLifecycleState.detached in lifecycle state generation (flutter/flutter#142523) 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 [email protected],[email protected],[email protected] 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 To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: ...

Fixed cursor blinking during selection.

4a86e53

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. f: material design flutter/packages/flutter/material repository. labels Jan 11, 2024

Renzo-Olivares reviewed Jan 11, 2024

View reviewed changes

Added some comments

9778e03

yiiim requested a review from Renzo-Olivares January 16, 2024 10:28

yiiim added 4 commits January 18, 2024 23:35

fix cursor blink

230fea7

Merge branch 'master' into fix_cursor_blink

50c45c7

fix test

1ebaf59

Merge branch 'fix_cursor_blink' of github.com:yiiim/flutter into fix_…

daa91f6

…cursor_blink

Renzo-Olivares reviewed Jan 18, 2024

View reviewed changes

packages/flutter/lib/src/rendering/editable.dart Show resolved Hide resolved

Renzo-Olivares reviewed Jan 18, 2024

View reviewed changes

justinmc requested a review from Renzo-Olivares January 25, 2024 22:55

yiiim added 2 commits January 31, 2024 23:37

add some comments

aad946c

Merge branch 'master' into fix_cursor_blink

5652c05

Renzo-Olivares reviewed Feb 1, 2024

View reviewed changes

Renzo-Olivares approved these changes Feb 1, 2024

View reviewed changes

justinmc self-requested a review February 1, 2024 23:12

yiiim added 2 commits February 4, 2024 23:54

fix nits

1750bc5

Merge branch 'master' into fix_cursor_blink

34a56ae

justinmc approved these changes Feb 7, 2024

View reviewed changes

justinmc merged commit 16e7218 into flutter:master Feb 7, 2024

engine-flutter-autoroll mentioned this pull request Feb 8, 2024

Roll Flutter from 8431cae8843c to 92c7ba819129 (31 revisions) flutter/packages#6078

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 8, 2024

16e7218 Fixed cursor blinking during selection. (flutter/flutter#141380)

2696ce2

engine-flutter-autoroll mentioned this pull request Feb 8, 2024

Roll Flutter from 8431cae8843c to eb5d0a434ef1 (33 revisions) flutter/packages#6079

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 8, 2024

16e7218 Fixed cursor blinking during selection. (flutter/flutter#141380)

a952246

HansMuller mentioned this pull request Feb 9, 2024

Flutter Framework Team: Notable Commits #121415

Closed

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

16e7218 Fixed cursor blinking during selection. (flutter/flutter#141380)

f52b18e

Fixed cursor blinking during selection. #141380

Fixed cursor blinking during selection. #141380

Uh oh!

Conversation

yiiim commented Jan 11, 2024

Pre-launch Checklist

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!

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

Choose a reason for hiding this comment

Uh oh!

yiiim commented Feb 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants