Fix OptionGroup elements losing focus on value change (#10451)

The misbehavior was caused by VOptionGroup.buildOptions recreating
associated panel on every change by removing and adding new elements.
With this fix applied it tries to update existing elements,
distinguishing them by assigned keys. It will recreate panel though if
elements are reordered or new elements were added/removed.

Change-Id: I1245b2ff30ce1932614c1eac37bd0131cbd29dd7

3 files changed, 174 insertions, 17 deletions

diff --git a/client/src/com/vaadin/client/ui/VOptionGroup.java b/client/src/com/vaadin/client/ui/VOptionGroup.java
index a4c8b6733c..455c7669f5 100644
--- a/client/src/com/vaadin/client/ui/VOptionGroup.java
+++ b/client/src/com/vaadin/client/ui/VOptionGroup.java
@@ -95,11 +95,29 @@ public class VOptionGroup extends VOptionGroupBase implements FocusHandler,
     }
 
     /*
-     * Return true if no elements were changed, false otherwise.
+     * Try to update content of existing elements, rebuild panel entirely
+     * otherwise
      */
     @Override
     public void buildOptions(UIDL uidl) {
-        panel.clear();
+        /*
+         * In order to retain focus, we need to update values rather than
+         * recreate panel from scratch (#10451). However, the panel will be
+         * rebuilt (losing focus) if number of elements or their order is
+         * changed.
+         */
+        HashMap<String, CheckBox> keysToOptions = new HashMap<String, CheckBox>();
+        for (Map.Entry<CheckBox, String> entry : optionsToKeys.entrySet()) {
+            keysToOptions.put(entry.getValue(), entry.getKey());
+        }
+        ArrayList<Widget> existingwidgets = new ArrayList<Widget>();
+        ArrayList<Widget> newwidgets = new ArrayList<Widget>();
+
+        // Get current order of elements
+        for (Widget wid : panel) {
+            existingwidgets.add(wid);
+        }
+
         optionsEnabled.clear();
 
         if (isMultiselect()) {
@@ -110,7 +128,6 @@ public class VOptionGroup extends VOptionGroupBase implements FocusHandler,
 
         for (final Iterator<?> it = uidl.getChildIterator(); it.hasNext();) {
             final UIDL opUidl = (UIDL) it.next();
-            CheckBox op;
 
             String itemHtml = opUidl.getStringAttribute("caption");
             if (!htmlContentAllowed) {
@@ -124,32 +141,48 @@ public class VOptionGroup extends VOptionGroupBase implements FocusHandler,
                         + Icon.CLASSNAME + "\" alt=\"\" />" + itemHtml;
             }
 
-            if (isMultiselect()) {
-                op = new VCheckBox();
-                op.setHTML(itemHtml);
-            } else {
-                op = new RadioButton(paintableId, itemHtml, true);
-                op.setStyleName("v-radiobutton");
-            }
+            String key = opUidl.getStringAttribute("key");
+            CheckBox op = keysToOptions.get(key);
+            if (op == null) {
+                // Create a new element
+                if (isMultiselect()) {
+                    op = new VCheckBox();
+                } else {
+                    op = new RadioButton(paintableId);
+                    op.setStyleName("v-radiobutton");
+                }
+                if (icon != null && icon.length() != 0) {
+                    Util.sinkOnloadForImages(op.getElement());
+                    op.addHandler(iconLoadHandler, LoadEvent.getType());
+                }
 
-            if (icon != null && icon.length() != 0) {
-                Util.sinkOnloadForImages(op.getElement());
-                op.addHandler(iconLoadHandler, LoadEvent.getType());
+                op.addStyleName(CLASSNAME_OPTION);
+                op.addClickHandler(this);
+
+                optionsToKeys.put(op, key);
             }
 
-            op.addStyleName(CLASSNAME_OPTION);
+            op.setHTML(itemHtml);
             op.setValue(opUidl.getBooleanAttribute("selected"));
             boolean optionEnabled = !opUidl
                     .getBooleanAttribute(OptionGroupConstants.ATTRIBUTE_OPTION_DISABLED);
             boolean enabled = optionEnabled && !isReadonly() && isEnabled();
             op.setEnabled(enabled);
             optionsEnabled.add(optionEnabled);
+
             setStyleName(op.getElement(),
                     ApplicationConnection.DISABLED_CLASSNAME,
                     !(optionEnabled && isEnabled()));
-            op.addClickHandler(this);
-            optionsToKeys.put(op, opUidl.getStringAttribute("key"));
-            panel.add(op);
+
+            newwidgets.add(op);
+        }
+
+        if (!newwidgets.equals(existingwidgets)) {
+            // Rebuild the panel, losing focus
+            panel.clear();
+            for (Widget wid : newwidgets) {
+                panel.add(wid);
+            }
         }
     }
 
diff --git a/uitest/src/com/vaadin/tests/components/optiongroup/OptionGroupRetainFocusKeyboardValueChange.html b/uitest/src/com/vaadin/tests/components/optiongroup/OptionGroupRetainFocusKeyboardValueChange.html
new file mode 100644
index 0000000000..046cac0e30
--- /dev/null
+++ b/uitest/src/com/vaadin/tests/components/optiongroup/OptionGroupRetainFocusKeyboardValueChange.html
@@ -0,0 +1,54 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
+<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
+<head profile="http://selenium-ide.openqa.org/profiles/test-case">
+<meta http-equiv="Content-Type" content="text/html; charset=UTF-8" />
+<link rel="selenium.base" href="" />
+<title>OptionGroupRetainFocusKeyboardValueChange</title>
+</head>
+<body>
+<table cellpadding="1" cellspacing="1" border="1">
+<thead>
+<tr><td rowspan="1" colspan="3">OptionGroupRetainFocusKeyboardValueChange</td></tr>
+</thead><tbody>
+<tr>
+	<td>open</td>
+	<td>/run/OptionGroupRetainFocusKeyboardValueChange?restartApplication</td>
+	<td></td>
+</tr>
+<tr>
+	<td>pressSpecialKey</td>
+	<td>vaadin=runOptionGroupRetainFocusKeyboardValueChange::/VVerticalLayout[0]/Slot[1]/VVerticalLayout[0]/Slot[0]/VOptionGroup[0]/domChild[0]/domChild[0]</td>
+	<td>space</td>
+</tr>
+<!-- The element 'A' should be selected and focused -->
+<tr>
+	<td>screenCapture</td>
+	<td></td>
+	<td></td>
+</tr>
+<tr>
+	<td>pressSpecialKey</td>
+	<td>vaadin=runOptionGroupRetainFocusKeyboardValueChange::/VVerticalLayout[0]/Slot[1]/VVerticalLayout[0]/Slot[0]/VOptionGroup[0]/domChild[0]/domChild[0]</td>
+	<td>down</td>
+</tr>
+<!-- The element 'B' should be selected and focused, the caption of first element is changed from 'A' to 'A+' -->
+<tr>
+	<td>screenCapture</td>
+	<td></td>
+	<td></td>
+</tr>
+<tr>
+	<td>pressSpecialKey</td>
+	<td>vaadin=runOptionGroupRetainFocusKeyboardValueChange::/VVerticalLayout[0]/Slot[1]/VVerticalLayout[0]/Slot[0]/VOptionGroup[0]/domChild[1]/domChild[0]</td>
+	<td>down</td>
+</tr>
+<!-- Elements 'B' and 'C' should be swapped; 'C' selected but not focused  -->
+<tr>
+	<td>screenCapture</td>
+	<td></td>
+	<td></td>
+</tr>
+</tbody></table>
+</body>
+</html>
diff --git a/uitest/src/com/vaadin/tests/components/optiongroup/OptionGroupRetainFocusKeyboardValueChange.java b/uitest/src/com/vaadin/tests/components/optiongroup/OptionGroupRetainFocusKeyboardValueChange.java
new file mode 100644
index 0000000000..570a300cb3
--- /dev/null
+++ b/uitest/src/com/vaadin/tests/components/optiongroup/OptionGroupRetainFocusKeyboardValueChange.java
@@ -0,0 +1,70 @@
+/*
+ * Copyright 2000-2013 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.tests.components.optiongroup;
+
+import com.vaadin.data.Property.ValueChangeEvent;
+import com.vaadin.data.Property.ValueChangeListener;
+import com.vaadin.server.VaadinRequest;
+import com.vaadin.tests.components.AbstractTestUI;
+import com.vaadin.ui.OptionGroup;
+
+/**
+ * Testcase for #10451
+ * 
+ * @author Vaadin Ltd
+ */
+public class OptionGroupRetainFocusKeyboardValueChange extends AbstractTestUI {
+
+    @Override
+    protected void setup(VaadinRequest request) {
+        final OptionGroup optiongroup = new OptionGroup();
+        optiongroup.addItem(1);
+        optiongroup.addItem(2);
+        optiongroup.addItem(3);
+        optiongroup.setItemCaption(1, "A");
+        optiongroup.setItemCaption(2, "B");
+        optiongroup.setItemCaption(3, "C");
+        optiongroup.setImmediate(true);
+
+        optiongroup.addValueChangeListener(new ValueChangeListener() {
+
+            @Override
+            public void valueChange(ValueChangeEvent event) {
+                if (optiongroup.isSelected(2)) {
+                    optiongroup.setItemCaption(1, "A+");
+                } else if (optiongroup.isSelected(3)) {
+                    optiongroup.removeItem(2);
+                    optiongroup.addItem(2);
+                    optiongroup.setItemCaption(2, "B");
+                }
+            }
+        });
+
+        addComponent(optiongroup);
+
+        optiongroup.focus();
+    }
+
+    @Override
+    protected String getTestDescription() {
+        return "OptionGroup should retain focus after it's value being changed with keyboard";
+    }
+
+    @Override
+    protected Integer getTicketNumber() {
+        return 10451;
+    }
+}