[RFC] Fix for Validation Issues

[chabad360]

Description:

This is a Request for Comments on a potential fix for #2964.

Fixes #2964

Checklist:

• Tests included.
• Lint and formatter run with no errors.
• Tests all pass.

Where applicable:

• Public APIs match existing style.
• Any breaking changes have a deprecation path or have been discussed.

[andydotxyz]

Thanks this seems to be heading in the right direction. I have a comment or two inline.

The main question is whether we can improve the rendered / changedAfterRender pairing. I don't know if it can be improved, but it feels a little less clean than the rest of the changes here.

[andydotxyz]

I'm not sure I understand this line - can you explain what it is for?

[chabad360]

Its necessary because we store the error a few lines down and if it's nil when there is an error, all hell breaks loose.

Although considering that I've added this it might not be necessary now that I look at it again.

Basically, if the item was invalid but the error was nil (because the entry didn't send an update and we didn't store the error until we got a new one) the form wouldn't show the error text and more importantly it would store a nil error which would signal to a parent that it was all kosher (which it wasn't).

[andydotxyz]

I think I understand thanks. If it is required to signal this state then perhaps a sentinel (named const/var error definition) would be better used with a suitable name so this is clear.

[chabad360]

The main question is whether we can improve the rendered / changedAfterRender pairing. I don't know if it can be improved, but it feels a little less clean than the rest of the changes here.

I don't like it either but I couldn't think of a better way to deal with that.

Additionally, there are still a few special cases in the Form for an Entry, so those need to go (I need to figure out exactly what they do first tho).

Also, I noticed that there is a regression where the error doesn't go away if the entry is emptied (very useful to figure out what was supposed to go there in the first place). This was originally done using some special handling (check for !Entry.dirty) but there isn't way to do this currently.

[chabad360]

Additionally, there are still a few special cases in the Form for an Entry, so those need to go (I need to figure out exactly what they do first tho).

Actually, now that I look at it, I think this could stay in. But I would prefer if there was a better way to communicate this case (of something that is technically a validator but isn't actually).

[andydotxyz]

I don't like it either but I couldn't think of a better way to deal with that.

We can always iterate :)

Additionally, there are still a few special cases in the Form for an Entry, so those need to go (I need to figure out exactly what they do first tho).

Agreed, thanks.

Also, I noticed that there is a regression where the error doesn't go away if the entry is emptied (very useful to figure out what was supposed to go there in the first place).

Good catch, but only if empty text is allowed :) if there is truly no error here then the Entry should signal that the error went away, just like any other new validation pass state.

Actually, now that I look at it, I think this could stay in. But I would prefer if there was a better way to communicate this case (of something that is technically a validator but isn't actually).

Hmm, this may be a new case we had not thought of - something that is validatable but is not actually going to be validated...

[chabad360]

Also, I noticed that there is a regression where the error doesn't go away if the entry is emptied (very useful to figure out what was supposed to go there in the first place).

Good catch, but only if empty text is allowed :)

No, I don't think so. The entry can still keep it's color red to indicate the problem, but there still needs to be a way for the user to re-determine the requested input (this can actually be an accessibility issue, namely for people with poor working memory).

The bigger question is what is the best way to communicate this. Perhaps we could include a special case for an empty error (or maybe a HintTextError, where the hint text is used in place of the error text.

Actually, now that I look at it, I think this could stay in. But I would prefer if there was a better way to communicate this case (of something that is technically a validator but isn't actually).

Hmm, this may be a new case we had not thought of - something that is validatable but is not actually going to be validated...

Now that I think about it again, I don't actually think it's an issue. Anything that has this capability should just return nil in the case where there is no validation anyway. The code is anyways only there so that we can skip initializing somethings (perhaps we want to keep it?), I don't believe it meaningfully affects UX.

[andydotxyz]

No, I don't think so. The entry can still keep it's color red to indicate the problem, but there still needs to be a way for the user to re-determine the requested input (this can actually be an accessibility issue, namely for people with poor working memory).

Sorry I don't understand this. What I was meaning is that an empty entry may still be an error - if an empty string is not allowed and the entry has been interacted with.

The bigger question is what is the best way to communicate this. Perhaps we could include a special case for an empty error (or maybe a HintTextError, where the hint text is used in place of the error text.

The FormItem already provides HintText which is indeed drawn in the same space as errors appear - is that what you mean?

[Jacalz]

Thanks for the contribution. I'm sorry that this got left for such a long time but I will have to close this per the rule that stale PRs can be closed after 6 months. If you want to continue working on this, you are more than welcome to rebase on latest develop and reopen (or open a new one if that's easier).