Add FormField validation states (touched, saved, etc) with ErrorStateMatcher#48155
Add FormField validation states (touched, saved, etc) with ErrorStateMatcher#48155yumin-chen wants to merge 1 commit into
Conversation
|
@justinmc @HansMuller Could you take a look at this PR please? 😃 |
|
Thanks for the PR! This is definitely a really high quality PR, but I want to make sure it's the right approach for solving this sort of problem. Is there anything that you can do with this approach that can't be done currently? You can do some similar stuff with onSaved and onTap. See this Dartpad for example: https://dartpad.dartlang.org/27082245d18da32b35f71a026ed1dc7a |
The separation of Furthermore, this type of error-after-touch form field is quite common in today's UX design. I'd even argue that it makes sense to become the default behavior of a form field out of the box. This As pointed out in #18885 by others, the lack of this feature marks a notable feature parity between Flutter and Angular, and adding this can help bridge the feature gap. |
|
This is a very welcome addition! I do currently use something like @justinmc attached, but this would be nice to have out of the box, or even as a default behaviour as @chen-yumin suggests. I'm not sure if this PR makes #48948 unnecessary? |
|
just putting my two cents here: I feel that From how I see it, we have 2 different issue, which I have elaborated in #48874 and #48876. |
I might not have worded their functionality clearly. Here is a comparison. They are very different:
An example of a password field could use both: TextFormField(
autovalidate: true,
obscureText: true,
validator: (password) => password.length < 6 ? "Error msg" : null,
errorStateMatcher: (field) => field.touched,
);Without the
Totally agree. It is not nice to have to put Edit: I've edited the main thread and added the comparison and examples too. |
|
I do understand the difference between Additionally,
I feel that we should focus on what we want to achieve: to be able to not show error before the field is touched(which is also discussed separately in #48993). |
|
For the scenario with the submit button, you could set |
|
I'm not saying it's not possible at all. The proposal I discussed in both issues involves calling |
|
If I'm understanding this correctly, what's needed is a way to defer highlighting invalid The approach Justin suggested works, but it requires one to mixup validation logic and a flag that indicates that the user has entered a value. The approach suggested here seems more general than is needed (do we really need to control this behavior on a per-field basis?) and "errorStateMatcher" doesn't seem like a particularly evocative name. Would it be sufficient to add a flag to Form that defers highlighting invalid fields until the user has provided input? |
|
@HansMuller just to clarify: We definitely don't want to trigger the errors before user has even started typing. Next question is, do we want to start validation "on touched" or "on saved" My preference is "on saved", since on touched would make the error appear before user had a chance to finish the input. I think there should be a way to choose the "mode". |
|
@HansMuller Yes, that's correct. That's exactly the problem this is trying solve here.
That's a very good point. I think per-form basis would make more sense. Since
True. We can come up with a better name if we decide this approach is OK. The ErrorStateMatcher is borrowed from Angular.
I think the solution @SpajicM proposed is good. Yes, we definitely need to let the user customize when they want the auto validation to perform, but I'd like to cover at least the following 4 cases:
If we could cover the above 4 cases then the flag should be efficient for most use cases IMO. However, I do think the concept of having an |
|
The use case of showing error on blur (on losing focus) is actually quite common too. I often times notice error message only show up after I switch to the next field, or click outside the input field. If we do go ahead with a flag, shouldn't we try to cover this too? Also the case of I can already think of 7 common cases:
Would a flag be sufficient in this case? |
|
Blur case is also quite nice to cover, I agree. The way I see it, flag enum is more intuitive and clean to use, while error state matcher offers more flexibility. The Flutter wiki states: If we go by that, enum is a better solution. |
|
Can we use separate flag instead of using bitwise or enum? Then we can start deprecating I actually feel that we should remove the behaviour of a field immediately during build, but I don't know how we feel on making breaking change. |
|
Why do you prefer this over enum? And about breaking change, I'm against it unless absolutely necessary. |
|
@tjonganthony Are you suggesting that we have multiple parameters for the same feature? Agreed with @SpajicM here. I'd prefer a single flag parameter over having multiple ones, or at least we should group them together as a single parameter. I don't think we can use bitwise operations to combine enum flags in Dart. Currently, Dart does not support enum with values (dart-lang/language#158). So unfortunately not with an enum -- a workaround would be using a class with static members, but that's a different discussion now. I do like the flag with bitwise operation syntax though. It reminds me of the good old Win32 programming days when most windows params are set with flags combined with bitwise operations. |
|
I'm still not too convinced with using a flag here, because:
Your thoughts? |
|
@SpajicM I was thinking it may have easier usability and more "backward compatible". As @chen-yumin mentioned, we may need to add more granular behaviour in the future that may interact with other behaviour. When that happens the enum will grow exponentially and the naming will be disastrous as we must not change the existing enum name. In regards for @chen-yumin multiple logical operator, I think you're thinking too much and trying to cover every bases.
|
|
If this was a simple Anyway, I'll layout what the 2 different APIs might look like here and let you guys decide which one you'd prefer. I'm happy to change my PR and go forward with either way.
Another thing I'd like to call out here, is the distinction between:
IMO, this feature is to stop the error from showing on the UI, but it does not stop the |
|
I tried coding up the three main validation strategies that I see, just to understand the problem better: https://dartpad.dartlang.org/9fb110c4dd1a910becb9bacd3422bdd5
The only one that's currently easy to do with just parameters is number 1. Number 2 requires some extra code in initState. Number 3 requires extra code and extra state. Well I definitely agree that Flutter is not making the most common use cases the easiest to build. The interaction between |
|
@justinmc, what's your suggested way forward then - Separate flags, enums, or ErrorStateMatcher? I prefer we listen to Googlers' suggestion :) Would like @HansMuller's input as well. |
So much pressure! :) Alright basically, I am still unconvinced that any of these suggestions would be an improvement overall. I agree that they would make common cases easier to achieve with fewer lines of code, but they would also increase the complexity of the widget. Would I definitely think that these are all valuable ideas for creating these kinds of forms, though. At the very least, maybe this deserves to be a pub package that wraps TextFormField and adds the class FocusTextFormField extends StatefulWidget {
FocusTextFormField({
Key key,
this.decoration,
this.keyboardType,
this.onFieldSubmitted,
this.textInputAction,
this.validator,
}) : super(key: key);
final InputDecoration decoration;
final TextInputType keyboardType;
final ValueChanged<String> onFieldSubmitted;
final TextInputAction textInputAction;
final FormFieldValidator<String> validator;
@override
State createState() => FocusTextFormFieldState();
}
class FocusTextFormFieldState extends State<FocusTextFormField> {
final GlobalKey<FormFieldState> _key = GlobalKey<FormFieldState>();
final FocusNode _focus = FocusNode();
bool _autovalidate = false;
@override
void initState() {
super.initState();
_focus.addListener(() {
if (_focus.hasFocus && !_autovalidate) {
setState(() {
_autovalidate = true;
});
}
});
}
@override
Widget build(BuildContext context) {
return TextFormField(
key: _key,
autovalidate: _autovalidate,
decoration: widget.decoration,
focusNode: _focus,
keyboardType: widget.keyboardType,
onFieldSubmitted: (String value) {
_focus.unfocus();
FocusScope.of(context).requestFocus(_focus);
widget.onFieldSubmitted(value);
},
textInputAction: widget.textInputAction,
validator: widget.validator,
);
}
}And you use it just like a normal TextFormField: FocusTextFormField(
decoration: const InputDecoration(
hintText: 'Email',
),
keyboardType: TextInputType.emailAddress,
textInputAction: TextInputAction.next,
validator: (value) => !value.contains('@') ? 'Not a valid email.' : null,
), |
|
Here's a slightly different proposal that I think addresses the use cases presented here. Form and FormField get an void onInteraction({ FormFieldState field, bool shown, bool touched, bool focused, bool changed, bool saved })The callback can call field.validate() if it wants to check the field's validity and show an error. We'll run the callback:
Currently, all of the parameters correspond to "edge triggers" (only one parameter will be true at a time). I think this is preferable to an enum because it's a little more flexible:
This callback is intended as an alternative to autovalidate. The Form's onInteraction callback may not be used if autovalidate is true. A FormField's onInteraction callback may not be used if autovalidate is true. Per #48948: |
|
I really like the idea that @HansMuller proposed. I think it will be able to cover most, if not all, cases that is covered here while not introducing new concept just for determining whether As per our chat also, we might be able to consider deprecating |
|
I like the idea as well! Looks like a good balance between ease of use and flexibility. Your thoughts @chen-yumin ? |
|
I like the
Is this Both "validator" and "autovalidate" are available per form too. I think it'd be very useful to apply at a higher level -- per form, and even per app default. I was even thinking having this callback under Thoughts? |
|
@chen-yumin The overlap crossed my mind as well. On one hand, About being per-field or per-form, my usecase is 100% per-form, but I think per-field should be added too, in case you have an exception, and need to override form-level behaviour for a single field. And about this rule being app-level - while it probably could be useful sometimes, I don't think we should go that far, the description of form behaviour should be kept 'close' to that form. |
|
I'm not sure deprecating I'm also confused by what the bool flags in
Can those bool flags become getters on the Also on a side note, we'd need separate variables to track focus gains (
Would it be helpful if we pass the
I'd argue that this feature is close to a theme style. Same reason you'd want to avoid styling every single button individually. It's quite often that we need to apply the same |
Theme is about defining visual styles - for example, the color and size of the error text under the field. The logic of when that error appears should be somewhere else. |
|
How would you like the idea of an Inherited Widget |
|
Just a few points while we figure this out. I think that the problems of applying validation behavior to all/multiple forms in an app should be solved using composition instead of theming or InheritedWidget. For example, see my comment above where I wrote the code for a widget called FocusTextFormField. By creating a wrapper on TextFormField, you can easily encapsulate and share your validation logic in a way that's more in-line with Flutter's way of doing things. To clarify about the booleans as I understand it, they describe the current event and do not hold state about previous interactions. For example, they don't tell you whether or not a field has ever been blurred in the past, and you would need to manage your own state if you need to know this. @chen-yumin What's the specific pattern that you're trying to build that requires you to know previous interactions like this? In another one of my comments above, I described the three main user experiences that I could think of off the top of my head in terms of when to show validation errors. All three of them should be doable with Hans's proposal without maintaining additional state at least. |
Not a fan of this idea - I'd like to be able to understand Form's behaviour by looking at the Form. Imagine you continue working on someone else's code, and then you gotta hunt around the tree to see which If you wanna reuse the validation logic, I think Justin's or my earlier suggestion should be fine. I still prefer @HansMuller solution, hopefully he can answer your earlier questions. |
With composition / inheritance, you can already achieve any of the validation behavior without the need of an I think the more verbose the syntax, the less value it provides. In terms of verbosity:
Other aspects about the
I'm concerned that the onInteraction: (changed, blurred, saved) => {
If (changed) {
field.validate();
}
if (blurred) {
field.validate();
// On blur, fetch user profile pic on the login screen
_fetchProfilePicture(field.value);
}
if (saved) {
_email = field.value;
_saveToLocalStorage(_email);
}
}It seems that
All of the patterns are doable with |
|
It appears that we won't find a silver bullet here :(
It aims to make the usual cases more easy - if you need something custom, like knowing the previous state, you'll have to write it yourself.
In my opinion, it's not really boilerplate if it defines your functionality.
I agree it's not an ideal separation here, but I also think you pushed your example a bit too far for common use case. What your example shows is flexibility - but most commonly it's gonna be if (saved || blurred || changed) {
form.validate();
}which doesn't look so bad to me. |
|
This is my simple solution to achieve it using Basically only validating the form when it's dirty. Modified from @justinmc 's solution. import 'package:flutter/material.dart';
class FocusTextFormField extends StatefulWidget {
FocusTextFormField({
Key key,
this.decoration,
this.keyboardType,
this.onFieldSubmitted,
this.textInputAction,
this.validator,
this.autovalidate
}) : super(key: key);
final InputDecoration decoration;
final TextInputType keyboardType;
final ValueChanged<String> onFieldSubmitted;
final TextInputAction textInputAction;
final FormFieldValidator<String> validator;
final bool autovalidate;
@override
State createState() => FocusTextFormFieldState();
}
class FocusTextFormFieldState extends State<FocusTextFormField> {
final GlobalKey<FormFieldState> _key = GlobalKey<FormFieldState>();
final FocusNode _focus = FocusNode();
bool _focused = false;
bool _dirty = false;
void _touchEventListener() {
if (_focus.hasFocus && !_focused) {
setState(() {
_focused = true;
});
}
}
void _dirtyEventListener() {
if (!_focus.hasFocus && _focused) {
setState(() {
_dirty = true;
});
}
}
@override
void initState() {
super.initState();
_focus.addListener(_touchEventListener);
_focus.addListener(_dirtyEventListener);
}
@override
Widget build(BuildContext context) {
print("Focused: $_focused");
print("Dirty: $_dirty");
return TextFormField(
key: _key,
autovalidate: widget.autovalidate && _dirty,
decoration: widget.decoration,
focusNode: _focus,
keyboardType: widget.keyboardType,
onFieldSubmitted: (String value) {
_focus.unfocus();
FocusScope.of(context).requestFocus(_focus);
widget.onFieldSubmitted(value);
},
textInputAction: widget.textInputAction,
validator: widget.validator,
);
}
} |
|
How to proceed on this? |
|
I'm going to close this PR as it seems like we're not coming to a consensus, but I agree there is a problem here that needs solving. Let's continue the discussion on #18885 if anyone still has ideas. I think the current best solution to this problem is to use composition to do the dirty work once and reuse it around the app. I'm going to plan to revisit this and try to improve our existing messaging around how to do form validation. I'll also be keeping an eye out for ways to improve the API. Thanks to everyone that has chimed in on this and especially @chen-yumin for opening the PR. This is the kind of great discussion that helps move open source projects like Flutter forward. |
Its not validating until Textfield loses the focus first time. |
|
I've published my example above as a pub package to try to make this stuff easier to do: easy_form_fields. You can just use Also, there are two PRs out right now seeking to make autovalidate not validate until an interaction has occurred: #56365 and #56132. That should make the focus case more straightforward. See the design doc if anyone has ideas. |
Description
This PR addresses the issue with
FormFieldwhere ifautovalidateandvalidatorare set, its initial state will be immediately validated, and could be marked as in error state and displayerrorTextto the user when the user has not touched the form field at all. It would be nice to provide a way to define when the error message should be shown, so the error can be hidden at the initial state.The proposed solution here is to add an
isErrorStateproperty to theFormFieldand give the ability for a custom definedErrorStateMatcher(named after Angular equivalent ErrorStateMatcher) that determines whether errors should be shown.ErrorStateMatcherreturnstrueerrorStateMatcher: nullorerrorStateMatcher: (_) => trueerrorStateMatcher: (field) => field.touchedThis approach separates the UI error showing logic (whether errors should be shown to UI) from the validation logic (whether field is valid), so we don't have to mix the logic and perform UI-related checks (e.g.
touchedorsaved) invalidator.Comparison with validator
hasErrorisErrorStatevalidator: (password) => password.length < 6 ? "Error" : null,errorStateMatcher: (field) => field.touched,An example of a password field could use both:
Related Issues
Fix #18885
Tests
I added the following tests:
form_test.dart:text_form_field_test.dart:dropdown_test.dart:Checklist
///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change