Skip to content

Commit

Permalink
fix: Fix client side validation overriding (#512) (#548)
Browse files Browse the repository at this point in the history
The workaround for overriding the client side validation for certain
components was causing an unnecessary roundtrip to the server side,
which also caused two warnings to be logged by Flow each time a disabled
or a readonly component was added to the UI. The warning happened twice
since two executeJS calls were enqueued: one immediately, and another
when attached. Apparently there was no good reason for this behavior.

This fix will only apply the executeJS once when the component is
attached and it will not require a separate roundtrip to workaround the
client side validation overriding the invalid value because the JS
execution is enqueued on the "beforeClientResponse" phase when the
server side invalid value has been resolved and can be used in the JS
execution.

Further improvement would be to move the FieldValidationUtil to its own
module and reused from there, but that is left for later (?).

Fixes vaadin/flow#8848

Co-authored-by: Pekka Hyvönen <[email protected]>
  • Loading branch information
DiegoCardoso and pleku authored Dec 29, 2020
1 parent 11616ee commit eab7ca6
Show file tree
Hide file tree
Showing 15 changed files with 214 additions and 145 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,6 @@ private DatePicker(LocalDate initialDate, boolean isInitialValueOptional) {
setInvalid(false);

addValueChangeListener(e -> validate());

FieldValidationUtil.disableClientValidation(this);
}

/**
Expand Down Expand Up @@ -334,6 +332,7 @@ protected void onAttach(AttachEvent attachEvent) {
if (i18n != null) {
setI18nWithJS();
}
FieldValidationUtil.disableClientValidation(this);
}

private void initConnector() {
Expand Down Expand Up @@ -633,6 +632,7 @@ public void setAutoOpen(boolean autoOpen) {
getElement().setProperty(PROP_AUTO_OPEN_DISABLED, !autoOpen);
}


/**
* When auto open is enabled, the dropdown will open when the field is clicked.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,7 @@
*/
package com.vaadin.flow.component.datepicker;

import com.vaadin.flow.component.Component;
import com.vaadin.flow.component.HasValidation;
import com.vaadin.flow.component.page.PendingJavaScriptResult;
import com.vaadin.flow.internal.StateNode;

/**
* Utility class for date picker web component to disable client
Expand All @@ -31,32 +29,40 @@ private FieldValidationUtil() {
// utility class should not be instantiated
}

static void disableClientValidation(Component component) {
// if the component was already attached override the validate()
overrideClientValidation(component);

component.addAttachListener(e -> overrideClientValidation(component));
static void disableClientValidation(DatePicker component) {
// Since this method should be called for every time when the component
// is attached to the UI, lets check that it is actually so
if (!component.isAttached()) {
throw new IllegalStateException(String.format(
"Component %s is not attached. Client side "
+ "validation can only be disabled for a component "
+ "when it has been attached to the UI and because "
+ "it should be called again once the component is "
+ "removed/added, you should call this method from "
+ "the onAttach() method of the component.",
component.toString()));
}
// Wait until the response is being written as the validation state
// should not change after that
final StateNode componentNode = component.getElement().getNode();
componentNode.runWhenAttached(ui -> ui.getInternals().getStateTree()
.beforeClientResponse(componentNode,
executionContext -> overrideClientValidation(
component)));
}

private static void overrideClientValidation(Component component) {
PendingJavaScriptResult javaScriptResult =
component.getElement()
.executeJs("this.validate = function() {" +
"return this.checkValidity(this._inputValue);};");
private static void overrideClientValidation(DatePicker component) {
StringBuilder expression = new StringBuilder(
"this.validate = function () {return this.checkValidity();};");

javaScriptResult.then(result -> {
if (component instanceof HasValidation && ((HasValidation) component).isInvalid()) {
// By default, the invalid flag is always false when a component is created.
// However, if the component is populated and validated in the same HTTP request,
// the server side state may have changed before the JavaScript disabling client
// side validation was properly executed. This can sometimes lead to a situation
// side validation was properly executed. This can sometimes lead to a situation
// where the client side thinks the value is valid (before client side validation
// was disabled) and the server side thinks the value is invalid. This will lead to
// strange behavior until the two states are synchronized again. To avoid this, we will
// explicitly change the client side value if the server side is invalid.
component.getElement().executeJs("this.invalid = true");
}
});
if (component.isInvalid()) {
/*
* By default the invalid flag is set to false. Workaround the case
* where the client side validation overrides the invalid state
* before the validation function itself is overridden above.
*/
expression.append("this.invalid = true;");
}
component.getElement().executeJs(expression.toString());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import java.util.Objects;

import com.vaadin.flow.component.AbstractField;
import com.vaadin.flow.component.AttachEvent;
import com.vaadin.flow.component.Component;
import com.vaadin.flow.component.Focusable;
import com.vaadin.flow.component.HasHelper;
Expand Down Expand Up @@ -147,8 +148,6 @@ public DateTimePicker(LocalDateTime initialDateTime) {

getElement().addEventListener("value-changed", e -> this.updateValue());
addValueChangeListener(e -> validate());

FieldValidationUtil.disableClientValidation(this);
}

/**
Expand Down Expand Up @@ -701,4 +700,10 @@ public boolean isAutoOpen() {
return !getElement().getProperty(PROP_AUTO_OPEN_DISABLED,false);
}

@Override
protected void onAttach(AttachEvent attachEvent) {
super.onAttach(attachEvent);
FieldValidationUtil.disableClientValidation(this);

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,7 @@
*/
package com.vaadin.flow.component.datetimepicker;

import com.vaadin.flow.component.Component;
import com.vaadin.flow.component.HasValidation;
import com.vaadin.flow.component.page.PendingJavaScriptResult;
import com.vaadin.flow.internal.StateNode;

/**
* Utility class for date time picker component to disable client side
Expand All @@ -31,36 +29,40 @@ private FieldValidationUtil() {
// utility class should not be instantiated
}

static void disableClientValidation(Component component) {
// if the component was already attached override the validate()
overrideClientValidation(component);

component.addAttachListener(e -> overrideClientValidation(component));
static void disableClientValidation(DateTimePicker component) {
// Since this method should be called for every time when the component
// is attached to the UI, lets check that it is actually so
if (!component.isAttached()) {
throw new IllegalStateException(String.format(
"Component %s is not attached. Client side "
+ "validation can only be disabled for a component "
+ "when it has been attached to the UI and because "
+ "it should be called again once the component is "
+ "removed/added, you should call this method from "
+ "the onAttach() method of the component.",
component.toString()));
}
// Wait until the response is being written as the validation state
// should not change after that
final StateNode componentNode = component.getElement().getNode();
componentNode.runWhenAttached(ui -> ui.getInternals().getStateTree()
.beforeClientResponse(componentNode,
executionContext -> overrideClientValidation(
component)));
}

private static void overrideClientValidation(Component component) {
PendingJavaScriptResult javaScriptResult = component.getElement()
.executeJs(
"this.validate = function () {return this.checkValidity();}");
private static void overrideClientValidation(DateTimePicker component) {
StringBuilder expression = new StringBuilder(
"this.validate = function () {return this.checkValidity();};");

javaScriptResult.then(result -> {
if (component instanceof HasValidation
&& ((HasValidation) component).isInvalid()) {
/*
* By default, the invalid flag is always false when a component
* is created. However, if the component is populated and
* validated in the same HTTP request, the server side state may
* have changed before the JavaScript disabling client side
* validation was properly executed. This can sometimes lead to
* a situation where the client side thinks the value is valid
* (before client side validation was disabled) and the server
* side thinks the value is invalid. This will lead to strange
* behavior until the two states are synchronized again. To
* avoid this, we will explicitly change the client side value
* if the server side is invalid.
*/
component.getElement().executeJs("this.invalid = true");
}
});
if (component.isInvalid()) {
/*
* By default the invalid flag is set to false. Workaround the case
* where the client side validation overrides the invalid state
* before the validation function itself is overridden above.
*/
expression.append("this.invalid = true;");
}
component.getElement().executeJs(expression.toString());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,34 +16,45 @@
package com.vaadin.flow.component.radiobutton;


import com.vaadin.flow.internal.StateNode;

class FieldValidationUtil {
private FieldValidationUtil() {

}

static <T> void disableClientValidation(RadioButtonGroup<T> component) {
// if the component was already attached override the validate()

component.addAttachListener(e -> overrideClientValidation(component));
// Since this method should be called for every time when the component
// is attached to the UI, lets check that it is actually so
if (!component.isAttached()) {
throw new IllegalStateException(String.format(
"Component %s is not attached. Client side validation "
+ "should be disabled when the component is "
+ "attached, thus this method needs to be called "
+ "from the onAttach method of the component.",
component.toString()));
}
// Wait until the response is being written as the validation state
// should not change after that
final StateNode componentNode = component.getElement().getNode();
componentNode.runWhenAttached(ui -> ui.getInternals().getStateTree()
.beforeClientResponse(componentNode,
executionContext -> overrideClientValidation(
component)));
}

private static <T> void overrideClientValidation(RadioButtonGroup<T> component) {
component.getElement()
.executeJs("this.validate = function () {" +
"return this.checkValidity();};");
StringBuilder expression = new StringBuilder(
"this.validate = function () {return this.checkValidity();};");

component.getUI().ifPresent(ui -> ui.beforeClientResponse(component, (e) -> {
if (component.isInvalid()) {
// By default, the invalid flag is always false when a component is created.
// However, if the component is populated and validated in the same HTTP request,
// the server side state may have changed before the JavaScript disabling client
// side validation was properly executed. This can sometimes lead to a situation
// where the client side thinks the value is valid (before client side validation
// was disabled) and the server side thinks the value is invalid. This will lead to
// strange behavior until the two states are synchronized again. To avoid this, we will
// explicitly change the client side value if the server side is invalid.
component.getElement().executeJs("this.invalid = true");
}
}));
if (component.isInvalid()) {
/*
* By default the invalid flag is set to false. Workaround the case
* where the client side validation overrides the invalid state
* before the validation function itself is overridden above.
*/
expression.append("this.invalid = true;");
}
component.getElement().executeJs(expression.toString());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,6 @@ public RadioButtonGroup() {
RadioButtonGroup::modelToPresentation, true);

registerValidation();
FieldValidationUtil.disableClientValidation(this);
}

@Override
Expand Down Expand Up @@ -247,6 +246,7 @@ protected void onAttach(AttachEvent attachEvent) {
if (getDataProvider() != null && dataProviderListenerRegistration == null) {
setupDataProviderListener(getDataProvider());
}
FieldValidationUtil.disableClientValidation(this);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import java.math.BigDecimal;
import java.util.Objects;

import com.vaadin.flow.component.AttachEvent;
import com.vaadin.flow.component.CompositionNotifier;
import com.vaadin.flow.component.HasHelper;
import com.vaadin.flow.component.HasSize;
Expand Down Expand Up @@ -105,8 +106,6 @@ public AbstractNumberField(SerializableFunction<String, T> parser,
setValueChangeMode(ValueChangeMode.ON_CHANGE);

addValueChangeListener(e -> validate());

FieldValidationUtil.disableClientValidation(this);
}

/**
Expand Down Expand Up @@ -427,4 +426,10 @@ public void setRequiredIndicatorVisible(boolean requiredIndicatorVisible) {
super.setRequiredIndicatorVisible(requiredIndicatorVisible);
this.required = requiredIndicatorVisible;
}

@Override
protected void onAttach(AttachEvent attachEvent) {
super.onAttach(attachEvent);
FieldValidationUtil.disableClientValidation(this);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import java.util.Objects;
import java.util.Optional;

import com.vaadin.flow.component.AttachEvent;
import com.vaadin.flow.component.CompositionNotifier;
import com.vaadin.flow.component.HasHelper;
import com.vaadin.flow.component.HasSize;
Expand Down Expand Up @@ -97,8 +98,6 @@ public BigDecimalField() {
setValueChangeMode(ValueChangeMode.ON_CHANGE);

addValueChangeListener(e -> validate());

FieldValidationUtil.disableClientValidation(this);
}

/**
Expand Down Expand Up @@ -449,4 +448,10 @@ private char getDecimalSeparator() {
String prop = getElement().getProperty("_decimalSeparator");
return prop == null || prop.isEmpty() ? '.' : getElement().getProperty("_decimalSeparator").charAt(0);
}

@Override
protected void onAttach(AttachEvent attachEvent) {
super.onAttach(attachEvent);
FieldValidationUtil.disableClientValidation(this);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package com.vaadin.flow.component.textfield;

import com.vaadin.flow.component.AttachEvent;
import com.vaadin.flow.component.CompositionNotifier;
import com.vaadin.flow.component.HasHelper;
import com.vaadin.flow.component.HasSize;
Expand Down Expand Up @@ -60,8 +61,6 @@ public EmailField() {
setValueChangeMode(ValueChangeMode.ON_CHANGE);

addValueChangeListener(e -> validate());

FieldValidationUtil.disableClientValidation(this);
}

/**
Expand Down Expand Up @@ -431,4 +430,10 @@ public void setRequiredIndicatorVisible(boolean requiredIndicatorVisible) {
protected void validate() {
setInvalid(getValidationSupport().isInvalid(getValue()));
}

@Override
protected void onAttach(AttachEvent attachEvent) {
super.onAttach(attachEvent);
FieldValidationUtil.disableClientValidation(this);
}
}
Loading

0 comments on commit eab7ca6

Please sign in to comment.