Feat : `TextField` cursor color matching M2 and M3 Spec in error state by hasnentai · Pull Request #119225 · flutter/flutter

hasnentai · 2023-01-26T09:03:17Z

TextField in error states should have a cursor color which matches other error-themed colours.

Screen

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.

LongCatIsLooong · 2023-01-26T21:08:34Z

Could you add the newline back at the end of text_field.dart? The analyzer is complaining (https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8790918503105134049/+/u/Analyze/analyze/test_stdout):

info • Missing a newline at the end of the file • packages/flutter/lib/src/material/text_field.dart:1465:46 • eol_at_end_of_file

hasnentai · 2023-01-26T21:18:29Z

I was wondering why the checks were failing. Thanks
I have added a new line in the file

justinmc

Thanks for fixing this! Mostly just minor comments below.

justinmc · 2023-01-26T23:42:13Z

packages/flutter/test/material/text_form_field_test.dart

  });
+
+
+    testWidgets('Error color for cursor while validation', (WidgetTester tester) async {


Outdent this, and remove the extra newline above.

ahh sorry, I just missed it. I have made the changes

justinmc · 2023-01-26T23:42:21Z

packages/flutter/test/material/text_form_field_test.dart

+    final EditableText textField =  tester.widget(find.byType(EditableText).first);
+    await tester.pump();
+    expect(textField.cursorColor, errorColor);
+


Remove the extra newline here.

justinmc · 2023-01-27T00:18:06Z

packages/flutter/test/material/text_form_field_test.dart

+
+    await tester.pumpWidget(
+      MaterialApp(
+        theme: ThemeData(colorScheme:  const ColorScheme.light(error: errorColor)),


I think there are multiple spaces before the const.

Also nit: Maybe split the colorScheme parameter onto its own line.

I have made it to a newline of color scheme to its own

justinmc · 2023-01-27T00:20:08Z

packages/flutter/lib/src/material/text_field.dart

        paintCursorAboveText = true;
        cursorOpacityAnimates = true;
-        cursorColor = widget.cursorColor ?? selectionStyle.cursorColor ?? cupertinoTheme.primaryColor;
+        cursorColor = _getCursorColor(selectionStyle, theme);


Should theme be cupertinoTheme here?

Great catch, Added Cupertino Theme for Mac and Ios platform case

justinmc · 2023-01-27T00:20:16Z

packages/flutter/lib/src/material/text_field.dart

        paintCursorAboveText = true;
        cursorOpacityAnimates = false;
-        cursorColor = widget.cursorColor ?? selectionStyle.cursorColor ?? cupertinoTheme.primaryColor;
+        cursorColor = _getCursorColor(selectionStyle, theme);


Same thing about cupertinoTheme here too.

justinmc · 2023-01-27T00:24:56Z

packages/flutter/lib/src/material/text_field.dart

+  /// Sets the cursor color to error color from decoration or from theme colorScheme if [_hasError] is true else
+  /// it will return the color from [widget.cursorColor] param if not null. if [widget.cursorColor] is null then it will return default style else return
+  /// color from theme colorScheme.


Really I think this entire comment is self explanatory by looking at the code and the method signature. I would just remove it.

justinmc · 2023-01-27T00:25:06Z

packages/flutter/lib/src/material/text_field.dart

+  /// Sets the cursor color to error color from decoration or from theme colorScheme if [_hasError] is true else
+  /// it will return the color from [widget.cursorColor] param if not null. if [widget.cursorColor] is null then it will return default style else return
+  /// color from theme colorScheme.
+  Color _getCursorColor(DefaultSelectionStyle selectionStyle, ThemeData theme){


Suggested change

Color _getCursorColor(DefaultSelectionStyle selectionStyle, ThemeData theme){

Color _getCursorColor (DefaultSelectionStyle selectionStyle, ThemeData theme) {

Added space

justinmc · 2023-01-27T00:25:21Z

packages/flutter/lib/src/material/text_field.dart

+  /// it will return the color from [widget.cursorColor] param if not null. if [widget.cursorColor] is null then it will return default style else return
+  /// color from theme colorScheme.
+  Color _getCursorColor(DefaultSelectionStyle selectionStyle, ThemeData theme){
+    if(_hasError){


Suggested change

if(_hasError){

if (_hasError) {

Added space

justinmc · 2023-01-27T00:26:27Z

packages/flutter/lib/src/material/text_field.dart

+  /// color from theme colorScheme.
+  Color _getCursorColor(DefaultSelectionStyle selectionStyle, ThemeData theme){
+    if(_hasError){
+      return _errorColor;


Nit: You could just inline this here rather than making it a getter, but either way works.

I have made it inline

hasnentai · 2023-01-27T15:38:15Z

Hi @justinmc Google Test is failing
What could be the possible reason ?

justinmc · 2023-02-02T21:57:30Z

Hmm it's not showing up in the Google testing tool for me. Can you try pushing a merge commit with master?

hasnentai · 2023-03-03T21:55:12Z

@justinmc Finally the checks are passing 😀. Please review it once

justinmc

LGTM 👍

I've triggered Google tests for this PR, so let's see what that says too before merge.

justinmc · 2023-03-03T23:04:09Z

packages/flutter/test/material/text_form_field_test.dart

+    const Color errorColor = Color(0xff123456);
+    await tester.pumpWidget(MaterialApp(
+      theme: ThemeData(
+        colorScheme: const ColorScheme.light(error: errorColor)


Missing a trailing comma here.

justinmc · 2023-03-03T23:04:44Z

packages/flutter/test/material/text_form_field_test.dart

    expect(editableText.magnifierConfiguration, equals(myTextMagnifierConfiguration));
  });
+
+  testWidgets('Error color for cursor while validation', (WidgetTester tester) async {


Nit: "while validation" => "during validation" or "while validating"

Updated the desc

justinmc · 2023-03-03T23:05:06Z

packages/flutter/test/material/text_form_field_test.dart

+      home: Material(
+        child: Center(
+          child: TextFormField(
+            enabled: true,


Is this needed?

justinmc

Renewing my LGTM.

Also, I forgot but we need a secondary reviewer. I'll ask around.

gnprice

LGTM

justinmc · 2023-03-09T00:35:23Z

Thanks for the secondary review! I'll plan to merge this tomorrow morning.

…ror state (flutter/flutter#119225)

flutter#119225) According to Material specs, cursor should be red in error state.

…ror state (flutter/flutter#119225)

justinmc · 2023-03-13T17:40:58Z

@hasnentai Should the handle also be red instead of blue? I made an issue to track this (#122538) but I couldn't find a spec or anything that showed it being red.

hasnentai · 2023-03-13T17:46:39Z

Hi, @justinmc I think having a red handle makes more sense in error state and also maintains the colour uniformity.Let me know if we have to implement the handle colour I would be more then happy to take it up 😊

justinmc · 2023-03-16T19:10:30Z

@hasnentai Can you confirm that the handle should be red by default on native Android? If so then I think we should definitely make the change.

hasnentai · 2023-03-16T19:12:14Z

On it.Will get back after checking on native

justinmc · 2023-03-16T19:13:50Z

Thanks!

…ror state (flutter/flutter#119225)

hasnentai added 2 commits January 26, 2023 07:09

fixed text feild cursor

add8764

Fix: Test Added

2082e02

flutter-dashboard bot added a: text input Entering text in a text field or keyboard related problems f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Jan 26, 2023

Fix: Added New Line

25b8af9

justinmc requested review from LongCatIsLooong and justinmc January 26, 2023 23:09

justinmc reviewed Jan 27, 2023

View reviewed changes

Fix: Review changes

ff35855

hasnentai requested review from justinmc and removed request for LongCatIsLooong January 27, 2023 07:08

hasnentai added 2 commits January 27, 2023 15:54

Fix: Review changes

8ea7dcf

Fix: Review changes

f110cf9

Merge branch 'master' into fix-text-feild

af05604

justinmc mentioned this pull request Mar 3, 2023

M3 textfield issues #114950

Open

5 tasks

justinmc approved these changes Mar 3, 2023

View reviewed changes

justinmc requested review from LongCatIsLooong and Renzo-Olivares March 3, 2023 23:07

hasnentai requested review from justinmc and removed request for LongCatIsLooong and Renzo-Olivares March 4, 2023 12:53

hasnentai requested review from christopherfujino, jmagman and keyonghan as code owners March 4, 2023 17:04

justinmc approved these changes Mar 6, 2023

View reviewed changes

justinmc mentioned this pull request Mar 6, 2023

media_query_test flake #122045

Closed

gnprice approved these changes Mar 7, 2023

View reviewed changes

justinmc merged commit c5da507 into flutter:master Mar 9, 2023

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

Roll Flutter from 5a279edc239f to c5da5070f10e (32 revisions) flutter/packages#3429

Closed

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

c5da507 Feat : TextField cursor color matching M2 and M3 Spec in er…

58e7efa

…ror state (flutter/flutter#119225)

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

Roll Flutter from 5a279edc239f to 1f426123230b (53 revisions) flutter/packages#3437

Closed

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

c5da507 Feat : TextField cursor color matching M2 and M3 Spec in er…

81a80a3

…ror state (flutter/flutter#119225)

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

Roll Flutter from 5a279edc239f to e98c53751ef9 (55 revisions) flutter/packages#3440

Closed

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

c5da507 Feat : TextField cursor color matching M2 and M3 Spec in er…

9e625bb

…ror state (flutter/flutter#119225)

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

Roll Flutter from 5a279edc239f to 3f2cca8eebfd (56 revisions) flutter/packages#3441

Closed

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

c5da507 Feat : TextField cursor color matching M2 and M3 Spec in er…

b3585ec

…ror state (flutter/flutter#119225)

hannah-hyj pushed a commit to hannah-hyj/flutter that referenced this pull request Mar 11, 2023

Feat : TextField cursor color matching M2 and M3 Spec in error state (

e6d95c1

flutter#119225) According to Material specs, cursor should be red in error state.

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

Roll Flutter from 5a279edc239f to f9ad42a32d7e (69 revisions) flutter/packages#3443

Closed

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

c5da507 Feat : TextField cursor color matching M2 and M3 Spec in er…

60da88d

…ror state (flutter/flutter#119225)

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

Roll Flutter from 5a279edc239f to 9ba0d08ebc07 (72 revisions) flutter/packages#3445

Merged

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

c5da507 Feat : TextField cursor color matching M2 and M3 Spec in er…

d9f85f3

…ror state (flutter/flutter#119225)

justinmc mentioned this pull request Mar 13, 2023

Selection handle should be red when invalid #122538

Closed

HansMuller mentioned this pull request Mar 16, 2023

Flutter Framework Team: Notable Commits #121415

Closed

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

c5da507 Feat : TextField cursor color matching M2 and M3 Spec in er…

2c839c0

…ror state (flutter/flutter#119225)

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

c5da507 Feat : TextField cursor color matching M2 and M3 Spec in er…

d3fb70b

…ror state (flutter/flutter#119225)

darshankawar mentioned this pull request Sep 12, 2023

TextFormField - when validation fails, cursor color is assigned to be the same as the error text color #134451

Closed

2 tasks

dam-ease mentioned this pull request Sep 27, 2023

TextFormField - when validation fails, cursor color is assigned to be the same as the error text color #135580

Closed

2 tasks

		});


		testWidgets('Error color for cursor while validation', (WidgetTester tester) async {

	Color _getCursorColor(DefaultSelectionStyle selectionStyle, ThemeData theme){
	Color _getCursorColor (DefaultSelectionStyle selectionStyle, ThemeData theme) {

Conversation

hasnentai commented Jan 26, 2023 • edited by guidezpl Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Screen

Pre-launch Checklist

Uh oh!

LongCatIsLooong commented Jan 26, 2023

Uh oh!

hasnentai commented Jan 26, 2023

Uh oh!

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

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!

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!

hasnentai commented Jan 27, 2023

Uh oh!

justinmc commented Feb 2, 2023

Uh oh!

hasnentai commented Mar 3, 2023

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hasnentai Mar 4, 2023 • 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!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc commented Mar 9, 2023

Uh oh!

justinmc commented Mar 13, 2023

Uh oh!

hasnentai commented Jan 26, 2023 •

edited by guidezpl

Loading

hasnentai Mar 4, 2023 •

edited

Loading