From 82682dc00df55bed239b30c8a2138ae1b76a9c75 Mon Sep 17 00:00:00 2001 From: Anna Koskinen Date: Fri, 12 Jul 2019 09:05:20 +0300 Subject: [PATCH] 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 --- .../com/vaadin/client/ui/VListSelect.java | 18 +++---- .../ListSelectMultiSelectionTest.java | 51 +++++++++++++++++++ 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 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 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 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()); -- 2.39.5