Ensure the selection has been changed before updating . (#11658)

- Initial fix attempt interfered with selection events, added regression
testing for those and found a better way to ensure shift selection works
on IE11 also with Windows 7.

Fixes #11608

2 files changed, 58 insertions, 11 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 2a3b98996e..f8b6bae23b 100644
--- a/client/src/main/java/com/vaadin/client/ui/VListSelect.java
+++ b/client/src/main/java/com/vaadin/client/ui/VListSelect.java
@@ -26,7 +26,6 @@ import com.google.gwt.user.client.ui.Composite;
 import com.google.gwt.user.client.ui.FlowPanel;
 import com.google.gwt.user.client.ui.HasEnabled;
 import com.google.gwt.user.client.ui.ListBox;
-import com.vaadin.client.BrowserInfo;
 import com.vaadin.client.FastStringSet;
 import com.vaadin.client.Focusable;
 import com.vaadin.client.connectors.AbstractMultiSelectConnector.MultiSelectWidget;
@@ -121,11 +120,6 @@ public class VListSelect extends Composite
             final JsonObject item = items.get(i);
             // reuse existing option if possible
             String key = MultiSelectWidget.getKey(item);
-            if (BrowserInfo.get().isIE11() && key != null) {
-                // IE11 doesn't handle numerical keys well on Win7,
-                // prevent incorrect type handling with extra character
-                key += " ";
-            }
             if (i < select.getItemCount()) {
                 select.setItemText(i, MultiSelectWidget.getCaption(item));
                 select.setValue(i, key);
@@ -133,7 +127,13 @@ public class VListSelect extends Composite
                 select.addItem(MultiSelectWidget.getCaption(item), key);
             }
             final boolean selected = MultiSelectWidget.isSelected(item);
-            select.setItemSelected(i, selected);
+
+            // Check that the selection has changed before updating it because
+            // IE11 causes problems on Windows 7 if we update without needing to
+            if (selected != select.isItemSelected(i)) {
+                select.setItemSelected(i, selected);
+            }
+
             if (selected) {
                 selectedItemKeys.add(key);
             }
@@ -155,10 +155,6 @@ public class VListSelect extends Composite
         for (int i = 0; i < select.getItemCount(); i++) {
             if (select.isItemSelected(i)) {
                 String key = select.getValue(i);
-                if (BrowserInfo.get().isIE11() && key != null) {
-                    // remove the IE11 workaround
-                    key = key.trim();
-                }
                 selectedItemKeys.add(key);
             }
         }
diff --git a/uitest/src/test/java/com/vaadin/tests/components/listselect/ListSelectMultiSelectionTest.java b/uitest/src/test/java/com/vaadin/tests/components/listselect/ListSelectMultiSelectionTest.java
index a5b2c6394c..116475c4bc 100644
--- a/uitest/src/test/java/com/vaadin/tests/components/listselect/ListSelectMultiSelectionTest.java
+++ b/uitest/src/test/java/com/vaadin/tests/components/listselect/ListSelectMultiSelectionTest.java
@@ -24,6 +24,7 @@ public class ListSelectMultiSelectionTest extends MultiBrowserTest {
     @Test
     public void testShiftSelect() {
         openTestURL();
+        selectMenuPath("Component", "Listeners", "Selection listener");
 
         ListSelectElement listSelect = $(ListSelectElement.class).first();
         Select select = new Select(listSelect.getSelectElement());
@@ -31,6 +32,10 @@ public class ListSelectMultiSelectionTest extends MultiBrowserTest {
                 .findElements(By.tagName("option"));
         options.get(0).click();
 
+        // ensure the selection event got through
+        assertEquals("1. Selected: [Item 0]", getLogs().get(0));
+
+        // ensure the state corresponds with the event
         List<WebElement> selected = select.getAllSelectedOptions();
         assertEquals(1, selected.size());
         assertEquals("Item 0", selected.get(0).getText());
@@ -39,6 +44,8 @@ public class ListSelectMultiSelectionTest extends MultiBrowserTest {
         options.get(1).click();
         new Actions(getDriver()).keyUp(Keys.SHIFT).perform();
 
+        assertEquals("2. Selected: [Item 0, Item 1]", getLogs().get(0));
+
         selected = select.getAllSelectedOptions();
         assertEquals(2, selected.size());
         assertEquals("Item 1", selected.get(1).getText());
@@ -48,6 +55,50 @@ public class ListSelectMultiSelectionTest extends MultiBrowserTest {
         new Actions(getDriver()).keyUp(Keys.SHIFT).perform();
 
         // ensure second shift selection added instead of moved
+        assertEquals("3. Selected: [Item 0, Item 1, Item 2]", getLogs().get(0));
+
+        selected = select.getAllSelectedOptions();
+        assertEquals(3, selected.size());
+        assertEquals("Item 2", selected.get(2).getText());
+        assertEquals("Item 0", selected.get(0).getText());
+    }
+
+    @Test
+    public void testShiftSelectWithKeys() {
+        openTestURL();
+        selectMenuPath("Component", "Listeners", "Selection listener");
+
+        ListSelectElement listSelect = $(ListSelectElement.class).first();
+        Select select = new Select(listSelect.getSelectElement());
+        List<WebElement> options = listSelect
+                .findElements(By.tagName("option"));
+        options.get(0).click();
+
+        // ensure the selection event got through
+        assertEquals("1. Selected: [Item 0]", getLogs().get(0));
+
+        // ensure the state corresponds with the event
+        List<WebElement> selected = select.getAllSelectedOptions();
+        assertEquals(1, selected.size());
+        assertEquals("Item 0", selected.get(0).getText());
+
+        new Actions(getDriver()).keyDown(Keys.SHIFT).perform();
+        new Actions(getDriver()).sendKeys(Keys.ARROW_DOWN).perform();
+        new Actions(getDriver()).keyUp(Keys.SHIFT).perform();
+
+        assertEquals("2. Selected: [Item 0, Item 1]", getLogs().get(0));
+
+        selected = select.getAllSelectedOptions();
+        assertEquals(2, selected.size());
+        assertEquals("Item 1", selected.get(1).getText());
+
+        new Actions(getDriver()).keyDown(Keys.SHIFT).perform();
+        new Actions(getDriver()).sendKeys(Keys.ARROW_DOWN).perform();
+        new Actions(getDriver()).keyUp(Keys.SHIFT).perform();
+
+        // ensure second shift selection added instead of moved
+        assertEquals("3. Selected: [Item 0, Item 1, Item 2]", getLogs().get(0));
+
         selected = select.getAllSelectedOptions();
         assertEquals(3, selected.size());
         assertEquals("Item 2", selected.get(2).getText());