Fix FormField implementation components TextFormField, DropdownButtonFormField, CupertinoTextFormFieldRow. The problem that their onChange() method cannot be executed when the parent Form component calls the reset() method by yeleibo · Pull Request #123108 · flutter/flutter

yeleibo · 2023-03-21T12:38:13Z

code sample

```

import 'package:flutter/cupertino.dart';
import 'package:flutter/material.dart';

void main() => runApp(const MyApp());

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      debugShowCheckedModeBanner: false,
      theme: ThemeData(useMaterial3: true),
      home: Example(),
    );
  }
}

class Example extends StatelessWidget {
  final _formKey = GlobalKey<FormState>();
  Example({super.key});

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Form(
          key: _formKey,
          child: Column(
            children: [
              CupertinoTextFormFieldRow(onChanged: (value) {
                print("CupertinoTextFormFieldRow widget onChange execute");
              }),
              TextFormField(onChanged: (value) {
                print("TextFormField widget onChange execute");
              }),
              DropdownButtonFormField(
                items: const [
                  DropdownMenuItem(
                    value: "item1",
                    child: Text("item1"),
                  )
                ],
                onChanged: (value) {
                  print("DropdownButtonFormField widget onChange execute");
                },
              ),
              ElevatedButton(
                  onPressed: () {
                    _formKey.currentState?.reset();
                  },
                  child: const Text("reset")),
            ],
          )),
    );
  }
}

…ieldRow widget reset function Fix the implementation of FormField components: TextFormField, DropdownButtonFormField, and CupertinoTextFormFieldRow. Address the issue where their onChange() method does not get executed when the parent Form component calls reset().

flutter-dashboard · 2023-03-21T12:38:17Z

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.

google-cla · 2023-03-21T12:38:17Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

justinmc

Thanks for the PR! Are you able to add a test for this?

justinmc · 2023-04-06T22:20:50Z

    }
+    _cupertinoTextFormFieldRow.onChanged?.call(_effectiveController!.text);
+
+


Nit: Too many newlines here.

…FormField

…hange_unexecuted

yeleibo · 2023-04-25T10:08:28Z

Thanks for the PR! Are you able to add a test for this?
Yes, tests have been added.
The Linux docs_test action check is not passing, but I'm not sure why.

justinmc

Thanks for adding the tests! I think the docs failures have to do with macros that don't exist. I commented about that below.

justinmc · 2023-04-27T17:28:12Z

  /// initialize its [TextEditingController.text] with [initialValue].
  final TextEditingController? controller;
-
+  /// {@macro flutter.material.TextField.onChanged}


This macro doesn't exist, did you mean flutter.material.editableText.onChanged? Or you could create this macro if you want the entire TextField.onChanged docs.

justinmc · 2023-04-27T17:28:40Z

  /// initialize its [TextEditingController.text] with [initialValue].
  final TextEditingController? controller;
-
+  /// {@macro flutter.material.CupertinoFormRow.onChanged}


This macro also doesn't seem to exist.

justinmc · 2023-04-27T17:29:04Z

+  testWidgets('CupertinoTextFormFieldRow onChanged to initialValue when Form reset', (WidgetTester tester) async {
+    final GlobalKey<FormFieldState<String>> stateKey = GlobalKey<FormFieldState<String>>();
+    final GlobalKey<FormState> formKey = GlobalKey<FormState>();
+    String value='initialValue';


Suggested change

String value='initialValue';

String value = 'initialValue';

And anywhere else you're not spacing this out needs to have spaces too.

Co-authored-by: Justin McCandless <justinjmccandless@gmail.com>

…hange_unexecuted

justinmc · 2023-05-08T20:39:32Z

Looks like there are still docs test failures, it mentions [onSubmitted].

…hange_unexecuted

yeleibo · 2023-05-11T10:34:52Z

Looks like there are still docs test failures, it mentions [onSubmitted].

I used {@macro flutter.widgets.editableText.onChanged} in the comment of the onChanged property of a TextFormFieldRow. However, {@template flutter.widgets.editableText.onChanged} references the onSubmitted property of its own widget (which is indicated in the comment as [onSubmitted]), while my widget does not have an onSubmitted property, so the docs failures

justinmc

Mostly nits here. Thanks for getting all of the checks to pass.

justinmc · 2023-05-18T23:28:07Z


+  /// Called when the user initiates a change to the TextField's
+  /// value: when they have inserted or deleted text or form reset.
+  final ValueChanged<String>? onChanged;


Nit: Add a newline after this.

justinmc · 2023-05-18T23:28:45Z


+  /// Called when the user initiates a change to the TextField's
+  /// value: when they have inserted or deleted text or form reset.
+  final ValueChanged<String>? onChanged;


Nit: Newline here too.

justinmc · 2023-05-18T23:29:19Z

+    await tester.enterText(find.byType(CupertinoTextField), 'changedValue');
+    expect(stateKey.currentState!.value,'changedValue');
+    expect(value, 'changedValue');
+    // form to reset


Change to: "Reset form."

justinmc · 2023-05-18T23:29:54Z

+
+    expect(value, isNull);
+    expect(stateKey.currentState!.value, equals('One'));
+    //value chang to Two


// Change value to "Two".

justinmc · 2023-05-18T23:30:20Z

+    await tester.pumpAndSettle();
+    expect(value, equals('Two'));
+    expect(stateKey.currentState!.value, equals('Two'));
+    // form to reset


// Reset form.

justinmc · 2023-05-18T23:30:23Z

+    await tester.enterText(find.byType(TextField), 'changedValue');
+    expect(stateKey.currentState!.value,'changedValue');
+    expect(value, 'changedValue');
+    // form to reset


"Reset form."

justinmc · 2023-05-18T23:30:35Z

+
+    expect(stateKey.currentState!.value, 'initialValue');
+    expect(value, 'initialValue');
+    //change value to 'changedValue'


// Change value to "changedValue".

justinmc · 2023-05-18T23:35:47Z

        _effectiveController!.text = widget.initialValue!;
      });
    }
+    _cupertinoTextFormFieldRow.onChanged?.call(_effectiveController!.text);


What if the value didn't actually change, should onChanged still be called?

justinmc · 2023-05-18T23:36:30Z

+    final DropdownButtonFormField<T> dropdownButtonFormField = widget as DropdownButtonFormField<T>;
+    assert(dropdownButtonFormField.onChanged != null);
+    dropdownButtonFormField.onChanged!(widget.initialValue);
+    super.reset();


In CupertinoTextFormFieldRow, reset calls super on the first line. Should this one also call it on the first line?

justinmc · 2023-05-18T23:37:43Z

    // manipulated, no setState call is needed here.
    _effectiveController.text = widget.initialValue ?? '';
+    _textFormField.onChanged?.call(_effectiveController.text);
    super.reset();


And this one calls super on the last line again. Not sure which is right 🤷 .

justinmc

Mostly nits here. Thanks for getting all of the checks to pass.

Hixie · 2023-07-18T22:52:07Z

@yeleibo Thanks for the contribution! Will you be able to continue working on this? It's totally fine if not; we can take it over (though it might take longer). I'm just asking because I'm trying to clean out our review queue.

bleroux · 2023-09-08T12:52:40Z

Closing this PR because I filed #134295 based on the work done here.
Thanks @yeleibo for initiating this work!

## Description This PR fixes form fields in order to call the `onChange` callback when the form is reset. This change is based on the work done in #123108. I considered adding the `onChange` callback to the `FormField` superclass but it would break existing code because two of the three subclasses defines the `onChange` callback with `ValueChanged<String>?` type and the third one defines it with `ValueChanged<String?>?`. ## Related Issue Fixes #123009. ## Tests Adds 3 tests.

## Description This PR fixes form fields in order to call the `onChange` callback when the form is reset. This change is based on the work done in flutter#123108. I considered adding the `onChange` callback to the `FormField` superclass but it would break existing code because two of the three subclasses defines the `onChange` callback with `ValueChanged<String>?` type and the third one defines it with `ValueChanged<String?>?`. ## Related Issue Fixes flutter#123009. ## Tests Adds 3 tests.

## Description This PR fixes form fields in order to call the `onChange` callback when the form is reset. This change is based on the work done in flutter/flutter#123108. I considered adding the `onChange` callback to the `FormField` superclass but it would break existing code because two of the three subclasses defines the `onChange` callback with `ValueChanged<String>?` type and the third one defines it with `ValueChanged<String?>?`. ## Related Issue Fixes flutter/flutter#123009. ## Tests Adds 3 tests.

flutter-dashboard Bot added a: text input Entering text in a text field or keyboard related problems p: cupertino_ui cupertino_ui package in flutter/packages p: material_ui material_ui package in flutter/packages framework flutter/packages/flutter repository. See also f: labels. labels Mar 21, 2023

Merge branch 'master' into fix_FormField_onChange_unexecuted

af56a75

justinmc reviewed Apr 6, 2023

View reviewed changes

yeleibo added 3 commits April 25, 2023 13:54

Code formatting optimization

4c13192

Add tests to CupertinoTextFormFieldRow, DropdownButtonFormField, Text…

bb3fd7a

…FormField

Merge remote-tracking branch 'upstream/master' into fix_FormField_onC…

efeccd1

…hange_unexecuted

justinmc reviewed Apr 27, 2023

View reviewed changes

yeleibo and others added 4 commits April 28, 2023 14:04

Update packages/flutter/test/cupertino/text_form_field_row_test.dart

4194740

Co-authored-by: Justin McCandless <justinjmccandless@gmail.com>

Fix doc_test failure issue

72b3e80

Merge remote-tracking branch 'upstream/master' into fix_FormField_onC…

88b714f

…hange_unexecuted

docoptimization

b164596

yeleibo added 2 commits May 11, 2023 16:37

modify onChanged doc comments

bb8a9dd

Merge remote-tracking branch 'upstream/master' into fix_FormField_onC…

0d0544b

…hange_unexecuted

Merge branch 'master' into fix_FormField_onChange_unexecuted

59eefe8

justinmc reviewed May 18, 2023

View reviewed changes

bleroux mentioned this pull request Sep 8, 2023

Form fields onChange callback should be called on reset #134295

Merged

bleroux closed this Sep 8, 2023

		}
		_cupertinoTextFormFieldRow.onChanged?.call(_effectiveController!.text);

Conversation

yeleibo commented Mar 21, 2023

Uh oh!

flutter-dashboard Bot commented Mar 21, 2023

Uh oh!

google-cla Bot commented Mar 21, 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!

Uh oh!

yeleibo commented Apr 25, 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!

Uh oh!

justinmc commented May 8, 2023

Uh oh!

yeleibo commented May 11, 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!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Hixie commented Jul 18, 2023

Uh oh!

bleroux commented Sep 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants