这是indexloc提供的服务,不要输入任何密码
Skip to content

feat: add errorBuilder property to customize errorText appearance #156275

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 25 commits into from

Conversation

MohiuddinM
Copy link
Contributor

@MohiuddinM MohiuddinM commented Oct 5, 2024

Fixes #133629
Fixes #135292

Added errorBuilder property to InputDecoration, allowing users to customize the appearance of validation errors shown under the TextFormField.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard

This comment was marked as resolved.

@github-actions github-actions bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Oct 5, 2024
@MohiuddinM MohiuddinM changed the title feat: add errorBuilder property feat: add errorBuilder property to customized errorText appearance Oct 5, 2024
@MohiuddinM MohiuddinM changed the title feat: add errorBuilder property to customized errorText appearance feat: add errorBuilder property to customize errorText appearance Oct 5, 2024
@MohiuddinM
Copy link
Contributor Author

@goderbauer Can I also deprecate InputDecoration.error in this PR, because it will no longer be needed as errorBuilder can be used to return a hard-coded widget too?

@Piinks Piinks requested a review from justinmc October 16, 2024 18:23
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bleroux I wonder if you have any thoughts after working on adding .error (#129275)? Should we consider deprecating .error and using only .errorBuilder?

The API has a lot of overlap here, between validator, error, errorText, and errorBuilder. Ideally we would trim it down, but I'm hoping to get a round to a bigger Form refactor in the future. So in the meantime I think it makes sense to add errorBuilder.

/// Use [errorBuilder] instead of [error] if you need to show the
/// validator error but at the same time also customize the error widget.
///
/// Only one of [error] or [errorBuilder] can be specified.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somewhere I think we should mention that errorText comes from TextFormField.validator. Is here the best place?

See also:

  • [TextFormField.validator], which passes its validation error as [InputDecoration.errorText] through to errorBuilder.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good now 👍

@bleroux
Copy link
Contributor

bleroux commented Oct 17, 2024

@bleroux I wonder if you have any thoughts after working on adding .error (#129275)? Should we consider deprecating .error and using only .errorBuilder?

@justinmc If we stay strictly look at the InputDecoration level what is the builder added value?
I mean compared to using either InputDecoration.error (which allow to provide a custom Widget with or without an error message in it) or InputDecoration.errorText (which creates a Text widget compliant to M3 spec).

If the use case is to show a custom message, one will have to provide both InputDecoration.errorText and the builder. But because the text is already known it could be use directly in the builder? In that case the builder body is no more than InputDecoration.error.
If the use case it to show a custom widget without text (error image, etc), one will have to write a builder and ignore the String parameter of the builder.

Looking at the mentioned issues it seems to be a problem with TextFormField implementation which does not provide enough Flexibility.
TextFormField overiddes the InputDecoration.errorText, see

decoration: effectiveDecoration.copyWith(errorText: field.errorText),

I think it would be better to add TextFormField.errorBuilder and, when not null, use it to override InputDecoration.error instead of InputDecoration.errorText.
If there are other technical constraints I missed, we can revisit this proposal. For the moment, I think it would be better to first consider adding it at the form level.

The API has a lot of overlap here, between validator, error, errorText, and errorBuilder. Ideally we would trim it down, but I'm hoping to get a round to a bigger Form refactor in the future. So in the meantime I think it makes sense to add errorBuilder.

We also have InputDecoration.labelText / InputDecoration.label, InputDecoration.helperText / InputDecoration.helper.
The fact that we would need a builder only for error because of validation feels that this is not the right level to add such a property (and why I would consider first adding it at the form level).

I'm 💯% for "a bigger Form refactor in the future"!

@MohiuddinM
Copy link
Contributor Author

The use case is that I wanna show error text from validator, with some added decorations.

Right now I can not do that because:

  • .error does not take validator error
  • using .error and validator together throws an error

After merging this PR:

  • Validator error, and errorText can be shown with added decorations
  • App no longer goes blank when using error and validator together

@bleroux
Copy link
Contributor

bleroux commented Oct 17, 2024

The use case is that I wanna show error text from validator, with some added decorations.

Right now I can not do that because:

  • .error does not take validator error
  • using .error and validator together throws an error

After merging this PR:

  • Validator error, and errorText can be shown with added decorations
  • App no longer goes blank when using error and validator together

@MohiuddinM
Your use case makes perfect sense and your PR quality is great. 🙏
My point is about where to put this builder.
Here is an alternative PR where the errorBuilder is defined at the TextFormField level : #157087

@xvrh
Copy link
Contributor

xvrh commented Oct 17, 2024

For me the correct location is definitely InputDecoration and it should even be in InputDecorationTheme too.

@MohiuddinM
Copy link
Contributor Author

I added it in InputDecoration because every related thing is defined there: error, errorText, errorBorder etc

@bleroux
Copy link
Contributor

bleroux commented Oct 17, 2024

it should even be in InputDecorationTheme too.

Having the possibility to customize the errorBuilder at the InputDecorationTheme level is definitely a good use case in favor of InputDecoration.errorBuilder.

@justinmc
Copy link
Contributor

justinmc commented Oct 17, 2024

@bleroux Ah I agree with you about this being a TextFormField problem, great point. I want to say that the InputDecorationTheme is a separate problem and we should move forward with something like @bleroux's #157087.

Separately I think we could still add InputDecorationTheme.errorBuilder. Have we put a WidgetBuilder into a Theme before?

Edit: Yes, for example ButtonStyle.backgroundBuilder. Thanks @QuncCccccc for helping me find that.

@MohiuddinM
Copy link
Contributor Author

Having InputDecorationTheme.errorBuilder will be a great addition, which can then be overridden by providing InputDecoration.errorBuilder

@justinmc
Copy link
Contributor

@MohiuddinM Would you be on board with modifying this PR so that errorBuilder is on TextFormField instead of InputDecoration? Then we can think about theming after that's merged.

@MohiuddinM
Copy link
Contributor Author

MohiuddinM commented Oct 18, 2024

@MohiuddinM Would you be on board with modifying this PR so that errorBuilder is on TextFormField instead of InputDecoration? Then we can think about theming after that's merged.

I do not think that is a good idea, right now I am dealing with 40 form fields and using the same InputDecoration (because the UI and theeming for all are the same, only labels are different). This is the same case most apps deal with, where text inputs need to look the same and hence share InputDecoration.

With this PR, I just have to add one more line (along with other UI adaptations, e.g., border styles, label styles, etc.), and I will get consistent UI across the board.

On the other hand with the TextFormField.errorBuilder people have to add the same code everywhere, hence making it harder to have a consistent UI.

@xvrh
Copy link
Contributor

xvrh commented Oct 19, 2024

The purpose of the Widget Function(String errorText) is really to add a custom design to an error message produced by the validator.

Like adding a custom background/border or an icon before it.

InputDecoration & InputDecorationTheme are good and consistent places for that.

If you force it in TextFormField, it's inconsistent with how you design the rest of the field.
Furthermore we'll have to implement it for DropdownButtonFormField too which is unfortunate.

@Piinks Piinks requested a review from justinmc October 23, 2024 18:15
@bleroux
Copy link
Contributor

bleroux commented Oct 28, 2024

@justinmc I think we can proceed with this PR.

InputDecorationTheme is a separate problem and we should move forward with something like @bleroux's #157087.

Separately I think we could still add InputDecorationTheme.errorBuilder.

It is true that these are separate problems, but for consistency and discoverability it seems better to have InputDecorationTheme.errorBuilder and InputDecoration.errorBuilder.

Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the arguments for keeping this on InputDecorator, I think that all makes sense. Just some small stuff to clean up here.

@@ -440,7 +451,9 @@ class _HelperErrorState extends State<_HelperError> with SingleTickerProviderSta
begin: const Offset(0.0, -0.25),
end: Offset.zero,
).evaluate(_controller.view),
child: widget.error ?? Text(
child: widget.error
?? widget.errorBuilder?.call(widget.errorText!)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implies that if errorBuilder is non-null, then errorText must also be non-null. Can you add an assertion for that? And we should probably mention it in the docs for errorBuilder too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is already an assert at the top of this function to confirm that either error or errorText is provided.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean is slightly different I think:

assert(errorBuilder == null || errorText != null, 'errorBuilder must be used with errorText')

And I think that should go in InputDecoration's initializers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding it at constructors won't work. errorBuilder will always be provided, and whether or not errorText exists depends on the validator (which is kind of the point of this PR). What we need to guard against is that errorBuilder should not be called without an errorText.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah you're right, errorText can be null, and we can't distinguish between null and not passed at all here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good discussion here 👍

I will say: even if they can't be moved to a public class, moving some of these asserts to the _HelperError constructor would make debugging a bit easier, since there would be a shorter stacktrace to follow.

@MohiuddinM
Copy link
Contributor Author

LGTM with nits: I think there is 1 more test to add, just for the assertion. See my comment below. But this is good to land either way. Thank you for working with us while we figured this out!

FYI there seems to be a docs failure that you'll need to fix.

The test you suggested is already added at line 5333

@MohiuddinM
Copy link
Contributor Author

@justinmc should the error widget be deprecated in this PR? It will have no use after errorBuilder is available since errorBuilder can also be used to show a hardcoded widget (ignoring the provided errorText).

@justinmc?

@justinmc
Copy link
Contributor

@justinmc should the error widget be deprecated in this PR? It will have no use after errorBuilder is available since errorBuilder can also be used to show a hardcoded widget (ignoring the provided errorText).

@justinmc?

Oh sorry for missing this. I feel like we should just keep it since they serve slightly different uses and the assertions should prevent people from using both. Any thoughts @bleroux?

@MohiuddinM
Copy link
Contributor Author

Oh sorry for missing this. I feel like we should just keep it since they serve slightly different uses and the assertions should prevent people from using both. Any thoughts @bleroux?

In that case, no further changes are needed in the PR. Please feel free to merge whenever you'd like.

@Piinks Piinks requested a review from nate-thegrate November 19, 2024 19:29
Copy link
Contributor

@nate-thegrate nate-thegrate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to add TextFormField.errorBuilder and, when not null, use it to override InputDecoration.error instead of InputDecoration.errorText.

I think overall, I agree with what @bleroux said earlier, my preference would be for this to be exclusive to TextFormField rather than having it on InputDecoration as well.

If we do keep it on InputDecoration, would it make sense to also add it to InputDecorationTheme?

@@ -440,7 +451,9 @@ class _HelperErrorState extends State<_HelperError> with SingleTickerProviderSta
begin: const Offset(0.0, -0.25),
end: Offset.zero,
).evaluate(_controller.view),
child: widget.error ?? Text(
child: widget.error
?? widget.errorBuilder?.call(widget.errorText!)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good discussion here 👍

I will say: even if they can't be moved to a public class, moving some of these asserts to the _HelperError constructor would make debugging a bit easier, since there would be a shorter stacktrace to follow.

@nate-thegrate
Copy link
Contributor

Based on this comment and this one, there seems to be a consensus for adding this to both InputDecoration and InputDecorationTheme.

There's a big caveat to this change: passing an inline callback into a ThemeData constructor breaks the equality check, which, given how many widgets use Theme.of(context), can lead to many unnecessary rebuilds (e.g. what was reported in #89127).
The workaround is pretty straightforward—define a function (top-level function/static method/class method) and pass in the function name instead of an inline callback.
Other options include waiting for dart-lang/language#1048, or useCallback from flutter_hooks.


Perhaps it doesn't need to be done in this PR, but at some point we should make sure that this info is accessible in our documentation, since I imagine it could trip up a lot of folks.

MohiuddinM and others added 2 commits November 20, 2024 18:30
Co-authored-by: Nate Wilson <nathan.wilson1232@gmail.com>
Co-authored-by: Nate Wilson <nathan.wilson1232@gmail.com>
@nate-thegrate nate-thegrate self-requested a review November 20, 2024 21:04
Copy link
Contributor

@nate-thegrate nate-thegrate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, pending Justin's comment being addressed.

I definitely feel that we should make a concerted effort to improve our documentation with regards to inline callbacks & equality checks, but that can stay separate from this PR.

Thanks for the contribution!

Comment on lines +412 to +415
assert(
_hasError,
'errorText or error must be provided'
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update!

Attaching asserts to the constructor is nice, but so is utilizing existing getters 👍

@@ -5346,6 +5346,38 @@ void main() {
throwsAssertionError,
);
});

testWidgets('InputDecorator throws when error and errorBuilder are provided', (WidgetTester tester) async {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to this, it'd be nice to have full test coverage here.

Copy link
Contributor

@nate-thegrate nate-thegrate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @MohiuddinM, apologies for approving and then pulling a switcheroo, but I feel concerned about whether or not landing this change is a good idea.

My main question is: what use case benefits from having errorBuilder as an InputDecoration field, and how does it compare to our current options?


I am dealing with 40 form fields and using the same InputDecoration (because the UI and theeming for all are the same, only labels are different). This is the same case most apps deal with, where text inputs need to look the same and hence share InputDecoration.

It'd be nice to allow a bunch of widgets to inherit this info from the ambient Theme, though they can also inherit it via class inheritance:

class MyTextField extends TextFormField {
  MyTextField({
    // ...
  }) : super(errorBuilder: _errorBuilder);

  static Widget _errorBuilder(String errorText) {
    return Text(errorText);
  }
}

It could also be structured as a widget that builds a form field, similar to #59059 (comment).

The fact that validators are exclusive to FormField widgets, along with how passing callbacks to a ThemeData can easily lead to performance setbacks for less experienced developers, leads me to believe that this perhaps isn't something we'd want as part of the InputDecoration API.

But I'd be very interested to hear your thoughts!

@MohiuddinM
Copy link
Contributor Author

MohiuddinM commented Nov 24, 2024

But I'd be very interested to hear your thoughts!

As we talked about before, there are no options right now because using error and validator together throws an error, and I think it's broken.

Also, from what I understand the performance concerns you have are about adding a callback to InputDecorationTheme, but there are already callbacks in themes (see #156275 (comment)).

Even then if you decide to keep your concerns then for the InputDecorationTheme we can just take a specialized widget there instead of a callback e.g.

abstract class InputErrorBuilder {
  const ErrorBuilder();

  ... other error-related properties

  Widget buildError(BuildContext context, String errorText);
}

and then a theme takes an instance of this class instead of a callback.

With the upcoming macros, this solution will play even nicer, because then we can turn a callback into the above with just an annotation:

@ErrorBuilder
Widget buildError(BuildContext context, String errorText);

@nate-thegrate
Copy link
Contributor

Thanks @MohiuddinM!

The specialized widget sounds very cool—I'm very much looking forward to macros! Though to be consistent with other Flutter APIs I think a callback would be ideal.

Apologies if there was a miscommunication: I'm totally fine with the errorBuilder field if it's added to TextFormField rather than InputDecoration. (I understand it'd be nice to have this for the dropdown form field as well—this could be something we add to the generic FormField widget… maybe at some point down the line, a default error builder inherited from an ancestor Form? Lots of possibilities!)

Overall, it seems to me like since validator is an aspect of a form field, so should be this builder callback. Let me know what you think.

@MohiuddinM
Copy link
Contributor Author

MohiuddinM commented Nov 24, 2024

The specialized widget sounds very cool—I'm very much looking forward to macros! Though to be consistent with other Flutter APIs I think a callback would be ideal.

Consistent with which Flutter API? There are plenty of places in themes where it takes instances of specific classes, instead of callbacks. I also prefer that since it is less likely to have breaking changes this way.

Apologies if there was a miscommunication: I'm totally fine with the errorBuilder field if it's added to TextFormField rather than InputDecoration. (I understand it'd be nice to have this for the dropdown form field as well—this could be something we add to the generic FormField widget… maybe at some point down the line, a default error builder inherited from an ancestor Form? Lots of possibilities!)

Overall, it seems to me like since validator is an aspect of a form field, so should be this builder callback. Let me know what you think.

I added it to InputDecoration since it is shared across different types of widgets, and thus provides an easy-to-use, reusable, and consistent way of customizing/styling an error message. Frankly speaking, I do not see any pros of adding it separately to all the widgets, instead of just adding it to InputDecoration.

@justinmc Some really good points for future refactoring for InputDecoration will be that:

  1. InputDecoration also takes an InputDecorator that builds the InputDecoration, instead of hard coding the default InputDecoration, and
  2. also to make the internals of InputDecorator protected, instead of private.

In this way, third-party packages can implement these changes themselves, and as a result, take some load off of the Flutter team.

@nate-thegrate
Copy link
Contributor

Consistent with which Flutter API?

Unique builder callbacks are fairly common, but as of right now I don't believe that any Flutter API asks for a widget with a unique build method.


I do not see any pros of adding it separately to all the widgets, instead of just adding it to InputDecoration.

The main pro, in my opinion, is that complicating the Form API is less detrimental than complicating the InputDecoration API.
I'm not a fan of the Form / FormField design (this problem with the validator is a good example), and I think it's fairly likely that it will be rewritten or reworked in some way. Assuming there indeed is an upcoming change to Form, it'd be great if there weren't any leftover artifacts of the previous API, e.g. inside InputDecoration.

@MohiuddinM
Copy link
Contributor Author

MohiuddinM commented Nov 24, 2024

Unique builder callbacks are fairly common, but as of right now I don't believe that any Flutter API asks for a widget with a unique build method.

Sorry for not making it clear. It's not a Widget, it's a class (just like many others in themes e,g -Decoration, -Style, etc.) that can contain error-related properties.

Edit: I updated my comment to make it clear

@nate-thegrate
Copy link
Contributor

Ah, gotcha! I'd be interested in exploring this a bit more, in the context of a larger Form improvement 👍

In the meantime, I'd still be happy to approve adding an errorBuilder to TextFormField (or the FormField superclass), but I would like to point out that in terms of achieving a customized, reusable error message style, no error builder is necessary—in my personal projects I prefer ignoring Form entirely and using a provider-esque state management package in its place.

@justinmc
Copy link
Contributor

@MohiuddinM Your problem is that you don't want to specify the errorBuilder in every TextFormField, you want to just specify it in the InputDecoration like this, right? You have lots of TextFormFields all over your application that all use the same InputDecoration it sounds like?

errorBuilder in InputDecoration (this PR) errorBuilder in TextFormField
TextFormField(
  autovalidateMode: AutovalidateMode.always,
  decoration: _myInputDecoration,
  validator: _myValidator,
),
TextFormField(
  autovalidateMode: AutovalidateMode.always,
  decoration: _myInputDecoration,
  validator: _myValidator,
  errorBuilder: _myErrorBuilder,
),

And you also don't want to write a wrapper around TextFormField or write your own TextFormField.

@MohiuddinM
Copy link
Contributor Author

@MohiuddinM Your problem is that you don't want to specify the errorBuilder in every TextFormField, you want to just specify it in the InputDecoration like this, right? You have lots of TextFormFields all over your application that all use the same InputDecoration it sounds like?
errorBuilder in InputDecoration (this PR) errorBuilder in TextFormField

TextFormField(
  autovalidateMode: AutovalidateMode.always,
  decoration: _myInputDecoration,
  validator: _myValidator,
),
TextFormField(
  autovalidateMode: AutovalidateMode.always,
  decoration: _myInputDecoration,
  validator: _myValidator,
  errorBuilder: _myErrorBuilder,
),

And you also don't want to write a wrapper around TextFormField or write your own TextFormField.

If there were an errorBuilder on TextFormField and DropDownFormField then ofc I would have just used that instead of arguing about that here.

But since there isn't one, so I came up with a solution that maximizes consistency and ease of use. If you don't like my design then feel free to close this PR, and add it the way you would like that would allow people to customize the errorText.

@justinmc
Copy link
Contributor

justinmc commented Dec 5, 2024

Alright I'm going to go ahead and close this in preference of a solution based on TextFormField like #157087 in order to maintain the separation of concerns between InputDecorator and TextFormField.

@MohiuddinM I really appreciate your work on this PR, thank you for opening it and for putting up with all of our discussion here. We wouldn't have been able to decide how this should be fixed without seeing it done this way first.

@justinmc justinmc closed this Dec 5, 2024
github-merge-queue bot pushed a commit that referenced this pull request Jan 30, 2025
## Description

This PR adds the `TextFormField.errorBuilder`property which makes it
possible to customize the widget used to display the error message.

Implementation based on
#156275 (comment)

## Related Issue

Fixes [Unable to use validator along with error widget in
TextFormField](#133629)
Fixes #135292

## Tests

Adds 1 tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
5 participants