Skip to content

Add FormField validation states (touched, saved, etc) with ErrorStateMatcher#48155

Closed
yumin-chen wants to merge 1 commit into
flutter:masterfrom
yumin-chen:error-state-matcher
Closed

Add FormField validation states (touched, saved, etc) with ErrorStateMatcher#48155
yumin-chen wants to merge 1 commit into
flutter:masterfrom
yumin-chen:error-state-matcher

Conversation

@yumin-chen

@yumin-chen yumin-chen commented Jan 3, 2020

Copy link
Copy Markdown
Contributor

Description

This PR addresses the issue with FormField where if autovalidate and validator are set, its initial state will be immediately validated, and could be marked as in error state and display errorText to 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 isErrorState property to the FormField and give the ability for a custom defined ErrorStateMatcher (named after Angular equivalent ErrorStateMatcher) that determines whether errors should be shown.

No ErrorStateMatcher Custom ErrorStateMatcher
Errors shown immediately to the user on initial state Errors only shown when a custom defined ErrorStateMatcher returns true
before after
errorStateMatcher: null or errorStateMatcher: (_) => true errorStateMatcher: (field) => field.touched

This 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. touched or saved) in validator.

Comparison with validator

validator errorStateMatcher
Defines whether field is valid. whether errors should be shown to the UI.
Determines hasError isErrorState
Example validator: (password) => password.length < 6 ? "Error" : null, errorStateMatcher: (field) => field.touched,

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,
);

Related Issues

Fix #18885

Tests

I added the following tests:

  • In form_test.dart:
    • touched is updated
    • saved is updated
    • isErrorState returns result from errorStateMatcher
  • In text_form_field_test.dart:
    • Passes decoration to underlying TextField
    • errorStateMatcher is called
  • In dropdown_test.dart:
    • DropdownButtonFormField creates InputDecorator with decoration indicating error state

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

  • No, no existing tests failed, so this is not a breaking change.
  • Yes, this is a breaking change.

@fluttergithubbot fluttergithubbot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Jan 3, 2020
@yumin-chen

Copy link
Copy Markdown
Contributor Author

@justinmc @HansMuller Could you take a look at this PR please? 😃

@justinmc

Copy link
Copy Markdown
Contributor

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

@yumin-chen

yumin-chen commented Jan 14, 2020

Copy link
Copy Markdown
Contributor Author

Is there anything that you can do with this approach that can't be done currently?

The separation of hasError (whether the input is valid) and isErrorState (whether error should be shown) can be a useful distinction for many modern UX design use cases. There are often times when we want to find out if the input is valid (with the validator) without necessarily having to show the error message to the user. With autovalidate and errorStateMatcher, this approach can keep hasError always up to date and always reflects the validity of the input while not showing the error, and this is something we currently cannot do easily.

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 errorStateMatcher approach can provide an easy and convenient way to implement this without having to use any state variable or any repetitive logic. I'd hate having to copy a bunch of boilerplate code and repeat the touched checking logic for every simple form field I implement.

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.

@SpajicM

SpajicM commented Jan 20, 2020

Copy link
Copy Markdown

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?

@tjonganthony

Copy link
Copy Markdown
Contributor

just putting my two cents here:

I feel that ErrorStateMatcher is duplicating what validator is supposed to be doing.

From how I see it, we have 2 different issue, which I have elaborated in #48874 and #48876.
To summarise: we should be able to know whether a Form/Field is valid without changing the UI. Another one is on the behaviour of autovalidation, which validate even before Field is touched(Additionally, this is inconsistent with Form's autovalidation as described in #48876).

@yumin-chen

yumin-chen commented Jan 21, 2020

Copy link
Copy Markdown
Contributor Author

I feel that ErrorStateMatcher is duplicating what validator is supposed to be doing.

I might not have worded their functionality clearly. Here is a comparison. They are very different:

validator errorStateMatcher
Purpose defines the condition whether field is valid. defines the condition whether errors should be shown to the UI.
Example validator: (password) => password.length < 6 ? "Error" : null, errorStateMatcher: (field) => field.touched,

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 errorStateMatcher, we'd have to use something like @justinmc attached, which is not only more verbose, but it also mixes the UI error showing logic (whether errors should be shown to UI) with the validation logic (whether field is valid).

We should be able to know whether a Form/Field is valid without changing the UI.

Totally agree. It is not nice to have to put touched check in the validator, because that would mix the UI logic with the actual validation. They should be separate, -- thus the need for a separate custom function to let user define the condition whether errors should be shown to the UI.


Edit: I've edited the main thread and added the comparison and examples too.

@tjonganthony

Copy link
Copy Markdown
Contributor

I do understand the difference between errorStateMatcher and validator.
However I feel that it is not the right approach to solve the problem we're facing as it causes FormField to have 2 properties that determines whether error is shown, which further complicate the widget.

Additionally, errorStateMatcher will have more complication in some workflow.
Let's say we have 15 fields with validation. We only want to show user the error if:

  • user touched the field
  • or user clicks on the submit button, where we will call Form.validate() and decide whether to proceed or show all the error
    In second case, all of the Field that hasn't been touched will not show an error because it has never been touched. Now we need to set all FormFieldState.touched to be true which is actually not semantically correct.

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 what it's worth, I can't foresee other cases needing errorStateMatcher besides checking whether the field has been touched.

@yumin-chen

Copy link
Copy Markdown
Contributor Author

For the scenario with the submit button, you could set errorStateMatcher: (field) => field.saved || field. touched, and on submit call save, then the error will all show up upon save.

@tjonganthony

Copy link
Copy Markdown
Contributor

I'm not saying it's not possible at all.
You just proved a point where you need to keep modifying errorStateMatcher and further complicate it with more state. Calling save is also a hack that you're using in order to leverage on an existing state.

The proposal I discussed in both issues involves calling form.validate() that does not change the existing behaviour at all, and additional property in FormField, instantlyValidate: false(tentative naming). Which is a more straightforward approach I believe.

@HansMuller

Copy link
Copy Markdown
Contributor

If I'm understanding this correctly, what's needed is a way to defer highlighting invalid autovalidate: true fields until after the user has entered a value. Presumably this is a common case.

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?

@SpajicM

SpajicM commented Jan 22, 2020

Copy link
Copy Markdown

@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".
Maybe having something like an enum would be a good solution:
AUTOVALIDATE_ALWAYS,
AUTOVALIDATE_ON_TOUCHED
AUTOVALIDATE_ON_SAVED

@yumin-chen

Copy link
Copy Markdown
Contributor Author

@HansMuller Yes, that's correct. That's exactly the problem this is trying solve here.

do we really need to control this behavior on a per-field basis?

That's a very good point. I think per-form basis would make more sense. Since autovalidate is supported both on the form and form field, we could support both here too.

"errorStateMatcher" doesn't seem like a particularly evocative name.

True. We can come up with a better name if we decide this approach is OK. The ErrorStateMatcher is borrowed from Angular.

Would it be sufficient to add a flag to Form that defers highlighting invalid fields until the user has provided input?

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:

  • always (current behavior)
  • touched
  • saved
  • (touched || saved) => The use case @tjonganthony pointed out earlier is a good example of when (touched || saved) can be quite useful.

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 errorStateMatcher can offer more flexibility because the user can have their custom-defined variables in there. (e.g. they could implement the case of showing error on blur, when the field loses focus, using the custom function). If we could offer a custom function, that'd be a great addition IMO.

@yumin-chen

yumin-chen commented Jan 22, 2020

Copy link
Copy Markdown
Contributor Author

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 (onBlur || onSaved)? In this case, I think a custom function would work the best.

I can already think of 7 common cases:

  • always (current behavior)
  • touched
  • blurred (lose focus)
  • saved
  • (touched || saved)
  • (blurred || saved)
  • never (always hide error from UI, but the logic remains)

Would a flag be sufficient in this case?

@SpajicM

SpajicM commented Jan 22, 2020

Copy link
Copy Markdown

@chen-yumin

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:
"Avoid implementing features you don’t need."

If we go by that, enum is a better solution.
Not sure if we could use bitwise operator | to combine the flags. That would bring down the count to 5.

@tjonganthony

Copy link
Copy Markdown
Contributor

Can we use separate flag instead of using bitwise or enum?
i.e make the parameter to be:

this.validateOnStart = true, // to not break the existing behaviour
this.validateOnTouch = false,
this.validateOnEdit = true,
this.validateOnBlur = false,

Then we can start deprecating autovalidate since the word auto is ambiguous anyway.

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.

@SpajicM

SpajicM commented Jan 23, 2020

Copy link
Copy Markdown

Why do you prefer this over enum?
I feel these options should be grouped somehow.

And about breaking change, I'm against it unless absolutely necessary.

@yumin-chen

Copy link
Copy Markdown
Contributor Author

@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.

@yumin-chen

Copy link
Copy Markdown
Contributor Author

I'm still not too convinced with using a flag here, because:

  1. For situations when you want to use multiple logical operators (&&, ||, and !) together to define the condition to show error.
    For example: If I want to show errors when (touched && blurred) || saved, there is no easy way you can define this with a flag. This is an example of when the we don't want to show errors just on blur, but also make sure it's been touched. The && operator can avoid the situation when the user was just tabbing through (switching between input fields) without editing anything.
  2. There are quite a few cases we need to cover, as stated above, and they might need to be combined with multiple logical operators (&&, ||, and !) creating even more permutations with more flags.
  3. Using bitwise operators to combine flags is nice, but currently Dart does not support Enum with values (Allow enum classes to have members dart-lang/language#158). Also, I don't often encounter the syntax of enum flags with bitwise operators in Dart, especially within Flutter. It is questionable if we should encourage this syntax.

Your thoughts?

@tjonganthony

tjonganthony commented Jan 24, 2020

Copy link
Copy Markdown
Contributor

@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.
However, I don't really have strong opinion about this as I don't think it will should grow too much.

In regards for @chen-yumin multiple logical operator, I think you're thinking too much and trying to cover every bases.

  • As a user, if I touch a field then blur it without making any edit and don't see any error, I would assume there's actually nothing wrong with the field.
  • When you are trying to save, you can call validate() and it will show all the error immediately, which totally make sense.

@yumin-chen

Copy link
Copy Markdown
Contributor Author

If this was a simple true or false option, or a simple choice with 3 or 4 options, I'd definitely go with a flag, but given that the states we come up with are not just separable individual cases but they might need to be evaluated together with one another as a combined state, I don't see the benefit of having an enum flag over a custom function here. I wouldn't like having flags like ON_TOUCHED_OR_ON_SAVED, ON_BLUR_OR_ON_SAVED. Wouldn't touched || saved be cleaner? Using flags can be nicer when there are fewer options and when options are separate individuals, but in this case I'd find using a function more intuitive, cleaner and more customizable and flexible.

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.

API Using an Enum Flag Using a Function
always showError: ShowError.always showErrorWhen: (_) => true
touched showError: ShowError.onTouched showErrorWhen: (field) => field.touched
blurred showError: ShowError.onBlurred showErrorWhen: (field) => field.blurred
saved showError: ShowError.onSaved showErrorWhen: (field) => field.saved
touched or saved showError: ShowError.onTouchedOrSaved showErrorWhen: (field) => field.touched || field.saved
blurred or saved showError: ShowError.onBlurredOrSaved showErrorWhen: (field) => field.blurred || field.saved
never showError: ShowError.never showErrorWhen: (_) => false

Another thing I'd like to call out here, is the distinction between:

  • Stops the error from showing
  • Stops auto-validating (Stops calling the validate function automatically)

IMO, this feature is to stop the error from showing on the UI, but it does not stop the validate function from being called automatically when autovalidate: true is set. That's because we want to separate the UI error showing logic (whether errors should be shown to UI) from the validation logic (whether field is valid). The auto validation can keep the current FormField.hasError always up to date as an indicator of if the form is valid (using validator). This would also allow validate function being manually called without necessarily triggering the error to be shown, solving the problem that's raised in #48874.

@justinmc

Copy link
Copy Markdown
Contributor

I tried coding up the three main validation strategies that I see, just to understand the problem better: https://dartpad.dartlang.org/9fb110c4dd1a910becb9bacd3422bdd5

  1. Don't validate until the entire form is submitted.
  2. Validate fields on blur (and validate the entire form when submitted).
  3. Validate fields on focus and on change (and validate the entire form when submitted).

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 autovalidate, .validate() and the showing of errors seems like it doesn't fit Flutter's declarative model as well as it could. Any changes need to make the overall API simpler and not even more complex, though.

@SpajicM

SpajicM commented Jan 30, 2020

Copy link
Copy Markdown

@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.

@justinmc

Copy link
Copy Markdown
Contributor

I prefer we listen to Googlers' suggestion :)

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 autovalidate need to be deprecated? It adds a layer of complexity on to the table posted above by @chen-yumin. Also, what happens when you call key.currentState.validate() when in each of the states in that table?

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 showErrorWhen parameter. As a simple example, you can easily create a wrapped TextFormField that encapsulates the cases in my example above. Here's case number 3 (validate on focus and change):

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,
),

@HansMuller

Copy link
Copy Markdown
Contributor

Here's a slightly different proposal that I think addresses the use cases presented here.

Form and FormField get an onInteraction callback parameter with this signature:

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:

  • When the field is first shown
  • When the field gains or loses the focus
  • The first time it's tapped after it gains the focus
  • Each time the value of the field changes
  • Each time the field is saved

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:

  • Additional parameters can be added without risking breaking code that switches on an enum value.
  • We could add parameters that correspond to "level triggers" later, if that's really necessary.

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:
Add a FormFieldState.checkValid() method that returns the value of the FormField's validator callback. Returns null if there's no validator callback.

@tjonganthony

Copy link
Copy Markdown
Contributor

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 FormField should be showing error or not.

As per our chat also, we might be able to consider deprecating autovalidate and onChanged since this callback is a superset of both. Maybe we can leave that for another PR though :)

@SpajicM

SpajicM commented Feb 7, 2020

Copy link
Copy Markdown

I like the idea as well! Looks like a good balance between ease of use and flexibility.

Your thoughts @chen-yumin ?

@yumin-chen

yumin-chen commented Feb 7, 2020

Copy link
Copy Markdown
Contributor Author

I like the onInteraction callback that @HansMuller proposed. But I have a couple of questions:

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.

onInteraction sounds to be a usual event callback similar to onSaved or onChanged, so I'd expect it having similar behavior as well. It's not uncommon to see validate() being called in onSaved or onChanged. Why should onInteraction be different and cannot be used with autovalidate? Should we put this restriction on the framework itself? People might come up with other use cases for onInteraction other than just validating errors. Is there a bit of an overlapping between onInteraction and other event callbacks? Would this confuse users into choosing onInteraction when they could otherwise just go for onChanged, for example?

Is this onInteraction per field, or would it make sense to add this callback to the Form as well? How would some of the attributes make sense if it's per form? For example: Would focused be attributed to any of its child fields having focus, and also pass that field as a parameter so you can validate() just for that field that has focus?

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 MaterialApp so they can set their defaultErrorStateMatcher (or whatever you call it) globally for the whole app. Being able to set this globally IMO is more useful than per field for this feature. How can we solve for this with onInteraction?

Thoughts?

@SpajicM

SpajicM commented Feb 7, 2020

Copy link
Copy Markdown

@chen-yumin The overlap crossed my mind as well.

On one hand, onChanged feels familiar, and intuitive, but if you think about it, changing the textbox implies the interaction with it. So it makes sense that being changed is a part onInteraction callback. Maybe we should deprecate it as @tjonganthony said.

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.
So if onInteration is defined at form level, all it's fields should behave like you've put onInteraction on the field itself. If field has it's own onInteraction, then it should use that.

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.

@yumin-chen

yumin-chen commented Feb 8, 2020

Copy link
Copy Markdown
Contributor Author

I'm not sure deprecating onChanged in favor of onInteraction is the right way forward. Event callbacks should be specific and relevant to the event they want to handle. onInteraction feels like an all-encompassing callback that handles all events without specificity. E.g. I'd want my onChanged and onBlur being separate callbacks instead of dumping them into a single big callback with a bunch of if-statements.

I'm also confused by what the bool flags in onInteration actually mean:

  • Are they supposed to be indicate what the event is (changed means it's an onChanged event, a subsequent save event will have changed to be false), or do they represent the sustained state since beginning (changed means the value has been changed and will not turn back to false until reset)?
  • The name onInteration gives me the impression that I can go if (changed) print("This is a onChanged event"), but it feels conflicting because I'm also under the impression that shown and focused represents a state that can be true regardless of what event it is.
  • If that only indicates the event type, then focused and shown will be false during an onChanged event, that'd be very confusing. And the users might have to again record the actual state using their own variables themselves. This defeats the whole purpose.
  • If they are just sustained states, how does the name onInteraction make sense, since you won't even know what is the interaction that triggered this? It's kind of like an all-encompassing onStateChange event without highlighting what's being changed. Should we also pass all previous states so they'd know what's changed (e.g. if (prev.focused != new.focused) print("onFocusChange"))? Even then, we cannot find out if it's an onChanged event because changed stays true for other events too.

Can those bool flags become getters on the FormFieldState? So you can use just field.hasFocus?

Also on a side note, we'd need separate variables to track focus gains (focused) and losses (blurred). Otherwise, we can't validate on blur. focused == false does not mean it's blurred. For error validation, I'm not interested in if it currently has focus, but whether it has ever gained focus, or has ever lost focus.

So if onInteration is defined at form level, all its fields should behave like you've put onInteraction on the field itself. If field has its own onInteraction, then it should use that.

Would it be helpful if we pass the FormState in onInteration as well? So you can access the form's state variables and call form.validate() or field.validate() depending on your need?

The description of form behaviour should be kept 'close' to that form.

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 errorStateMatcher on all forms and fields across the board. Maybe we should add this to ThemeData? My understanding it is part of the visual theme to determine when to show error. -- ThemeData::errorStateMatcher

@SpajicM

SpajicM commented Feb 8, 2020

Copy link
Copy Markdown

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 errorStateMatcher on all forms and fields across the board. Maybe we should add this to ThemeData? My understanding it is part of the visual theme to determine when to show error. -- ThemeData::errorStateMatcher

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.
If you want to reuse onInteraction, put it in a VoidCallback or Function

@yumin-chen

Copy link
Copy Markdown
Contributor Author

How would you like the idea of an Inherited Widget FormAutoValidator that holds the validateOn callback? This would give you the freedom to define the scope it applies to -- wrap your entire app in it if you want to. And it does not introduce any API change to the current Form, or Field, though it is an alternative to autovalidate and aims to deprecate it.

@justinmc

Copy link
Copy Markdown
Contributor

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.

@SpajicM

SpajicM commented Feb 10, 2020

Copy link
Copy Markdown

How would you like the idea of an Inherited Widget FormAutoValidator that holds the validateOn callback? This would give you the freedom to define the scope it applies to -- wrap your entire app in it if you want to. And it does not introduce any API change to the current Form, or Field, though it is an alternative to autovalidate and aims to deprecate it.

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 FormAutoValidator widget affects your Form to see why it behaves the way it does.

If you wanna reuse the validation logic, I think Justin's or my earlier suggestion should be fine.
If you have that many Forms all across the app - maybe you should reconsider your UX :)

I still prefer @HansMuller solution, hopefully he can answer your earlier questions.

@yumin-chen

Copy link
Copy Markdown
Contributor Author

... should be solved using composition.

With composition / inheritance, you can already achieve any of the validation behavior without the need of an onInteraction or errorStateMatcher callback, just like the FocusTextFormField you provided. This feature does not offer anything that can't already be done currently. The aim here is only to make it more convenient, with less verbose syntax, and get rid of more boilerplate code. To me, it'd be disheartening to know I'll have to resort to composition / inheritance, with or without this feature. It'd be ideal to see this functionality being offered out of the box without entailing a layer of our own custom widget.


I think the more verbose the syntax, the less value it provides. In terms of verbosity:

  • Having to explicitly call validate() in the onInteraction callback makes the syntax more verbose and much less declarative (more imperative), compared to a dedicated validateOn callback.
  • Having to define this attribute for every field / form adds another layer of verbosity, more boilerplate code.

Other aspects about the onInteraction callback that I'm not a fan of:

  • Mixing multiple event listeners in a single callback.
  • Further mixing validation behavior logic into the same callback that contain other logic already.

I'm concerned that the onInteraction would become a rabbit hole jumble of logics, for the lack of a better expression:

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 onInteraction:

  • does not offer as much value in terms of verbosity.
  • encourages mixing up logic and seem more messy.

What's the specific pattern that you're trying to build that requires you to know previous interactions like this?

All of the patterns are doable with onInteraction without maintaining additional state, as far as I'm concerned. 👍 My only concern is that because onInteraction mixes multiple events. It might encourage syntax like what's in your previous example https://dartpad.dartlang.org/27082245d18da32b35f71a026ed1dc7a where they might just record the state themselves with onInteraction and then use the state in validator. How can you encourage the users to just call validate in onInteraction?

@SpajicM

SpajicM commented Feb 10, 2020

Copy link
Copy Markdown

It appears that we won't find a silver bullet here :(

This feature does not offer anything that can't already be done currently. The aim here is only to make it more convenient, with less verbose syntax, and get rid of more boilerplate code.

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.

Having to define this attribute for every field / form adds another layer of verbosity, more boilerplate code.

In my opinion, it's not really boilerplate if it defines your functionality.
It's not the code everyone has to write to get the one same result (like for example setOnClickListeners in Android/Java)

I'm concerned that the onInteraction would become a rabbit hole jumble of logics, for the lack of a better expression

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.

@lzhuor

lzhuor commented Feb 22, 2020

Copy link
Copy Markdown

This is my simple solution to achieve it using FocusNode in an extended Widget called FocusTextFormField.

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,
    );
  }
}

@SpajicM

SpajicM commented Mar 5, 2020

Copy link
Copy Markdown

How to proceed on this?

@Piinks Piinks added the a: text input Entering text in a text field or keyboard related problems label Mar 18, 2020
@justinmc

Copy link
Copy Markdown
Contributor

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.

@yalva

yalva commented May 20, 2020

Copy link
Copy Markdown

This is my simple solution to achieve it using FocusNode in an extended Widget called FocusTextFormField.

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,
    );
  }
}

Its not validating until Textfield loses the focus first time.

@justinmc

Copy link
Copy Markdown
Contributor

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 EasyTextFormFieldFocus or EasyTextFormFieldUnfocus like a normal TextFormField, but it will validate on focus/unfocus.

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.

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Jul 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add TextFormField validation states (dirty, touched, etc)

10 participants