Skip to content
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

Align field components behavior initially set as invalid with no constraints #4170

Open
web-padawan opened this issue Jul 12, 2022 · 5 comments
Labels
refactor Internal improvement validation

Comments

@web-padawan
Copy link
Member

web-padawan commented Jul 12, 2022

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:

checkValidity() {
if (this.inputElement && this._hasValidConstraints(this.constructor.constraints.map((c) => this[c]))) {
return this.inputElement.checkValidity();
}
return !this.invalid;
}

checkValidity() {
return !this.required || !!this.value;
}

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:

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 web-padawan added the refactor Internal improvement label Jul 12, 2022
@vursen
Copy link
Contributor

vursen commented Jul 12, 2022

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.

@web-padawan
Copy link
Member Author

web-padawan commented Jul 14, 2022

Here is the original request for this feature in vaadin-text-field: vaadin/vaadin-text-field#130
It has been implemented by vaadin/vaadin-text-field#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.

@vursen
Copy link
Contributor

vursen commented Aug 18, 2022

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
Copy link
Contributor

vursen commented Aug 18, 2022

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
Copy link
Contributor

vursen commented Aug 18, 2022

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.

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

No branches or pull requests

2 participants