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

feat: integrate client-side validation into TextField #3429

Merged
merged 25 commits into from
Sep 2, 2022

Conversation

vursen
Copy link
Contributor

@vursen vursen commented Jul 4, 2022

Description

  • Implement HasClientValidation.
  • Implement ClientValidationUtil.
  • Implement ValidationStatusChangeEventListener.
  • Add ITs ensuring the constraint validation is run on the validated event.
  • Add ITs ensuring the binder validation is run on the validated event.
  • Add ITs ensuring only the server can set TextField to valid.
  • Add ITs for basic constraint validation.
  • Add ITs for basic constraint validation when used through binders.

The integration tests are implemented as abstract classes so that they can be reused later for NumberField, PasswordField, and so on.

Depends on

Part of vaadin/platform#3066

Type of change

  • Feature

Checklist

  • I have read the contribution guide: https://vaadin.com/docs-beta/latest/guide/contributing/overview/
  • I have added a description following the guideline.
  • The issue is created in the corresponding repository and I have referenced it.
  • I have added tests to ensure my change is effective and works as intended.
  • New and existing tests are passing locally with my change.
  • I have performed self-review and corrected misspellings.

@vursen vursen changed the title feat: validate TextField on validated event (WIP) feat: run TextField server-side validation on validated event (WIP) Jul 4, 2022
@vursen vursen changed the title feat: run TextField server-side validation on validated event (WIP) feat: validate TextField on client-side validated event (WIP) Jul 4, 2022
@vursen vursen force-pushed the feat/text-field/validated-event branch 5 times, most recently from 71c7794 to 95f8f77 Compare July 4, 2022 10:07
@vursen vursen force-pushed the feat/text-field/validated-event branch 3 times, most recently from 721529d to b765d15 Compare July 4, 2022 12:53
@vursen vursen changed the title feat: validate TextField on client-side validated event (WIP) feat: validate TextField on client-side validated event Jul 5, 2022
@vursen
Copy link
Contributor Author

vursen commented Jul 5, 2022

Some ITs have failed which is apparently caused by the fact that we don't trigger the binder validation on the validated event.

Scenario:

  1. The field doesn't pass the binder validation so it gets the invalid attribute.
  2. The user triggers a blur event which leads to firing a validated event.
  3. The field passes the constraint validation but the binder validation hasn't been run so the invalid attribute gets removed (INCORRECT).

@vursen
Copy link
Contributor Author

vursen commented Jul 7, 2022

I actually encountered a more heavy challenge which is that the validated event may happen during the web component's initialization which may result in an invalid reset.

Example:

textField = new TextField();
textField.setValue("Some not empty value");
textField.setInvalid(true);
add(textField)

What happens here is Polymer triggers value observers (because of the provided initial value) during the web component's initialization which subsequently triggers a validated event (because of this logic in the web component). As a result, Flow resets the invalid state since everything is valid in terms of the Flow validation (no constraints, no binder and etc).

I'm trying to fix that in the web components.

@@ -81,6 +83,8 @@ private TextField(boolean isInitialValueOptional) {
setValueChangeMode(ValueChangeMode.ON_CHANGE);

addValueChangeListener(e -> validate());

addClientValidatedEventListener(e -> validate());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's still one missing piece, which is to fire the ValidationStatusChangeEvent when the client side validation changes. Testing how it works, it seems that once the event is dispatched, the binder will take the validation status sent on the event to set the component is valid/invalid. So, the getDefaultValidator() is not called.

The thing I am still thinking about is how the validate method comes into play here. It needs to be called so that the component being used without binder will get the invalid state correctly set. But when binder is being used, we would probably need to call getValidationSupport().isInvalid(getValue()) to get the validation result and send it to binder with the event.

Copy link
Contributor Author

@vursen vursen Jul 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. My first impression of ValidationStatusChangeEventListener was that we will need to subscribe the binder to ClientValidatedEvent by adding ValidationStatusChangeEventListener for ClientValidatedEvent.

Copy link
Contributor Author

@vursen vursen Jul 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, it seems that the way it should work is:

  1. ValueChange triggers constraint validation and then binder validation (this is already done).
  2. ClientValidated triggers constraint validation and then binder validation by invoking ValidationStatusChangeEventListener listeners.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually had a wrong assumption here:

So, the getDefaultValidator() is not called.

My mistake was because I placed the breakpoint on the method body instead of the lambda's. What I can understand now is:

  1. Client side validation fires validated event
  2. Flow component listens the event and fires ValidationStatusChangeEventListener
  3. Binder listens to this event and calls its validate() method
  4. Eventually, validate() method will call the lambda created on getDefaultValidator().
  5. Component's checkValidity() method is called.

@vursen vursen force-pushed the feat/text-field/validated-event branch 3 times, most recently from ae90842 to b33b4fc Compare July 20, 2022 08:20
@vursen vursen changed the title feat: validate TextField on client-side validated event feat: integrate client-side validation into TextField Jul 20, 2022
@vursen vursen force-pushed the feat/text-field/validated-event branch from 50cb675 to 40edcd9 Compare July 20, 2022 14:57
@vursen
Copy link
Contributor Author

vursen commented Jul 20, 2022

The build succeeded. I think the only remaining thing now is to put changes behind a feature flag and choose between setInputValue vs setValue TB methods (#3484).

@vursen vursen force-pushed the feat/text-field/validated-event branch 3 times, most recently from f59b6fe to 22fbacb Compare July 22, 2022 14:59
return addClientValidatedEventListener(event -> {
listener.validationStatusChanged(
new ValidationStatusChangeEvent<String>(this,
!isInvalid()));
Copy link
Contributor Author

@vursen vursen Jul 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose of the second parameter of ValidationStatusChangeEvent is not clear to me given that it is not used anywhere.

@vursen vursen force-pushed the feat/text-field/validated-event branch from 5757145 to 4bc2dac Compare September 1, 2022 13:15
@vursen vursen requested review from yuriy-fix and removed request for DiegoCardoso September 1, 2022 13:32
@vursen vursen enabled auto-merge (squash) September 2, 2022 06:29
@sonarcloud
Copy link

sonarcloud bot commented Sep 2, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell D 36 Code Smells

No Coverage information No Coverage information
7.0% 7.0% Duplication

@vursen vursen merged commit 0c5a226 into master Sep 2, 2022
@vursen vursen deleted the feat/text-field/validated-event branch September 2, 2022 06:38
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 23.2.0.

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

Successfully merging this pull request may close these issues.

4 participants