refactor: do not rely on _hasInputValue for detecting bad input (#6846)

Author: vursen

vaadin-date-picker-flow-parent/vaadin-date-picker-flow/src/main/java/com/vaadin/flow/component/datepicker/DatePicker.java

@@ -759,7 +759,7 @@ public void setValue(LocalDate value) {
         boolean isOldValueEmpty = valueEquals(oldValue, getEmptyValue());
         boolean isNewValueEmpty = valueEquals(value, getEmptyValue());
         boolean isValueRemainedEmpty = isOldValueEmpty && isNewValueEmpty;
-        boolean isInputValuePresent = isInputValuePresent();
+        String oldInputElementValue = getInputElementValue();
 
         // When the value is cleared programmatically, there is no change event
         // that would synchronize _inputElementValue, so we reset it ourselves
@@ -772,7 +772,7 @@ public void setValue(LocalDate value) {
 
         // Revalidate if setValue(null) didn't result in a value change but
         // cleared bad input
-        if (isValueRemainedEmpty && isInputValuePresent) {
+        if (isValueRemainedEmpty && !oldInputElementValue.isEmpty()) {
             validate();
             fireValidationStatusChangeEvent();
         }

vaadin-text-field-flow-parent/vaadin-text-field-flow/src/main/java/com/vaadin/flow/component/textfield/BigDecimalField.java

@@ -25,7 +25,6 @@
 import java.util.function.Function;
 
 import com.vaadin.flow.component.AttachEvent;
-import com.vaadin.flow.component.Synchronize;
 import com.vaadin.flow.component.Tag;
 import com.vaadin.flow.component.UI;
 import com.vaadin.flow.component.dependency.JsModule;
@@ -123,7 +122,7 @@ public class BigDecimalField extends TextFieldBase<BigDecimalField, BigDecimal>
         boolean fromComponent = context == null;
 
         boolean hasBadInput = valueEquals(value, getEmptyValue())
-                && isInputValuePresent();
+                && !getInputElementValue().isEmpty();
         if (hasBadInput) {
             return ValidationResult.error(getI18nErrorMessage(
                     BigDecimalFieldI18n::getBadInputErrorMessage));
@@ -311,27 +310,16 @@ public void setValue(BigDecimal value) {
         boolean isOldValueEmpty = valueEquals(oldValue, getEmptyValue());
         boolean isNewValueEmpty = valueEquals(value, getEmptyValue());
         boolean isValueRemainedEmpty = isOldValueEmpty && isNewValueEmpty;
-        boolean isInputValuePresent = isInputValuePresent();
-
-        // When the value is cleared programmatically, reset hasInputValue
-        // so that the following validation doesn't treat this as bad input.
-        if (isNewValueEmpty) {
-            getElement().setProperty("_hasInputValue", false);
-        }
+        String oldInputElementValue = getInputElementValue();
 
         super.setValue(value);
 
-        if (isValueRemainedEmpty && isInputValuePresent) {
-            // Clear the input element from possible bad input.
-            getElement().executeJs("this._inputElementValue = ''");
+        // Revalidate and clear input element value if setValue(null) didn't
+        // result in a value change but there was bad input.
+        if (isValueRemainedEmpty && !oldInputElementValue.isEmpty()) {
+            setInputElementValue("");
             validate();
             fireValidationStatusChangeEvent();
-        } else {
-            // Restore the input element's value in case it was cleared
-            // in the above branch. That can happen when setValue(null)
-            // and setValue(...) are subsequently called within one round-trip
-            // and there was bad input.
-            getElement().executeJs("this._inputElementValue = this.value");
         }
     }
 
@@ -402,15 +390,12 @@ private void fireValidationStatusChangeEvent() {
                 .forEach(listener -> listener.validationStatusChanged(event));
     }
 
-    /**
-     * Returns whether the input element has a value or not.
-     *
-     * @return <code>true</code> if the input element's value is populated,
-     *         <code>false</code> otherwise
-     */
-    @Synchronize(property = "_hasInputValue", value = "has-input-value-changed")
-    private boolean isInputValuePresent() {
-        return getElement().getProperty("_hasInputValue", false);
+    private String getInputElementValue() {
+        return getElement().getProperty("value", "");
+    }
+
+    private void setInputElementValue(String value) {
+        getElement().setProperty("value", value);
     }
 
     /**

vaadin-text-field-flow-parent/vaadin-text-field-flow/src/test/java/com/vaadin/flow/component/textfield/validation/BigDecimalFieldBasicValidationTest.java

@@ -15,13 +15,14 @@
  */
 package com.vaadin.flow.component.textfield.validation;
 
-import java.lang.reflect.Method;
 import java.math.BigDecimal;
 
 import org.junit.Assert;
 import org.junit.Test;
 
+import com.vaadin.flow.component.Component;
 import com.vaadin.flow.component.textfield.BigDecimalField;
+import com.vaadin.flow.dom.Element;
 import com.vaadin.tests.validation.AbstractBasicValidationTest;
 
 public class BigDecimalFieldBasicValidationTest
@@ -34,23 +35,21 @@ public void addValidationStatusChangeListener_addAnotherListenerOnInvocation_noE
         });
 
         // Trigger ValidationStatusChangeEvent
-        simulateBadInput();
+        fakeClientPropertyChange(testField, "value", "foo");
         testField.clear();
     }
 
     @Test
     public void badInput_validate_emptyErrorMessageDisplayed() {
-        testField.getElement().setProperty("_hasInputValue", true);
-        simulateBadInput();
+        fakeClientPropertyChange(testField, "value", "foo");
         Assert.assertEquals("", testField.getErrorMessage());
     }
 
     @Test
     public void badInput_setI18nErrorMessage_validate_i18nErrorMessageDisplayed() {
         testField.setI18n(new BigDecimalField.BigDecimalFieldI18n()
                 .setBadInputErrorMessage("Value has invalid format"));
-        testField.getElement().setProperty("_hasInputValue", true);
-        simulateBadInput();
+        fakeClientPropertyChange(testField, "value", "foo");
         Assert.assertEquals("Value has invalid format",
                 testField.getErrorMessage());
     }
@@ -104,16 +103,10 @@ protected BigDecimalField createTestField() {
         return new BigDecimalField();
     }
 
-    private void simulateBadInput() {
-        testField.getElement().setProperty("_hasInputValue", true);
-
-        try {
-            Method validateMethod = BigDecimalField.class
-                    .getDeclaredMethod("validate");
-            validateMethod.setAccessible(true);
-            validateMethod.invoke(testField);
-        } catch (Exception e) {
-            throw new RuntimeException(e);
-        }
+    private void fakeClientPropertyChange(Component component, String property,
+            String value) {
+        Element element = component.getElement();
+        element.getStateProvider().setProperty(element.getNode(), property,
+                value, false);
     }
 }

vaadin-time-picker-flow-parent/vaadin-time-picker-flow/src/main/java/com/vaadin/flow/component/timepicker/TimePicker.java

@@ -366,27 +366,20 @@ public void setValue(LocalTime value) {
         boolean isOldValueEmpty = valueEquals(oldValue, getEmptyValue());
         boolean isNewValueEmpty = valueEquals(value, getEmptyValue());
         boolean isValueRemainedEmpty = isOldValueEmpty && isNewValueEmpty;
-        boolean isInputValuePresent = isInputValuePresent();
+        String oldInputElementValue = getInputElementValue();
 
-        // When the value is cleared programmatically, reset hasInputValue
-        // so that the following validation doesn't treat this as bad input.
+        // When the value is cleared programmatically, there is no change event
+        // that would synchronize _inputElementValue, so we reset it ourselves
+        // to prevent the following validation from treating this as bad input.
         if (isNewValueEmpty) {
-            getElement().setProperty("_hasInputValue", false);
+            setInputElementValue("");
         }
 
         super.setValue(value);
 
-        // Clear the input element from possible bad input.
-        if (isValueRemainedEmpty && isInputValuePresent) {
-            // The check for value presence guarantees that a non-empty value
-            // won't get cleared when setValue(null) and setValue(...) are
-            // subsequently called within one round-trip.
-            // Flow only sends the final component value to the client
-            // when you update the value multiple times during a round-trip
-            // and the final value is sent in place of the first one, so
-            // `executeJs` can end up invoked after a non-empty value is set.
-            getElement()
-                    .executeJs("if (!this.value) this._inputElementValue = ''");
+        // Revalidate if setValue(null) didn't result in a value change but
+        // cleared bad input
+        if (isValueRemainedEmpty && !oldInputElementValue.isEmpty()) {
             validate();
             fireValidationStatusChangeEvent();
         }
@@ -453,9 +446,33 @@ private void fireValidationStatusChangeEvent() {
      * @return <code>true</code> if the input element's value is populated,
      *         <code>false</code> otherwise
      */
-    @Synchronize(property = "_hasInputValue", value = "has-input-value-changed")
     protected boolean isInputValuePresent() {
-        return getElement().getProperty("_hasInputValue", false);
+        return !getInputElementValue().isEmpty();
+    }
+
+    /**
+     * Gets the value of the input element. This value is updated on the server
+     * when the web component dispatches a `change` or `unparsable-change`
+     * event. Except when clearing the value, {@link #setValue(LocalTime)} does
+     * not update the input element value on the server because it requires date
+     * formatting, which is implemented on the web component's side.
+     *
+     * @return the value of the input element
+     */
+    @Synchronize(property = "_inputElementValue", value = { "change",
+            "unparsable-change" })
+    private String getInputElementValue() {
+        return getElement().getProperty("_inputElementValue", "");
+    }
+
+    /**
+     * Sets the value of the input element.
+     *
+     * @param value
+     *            the value to set
+     */
+    private void setInputElementValue(String value) {
+        getElement().setProperty("_inputElementValue", value);
     }
 
     /**

vaadin-time-picker-flow-parent/vaadin-time-picker-flow/src/test/java/com/vaadin/flow/component/timepicker/tests/validation/BasicValidationTest.java

@@ -20,8 +20,10 @@
 import org.junit.Assert;
 import org.junit.Test;
 
+import com.vaadin.flow.component.Component;
 import com.vaadin.flow.component.timepicker.TimePicker;
 import com.vaadin.flow.dom.DomEvent;
+import com.vaadin.flow.dom.Element;
 import com.vaadin.flow.internal.nodefeature.ElementListenerMap;
 import com.vaadin.tests.validation.AbstractBasicValidationTest;
 
@@ -37,23 +39,24 @@ public void addValidationStatusChangeListener_addAnotherListenerOnInvocation_noE
         });
 
         // Trigger ValidationStatusChangeEvent
-        testField.getElement().setProperty("_hasInputValue", true);
+        fakeClientPropertyChange(testField, "_inputElementValue", "foo");
         testField.clear();
     }
 
     @Test
     public void badInput_validate_emptyErrorMessageDisplayed() {
         testField.getElement().setProperty("_hasInputValue", true);
-        fireUnparsableChangeDomEvent();
+        fakeClientPropertyChange(testField, "_inputElementValue", "foo");
+        fakeClientDomEvent(testField, "unparsable-change");
         Assert.assertEquals("", testField.getErrorMessage());
     }
 
     @Test
     public void badInput_setI18nErrorMessage_validate_i18nErrorMessageDisplayed() {
         testField.setI18n(new TimePicker.TimePickerI18n()
                 .setBadInputErrorMessage("Time has invalid format"));
-        testField.getElement().setProperty("_hasInputValue", true);
-        fireUnparsableChangeDomEvent();
+        fakeClientPropertyChange(testField, "_inputElementValue", "foo");
+        fakeClientDomEvent(testField, "unparsable-change");
         Assert.assertEquals("Time has invalid format",
                 testField.getErrorMessage());
     }
@@ -138,10 +141,16 @@ protected TimePicker createTestField() {
         return new TimePicker();
     }
 
-    private void fireUnparsableChangeDomEvent() {
-        DomEvent unparsableChangeDomEvent = new DomEvent(testField.getElement(),
-                "unparsable-change", Json.createObject());
-        testField.getElement().getNode().getFeature(ElementListenerMap.class)
-                .fireEvent(unparsableChangeDomEvent);
+    private void fakeClientDomEvent(Component component, String eventName) {
+        Element element = component.getElement();
+        DomEvent event = new DomEvent(element, eventName, Json.createObject());
+        element.getNode().getFeature(ElementListenerMap.class).fireEvent(event);
+    }
+
+    private void fakeClientPropertyChange(Component component, String property,
+            String value) {
+        Element element = component.getElement();
+        element.getStateProvider().setProperty(element.getNode(), property,
+                value, false);
     }
 }