Avoid rebuilding VListSelect DOM for each round trip (#14765)

Change-Id: Iebcea0fd80eb9c1e0f14357fa09a264db5f1ee06

6 files changed, 529 insertions, 13 deletions

diff --git a/client/src/main/java/com/vaadin/client/ui/VListSelect.java b/client/src/main/java/com/vaadin/client/ui/VListSelect.java
index b6f4f0c722..83e25163b3 100644
--- a/client/src/main/java/com/vaadin/client/ui/VListSelect.java
+++ b/client/src/main/java/com/vaadin/client/ui/VListSelect.java
@@ -17,11 +17,14 @@
 package com.vaadin.client.ui;
 
 import java.util.ArrayList;
+import java.util.HashSet;
 import java.util.Iterator;
+import java.util.Set;
 
 import com.google.gwt.event.dom.client.ChangeEvent;
 import com.google.gwt.user.client.ui.ListBox;
 import com.vaadin.client.UIDL;
+import com.vaadin.shared.util.SharedUtil;
 
 public class VListSelect extends VOptionGroupBase {
 
@@ -67,34 +70,69 @@ public class VListSelect extends VOptionGroupBase {
 
     @Override
     public void buildOptions(UIDL uidl) {
-        int scrollTop = select.getElement().getScrollTop();
-        int rowCount = getRows();
         select.setMultipleSelect(isMultiselect());
-        select.clear();
+
+        Set<String> previousKeys = new HashSet<String>();
+        for (int i = 0; i < select.getItemCount(); i++) {
+            previousKeys.add(select.getValue(i));
+        }
+
+        int nextIndex = 0;
         if (!isMultiselect() && isNullSelectionAllowed()
                 && !isNullSelectionItemAvailable()) {
             // can't unselect last item in singleselect mode
+            updateOrCreateItem("", "null", nextIndex++, previousKeys);
             select.addItem("", (String) null);
+
+            // Null select item can't be selected programmatically, but will
+            // remain selected if it was selected by the user. There's no
+            // need to deselect when something else is selected since it's only
+            // used in single select mode.
         }
         for (final Iterator<?> i = uidl.getChildIterator(); i.hasNext();) {
             final UIDL optionUidl = (UIDL) i.next();
-            select.addItem(optionUidl.getStringAttribute("caption"),
-                    optionUidl.getStringAttribute("key"));
+            updateOrCreateItem(optionUidl.getStringAttribute("caption"),
+                    optionUidl.getStringAttribute("key"), nextIndex,
+                    previousKeys);
             if (optionUidl.hasAttribute("selected")) {
-                int itemIndex = select.getItemCount() - 1;
-                select.setItemSelected(itemIndex, true);
-                lastSelectedIndex = itemIndex;
+                select.setItemSelected(nextIndex, true);
+                lastSelectedIndex = nextIndex;
+            } else {
+                select.setItemSelected(nextIndex, false);
             }
+            nextIndex++;
         }
+
+        // Remove any trailing items not in the UIDL
+        while (select.getItemCount() > nextIndex) {
+            select.removeItem(nextIndex);
+        }
+
         if (getRows() > 0) {
             select.setVisibleItemCount(getRows());
         }
-        // FIXME: temporary hack for preserving the scroll state when the
-        // contents haven't been changed obviously. This should be dealt with in
-        // the rewrite.
-        if (rowCount == getRows()) {
-            select.getElement().setScrollTop(scrollTop);
+    }
+
+    private void updateOrCreateItem(String caption, String key, int index,
+            Set<String> previousKeys) {
+        if (previousKeys.remove(key)) {
+            while (select.getItemCount() >= index) {
+                String keyAtIndex = select.getValue(index);
+                if (SharedUtil.equals(key, keyAtIndex)) {
+                    select.setItemText(index, caption);
+                    return;
+                } else {
+                    // Assume the item we're looking at has simply been removed
+                    // and that the next item will match our key
+                    select.removeItem(index);
+                    previousKeys.remove(keyAtIndex);
+                }
+            }
         }
+
+        // We end up here for new items or if we removed all following items
+        // while looking for a match
+        select.insertItem(caption, key, index);
     }
 
     @Override
diff --git a/uitest/src/main/java/com/vaadin/tests/components/listselect/ListSelectAddRemoveItems.java b/uitest/src/main/java/com/vaadin/tests/components/listselect/ListSelectAddRemoveItems.java
new file mode 100644
index 0000000000..56e9d1f419
--- /dev/null
+++ b/uitest/src/main/java/com/vaadin/tests/components/listselect/ListSelectAddRemoveItems.java
@@ -0,0 +1,140 @@
+/*
+ * Copyright 2000-2014 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.listselect;
+
+import com.vaadin.data.util.IndexedContainer;
+import com.vaadin.server.VaadinRequest;
+import com.vaadin.tests.components.AbstractTestUIWithLog;
+import com.vaadin.ui.Button;
+import com.vaadin.ui.Button.ClickEvent;
+import com.vaadin.ui.ListSelect;
+
+public class ListSelectAddRemoveItems extends AbstractTestUIWithLog {
+
+    private IndexedContainer container = new IndexedContainer();
+
+    @Override
+    protected void setup(VaadinRequest request) {
+        ListSelect listSelect = new ListSelect("ListSelect", container);
+        listSelect.setWidth("100px");
+        listSelect.setRows(10);
+
+        resetContainer();
+        logContainer();
+
+        addComponent(listSelect);
+        addComponent(new Button("Reset", new Button.ClickListener() {
+            @Override
+            public void buttonClick(ClickEvent event) {
+                resetContainer();
+                log.clear();
+                logContainer();
+            }
+        }));
+
+        addComponent(new Button("Add first", new Button.ClickListener() {
+            @Override
+            public void buttonClick(ClickEvent event) {
+                container.addItemAt(0, "first");
+                logContainer();
+            }
+        }));
+
+        addComponent(new Button("Add middle", new Button.ClickListener() {
+            @Override
+            public void buttonClick(ClickEvent event) {
+                container.addItemAt(container.size() / 2, "middle");
+                logContainer();
+            }
+        }));
+
+        addComponent(new Button("Add last", new Button.ClickListener() {
+            @Override
+            public void buttonClick(ClickEvent event) {
+                container.addItem("last");
+                logContainer();
+            }
+        }));
+
+        addComponent(new Button("Swap", new Button.ClickListener() {
+            @Override
+            public void buttonClick(ClickEvent event) {
+                Object lastItem = container.lastItemId();
+                Object firstItem = container.firstItemId();
+                if (lastItem != firstItem) {
+                    container.removeItem(lastItem);
+                    container.removeItem(firstItem);
+
+                    container.addItemAt(0, lastItem);
+                    container.addItem(firstItem);
+                }
+
+                logContainer();
+            }
+        }));
+
+        addComponent(new Button("Remove first", new Button.ClickListener() {
+            @Override
+            public void buttonClick(ClickEvent event) {
+                container.removeItem(container.firstItemId());
+                logContainer();
+            }
+        }));
+
+        addComponent(new Button("Remove middle", new Button.ClickListener() {
+            @Override
+            public void buttonClick(ClickEvent event) {
+                container.removeItem(container.getIdByIndex(container.size() / 2));
+                logContainer();
+            }
+        }));
+
+        addComponent(new Button("Remove last", new Button.ClickListener() {
+            @Override
+            public void buttonClick(ClickEvent event) {
+                container.removeItem(container.lastItemId());
+                logContainer();
+            }
+        }));
+
+    }
+
+    private void logContainer() {
+        StringBuilder b = new StringBuilder();
+        for (int i = 0; i < container.size(); i++) {
+            Object id = container.getIdByIndex(i);
+            if (i != 0) {
+                b.append(", ");
+            }
+            b.append(id);
+        }
+
+        log(b.toString());
+    }
+
+    public void resetContainer() {
+        container.removeAllItems();
+        for (String value : new String[] { "a", "b", "c" }) {
+            container.addItem(value);
+        }
+    }
+
+    @Override
+    protected String getTestDescription() {
+        return "Test for verifying that items are added to and removed from the correct locations";
+    }
+
+}
diff --git a/uitest/src/main/java/com/vaadin/tests/components/listselect/ListSelectPushSelectionChanges.java b/uitest/src/main/java/com/vaadin/tests/components/listselect/ListSelectPushSelectionChanges.java
new file mode 100644
index 0000000000..9518456066
--- /dev/null
+++ b/uitest/src/main/java/com/vaadin/tests/components/listselect/ListSelectPushSelectionChanges.java
@@ -0,0 +1,70 @@
+/*
+ * Copyright 2000-2014 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.listselect;
+
+import java.util.Arrays;
+import java.util.Collection;
+
+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.Button;
+import com.vaadin.ui.Button.ClickEvent;
+import com.vaadin.ui.ListSelect;
+import com.vaadin.ui.OptionGroup;
+
+public class ListSelectPushSelectionChanges extends AbstractTestUI {
+
+    @Override
+    protected void setup(VaadinRequest request) {
+        Collection<String> options = Arrays.asList("a", "b", "c");
+
+        final ListSelect listSelect = new ListSelect("ListSelect", options);
+        listSelect.setNullSelectionAllowed(true);
+
+        final OptionGroup optionGroup = new OptionGroup("OptionGroup", options);
+
+        // Option group changes propagate to the list select
+        listSelect.setPropertyDataSource(optionGroup);
+
+        final OptionGroup modeSelect = new OptionGroup("Mode", Arrays.asList(
+                "Single", "Multi"));
+        modeSelect.setValue("Single");
+        modeSelect.addValueChangeListener(new ValueChangeListener() {
+            @Override
+            public void valueChange(ValueChangeEvent event) {
+                optionGroup.setValue(null);
+
+                boolean multiSelect = "Multi".equals(modeSelect.getValue());
+
+                listSelect.setMultiSelect(multiSelect);
+                optionGroup.setMultiSelect(multiSelect);
+            }
+        });
+
+        Button selectNullButton = new Button("Select null",
+                new Button.ClickListener() {
+                    @Override
+                    public void buttonClick(ClickEvent event) {
+                        listSelect.setValue(null);
+                        listSelect.markAsDirty();
+                    }
+                });
+
+        addComponents(modeSelect, listSelect, optionGroup, selectNullButton);
+    }
+}
diff --git a/uitest/src/test/java/com/vaadin/tests/components/listselect/ListSelectAddRemoveItemsTest.java b/uitest/src/test/java/com/vaadin/tests/components/listselect/ListSelectAddRemoveItemsTest.java
new file mode 100644
index 0000000000..dd694995f1
--- /dev/null
+++ b/uitest/src/test/java/com/vaadin/tests/components/listselect/ListSelectAddRemoveItemsTest.java
@@ -0,0 +1,63 @@
+/*
+ * Copyright 2000-2014 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.listselect;
+
+import java.util.Arrays;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+import com.vaadin.testbench.elements.ButtonElement;
+import com.vaadin.testbench.elements.ListSelectElement;
+import com.vaadin.tests.tb3.SingleBrowserTest;
+
+public class ListSelectAddRemoveItemsTest extends SingleBrowserTest {
+    @Test
+    public void testAddAndRemove() {
+        openTestURL();
+        assertOptions("", "a", "b", "c");
+
+        click("Add first");
+        assertOptions("", "first", "a", "b", "c");
+
+        click("Swap");
+        assertOptions("", "c", "a", "b", "first");
+
+        click("Remove first");
+        assertOptions("", "a", "b", "first");
+
+        click("Add middle");
+        assertOptions("", "a", "middle", "b", "first");
+
+        click("Add last");
+        assertOptions("", "a", "middle", "b", "first", "last");
+
+        click("Remove middle");
+        assertOptions("", "a", "middle", "first", "last");
+
+        click("Reset");
+        assertOptions("", "a", "b", "c");
+    }
+
+    private void assertOptions(String... options) {
+        Assert.assertEquals(Arrays.asList(options), $(ListSelectElement.class)
+                .first().getOptions());
+    }
+
+    private void click(String caption) {
+        $(ButtonElement.class).caption(caption).first().click();
+    }
+}
diff --git a/uitest/src/test/java/com/vaadin/tests/components/listselect/ListSelectNoDomRebuildTest.java b/uitest/src/test/java/com/vaadin/tests/components/listselect/ListSelectNoDomRebuildTest.java
new file mode 100644
index 0000000000..97b475e092
--- /dev/null
+++ b/uitest/src/test/java/com/vaadin/tests/components/listselect/ListSelectNoDomRebuildTest.java
@@ -0,0 +1,70 @@
+/*
+ * Copyright 2000-2014 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.listselect;
+
+import java.util.List;
+
+import org.junit.Assert;
+import org.junit.Test;
+import org.openqa.selenium.By;
+import org.openqa.selenium.Keys;
+import org.openqa.selenium.WebElement;
+import org.openqa.selenium.interactions.Actions;
+
+import com.vaadin.testbench.elements.ListSelectElement;
+import com.vaadin.tests.tb3.SingleBrowserTest;
+
+public class ListSelectNoDomRebuildTest extends SingleBrowserTest {
+    @Override
+    protected Class<?> getUIClass() {
+        return ListSelects.class;
+    }
+
+    @Test
+    public void testNoDomRebuild() {
+        openTestURL();
+
+        // Testbench doesn't seem to support sending key events to the right
+        // location, so we will just verify that the DOM is not rebuilt
+        selectMenuPath("Component", "Selection", "Multi select");
+        selectMenuPath("Component", "Listeners", "Value change listener");
+
+        ListSelectElement list = $(ListSelectElement.class).first();
+        List<WebElement> options = list.findElements(By.tagName("option"));
+
+        assertNotStale(options);
+
+        options.get(4).click();
+
+        assertNotStale(options);
+
+        new Actions(driver).keyDown(Keys.SHIFT).perform();
+        options.get(2).click();
+        options.get(6).click();
+        new Actions(driver).keyUp(Keys.SHIFT).perform();
+
+        assertNotStale(options);
+    }
+
+    private void assertNotStale(List<WebElement> options) {
+        for (WebElement element : options) {
+            // We really don't expect the text to be null, mainly doing this
+            // since getText() will throw if the element is detached.
+            Assert.assertNotNull(element.getText());
+        }
+    }
+
+}
diff --git a/uitest/src/test/java/com/vaadin/tests/components/listselect/ListSelectPushSelectionChangesTest.java b/uitest/src/test/java/com/vaadin/tests/components/listselect/ListSelectPushSelectionChangesTest.java
new file mode 100644
index 0000000000..a13482d175
--- /dev/null
+++ b/uitest/src/test/java/com/vaadin/tests/components/listselect/ListSelectPushSelectionChangesTest.java
@@ -0,0 +1,135 @@
+/*
+ * Copyright 2000-2014 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.listselect;
+
+import java.util.List;
+
+import org.junit.Assert;
+import org.junit.Test;
+import org.openqa.selenium.By;
+import org.openqa.selenium.WebElement;
+import org.openqa.selenium.support.ui.Select;
+
+import com.vaadin.testbench.elements.ButtonElement;
+import com.vaadin.testbench.elements.ListSelectElement;
+import com.vaadin.testbench.elements.OptionGroupElement;
+import com.vaadin.tests.tb3.SingleBrowserTestPhantomJS2;
+
+public class ListSelectPushSelectionChangesTest extends
+        SingleBrowserTestPhantomJS2 {
+    @Test
+    public void testMultiSelectBehavior() {
+        openTestURL();
+
+        Assert.assertEquals(
+                "Should have null item + 3 options in single selection mode",
+                4, getListSelect().getOptions().size());
+
+        $(OptionGroupElement.class).caption("Mode").first()
+                .selectByText("Multi");
+
+        Assert.assertEquals(
+                "Should have 3 options but no null item in multi selection mode",
+                3, getListSelect().getOptions().size());
+
+        selectOptionGroup("a");
+        Assert.assertEquals("a", getSelectValue());
+
+        selectOptionGroup("b");
+        Assert.assertEquals("a,b", getSelectValue());
+
+        selectOptionGroup("a");
+        Assert.assertEquals(
+                "Clicking selected item should deselct in multi selection mode",
+                "b", getSelectValue());
+
+        selectNull();
+        Assert.assertEquals("", getSelectValue());
+    }
+
+    @Test
+    public void testSingleSelectBehavior() {
+        openTestURL();
+
+        selectOptionGroup("a");
+        Assert.assertEquals("a", getSelectValue());
+
+        selectOptionGroup("b");
+        Assert.assertEquals("b", getSelectValue());
+
+        selectOptionGroup("b");
+        Assert.assertEquals(
+                "Selecting the selected item again should not deselect in single selection mode",
+                "b", getSelectValue());
+
+        selectNull();
+        Assert.assertEquals("", getSelectValue());
+        Assert.assertEquals(
+                "Not even the single select item should be selected after setValue(null)",
+                0, getSelectCount());
+
+        selectOptionGroup("c");
+        Assert.assertEquals("c", getSelectValue());
+
+        getListSelect().selectByText("");
+        Assert.assertEquals("", getSelectValue());
+        Assert.assertEquals(
+                "Null select item should remain selected if clicked by the user",
+                1, getSelectCount());
+
+        selectNull();
+        Assert.assertEquals("", getSelectValue());
+        Assert.assertEquals(
+                "Null select item should remain selected even after a repaint",
+                1, getSelectCount());
+    }
+
+    private ListSelectElement getListSelect() {
+        return $(ListSelectElement.class).first();
+    }
+
+    private int getSelectCount() {
+        return getSelectedOptions().size();
+    }
+
+    private void selectNull() {
+        $(ButtonElement.class).first().click();
+    }
+
+    private String getSelectValue() {
+        List<WebElement> selectedOptions = getSelectedOptions();
+
+        StringBuilder value = new StringBuilder();
+        for (int i = 0; i < selectedOptions.size(); i++) {
+            if (i != 0) {
+                value.append(',');
+            }
+            value.append(selectedOptions.get(i).getText());
+        }
+        return value.toString();
+    }
+
+    private List<WebElement> getSelectedOptions() {
+        ListSelectElement listSelect = getListSelect();
+        Select select = new Select(listSelect.findElement(By.tagName("select")));
+        return select.getAllSelectedOptions();
+    }
+
+    private void selectOptionGroup(String value) {
+        $(OptionGroupElement.class).caption("OptionGroup").first()
+                .selectByText(value);
+    }
+}