Keyboard shift-selection now works as expected in VScrollTable. (#14094)

Change-Id: I0dcd9f75cd30fe91c17ca0755241e73a37da79ec

4 files changed, 78 insertions, 14 deletions

diff --git a/client/src/com/vaadin/client/ui/VScrollTable.java b/client/src/com/vaadin/client/ui/VScrollTable.java
index b83a7b8137..9b0bc3d730 100644
--- a/client/src/com/vaadin/client/ui/VScrollTable.java
+++ b/client/src/com/vaadin/client/ui/VScrollTable.java
@@ -1093,9 +1093,17 @@ public class VScrollTable extends FlowPanel implements HasWidgets,
                             /*
                              * The focus is no longer on a selected row. Move
                              * focus to the selected row. (#10522)
+                             * 
+                             * Don't do this for multiselect (#13341).
+                             * 
+                             * Checking the selection mode here instead of in
+                             * setRowFocus allows keyboard shift+downarrow
+                             * selection to work as expected.
                              */
+                            if (isSingleSelectMode()) {
+                                setRowFocus(row);
+                            }
 
-                            setRowFocus(row);
                         }
                     }
                     if (selected != row.isSelected()) {
@@ -7248,14 +7256,7 @@ public class VScrollTable extends FlowPanel implements HasWidgets,
             // Set new focused row
             focusedRow = row;
 
-            /*
-             * Don't scroll to the focused row when in multiselect mode.
-             * (#13341)
-             */
-
-            if (isSingleSelectMode()) {
-                ensureRowIsVisible(row);
-            }
+            ensureRowIsVisible(row);
 
             return true;
         }
diff --git a/uitest/src/com/vaadin/tests/components/table/SelectAllConstantViewport.java b/uitest/src/com/vaadin/tests/components/table/SelectAllConstantViewport.java
index 5a406eac48..35ac63efe2 100644
--- a/uitest/src/com/vaadin/tests/components/table/SelectAllConstantViewport.java
+++ b/uitest/src/com/vaadin/tests/components/table/SelectAllConstantViewport.java
@@ -65,7 +65,7 @@ public class SelectAllConstantViewport extends AbstractTestUIWithLog {
             table.addItem(new Object[] { new Integer(i) }, new Integer(i));
         }
 
-        table.setCurrentPageFirstItemIndex(185);
+        table.setCurrentPageFirstItemIndex(178);
 
         final CssLayout layout = new CssLayout();
         layout.addComponent(selectAllCheckbox);
@@ -82,7 +82,9 @@ public class SelectAllConstantViewport extends AbstractTestUIWithLog {
     @Override
     protected String getTestDescription() {
 
-        return "The scroll position of a table with many items should remain constant if all items are selected.";
+        return "The scroll position of a table with many items should remain constant if all items are "
+                + "selected. The scroll position should change if the user uses the keyboard to select "
+                + "multiple lines with shift+arrowkeys.";
     }
 
     /*
diff --git a/uitest/src/com/vaadin/tests/components/table/SelectAllConstantViewportTest.java b/uitest/src/com/vaadin/tests/components/table/SelectAllConstantViewportTest.java
index 0e7a7c08a4..257efef6a2 100644
--- a/uitest/src/com/vaadin/tests/components/table/SelectAllConstantViewportTest.java
+++ b/uitest/src/com/vaadin/tests/components/table/SelectAllConstantViewportTest.java
@@ -15,14 +15,20 @@
  */
 package com.vaadin.tests.components.table;
 
+import static org.hamcrest.CoreMatchers.containsString;
 import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.CoreMatchers.not;
 import static org.hamcrest.MatcherAssert.assertThat;
 
 import java.io.IOException;
+import java.util.List;
 
 import org.junit.Test;
+import org.openqa.selenium.Keys;
 import org.openqa.selenium.WebDriver;
 import org.openqa.selenium.WebElement;
+import org.openqa.selenium.interactions.Actions;
+import org.openqa.selenium.remote.DesiredCapabilities;
 import org.openqa.selenium.support.ui.ExpectedCondition;
 
 import com.vaadin.testbench.By;
@@ -33,7 +39,7 @@ import com.vaadin.tests.tb3.MultiBrowserTest;
 public class SelectAllConstantViewportTest extends MultiBrowserTest {
 
     @Test
-    public void testViewportUnchanged() throws IOException {
+    public void testViewportUnchangedwithMultiSel() throws IOException {
         openTestURL();
 
         CheckBoxElement checkbox = $(CheckBoxElement.class).first();
@@ -51,11 +57,58 @@ public class SelectAllConstantViewportTest extends MultiBrowserTest {
 
         int rowLocation = row.getLocation().getY();
 
-        // use click x,y with non-zero offset to actually toggle the checkbox. (#13763)
+        // use click x,y with non-zero offset to actually toggle the checkbox.
+        // (#13763)
         checkbox.click(5, 5);
         int newRowLocation = row.getLocation().getY();
 
         assertThat(newRowLocation, is(rowLocation));
 
     }
+
+    @Test
+    public void testViewportChangedwithKeyboardSel() throws IOException {
+        openTestURL();
+
+        WebElement cell = $(TableElement.class).first().getCell(190, 0);
+        final WebElement scrollPositionDisplay = getDriver().findElement(
+                By.className("v-table-scrollposition"));
+        waitUntilNot(new ExpectedCondition<Boolean>() {
+
+            @Override
+            public Boolean apply(WebDriver input) {
+                return scrollPositionDisplay.isDisplayed();
+            }
+        }, 10);
+
+        int rowLocation = cell.getLocation().getY();
+
+        // select downwards with shift (#14094)
+
+        cell.click();
+
+        final WebElement row = getDriver().findElement(
+                By.className("v-selected"));
+
+        assertThat(row.getAttribute("class"), containsString("selected"));
+
+        // for some reason phantomJS does not support keyboard actions
+
+        Actions action = new Actions(getDriver());
+        action.keyDown(Keys.SHIFT)
+                .sendKeys(Keys.ARROW_DOWN, Keys.ARROW_DOWN, Keys.ARROW_DOWN,
+                        Keys.ARROW_DOWN, Keys.ARROW_DOWN, Keys.ARROW_DOWN,
+                        Keys.ARROW_DOWN).keyUp(Keys.SHIFT).build().perform();
+
+        int newRowLocation = cell.getLocation().getY();
+
+        assertThat(newRowLocation, is(not(rowLocation)));
+
+    }
+
+    @Override
+    public List<DesiredCapabilities> getBrowsersToTest() {
+        // phantomJS does not support keyboard actions
+        return getBrowsersExcludingPhantomJS();
+    }
 }
diff --git a/uitest/src/com/vaadin/tests/tb3/MultiBrowserTest.java b/uitest/src/com/vaadin/tests/tb3/MultiBrowserTest.java
index ccbb6ca872..ffa0b83dc2 100644
--- a/uitest/src/com/vaadin/tests/tb3/MultiBrowserTest.java
+++ b/uitest/src/com/vaadin/tests/tb3/MultiBrowserTest.java
@@ -41,7 +41,8 @@ import org.openqa.selenium.remote.DesiredCapabilities;
 public abstract class MultiBrowserTest extends PrivateTB3Configuration {
 
     protected List<DesiredCapabilities> getBrowsersExcludingIE() {
-        List<DesiredCapabilities> browsers = new ArrayList<DesiredCapabilities>(getAllBrowsers());
+        List<DesiredCapabilities> browsers = new ArrayList<DesiredCapabilities>(
+                getAllBrowsers());
         browsers.remove(Browser.IE8.getDesiredCapabilities());
         browsers.remove(Browser.IE9.getDesiredCapabilities());
         browsers.remove(Browser.IE10.getDesiredCapabilities());
@@ -50,6 +51,13 @@ public abstract class MultiBrowserTest extends PrivateTB3Configuration {
         return browsers;
     }
 
+    protected List<DesiredCapabilities> getBrowsersExcludingPhantomJS() {
+        List<DesiredCapabilities> browsers = new ArrayList<DesiredCapabilities>(
+                getAllBrowsers());
+        browsers.remove(Browser.PHANTOMJS.getDesiredCapabilities());
+        return browsers;
+    }
+
     public enum Browser {
         FIREFOX(BrowserUtil.firefox(24)), CHROME(BrowserUtil.chrome(33)), SAFARI(
                 BrowserUtil.safari(7)), IE8(BrowserUtil.ie(8)), IE9(BrowserUtil