Issues with Validation

[chabad360]

This is based on a discussion I had with @andydotxyz on discord.

TL;DR: Currently, Forms do not treat validation as a black box, and contain special behavior for an Entry.
The problem with just removing this behavior is that we need to do something different to ensure that we don't startle the end user with errors until they interact with them and currently this works by affecting the internal state of an Entry.
The question at hand is: How can we fix this so a Form exhibits the same behavior for all Validators and can it be done without changing the Validator interface?

Edit (6/5/22): added a note about user interaction state
Edit (8/5/22): added note about multiple consumers and TL;DR

Problem:

We have two categories in validation: Producers (Validators) and Consumers (any thing that uses SetOnValidationError()).

The contract for producers and consumers (and the user) is as follows:

• Producers must always pass on an update to the validation state.
• Consumers can also be Producers (and perhaps they should always be).
• All producers and consumers that are user facing mustn't alert the user to their invalid state (unless there is no other way to indicate their invalid state) until after the user has interacted with them at least once.

The current problem is two-fold:

1. This contract is currently only fulfilled when the producer is an Entry and the consumer is a Form.
2. This contract is not really always fulfilled because the current convention is that producers only pass on the new state when it has changed meaningfully from the previous one.

To be clear: on a lower level, currently this contract is only fulfilled from the end user's stand point and not internally.

Solutions:

Solution A:

Add SetValidationError() to the Validator interface and set an initial error on the children. This fulfills the contract by using an error that everything knows to ignore, but also is still an invalid state.

Pros:

• We use this technique already for Entrys, so it should work elsewhere (considering that it is the reference implementation for other Validators).
• Because any change in the validation status will not be this initial error, it will for certain be a meaningful change (so it will always update).
• It would require the least changes to existing logic (I think).
• Having this method also allows us to quickly circumvent a validation error if we want.

Cons:

• This would constitute a breaking API change.
• An additional rule would be added to the contact: Producers must pass on an update of state from their parent to their children.
• Every consumer would need to implement the logic to set this "initial error" on initialization (because there is no guarantee that it has a parent). So ultimately this solution isn't really better.
• This requires allowing modifying what is effectively internal state of a producer, which is almost definitely a bad idea.

Ultimately, this solution is probably the wrong one and therefore:

Solution B:

Fully divorce the presentation of the error from the (change in) state of it. So when the widget is first rendered it will not show anything alerting (i.e. red colored) until Validate() has been called.

Pros:

• This method fully fulfills the contract.
• It allows us to treat each Validator as a black box (like it should be).
• This method should be a bit more clear in the implementation.

Cons:

• There doesn't appear to be a way to determine if a widget has been rendered (this is mainly a problem for anything that extends a widget).
• I think this is enough of a behavior change that it could be considered breaking.
• The logic for this is a bit more complicated and we don't yet know what it would actually look like.

Notes:

Different reactions based on state of user interaction:

There is reason to amend the the third rule of the contract to:

• All producers and consumers that are user facing mustn't alert the user to their invalid state (unless there is no other way to indicate their invalid state) until after the user has finished interacting with them.

If we accept this change (which Form and Entry already adheres to), this will complicate how the state is passed on.
The problem with the way Form does it, is that is gets the internal state from Entry (by virtue of being in the same package) to determine if the Entry is focused. This causes it not to work on anything other than an actual Entry. So one way to fix this would be to add a clause to the first rule:

• Producers must always pass on an update to the validation state only as long as the user is not interacting with them.

However, this would mean that the validation state (at least in the consumer) isn't updated at all until after the user is no longer interacting with the producer, which is no good. It is useful for a consumer to demonstrate that the producer state is valid (ex. the submit button on a Form is enabled as soon as valid input is provided), so there needs to be some way to pass up that the state has changed while also including the fact that it hasn't finished changing (or something like that, i.e. the widget is still focused), or there needs to be a way to determine it's still being edited.

Multiple Consumers:

Another question is should SetOnValidationChanged() not overwrite previously set callbacks? This would add a clause to the first rule:

• Producers must always pass on an update to the validation state to all it's consumers.

Additionally, this would likely require adding a Delete() method to the Validator interface, or to just create a new interface (something like MultipleParentValidator) that includes Delete(). The second method is fine as it is makes no real difference to a parent as long as it receives an update. But honestly, I don't think being able to delete a callback is strictly required.

[chabad360]

I'm going to attempt to implement solution B (once #2781 is merged) and I'll see what comes out of it. In the mean time, any one who has anything to say please do, because I don't want to miss anything.

[andydotxyz]

• If we accept this change (which Form and Entry already adheres to), this will complicate how the state is passed on.
  The problem with the way Form does it, is that is gets the internal state from Entry (by virtue of being in the same package) to determine if the Entry is focused. This causes it not to work on anything other than an actual Entry.

There are public APIs to get focus state if that is required. The Canvas can be asked what is focused and we can see if that is the same as the object you are curious about.

[andydotxyz]

• Producers must always pass on an update to the validation state only as long as the user is not interacting with them.

I don’t agree with this amendment, it is imposing render info onto internal state. As discussed on the call earlier it is strange that a form showing an error text for an entry will reset when the entry gains focus. Form and entry should not be as tightly coupled as they are.

[chabad360]

I don’t agree with this amendment, it is imposing render info onto internal state. As discussed on the call earlier it is strange that a form showing an error text for an entry will reset when the entry gains focus. Form and entry should not be as tightly coupled as they are.

I don't like it either, but I wasn't sure how to determine what is focused. Could you elaborate on how you would do that, cause I'm not really able to figure out how that would work.

[chabad360]

New note:

Multiple Consumers:

Another question is should SetOnValidationChanged() not overwrite previously set callbacks? This would add a clause to the first rule:

• Producers must always pass on an update to the validation state to all it's consumers.

Additionally, this would likely require adding a Delete() method to the Validator interface, or to just create a new interface (something like MultipleParentValidator) that includes Delete(). The second method is fine as it is makes no real difference to a parent as long as it receives an update. But honestly, I don't think being able to delete a callback is strictly required.

[andydotxyz]

Let's not extend the scope of things more than required.
It seems reasonable (unless you have a use-case not described) that each validation is fed back to the user only by the current widget and the form or other container that encapsulates it.

[chabad360]

Fair enough. I don't think I'm gonna remove it (for posterity's sake), but that should be fine.

[chabad360]

I don’t agree with this amendment, it is imposing render info onto internal state. As discussed on the call earlier it is strange that a form showing an error text for an entry will reset when the entry gains focus. Form and entry should not be as tightly coupled as they are.

While I agree that form and entry shouldn't be as coupled as they are, some of the UX that results from this is useful and very user friendly. So I would like to preserve that behavior somehow, just without the tight coupling. I agree that this amendment isn't a good way to do it, so maybe we could do it by using error wrapping. If the entry is focused the validation error could wrap (or be wrapped) by a ErrorStillEditing (or something like that) and if that is present, something like a form can "acknowledge but ignore" there error (like it currently does).