web-components icon indicating copy to clipboard operation
web-components copied to clipboard

Published 20 hours ago • verified publisher icon vaadin

Align field components behavior initially set as invalid with no constraints

Open web-padawan opened this issue 3 years ago • 5 comments

Describe your motivation

We have some misalignment in how checkValidity() is implemented in different field components. Some components have their own implementation, whereas others inherit from one of the mixins:

https://github.com/vaadin/web-components/blob/f4e19b7e948cd90e108edeb0483e70f68ad1b934/packages/field-base/src/input-constraints-mixin.js#L49-L54

https://github.com/vaadin/web-components/blob/f4e19b7e948cd90e108edeb0483e70f68ad1b934/packages/field-base/src/validate-mixin.js#L57-L59

This fact causes difference in what happens when pressing Tab to focus and then blur component that has invalid set as attribute without e.g. required or other constraints, if applicable (pattern, min and max, etc).

  1. Components that use checkValidity() inherited from InputConstraintsMixin (text-field, combo-box) do not re-validate on blur: the field remains invalid until user changes its value.
  2. Other components that have custom checkValidity() (date-picker, time-picker, select) or inherit from ValidateMixin (radio-group) become valid on blur, without changing their value

Example

<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="UTF-8" />
    <meta http-equiv="X-UA-Compatible" content="IE=edge" />
    <meta name="viewport" content="width=device-width, initial-scale=1.0" />
    <title>Validation</title>
    <script type="module">
      import '@vaadin/checkbox-group';
      import '@vaadin/combo-box';
      import '@vaadin/date-picker';
      import '@vaadin/date-time-picker';
      import '@vaadin/multi-select-combo-box';
      import '@vaadin/radio-group';
      import '@vaadin/select';
      import '@vaadin/text-field';
      import '@vaadin/time-picker';
    </script>
  </head>
  <body>
    <vaadin-combo-box label="Combo-box" invalid error-message="Invalid"></vaadin-combo-box>
    <br>
    <vaadin-checkbox-group label="Checkbox group" invalid error-message="Invalid">
      <vaadin-checkbox value="en" label="English"></vaadin-checkbox>
      <vaadin-checkbox value="fr" label="Français"></vaadin-checkbox>
      <vaadin-checkbox value="de" label="Deutsch"></vaadin-checkbox>
    </vaadin-checkbox-group>
    <br>
    <vaadin-date-picker label="Date picker" invalid error-message="Invalid"></vaadin-date-picker>
    <br>
    <vaadin-date-time-picker label="Date-time picker" invalid error-message="Invalid"></vaadin-date-time-picker>
    <br>
    <vaadin-multi-select-combo-box label="Multi-select combo-box" invalid error-message="Invalid"></vaadin-multi-select-combo-box>
    <br>
    <vaadin-radio-group label="Travel class" invalid error-message="Invalid">
      <vaadin-radio-button value="economy" label="Economy"></vaadin-radio-button>
      <vaadin-radio-button value="business" label="Business"></vaadin-radio-button>
      <vaadin-radio-button value="firstClass" label="First Class"></vaadin-radio-button>
    </vaadin-radio-group>
    <br>
    <vaadin-select label="Select" invalid error-message="Invalid"></vaadin-select>
    <br>
    <vaadin-text-field label="Text field" invalid error-message="Invalid"></vaadin-text-field>
    <br>
    <vaadin-time-picker label="Time picker" invalid error-message="Invalid"></vaadin-time-picker>
    <br>
  </body>
</html>

Describe the solution you'd like

Ideally, we should align all the components that have checkValidity() to only validate if constraints are provided. Otherwise, this logic should be executed to prevent resetting invalid state on blur or validate() calls:

https://github.com/vaadin/web-components/blob/f4e19b7e948cd90e108edeb0483e70f68ad1b934/packages/field-base/src/input-constraints-mixin.js#L53

The following components need to be updated:

  • vaadin-checkbox-group
  • vaadin-custom-field
  • vaadin-date-picker
  • vaadin-date-time-picker
  • vaadin-multi-select-combo-box
  • vaadin-radio-group
  • vaadin-select
  • vaadin-time-picker

See also #1750 which describes the same issue for vaadin-date-picker.

web-padawan avatar Jul 12 '22 08:07 web-padawan

Otherwise, this logic should be executed to prevent resetting invalid state on blur or validate() calls:

This approach doesn't seem to take into account the case when the developer removes all the constraints at the moment the component is already invalid (e.g. because its value didn't satisfy some of the removing constraints during previous validation). In that case, the component will stay invalid forever which is probably undesirable.

vursen avatar Jul 12 '22 09:07 vursen

Here is the original request for this feature in vaadin-text-field: https://github.com/vaadin/vaadin-text-field/issues/130 It has been implemented by https://github.com/vaadin/vaadin-text-field/pull/149 but not ported to other components later.

we would like to be able to manage invalid and errorMessage properties ourselves and be 100% sure that it won't change if there is no required and pattern fields set (now it does change which is an issue to us)

So, the assumption is that developers might want to manage setting invalid property programmatically.

web-padawan avatar Jul 14 '22 10:07 web-padawan

Just returning !this.invalid in checkValidity, if no constraints are provided, is not going to work for components whose validation also checks for bad input e.g. date-picker. This check makes sense regardless of the presence of any constraints.

Scenario:

  1. The field is valid and has no constraints.
  2. The user tries to commit something that cannot be parsed.
  3. The field becomes invalid.
  4. The user corrects the input and makes another attempt to commit it.

Expected: The field is valid. Actual: The field will never get back valid because no constraints and therefore checkValidity returns !this.invalid.

Needs to be figured out how we could work this around.

vursen avatar Aug 18 '22 10:08 vursen

Maybe instead of the !this.invalid trick in checkValidity, we should ensure that validation doesn't happen on blur when nothing has been changed. This however doesn't give absolute guarantee that invalid will be preserved in all possible situations when added by the user. For example, it may still be reset in the above case. But this solution can be still good enough.

vursen avatar Aug 18 '22 11:08 vursen

we should ensure that validation doesn't happen on blur when nothing has been changed.

This is actually not a correct strategy too as it will break the required case when we want to mark the field invalid only after the user has interacted with it. The user may focus and immediately blur the field, so the value won't be changed but the validation has to happen anyway to make the field red.

Looking at this issue from different angles, I'm getting convinced that it is quite not feasible to support both the manual invalid control and the build-in validation unless we introduce a flag or something for sort of disabling the build-in validation or preventing it from updating the invalid state. There is just no way that would allow us to distinguish when invalid is set by the user and when by the component in all the cases.

vursen avatar Aug 18 '22 14:08 vursen