Skip to content

Validation - field Email can not be empty if "touched" #402

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
fabarea opened this issue Nov 18, 2016 · 17 comments
Closed

Validation - field Email can not be empty if "touched" #402

fabarea opened this issue Nov 18, 2016 · 17 comments

Comments

@fabarea
Copy link

fabarea commented Nov 18, 2016

Read the doc OK

Description

Consider we have a field email with format: email. If we leave the field as pristine - and we don't require the field - we can save it without problem. But as soon as you start typing an email address and then remove it for some reason, the form is not considered valid anymore as field "email" wrongly expects a value.

Live: https://jsfiddle.net/fab1en/yn1ay7op/2/

Expected behavior

Field email should only be validated agains format email if it contains a value

@atkawa7
Copy link

atkawa7 commented Nov 18, 2016

This is because email is not required so validation will not validate email. Your required array is
required: ["name"]

Probably you meant

required: ["name", "email"]

@fabarea
Copy link
Author

fabarea commented Nov 18, 2016

I don't want "email" to be required. It must be optional in my use case. It only validate against "email" if it has a value.

@maartenth
Copy link
Contributor

Good catch, I think that this bug is due to the invisible difference between 'no value' and the empty string: if you don't touch the field, the resulting formData looks like {name: '(name)'}, but if you do touch the field, the result looks like {name: '(name)', email: ''}. I think the right approach for non-required fields would be to always delete empty non-required values from the formData. That would lead to a better UX in my opinion.

@n1k0
Copy link
Collaborator

n1k0 commented Nov 21, 2016

We'd need a reset button for each field, so their individual value could be reinitialized to undefined. I could swear we had an issue for it, but couldn't find it back.

@crumblix
Copy link
Collaborator

crumblix commented Nov 24, 2016

OK, so I came across this as part of a separate issue to do with consistency in mandatory fields. (Issue #410).

It is the old battle between empty string and NULL.

My 2cents worth (for what it's worth) is that although empty string is a valid string type, it is rarely if ever used that way in a web context.

An HTML input control does not support NULL. Only empty string. And an empty string triggers does not count "as a string" for a mandatory check (via the required attribute).

So while technically the JSON type supports both, it appears (that in a webcontext) most users don't expect or see a difference between NULL and empty string.

As a result currently the HTML required tag kicks in to say that the an empty string is required .... however our validation check says it's a good string. That's the inconsistency. So when the error list is returned it may only contain some of the missing required fields.

The options as I see it are:

  1. Live with the inconsistency. The returned error list on validation will never include a standard text field.
  2. Add a clear button (or "cross" button) to be able to send a NULL (undefined) in addition to the empty string from the text input.
  3. Decide that empty string means NULL in this context, and pass (undefined) in instead of empty string.

I must admit I prefer 3. I personally don't like the inconsistency of only returning some "missing" required fields on validation, and that one validation check (the required attribute) says it's missing while the other says it isn't.

Option 2 will just confuse users who didn't even realise there was an issue in the first place. To most of them there really is no difference, and they'd be right in 99% of applications and instances.

Option 3 is consistent with the way the HTML "required" attribute works. And that makes it the standard as far as I'm concerned. That's the interface users are going through, and it doesn't support by default the distinction between NULL and empty string. The JSON standard itself might, but that's not the interface users are using.

If we must go the button route, may I make a further suggestion that it is an option rather than the default? (Off by default that is). It just will confuse too many users. In the 1% of cases that matter surely the developer can turn the option on?

And when the option is OFF, I suggest it means the NULL could then be equivalent to empty string, and be can be internally consistent my passing "undefined" into onChange instead of empty string. :)

Thanks for taking the time to read, hope it helps! And thanks for all the great work you've all been doing :)

@maartenth
Copy link
Contributor

Please note the difference between null and 'not defined', because in your bug report above, the email field was actually 'not defined' (= omitted from the output) and not null. (The value undefined does not exist in JSON.) If a property is not required, it means that it may be omitted / 'not defined', but not that it may be null.

type: ["string", "null"] is a valid JSON Schema syntax indicating that it may be null, but this library doesn't currently support multiple types arrays. This would be a different feature and another problem to solve than this issue.

I would like to have empty input indicating that the value must be omitted from the output. For non-required fields, this is always what you want in my experience. And for required fields, this yields the right error message ('this field is required'), which is also what you want. Otherwise, with an empty string, the error message differs:

  • if the field was still untouched, you get 'the 'email' field is required' message on the containing object
  • if the field was touched, you get 'does not conform to the "email" format' message on the field

And, last but not least, when you mark a string as required, you don't ever want the empty string. People may then fill out a form with required fields by typing one character in every field, deleting it, and submit it.

(For numbers, this problem does not exist, because the empty string is not convertable to a number, so the distinction between not defined and a number is always visible.)

@crumblix
Copy link
Collaborator

Actually you are completely right. There certainly is a difference between NULL and not defined and empty string. I've found it very useful in R in particular.

I agree with marteenth. Empty string should be ommited from the output. That was the basis for suggestion in #410.

As I mentioned in my message above, I still think the key is that the input control itself cannot distinguish on it's own between empty string, NULL, and undefined (or NA if you're an R user).

This is the interface people use for forms every day on the web, and that's the context we are working within. What is the best way to represent the data that our application actually requires and the user expects?

I suspect (as this example show) we are trying to shoe-horn functionality into a control that cannot distinguish between the various difference.

We certainly could make it work, with buttons etc, but where does that leave the user experience?
I can't imagine what most people would think if we cleared a text input with a button and it gave a different result or error message to simply clearing with backspace. I think that would be an unfortunate default.

I still suspect that we are potentially sacrificing the 99% of cases for the 1%.

Assume (for the moment) that we treat NULL the same as undefined and empty string. What real world websites or apps would we break? I can think of perhaps a couple and I suspect they'd be easy to work around. Truth be told the system is actually nice and flexible, and writing your own custom control is dead easy. If the functionality isn't quite the way you want you can easily make your own custom control.

The HTML required tag already disagrees with our JSON schema validation, where it thinks empty string is not a valid string, while our explicit validation thinks empty string is a valid string.

When the interface doesn't really support the difference I think we should think carefully before we try and make it.

I would prefer a default that works "out of the box" for the 99% of cases, because that's what users expect and will understand. And then (if we must ... or someone cares!) implement an option that gives a button to clear, but I would suggest that shouldn't be the default.

So the patch I was suggesting on issue #410 would actually eliminate this problem, because it addresses that 99%, and I created that patch without even knowing about this issue. It was to address the inconsistency between of HTML "required" and our validation. The email empty string would have been converted to "undefined" and so would no longer be a string to be validated. The entire "email" attribute is removed from the JSON result data.

Not only is this consistent with the HTML required attribute, but also with how our number and date inputs work. When we make them an "empty string" input, we pass through undefined and they disappear from the JSON data as well.

To my mind it's simple clean and consistent. Is it the only way? Absolutely not, and I totally respect that it's not perfect, but to my mind it's better than the alternatives I've read. However, I'd be as pleased as punch if someone came up with a better one! I'm after a good user experience and consistency if we can do that another way I'm good with that :)

I haven't (and won't) submit my changes as a PR until we reach a consensus on this issue, but it is a trivial fix.

@fabarea
Copy link
Author

fabarea commented Jan 4, 2017

Ping. Could we move forward with the topic? #410 looks as a way to fix the issue.

@tsemerad
Copy link

tsemerad commented Jan 6, 2017

I agree. I think @crumblix's solution is ideal.

@MoOx
Copy link
Contributor

MoOx commented Jan 17, 2017

I am facing this issue as well and would expect empty field act the same as a untouched field.

Btw, some browser have already a clean button (eg below for input date on chrome - the cross is "native")

screen shot 2017-01-17 at 11 33 31

I think a button will break the classic way (select + backspace) to clean a field and will provide bad UX.

As it has been said, html is already inconsistent with json so we should live with that and provide a solution that will match 99% of the use case ("I remove the value by hand and I expect it to be considered as undefined").

@n1k0
Copy link
Collaborator

n1k0 commented Jan 17, 2017

It seems we're reaching consensus here, even if I'd prefer having a clear button. But I understand the needs for mimicking current regular forms behavior.

Who wants to work on this?

@MoOx
Copy link
Contributor

MoOx commented Jan 17, 2017

It seems #410 referenced some commits

I think the first one is pretty simple & clear about what it does.

Is this a good start @n1k0? What would be necessary to add?

@maartenth
Copy link
Contributor

+1, 2aad37f looks good

@n1k0
Copy link
Collaborator

n1k0 commented Jan 17, 2017

Is this a good start @n1k0? What would be necessary to add?

Yeah, 2aad37f seems to be what we've described above. Now what I need is a proper PR, full test coverage and docs. Who's in? @crumblix maybe?

Also, if we now remove object field properties whenever they contain an empty string (this is basically what setting it to undefined does), what do we do for empty arrays?

Edit: It seems #420 takes care of the Array case. I'd love having a single, consistent PR to review though. I promise to give it the attention it deserves as it seems to be something important to users of this lib.

@crumblix
Copy link
Collaborator

As agreed, I'll merge my changes from #410 into #420 and take it from there :)

@crumblix
Copy link
Collaborator

This has now all been wrapped up in a new PR #442.

@n1k0
Copy link
Collaborator

n1k0 commented Feb 13, 2017

Fixed by #442.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants