Add useStrictAutovalidateMode to respect per-field autovalidateMode #180822
Conversation
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the useStrictAutovalidateMode property to the Form widget, providing more granular control over field validation. The implementation is well-structured, particularly the refactoring of autovalidation logic into the _shouldAutoValidate getter, which improves readability. However, I've identified a significant issue with the caching of FormState within FormFieldState that could lead to incorrect behavior and memory leaks when the widget tree changes dynamically. I've provided a detailed comment and a code suggestion to address this. Additionally, there's a minor cleanup opportunity in one of the new tests. Overall, this is a valuable feature, and with the suggested fix, it will be a solid contribution.
# Conflicts: # packages/flutter/test/widgets/form_test.dart
justinmc
left a comment
There was a problem hiding this comment.
I think validation should have been handled programmatically instead of via an enum, but that's a major change that will have to come in another iteration of Form. For now I agree that this is a problem we should solve along these lines. See my comment asking about using the null value of autovalidateMode.
| /// Whether descendant [FormField]s should ignore the [Form.autovalidateMode] | ||
| /// and strictly use their own [FormField.autovalidateMode]. | ||
| /// | ||
| /// When true, each [FormField] strictly follows its own | ||
| /// [FormField.autovalidateMode], ignoring the [Form]'s setting. | ||
| /// | ||
| /// When false, the [Form]'s [AutovalidateMode] takes precedence and determines | ||
| /// the automatic validation behavior of all descendant fields. | ||
| /// | ||
| /// Calling [FormState.validate] always triggers validation for all fields, | ||
| /// regardless of this setting. | ||
| /// | ||
| /// Defaults to false. | ||
| final bool useStrictAutovalidateMode; |
There was a problem hiding this comment.
Could this be done by seeing if autovalidateMode is null, or why wouldn't that work?
There was a problem hiding this comment.
Regarding the null value of autovalidateMode, to be honest, I didn’t give it much thought. I used useStrictAutovalidateMode to make the validation behavior explicit: it ensures that each FormField either follows its own autovalidateMode or inherits the Form’s, depending on what I want.
There was a problem hiding this comment.
One thing I’ve been thinking about is moving this to the FormField level instead of the Form. That might make it easier to manage which fields should not be validated. I’m not sure why someone would want that, but it could still be a good option, especially since we currently don’t have programmatic validation.
| _formState?._fieldDidChange(); | ||
| } | ||
|
|
||
| bool get _shouldValidate { |
There was a problem hiding this comment.
This is a nice way to encapsulate this important logic 👍
| /// [FormState.validateGranularly]. | ||
| manual; | ||
|
|
||
| bool get isAuto => this == _FormValidation.auto; |
There was a problem hiding this comment.
Is this necessary? Is it just to save a few lines?
This PR introduces the
useStrictAutovalidateModeproperty to the Form widget. When enabled, each descendant FormField respects its ownautovalidateMode, allowing fields to opt out of automatic validation even if the parent Form has a globalAutovalidateModeset.Fixes #125766
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.