Fix RadioButtonGroup selection updates to client (#9749)

This patch provides a simple fix for the majority of issues. There are still issues that should be fixes by refactoring parts of the logic in AbstractSingleSelect.

This patch does not unify the handling of empty values in the TestBench elements of various AbstractSingleSelects.

Fixes #9494

9 files changed, 266 insertions, 33 deletions

diff --git a/client/src/main/java/com/vaadin/client/ui/VRadioButtonGroup.java b/client/src/main/java/com/vaadin/client/ui/VRadioButtonGroup.java
index 7cfaf7572e..1386005351 100644
--- a/client/src/main/java/com/vaadin/client/ui/VRadioButtonGroup.java
+++ b/client/src/main/java/com/vaadin/client/ui/VRadioButtonGroup.java
@@ -227,9 +227,13 @@ public class VRadioButtonGroup extends FocusableFlowPanelComposite
     }
 
     public void selectItemKey(String selectedItemKey) {
-        RadioButton radioButton = keyToOptions.get(selectedItemKey);
-        if (radioButton != null) {// Items might not be loaded yet
-            radioButton.setValue(true);
+        if (selectedItemKey != null) {
+            RadioButton radioButton = keyToOptions.get(selectedItemKey);
+            if (radioButton != null) { // Items might not be loaded yet
+                radioButton.setValue(true);
+            }
+        } else {
+            keyToOptions.values().forEach(button -> button.setValue(false));
         }
     }
 }
diff --git a/server/src/main/java/com/vaadin/ui/AbstractSingleSelect.java b/server/src/main/java/com/vaadin/ui/AbstractSingleSelect.java
index 2b0e6b7d01..f9eed7fef3 100644
--- a/server/src/main/java/com/vaadin/ui/AbstractSingleSelect.java
+++ b/server/src/main/java/com/vaadin/ui/AbstractSingleSelect.java
@@ -230,10 +230,8 @@ public abstract class AbstractSingleSelect<T> extends AbstractListing<T>
         T oldSelection = getSelectedItem().orElse(getEmptyValue());
         doSetSelectedKey(key);
 
-        // Update diffstate so that a change will be sent to the client if the
-        // selection is changed to its original value
-        updateDiffstate("selectedItemKey",
-                key == null ? Json.createNull() : Json.create(key));
+        // Set diffstate to something that will always send selection to client
+        updateDiffstate("selectedItemKey", Json.createObject());
 
         fireEvent(new SingleSelectionEvent<>(AbstractSingleSelect.this,
                 oldSelection, true));
diff --git a/server/src/main/java/com/vaadin/ui/ComboBox.java b/server/src/main/java/com/vaadin/ui/ComboBox.java
index 73d9ad84cf..af100e03f1 100644
--- a/server/src/main/java/com/vaadin/ui/ComboBox.java
+++ b/server/src/main/java/com/vaadin/ui/ComboBox.java
@@ -25,11 +25,11 @@ import java.util.Objects;
 import java.util.Set;
 import java.util.stream.Stream;
 
-import com.vaadin.data.ValueProvider;
 import org.jsoup.nodes.Element;
 
 import com.vaadin.data.HasFilterableDataProvider;
 import com.vaadin.data.HasValue;
+import com.vaadin.data.ValueProvider;
 import com.vaadin.data.provider.CallbackDataProvider;
 import com.vaadin.data.provider.DataCommunicator;
 import com.vaadin.data.provider.DataKeyMapper;
@@ -57,7 +57,6 @@ import com.vaadin.ui.declarative.DesignAttributeHandler;
 import com.vaadin.ui.declarative.DesignContext;
 import com.vaadin.ui.declarative.DesignFormatter;
 
-import elemental.json.Json;
 import elemental.json.JsonObject;
 
 /**
@@ -189,7 +188,8 @@ public class ComboBox<T> extends AbstractSingleSelect<T>
     public ComboBox() {
         super(new DataCommunicator<T>() {
             @Override
-            protected DataKeyMapper<T> createKeyMapper(ValueProvider<T,Object> identifierGetter) {
+            protected DataKeyMapper<T> createKeyMapper(
+                    ValueProvider<T, Object> identifierGetter) {
                 return new KeyMapper<T>(identifierGetter) {
                     @Override
                     public void remove(T removeobj) {
@@ -831,23 +831,6 @@ public class ComboBox<T> extends AbstractSingleSelect<T>
                 q -> sizeCallback.applyAsInt(q.getFilter().orElse(""))));
     }
 
-    @Override
-    protected void setSelectedFromClient(String key) {
-        super.setSelectedFromClient(key);
-
-        /*
-         * The client side for combo box always expects a state change for
-         * selectedItemKey after it has sent a selection change. This means that
-         * we must store a value in the diffstate that guarantees that a new
-         * value will be sent, regardless of what the value actually is at the
-         * time when changes are sent.
-         *
-         * Keys are always strings (or null), so using a non-string type will
-         * always trigger a diff mismatch and a resend.
-         */
-        updateDiffstate("selectedItemKey", Json.create(0));
-    }
-
     /**
      * Predicate to check {@link ComboBox} item captions against user typed
      * strings.
diff --git a/testbench-api/src/main/java/com/vaadin/testbench/elements/AbstractSingleSelectElement.java b/testbench-api/src/main/java/com/vaadin/testbench/elements/AbstractSingleSelectElement.java
new file mode 100644
index 0000000000..c66e736a1d
--- /dev/null
+++ b/testbench-api/src/main/java/com/vaadin/testbench/elements/AbstractSingleSelectElement.java
@@ -0,0 +1,47 @@
+/*
+ * Copyright 2000-2016 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.testbench.elements;
+
+import com.vaadin.testbench.elementsbase.ServerClass;
+
+/**
+ * A common base element class for all single select components.
+ * 
+ * @since 8.1.1
+ */
+@ServerClass("com.vaadin.ui.AbstractSingleSelect")
+public abstract class AbstractSingleSelectElement
+        extends AbstractSelectElement {
+
+    /**
+     * Selects the first option in this single select component that matches the
+     * given text.
+     * 
+     * @param text
+     *            the text to select by
+     */
+    public abstract void selectByText(String text);
+
+    /**
+     * Return value of this single select component.
+     * <p>
+     * <strong>Note:</strong> If there is no value selected the behavior of
+     * subclasses varies. Pay attention on the actual implementation.
+     *
+     * @return the value
+     */
+    public abstract String getValue();
+}
diff --git a/testbench-api/src/main/java/com/vaadin/testbench/elements/ComboBoxElement.java b/testbench-api/src/main/java/com/vaadin/testbench/elements/ComboBoxElement.java
index 11b823e656..2eaef4f015 100644
--- a/testbench-api/src/main/java/com/vaadin/testbench/elements/ComboBoxElement.java
+++ b/testbench-api/src/main/java/com/vaadin/testbench/elements/ComboBoxElement.java
@@ -28,7 +28,7 @@ import com.vaadin.testbench.TestBenchElement;
 import com.vaadin.testbench.elementsbase.ServerClass;
 
 @ServerClass("com.vaadin.ui.ComboBox")
-public class ComboBoxElement extends AbstractSelectElement {
+public class ComboBoxElement extends AbstractSingleSelectElement {
 
     private static org.openqa.selenium.By bySuggestionPopup = By
             .vaadin("#popup");
@@ -199,7 +199,7 @@ public class ComboBoxElement extends AbstractSelectElement {
     }
 
     /**
-     * Return value of the combo box element
+     * Return value of the combo box element.
      *
      * @return value of the combo box element
      */
diff --git a/testbench-api/src/main/java/com/vaadin/testbench/elements/NativeSelectElement.java b/testbench-api/src/main/java/com/vaadin/testbench/elements/NativeSelectElement.java
index d932af1966..f6b0426c3e 100644
--- a/testbench-api/src/main/java/com/vaadin/testbench/elements/NativeSelectElement.java
+++ b/testbench-api/src/main/java/com/vaadin/testbench/elements/NativeSelectElement.java
@@ -17,6 +17,7 @@ package com.vaadin.testbench.elements;
 
 import java.util.List;
 
+import org.openqa.selenium.NoSuchElementException;
 import org.openqa.selenium.WebElement;
 import org.openqa.selenium.support.ui.Select;
 
@@ -25,7 +26,7 @@ import com.vaadin.testbench.TestBenchElement;
 import com.vaadin.testbench.elementsbase.ServerClass;
 
 @ServerClass("com.vaadin.ui.NativeSelect")
-public class NativeSelectElement extends AbstractSelectElement {
+public class NativeSelectElement extends AbstractSingleSelectElement {
     private Select select;
     private WebElement selectElement;
 
@@ -70,9 +71,13 @@ public class NativeSelectElement extends AbstractSelectElement {
     /**
      * Return value of the selected item in the native select element.
      *
-     * @return value of the selected item in the native select element
+     * @return value of the selected item
+     * 
+     * @throws NoSuchElementException
+     *             if no value is selected
      */
-    public String getValue() {
+
+    public String getValue() throws NoSuchElementException {
         return select.getFirstSelectedOption().getText();
     }
 
diff --git a/testbench-api/src/main/java/com/vaadin/testbench/elements/RadioButtonGroupElement.java b/testbench-api/src/main/java/com/vaadin/testbench/elements/RadioButtonGroupElement.java
index 4feeaae6ff..4cff189ea7 100644
--- a/testbench-api/src/main/java/com/vaadin/testbench/elements/RadioButtonGroupElement.java
+++ b/testbench-api/src/main/java/com/vaadin/testbench/elements/RadioButtonGroupElement.java
@@ -25,7 +25,7 @@ import com.vaadin.testbench.TestBenchElement;
 import com.vaadin.testbench.elementsbase.ServerClass;
 
 @ServerClass("com.vaadin.ui.RadioButtonGroup")
-public class RadioButtonGroupElement extends AbstractSelectElement {
+public class RadioButtonGroupElement extends AbstractSingleSelectElement {
 
     private static org.openqa.selenium.By bySelectOption = By
             .className("v-select-option");
diff --git a/uitest/src/main/java/com/vaadin/tests/components/abstractsingleselect/AbstractSingleSelection.java b/uitest/src/main/java/com/vaadin/tests/components/abstractsingleselect/AbstractSingleSelection.java
new file mode 100644
index 0000000000..55f61b4134
--- /dev/null
+++ b/uitest/src/main/java/com/vaadin/tests/components/abstractsingleselect/AbstractSingleSelection.java
@@ -0,0 +1,68 @@
+package com.vaadin.tests.components.abstractsingleselect;
+
+import com.vaadin.annotations.Widgetset;
+import com.vaadin.data.provider.Query;
+import com.vaadin.server.VaadinRequest;
+import com.vaadin.tests.components.AbstractTestUI;
+import com.vaadin.ui.AbstractSingleSelect;
+import com.vaadin.ui.Button;
+import com.vaadin.ui.ComboBox;
+import com.vaadin.ui.NativeSelect;
+import com.vaadin.ui.RadioButtonGroup;
+
+@Widgetset("com.vaadin.DefaultWidgetSet")
+@SuppressWarnings({ "unchecked", "rawtypes" })
+public class AbstractSingleSelection extends AbstractTestUI {
+
+    /* Initial placeholder component */
+    AbstractSingleSelect<String> component = new ComboBox<>();
+
+    @Override
+    protected void setup(VaadinRequest request) {
+        NativeSelect<Class<? extends AbstractSingleSelect>> componentSelect = new NativeSelect<>();
+        componentSelect.setItems(RadioButtonGroup.class, NativeSelect.class,
+                ComboBox.class);
+        componentSelect.setItemCaptionGenerator(Class::getSimpleName);
+
+        componentSelect.setEmptySelectionAllowed(false);
+        componentSelect.addValueChangeListener(singleSelectClass -> {
+            createComponent(singleSelectClass.getValue());
+        });
+
+        addComponent(componentSelect);
+        addComponent(component); // This will be replaced in createComponent
+        addComponent(
+                new Button("Deselect", e -> component.setSelectedItem(null)));
+        addComponent(new Button("Select Bar",
+                e -> component.setSelectedItem("Bar")));
+        addComponent(new Button("Refresh",
+                e -> component.getDataProvider().refreshAll()));
+
+        // Select a value from native select to create the initial component
+        componentSelect.getDataProvider().fetch(new Query<>()).findFirst()
+                .ifPresent(componentSelect::setSelectedItem);
+    }
+
+    private void createComponent(
+            Class<? extends AbstractSingleSelect> singleSelectClass) {
+        try {
+            AbstractSingleSelect<String> select = singleSelectClass
+                    .newInstance();
+            select.setItems("Foo", "Bar", "Baz", "Reset");
+            select.setSelectedItem("Bar");
+
+            select.addValueChangeListener(e -> {
+                if ("Reset".equals(e.getValue())) {
+                    select.setSelectedItem("Bar");
+                }
+            });
+
+            select.setId("testComponent");
+
+            replaceComponent(component, select);
+            component = select;
+        } catch (InstantiationException | IllegalAccessException e1) {
+            throw new RuntimeException("Component creation failed", e1);
+        }
+    }
+}
diff --git a/uitest/src/test/java/com/vaadin/tests/components/abstractsingleselect/AbstractSingleSelectionTest.java b/uitest/src/test/java/com/vaadin/tests/components/abstractsingleselect/AbstractSingleSelectionTest.java
new file mode 100644
index 0000000000..4623dac714
--- /dev/null
+++ b/uitest/src/test/java/com/vaadin/tests/components/abstractsingleselect/AbstractSingleSelectionTest.java
@@ -0,0 +1,128 @@
+package com.vaadin.tests.components.abstractsingleselect;
+
+import java.util.Collection;
+import java.util.LinkedHashMap;
+import java.util.Map;
+
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized.Parameters;
+import org.openqa.selenium.NoSuchElementException;
+
+import com.vaadin.testbench.elements.AbstractSingleSelectElement;
+import com.vaadin.testbench.elements.ButtonElement;
+import com.vaadin.testbench.elements.ComboBoxElement;
+import com.vaadin.testbench.elements.NativeSelectElement;
+import com.vaadin.testbench.elements.RadioButtonGroupElement;
+import com.vaadin.tests.tb3.ParameterizedTB3Runner;
+import com.vaadin.tests.tb3.SingleBrowserTest;
+
+@RunWith(ParameterizedTB3Runner.class)
+public class AbstractSingleSelectionTest extends SingleBrowserTest {
+
+    private static final Map<String, Class<? extends AbstractSingleSelectElement>> elementClasses = new LinkedHashMap<>();
+
+    @Parameters
+    public static Collection<String> getElementClassNames() {
+        if (elementClasses.isEmpty()) {
+            elementClasses.put("RadioButtonGroup",
+                    RadioButtonGroupElement.class);
+            elementClasses.put("NativeSelect", NativeSelectElement.class);
+            elementClasses.put("ComboBox", ComboBoxElement.class);
+        }
+
+        return elementClasses.keySet();
+    }
+
+    private String elementClassName;
+
+    public void setElementClassName(String elementClassName) {
+        this.elementClassName = elementClassName;
+    }
+
+    @Before
+    public void before() {
+        openTestURL();
+    }
+
+    @Test
+    public void testSelectNull() {
+        $(NativeSelectElement.class).first().selectByText(elementClassName);
+
+        assertInitial();
+
+        $(ButtonElement.class).caption("Deselect").first().click();
+
+        AbstractSingleSelectElement selectElement = getSelectElement();
+        // TODO: TB API behavior should be unified.
+        if (selectElement instanceof RadioButtonGroupElement) {
+            Assert.assertNull("No value should be selected",
+                    selectElement.getValue());
+        } else if (selectElement instanceof ComboBoxElement) {
+            Assert.assertTrue("No value should be selected",
+                    selectElement.getValue().isEmpty());
+        } else {
+            // NativeSelectElement throws if no value is selected.
+            try {
+                selectElement.getValue();
+                Assert.fail("No value should be selected");
+            } catch (NoSuchElementException e) {
+                // All is fine.
+            }
+        }
+    }
+
+    @Test
+    public void testSelectOnClientAndRefresh() {
+        $(NativeSelectElement.class).first().selectByText(elementClassName);
+
+        assertInitial();
+
+        AbstractSingleSelectElement select = getSelectElement();
+        select.selectByText("Baz");
+        Assert.assertEquals("Value should change", "Baz", select.getValue());
+
+        $(ButtonElement.class).caption("Refresh").first().click();
+        Assert.assertEquals("Value should stay the same through refreshAll",
+                "Baz", select.getValue());
+    }
+
+    @Test
+    public void testSelectOnClientAndResetValueOnServer() {
+        $(NativeSelectElement.class).first().selectByText(elementClassName);
+
+        assertInitial();
+
+        AbstractSingleSelectElement select = getSelectElement();
+        select.selectByText("Baz");
+        Assert.assertEquals("Value should change", "Baz", select.getValue());
+
+        $(ButtonElement.class).caption("Select Bar").first().click();
+        Assert.assertEquals("Original value should be selected again", "Bar",
+                select.getValue());
+    }
+
+    @Test
+    public void testSelectOnClientAndResetValueOnServerInListener() {
+        $(NativeSelectElement.class).first().selectByText(elementClassName);
+
+        assertInitial();
+
+        AbstractSingleSelectElement rbg = getSelectElement();
+        rbg.selectByText("Reset");
+        // Selecting "Reset" selects "Bar" on server. Value was initially "Bar"
+        Assert.assertEquals("Original value should be selected again", "Bar",
+                rbg.getValue());
+    }
+
+    private void assertInitial() {
+        Assert.assertEquals("Initial state unexpected", "Bar",
+                getSelectElement().getValue());
+    }
+
+    private AbstractSingleSelectElement getSelectElement() {
+        return $(elementClasses.get(elementClassName)).id("testComponent");
+    }
+}