From 441371ac98b2a99822ca1639a6823b23be93c229 Mon Sep 17 00:00:00 2001 From: Markus Koivisto Date: Thu, 26 Jun 2014 16:35:42 +0300 Subject: [PATCH] Keyboard shift-selection now works as expected in VScrollTable. (#14094) Change-Id: I0dcd9f75cd30fe91c17ca0755241e73a37da79ec --- .../com/vaadin/client/ui/VScrollTable.java | 19 ++++--- .../table/SelectAllConstantViewport.java | 6 +- .../table/SelectAllConstantViewportTest.java | 57 ++++++++++++++++++- .../vaadin/tests/tb3/MultiBrowserTest.java | 10 +++- 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 59645aa6d3..5e6207f53f 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() { + + @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 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 getBrowsersExcludingIE() { - List browsers = new ArrayList(getAllBrowsers()); + List browsers = new ArrayList( + 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 getBrowsersExcludingPhantomJS() { + List browsers = new ArrayList( + 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 -- 2.39.5