Add useStrictAutovalidateMode to respect per-field autovalidateMode#180822
Add useStrictAutovalidateMode to respect per-field autovalidateMode#180822Mairramer wants to merge 18 commits intoflutter:masterfrom
Conversation
github-actions
bot
added
the
framework
|
/gemini review |
![gemini-code-assist[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
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
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.
There was a problem hiding this comment.
Would something like this give you enough control and be doable?
- When FormField.autovalidateMode is non-null
- Always use it and ignore Form.autovalidateMode.
- When FormField.autovalidateMode is null
- Defer to Form.autovalidateMode.
- If Form.autovalidateMode is also null, default to .disabled.
Or maybe that's a potential breaking change for existing users?
There was a problem hiding this comment.
In my opinion, since both currently default to AutovalidateMode.disabled, and because null is effectively treated the same as AutovalidateMode.disabled, changing this behavior would indeed be a breaking change.
Introducing useStrictAutovalidateMode addresses this by preserving existing behavior and avoiding regressions. If we were to adopt the alternative approach you suggested, it would result in a breaking change, albeit one that could be justified as correcting an unintuitive or inconsistent behavior rather than introducing a new feature.
There was a problem hiding this comment.
@Mairramer Are you currently facing the problem described in #125766 in your own work? If so could you describe what you want to achieve there and how you expected autovalidateMode to work? Just trying to make sure I'm considering what devs want here.
Current behavior: The value on Form takes precedence, except if it's disabled (or explicitly null).
My interpretation of #125766 is that users find the current behavior unintuitive (I do too). They expect FormField.autovalidateMode to override Form.autovalidateMode. In the long run, that's the direction that I think Flutter should go, but as I've said elsewhere, Form needs a rewrite and maybe this isn't the time to make a big breaking change.
So that leaves us with the option of introducing a sub-optimal new API for the short term. My question is, does that solve a significant user problem today that makes it worth adding the complexity?
There was a problem hiding this comment.
@Mairramer Are you currently facing the problem described in #125766 in your own work? If so could you describe what you want to achieve there and how you expected autovalidateMode to work? Just trying to make sure I'm considering what devs want here.
At the moment, this isn’t a major issue for me. Since I work at a company with a design system, it helps mitigate these problems to some extent. However, introducing this would still benefit many of the wrappers we currently maintain.
Current behavior: The value on Form takes precedence, except if it's disabled (or explicitly null).
My interpretation of #125766 is that users find the current behavior unintuitive (I do too). They expect FormField.autovalidateMode to override Form.autovalidateMode. In the long run, that's the direction that I think Flutter should go, but as I've said elsewhere, Form needs a rewrite and maybe this isn't the time to make a big breaking change.
So that leaves us with the option of introducing a sub-optimal new API for the short term. My question is, does that solve a significant user problem today that makes it worth adding the complexity?
I implemented this following your suggestion #180822 (comment). However, it introduces a breaking change: widget.validator cannot call setState. Please see this commit for context: 1182874, similar to this a3d0181 .
I’d like to confirm the best way to avoid this breaking change during the first build.
My proposal to introduce useStrictAutovalidateMode is intended to preserve the current behavior without introducing breaking changes at this time.
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
I want to be sure of the useStrictAutovalidateMode API before we add it, see my comment.
| /// 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.
Would something like this give you enough control and be doable?
- When FormField.autovalidateMode is non-null
- Always use it and ignore Form.autovalidateMode.
- When FormField.autovalidateMode is null
- Defer to Form.autovalidateMode.
- If Form.autovalidateMode is also null, default to .disabled.
Or maybe that's a potential breaking change for existing users?
github-actions
bot
added
a: text input
…ation logic and improve autovalidate behavior in tests
github-actions
bot
removed
a: text input
|
I’ve submitted a new PR following the expected pattern #182752. |
|
Closing this in favor of #182752 |
2 participants
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.