Disable cursor opacity animation on macOS, make iOS cursor animation discrete #104335

LongCatIsLooong · 2022-05-21T21:09:53Z

Disable animated blinking cursor on macOS. The caret is an NSView that gets removed/added back periodically in AppKit text input views, its opacity doesn't change. This fixes Too much CPU usage with TextField caret on macOS #85781, CPU usage stays below/around 1% in my test app.
Make the iOS cursor blinking opacity animation discrete to match the native. This should also make the animation more energy efficient as we're no longer interpolating between the keyframes.
Disable "briefly show password" on iOS, since secured input fields don't seem to be doing that on iOS 15.4

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.

cyanglaz · 2022-06-15T17:25:18Z

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

+  _DiscreteKeyFrameSimulation.iOSBlinkingCaret() : this._(_KeyFrame.iOSBlinkingCaretKeyFrames);
+  _DiscreteKeyFrameSimulation._(this._keyFrames)
+    : assert(_keyFrames.isNotEmpty),
+      assert(_keyFrames.last.time < maxDuration);


Should we also make sure _keyFrames is sorted in an assert?

cyanglaz · 2022-06-15T17:28:25Z

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

+
+  @override
+  double x(double time) {
+    time = time % maxDuration;


Reuse the time parameter is a little hard to comprehend. What does the new "time" mean? Maybe create a new variable with a different name to better describe what is time % maxDuration? Or maybe add a comment to explain. (I'd still have a new variable for it)

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

cyanglaz · 2022-06-16T17:45:31Z

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

@@ -3489,7 +3543,7 @@ class EditableTextState extends State<EditableText> with AutomaticKeepAliveClien
      text = widget.obscuringCharacter * text.length;
      // Reveal the latest character in an obscured field only on mobile.


This comment has to be updated. Also a good place to explain why iOS is excluded, maybe add the issue link?

@LongCatIsLooong is this affected by your brieflyShowPassword thing, or was that Android only?

Oh I see you moved around a test related to this below.

Yeah that was for Android only.

cyanglaz · 2022-06-16T17:47:34Z

packages/flutter/test/widgets/editable_text_cursor_test.dart

+    await verifyKeyFrame(opacity: 0.5,  at: 925000);
+    await verifyKeyFrame(opacity: 0.75, at: 962500);
+    await verifyKeyFrame(opacity: 1.0,  at: 1000000);
+  }, variant: const TargetPlatformVariant(<TargetPlatform>{ TargetPlatform.iOS }));


Didn't we need to keep the macOS version of the test?

LongCatIsLooong

Changed the simulation to no longer wrap around when the input time becomes >= maxDuration. EditableTextState ends the animation anyways.

cyanglaz

LGTM

justinmc

LGTM 👍

justinmc · 2022-06-22T16:26:49Z

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

+// A time-value pair that represents a key frame in an animation.
+class _KeyFrame {
+  const _KeyFrame(this.time, this.value);
+  // Values extracted from iOS 15.4 UIKit.


How did you actually extract these values? Just out of my own curiosity.

justinmc · 2022-06-22T16:36:24Z

packages/flutter/test/material/text_field_test.dart

-  }, variant: const TargetPlatformVariant(<TargetPlatform>{ TargetPlatform.iOS,  TargetPlatform.macOS }));
-
-  // Regression test for https://github.com/flutter/flutter/issues/78918.
+ // Regression test for https://github.com/flutter/flutter/issues/78918.


I think you accidentally removed a space here if I'm reading the diff right.

justinmc · 2022-06-22T16:42:16Z

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

@@ -3489,7 +3543,7 @@ class EditableTextState extends State<EditableText> with AutomaticKeepAliveClien
      text = widget.obscuringCharacter * text.length;
      // Reveal the latest character in an obscured field only on mobile.


@LongCatIsLooong is this affected by your brieflyShowPassword thing, or was that Android only?

justinmc · 2022-06-22T16:43:11Z

packages/flutter/test/widgets/editable_text_cursor_test.dart

+      // verifies the opacity immediately *before* each key frame to avoid
+      // fp precision issues.


justinmc · 2022-06-22T16:44:16Z

packages/flutter/test/widgets/editable_text_cursor_test.dart

+      lastVerifiedOpacity = opacity;
+    }
+
+    await verifyKeyFrame(opacity: 1.0,  at: 500000);


Nit: Is it possible to reference the values in iOSBlinkingCaretKeyFrames instead of hardcoding them?

I'd prefer not to expose that animation, otherwise we would have to maintain that as a public API I think?

Oh right, I didn't realize it was in a private class. Sounds good as-is.

justinmc · 2022-06-22T16:48:02Z

packages/flutter/test/widgets/editable_text_cursor_test.dart

+      await tester.pump(const Duration(milliseconds: 100));
+      expect(tester.hasRunningAnimations, false);
+    }
+  }, variant: TargetPlatformVariant(TargetPlatform.values.toSet()..remove(TargetPlatform.iOS)));


Nice way to exclude a platform. See also #106216 CC @antholeole.

Almost exactly how 106216 is implemented! Depending on what gets in first, I’ll either change this to use the new API or this should use the new API.

Another good use case :).

justinmc · 2022-06-22T16:48:42Z

packages/flutter/test/widgets/editable_text_cursor_test.dart

@@ -956,4 +970,40 @@ void main() {
    );
    EditableText.debugDeterministicCursor = false;
  }, variant: const TargetPlatformVariant(<TargetPlatform>{ TargetPlatform.iOS,  TargetPlatform.macOS }));
+
+  testWidgets('password briefly does not show last character on Android if turned off', (WidgetTester tester) async {


This mentions Android but it's run on the current platform? Shouldn't it fail on iOS?

Renamed the test

justinmc · 2022-06-22T16:50:02Z

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

@@ -3489,7 +3543,7 @@ class EditableTextState extends State<EditableText> with AutomaticKeepAliveClien
      text = widget.obscuringCharacter * text.length;
      // Reveal the latest character in an obscured field only on mobile.


Oh I see you moved around a test related to this below.

…imation discrete (flutter/flutter#104335)

…animation discrete (flutter/flutter#104335)

…imation discrete (flutter#104335)"

…imation discrete (#104335)" (#106762)

…imation discrete (flutter#104335)" (flutter#106762)

…imation discrete (#104335)" (#106762) (#106766) Co-authored-by: LongCatIsLooong <31859944+LongCatIsLooong@users.noreply.github.com>

animation discrete (flutter#104335)" (reverted in flutter#106762) This reverts commit 8124191.

…imation discrete (#104335)" (#106893)

…discrete (flutter#104335)

…imation discrete (flutter#104335)" (flutter#106762)

…imation discrete (flutter#104335)" (flutter#106893)

…animation discrete (flutter/flutter#104335)

…imation discrete (flutter/flutter#104335)

LongCatIsLooong added 3 commits May 21, 2022 02:13

wip

656e971

Disable ios password reveal

acdd5fa

clean up

cc3abc3

LongCatIsLooong requested review from justinmc and goderbauer May 21, 2022 21:09

LongCatIsLooong added 2 commits June 14, 2022 11:02

clean up

567100e

Merge remote-tracking branch 'upstream/master' into ios-animated-cursor

503e7c1

cyanglaz reviewed Jun 16, 2022

View reviewed changes

LongCatIsLooong added 3 commits June 16, 2022 13:27

Don't wrap in KeyFrameSimulation

5b70377

Add test

01a1b9b

update comment

2940777

LongCatIsLooong commented Jun 16, 2022

View reviewed changes

LongCatIsLooong requested a review from cyanglaz June 16, 2022 20:46

cyanglaz approved these changes Jun 22, 2022

View reviewed changes

justinmc approved these changes Jun 22, 2022

View reviewed changes

review

c2b004c

LongCatIsLooong added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 22, 2022

auto-submit bot merged commit 60f30e5 into flutter:master Jun 22, 2022

LongCatIsLooong deleted the ios-animated-cursor branch June 22, 2022 18:14

engine-flutter-autoroll mentioned this pull request Jun 22, 2022

Roll Flutter from dfaec11ba519 to 60f30e5d3ed4 (23 revisions) flutter/plugins#6015

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jun 22, 2022

60f30e5 Disable cursor opacity animation on macOS, make iOS cursor an…

64eeb60

…imation discrete (flutter/flutter#104335)

engine-flutter-autoroll mentioned this pull request Jun 23, 2022

Roll Flutter from 2540547f06d6 to a30012b27505 (11 revisions) flutter/packages#2273

Closed

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

60f30e5d3 Disable cursor opacity animation on macOS, make iOS cursor …

fc1023c

…animation discrete (flutter/flutter#104335)

engine-flutter-autoroll mentioned this pull request Jun 23, 2022

Roll Flutter from 2540547f06d6 to fe41b6abcd53 (12 revisions) flutter/packages#2274

Merged

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

60f30e5d3 Disable cursor opacity animation on macOS, make iOS cursor …

bd93e48

…animation discrete (flutter/flutter#104335)

TahaTesser mentioned this pull request Jun 27, 2022

Textfield cursor does not disappear on losing the focus #106602

Closed

LongCatIsLooong added a commit to LongCatIsLooong/flutter that referenced this pull request Jun 28, 2022

Revert "Disable cursor opacity animation on macOS, make iOS cursor an…

c2a5644

…imation discrete (flutter#104335)"

auto-submit bot pushed a commit that referenced this pull request Jun 28, 2022

Revert "Disable cursor opacity animation on macOS, make iOS cursor an…

8124191

…imation discrete (#104335)" (#106762)

XilaiZhang pushed a commit to XilaiZhang/flutter that referenced this pull request Jun 28, 2022

Revert "Disable cursor opacity animation on macOS, make iOS cursor an…

9c9ce69

…imation discrete (flutter#104335)" (flutter#106762)

XilaiZhang mentioned this pull request Jun 28, 2022

[cherry pick] Revert "Disable cursor opacity animation on macOS, make iOS cursor an… #106766

Merged

XilaiZhang added a commit that referenced this pull request Jun 28, 2022

Revert "Disable cursor opacity animation on macOS, make iOS cursor an…

a8580fb

…imation discrete (#104335)" (#106762) (#106766) Co-authored-by: LongCatIsLooong <31859944+LongCatIsLooong@users.noreply.github.com>

LongCatIsLooong added a commit to LongCatIsLooong/flutter that referenced this pull request Jun 30, 2022

Reland "Disable cursor opacity animation on macOS, make iOS cursor

ef81ff6

animation discrete (flutter#104335)" (reverted in flutter#106762) This reverts commit 8124191.

auto-submit bot pushed a commit that referenced this pull request Jul 8, 2022

Reland "Disable cursor opacity animation on macOS, make iOS cursor an…

c58dca2

…imation discrete (#104335)" (#106893)

camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022

Disable cursor opacity animation on macOS, make iOS cursor animation …

b4b297a

…discrete (flutter#104335)

camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022

Revert "Disable cursor opacity animation on macOS, make iOS cursor an…

7d92cf2

…imation discrete (flutter#104335)" (flutter#106762)

camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022

Reland "Disable cursor opacity animation on macOS, make iOS cursor an…

c1bd4c5

…imation discrete (flutter#104335)" (flutter#106893)

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

60f30e5d3 Disable cursor opacity animation on macOS, make iOS cursor …

ccd2129

…animation discrete (flutter/flutter#104335)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 30, 2022

60f30e5 Disable cursor opacity animation on macOS, make iOS cursor an…

3380e3a

…imation discrete (flutter/flutter#104335)

blaugold mentioned this pull request Oct 10, 2022

TextFormField focus consumes enormous system resources on simulator & iPhone #113192

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable cursor opacity animation on macOS, make iOS cursor animation discrete #104335

Disable cursor opacity animation on macOS, make iOS cursor animation discrete #104335

LongCatIsLooong commented May 21, 2022 •

edited

Loading

cyanglaz Jun 15, 2022

cyanglaz Jun 15, 2022

cyanglaz Jun 16, 2022

justinmc Jun 22, 2022

justinmc Jun 22, 2022

LongCatIsLooong Jun 22, 2022

cyanglaz Jun 16, 2022

LongCatIsLooong left a comment

cyanglaz left a comment

justinmc left a comment

justinmc Jun 22, 2022

justinmc Jun 22, 2022

justinmc Jun 22, 2022

justinmc Jun 22, 2022

justinmc Jun 22, 2022

LongCatIsLooong Jun 22, 2022

justinmc Jun 22, 2022

justinmc Jun 22, 2022

antholeole Jun 22, 2022

justinmc Jun 22, 2022

LongCatIsLooong Jun 22, 2022

justinmc Jun 22, 2022

		@@ -3489,7 +3543,7 @@ class EditableTextState extends State<EditableText> with AutomaticKeepAliveClien
		text = widget.obscuringCharacter * text.length;
		// Reveal the latest character in an obscured field only on mobile.

		// verifies the opacity immediately before each key frame to avoid
		// fp precision issues.

Disable cursor opacity animation on macOS, make iOS cursor animation discrete #104335

Disable cursor opacity animation on macOS, make iOS cursor animation discrete #104335

Conversation

LongCatIsLooong commented May 21, 2022 • edited Loading

Pre-launch Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LongCatIsLooong left a comment

Choose a reason for hiding this comment

cyanglaz left a comment

Choose a reason for hiding this comment

justinmc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LongCatIsLooong commented May 21, 2022 •

edited

Loading