From 98b8761a9b87f7e683d84b56724473cf21461943 Mon Sep 17 00:00:00 2001 From: Vaadin Bot Date: Mon, 11 Nov 2024 16:47:47 +0100 Subject: [PATCH] fix: client side value binding logic (#20431) (#20443) Changes client side value binding logic so that if the user modifies the value during server round-trip, the value earlier sent to the server no longer overwrites user's changes once the round-trip finishes. Instead, user's changes are preserved. However, if the server-side value change handling logic actually changes the value and returns the new value to the client, that value will override any user input during round-trip. Fixes #20365 Co-authored-by: Teppo Kurki --- .../binding/SimpleElementBindingStrategy.java | 22 ++++++- .../client/flow/nodefeature/MapProperty.java | 32 +++++++++ .../uitest/ui/ClientSideValueChangeView.java | 62 ++++++++++++++++++ .../uitest/ui/ClientSideValueChangeIT.java | 65 +++++++++++++++++++ 4 files changed, 178 insertions(+), 3 deletions(-) create mode 100644 flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/ClientSideValueChangeView.java create mode 100644 flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/ClientSideValueChangeIT.java diff --git a/flow-client/src/main/java/com/vaadin/client/flow/binding/SimpleElementBindingStrategy.java b/flow-client/src/main/java/com/vaadin/client/flow/binding/SimpleElementBindingStrategy.java index 31c5a8e925c..bf5b876ff22 100644 --- a/flow-client/src/main/java/com/vaadin/client/flow/binding/SimpleElementBindingStrategy.java +++ b/flow-client/src/main/java/com/vaadin/client/flow/binding/SimpleElementBindingStrategy.java @@ -15,7 +15,7 @@ */ package com.vaadin.client.flow.binding; -import java.util.function.BiConsumer; +import java.util.Optional; import java.util.function.Consumer; import java.util.function.Supplier; @@ -737,11 +737,20 @@ private void updateProperty(MapProperty mapProperty, Element element) { if (mapProperty.hasValue()) { Object treeValue = mapProperty.getValue(); Object domValue = WidgetUtil.getJsProperty(element, name); + Optional previousDomValue = mapProperty + .getPreviousDomValue(); + + // User might have modified DOM value during server round-trip. + // That is why we only want to update to the tree value if the tree + // value is different from the pre-server-round-trip DOM value. + boolean updateToTreeValue = previousDomValue + .map(o -> !WidgetUtil.equals(treeValue, o)).orElse(true); + // We compare with the current property to avoid setting properties // which are updated on the client side, e.g. when synchronizing // properties to the server (won't work for readonly properties). - if (WidgetUtil.isUndefined(domValue) - || !WidgetUtil.equals(domValue, treeValue)) { + if (updateToTreeValue && (WidgetUtil.isUndefined(domValue) + || !WidgetUtil.equals(domValue, treeValue))) { Reactive.runWithComputation(null, () -> WidgetUtil.setJsProperty(element, name, PolymerUtils.createModelTree(treeValue))); @@ -753,6 +762,7 @@ private void updateProperty(MapProperty mapProperty, Element element) { // the value WidgetUtil.setJsProperty(element, name, null); } + mapProperty.clearPreviousDomValue(); } private void updateStyleProperty(MapProperty mapProperty, Element element) { @@ -1317,6 +1327,12 @@ private void handleDomEvent(Event event, BindingContext context) { eventData.put(expressionString, expressionValue); } } + synchronizeProperties.forEach(name -> { + NodeMap map = node.getMap(NodeFeatures.ELEMENT_PROPERTIES); + MapProperty mapProperty = map.getProperty(name); + Object domValue = WidgetUtil.getJsProperty(element, name); + mapProperty.setPreviousDomValue(domValue); + }); JsMap commands = JsCollections.map(); synchronizeProperties.forEach(name -> commands.set(name, diff --git a/flow-client/src/main/java/com/vaadin/client/flow/nodefeature/MapProperty.java b/flow-client/src/main/java/com/vaadin/client/flow/nodefeature/MapProperty.java index 67404d07d9d..4550391f6b0 100644 --- a/flow-client/src/main/java/com/vaadin/client/flow/nodefeature/MapProperty.java +++ b/flow-client/src/main/java/com/vaadin/client/flow/nodefeature/MapProperty.java @@ -17,6 +17,7 @@ import java.util.Map; import java.util.Objects; +import java.util.Optional; import com.vaadin.client.flow.StateNode; import com.vaadin.client.flow.StateTree; @@ -65,6 +66,7 @@ protected void dispatchEvent(MapPropertyChangeListener listener, private boolean hasValue = false; private final boolean forceValueUpdate; + private Optional previousDomValue = Optional.empty(); /** * Creates a new property. @@ -333,4 +335,34 @@ public Runnable getSyncToServerCommand(Object newValue) { } return NO_OP; } + + /** + * Stores previous DOM value of this property for detection of value + * modification by the user during the server round-trip. + * + * @param previousDomValue + * DOM value of property prior to server round-trip start. Can be + * null; + */ + public void setPreviousDomValue(Object previousDomValue) { + this.previousDomValue = Optional.ofNullable(previousDomValue); + } + + /** + * Returns previous DOM value of this property for detection of value + * modification by the user during the server round-trip. + * + * @return Optional of previous DOM value. Empty optional is returned if + * previous value has not been stored. + */ + public Optional getPreviousDomValue() { + return previousDomValue; + } + + /** + * Clears the previous DOM value of this property. + */ + public void clearPreviousDomValue() { + this.previousDomValue = Optional.empty(); + } } diff --git a/flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/ClientSideValueChangeView.java b/flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/ClientSideValueChangeView.java new file mode 100644 index 00000000000..c5660c95de5 --- /dev/null +++ b/flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/ClientSideValueChangeView.java @@ -0,0 +1,62 @@ +/* + * Copyright 2000-2024 Vaadin Ltd. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ +package com.vaadin.flow.uitest.ui; + +import com.vaadin.flow.component.html.Input; +import com.vaadin.flow.component.html.Span; +import com.vaadin.flow.router.Route; +import com.vaadin.flow.uitest.servlet.ViewTestLayout; + +@Route(value = "com.vaadin.flow.uitest.ui.ClientSideValueChangeView", layout = ViewTestLayout.class) +public class ClientSideValueChangeView extends AbstractDivView { + + @Override + protected void onShow() { + Input input = new Input(); + input.setId("inputfield"); + + input.addValueChangeListener(e -> { + try { + Thread.sleep(1000); + } catch (InterruptedException ex) { + throw new RuntimeException(ex); + } + Span span = new Span("done"); + span.setId("status"); + add(span); + }); + + add(input); + + Input input2 = new Input(); + input2.setId("inputfieldserversetsvalue"); + + input2.addValueChangeListener(e -> { + try { + Thread.sleep(1000); + } catch (InterruptedException ex) { + throw new RuntimeException(ex); + } + input2.setValue("fromserver"); + Span span = new Span("done"); + span.setId("statusserversetsvalue"); + add(span); + }); + + add(input2); + } + +} diff --git a/flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/ClientSideValueChangeIT.java b/flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/ClientSideValueChangeIT.java new file mode 100644 index 00000000000..22dc2814a66 --- /dev/null +++ b/flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/ClientSideValueChangeIT.java @@ -0,0 +1,65 @@ +/* + * Copyright 2000-2024 Vaadin Ltd. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ +package com.vaadin.flow.uitest.ui; + +import org.junit.Assert; +import org.junit.Test; +import org.openqa.selenium.By; +import org.openqa.selenium.support.ui.ExpectedConditions; + +import com.vaadin.flow.component.html.testbench.InputTextElement; +import com.vaadin.flow.testutil.ChromeBrowserTest; + +public class ClientSideValueChangeIT extends ChromeBrowserTest { + + @Test + public void clientSideValueEntryDuringRoundTrip_enteredValueShouldNotBeOverridden() { + open(); + + getCommandExecutor().disableWaitForVaadin(); + + InputTextElement input = $(InputTextElement.class).id("inputfield"); + input.setValue("abc"); + input.sendKeys("123"); + + waitUntil(ExpectedConditions.presenceOfElementLocated(By.id("status"))); + + Assert.assertEquals( + "User input during round-trip was unexpectedly overridden", + "abc123", + $(InputTextElement.class).id("inputfield").getValue()); + } + + @Test + public void clientSideValueEntryDuringRoundTrip_serverChangesValue_serverValueShouldBeUsed() { + open(); + + getCommandExecutor().disableWaitForVaadin(); + + InputTextElement input = $(InputTextElement.class) + .id("inputfieldserversetsvalue"); + input.setValue("abc"); + input.sendKeys("123"); + + waitUntil(ExpectedConditions + .presenceOfElementLocated(By.id("statusserversetsvalue"))); + + Assert.assertEquals( + "Value set by server during round-trip was unexpectedly overridden", + "fromserver", $(InputTextElement.class) + .id("inputfieldserversetsvalue").getValue()); + } +}