fix: Binder should not set non-error fields invalid

[tepi]

Set field as invalid only if ValidationResult.isError() returns true. Add tests to check validation with each ErrorLevel.

[tepi]

This is a behavior breaking change, so might need to wait for 25.0. On the other hand, specified error levels for validation are probably not a very commonly used feature.

[sonarcloud]

Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

[github-actions]

Test Results

1 094 files  ±0  1 094 suites  ±0   1h 18m 34s ⏱️ - 1m 0s
6 936 tests +4  6 888 ✅ +4  48 💤 ±0  0 ❌ ±0 
7 285 runs   - 1  7 225 ✅  - 1  60 💤 ±0  0 ❌ ±0 

Results for commit a15ac1c. ± Comparison against base commit 0339e8b.

♻️ This comment has been updated with latest results.

[knoobie]

Just to make sure: Does this interfere in any kind with how the web components handle the error? e.g. they don't show the error message because invalid=false? Otherwise those #11384 (comment) would fail and we heavily rely on custom error level to show e.g. "Information" for the user, which are NOT allowed to block the submit; but should be visible for the user once they input "trash"

Edit: Looks like it could drastically break apps; see https://github.com/vaadin/web-components/blob/feb975686471b588dbbb11418d9bf1bf33f22022/packages/field-base/src/error-controller.js#L105

[tepi]

Good point, this probably needs more thinking. We might need another flag(s) besides invalid to fix this properly (hasInfo, hasWarning?). With current implementation you can have a Binder which is valid, with all of its fields being invalid, just because isError is checked in some places but not in others.

You mentioned you're showing information to the user, but are you showing warnings also? Seems to me with the current single invalid flag that is impossible?

[knoobie]

With current implementation you can have a Binder which is valid, with all of its fields being invalid, just because isError is checked in some places but not in others.

I totally understand your concern here. This needs definitly a solution where multiple level of errors can be shown; some making the bean & field invalid; some not.

We are currently doing it like so:

• field blue / "INFO: Text" -> Field invalid + Bean valid --> Send Form allowed
• field red / "Error Text" -> Field invalid + Bean invalid --> Send Form forbidden

(Sorry for the bad example texts.. had to remove sensitive informations)

Edit: The different error level are just styling based on the error level on the client +

      if (value == good) {
        return ValidationResult.ok();
      } else  if (value == okayish) {
        return ValidationResult.create("Blue: This looks okayish, but stupid. We might still accept it.", ErrorLevel.INFO);
      } else {
        return ValidationResult.create("Red: This is real shit, stop that input immediatly.", ErrorLevel.ERROR);
      }

[tepi]

Yes, the use case is definitely valid. And also just noticed that we are sort of using the error theme name to 'communicate' the error level to the components. Not sure if components are actually using that info for anything though.

[knoobie]

We also have this in a multi-tiered Combobox where we say something like this: "Hey, you have selected B.. B isn't a good value. When you send the form, we are going to replace B with A."

There is also a "recent" discussion regarding this, were the flow-component broke in v24 some of our implemtations: vaadin/flow-components#4804

[knoobie]

See me edit; we use exactly that name to apply different styling on the client :)

/*
  --- Fields styling to apply ErrorLevel.INFO styling
 */
[invalid][theme~='info'] {
  --vaadin-input-field-border-color: var(--info-color-50pct);
}

/* ensure that all vaadin fields that are themed info and have a part 'error-message' are styled as blue/info-ish */
[invalid][theme~='info']::part(error-message) {
  color: var(--info-text-color);
}

[invalid][theme~='info']::part(input-field) {
  background-color: var(--info-color-10pct);
}