From: Anna Miroshnik Date: Thu, 2 Oct 2014 12:02:55 +0000 (+0400) Subject: Re-adding content in Table causes table to lose scroll position (#14581) X-Git-Tag: 7.4.0.beta1~166 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=0abf13890b8c2391a609d63995a862b831e2f078;p=vaadin-framework.git Re-adding content in Table causes table to lose scroll position (#14581) At this moment behavior of restoring scroll position more like as in 7.2.6. But restoring is only in case of removeAll() - addAll() (or add() one time - then restore index is reset). In 7.2.6 restoring of scroll position was the result of client defect (scrolling in lazyScroller was not changed if variable "firstvisible" fromserver was 0). This "defect" was fixed in one of the patches. Change-Id: I2e2fb8749ec95f3409caeacafff46c4c29159e74 --- diff --git a/server/src/com/vaadin/ui/Table.java b/server/src/com/vaadin/ui/Table.java index 3a1ab82be5..34d48a9b18 100644 --- a/server/src/com/vaadin/ui/Table.java +++ b/server/src/com/vaadin/ui/Table.java @@ -422,61 +422,11 @@ public class Table extends AbstractSelect implements Action.Container, private Object currentPageFirstItemId = null; /* - * This class stores the hashcode and scroll position of the previous - * container so that the scroll position can be restored if the same - * container is removed and then re-added. This resolves #14581. + * If all rows get removed then scroll position of the previous container + * can be restored after re-adding/replacing rows via addAll(). This + * resolves #14581. */ - protected static class ScrollPositionRepairOnReAddAllRowsData implements - Serializable { - - private static final long serialVersionUID = 1L; - // current page first item index (to repair scroll position) - private int itemIndex; - /* - * hashCode() of container before it was cleared via - * container.removeAllItems(); - */ - private int containerHashCode; - - public ScrollPositionRepairOnReAddAllRowsData() { - itemIndex = -1; - containerHashCode = -1; - } - - public int getItemId() { - return itemIndex; - } - - public void setItemId(int itemId) { - itemIndex = itemId; - } - - public void resetItemId() { - itemIndex = -1; - } - - public void setContainerData(Container container) { - if (container != null) { - containerHashCode = container.hashCode(); - } else { - containerHashCode = -1; - } - } - - public boolean needRepairScrollPosition(Container newContainer) { - return (itemIndex != -1) && isTheSameContainer(newContainer); - } - - private boolean isTheSameContainer(Container newContainer) { - boolean theSame = false; - if (newContainer != null) { - theSame = (newContainer.hashCode() == containerHashCode); - } - return theSame; - } - } - - private final ScrollPositionRepairOnReAddAllRowsData scrollRepairOnReAdding = new ScrollPositionRepairOnReAddAllRowsData(); + private int repairOnReAddAllRowsDataScrollPositionItemIndex = -1; /** * Index of the first item on the current page. @@ -2734,10 +2684,6 @@ public class Table extends AbstractSelect implements Action.Container, newDataSource = new IndexedContainer(); } - if (scrollRepairOnReAdding != null) { - // this is important if container is set for table after filling - scrollRepairOnReAdding.setContainerData(newDataSource); - } Collection generated; if (columnGenerators != null) { generated = columnGenerators.keySet(); @@ -4571,14 +4517,22 @@ public class Table extends AbstractSelect implements Action.Container, int currentFirstItemIndex = getCurrentPageFirstItemIndex(); if (event.getContainer().size() == 0) { - scrollRepairOnReAdding.setItemId(getCurrentPageFirstItemIndex()); + repairOnReAddAllRowsDataScrollPositionItemIndex = getCurrentPageFirstItemIndex(); } else { - if (scrollRepairOnReAdding.needRepairScrollPosition(event - .getContainer())) { - currentFirstItemIndex = scrollRepairOnReAdding.getItemId(); + if (repairOnReAddAllRowsDataScrollPositionItemIndex != -1) { + currentFirstItemIndex = repairOnReAddAllRowsDataScrollPositionItemIndex; + /* + * Reset repairOnReAddAllRowsDataScrollPositionItemIndex. + * + * Next string should be commented (removed) if we want to have + * possibility to restore scroll position during adding items to + * container one by one via add() but not only addAll(). The + * problem in this case: we cannot track what happened between + * add() and add()... So it is ambiguous where to stop restore + * scroll position. + */ + repairOnReAddAllRowsDataScrollPositionItemIndex = -1; } - scrollRepairOnReAdding.resetItemId(); - scrollRepairOnReAdding.setContainerData(event.getContainer()); } // ensure that page still has first item in page, ignore buffer refresh diff --git a/uitest/src/com/vaadin/tests/components/table/TableRepairsScrollPositionOnReAddingAllRows.java b/uitest/src/com/vaadin/tests/components/table/TableRepairsScrollPositionOnReAddingAllRows.java index d9cbf007df..df06580dae 100644 --- a/uitest/src/com/vaadin/tests/components/table/TableRepairsScrollPositionOnReAddingAllRows.java +++ b/uitest/src/com/vaadin/tests/components/table/TableRepairsScrollPositionOnReAddingAllRows.java @@ -11,6 +11,12 @@ import com.vaadin.ui.Button; import com.vaadin.ui.Button.ClickListener; import com.vaadin.ui.Table; +/** + * Scroll position should be restored when removing and re-adding all rows in + * Table. + * + * @author Vaadin Ltd + */ public class TableRepairsScrollPositionOnReAddingAllRows extends AbstractTestUI { private static final long serialVersionUID = 1L; @@ -19,32 +25,130 @@ public class TableRepairsScrollPositionOnReAddingAllRows extends AbstractTestUI protected void setup(VaadinRequest request) { final BeanItemContainer cont = new BeanItemContainer( TableItem.class); - final List itemList = new ArrayList(); + final List restoringItemList = new ArrayList(); - Button button1 = new Button("ReAdd rows"); - button1.setId("button1"); - button1.addClickListener(new ClickListener() { + final Table table = new Table(); + table.setWidth("400px"); + table.setPageLength(-1); + table.setContainerDataSource(cont); + table.setSelectable(true); + + Button buttonRestore = new Button("Restore table rows"); + buttonRestore.setId("buttonRestore"); + buttonRestore.addClickListener(new ClickListener() { @Override public void buttonClick(com.vaadin.ui.Button.ClickEvent event) { cont.removeAllItems(); - cont.addAll(itemList); + cont.addAll(restoringItemList); } }); + Button buttonReAddAllViaAddAll = new Button("Re-add rows all at once"); + buttonReAddAllViaAddAll.setId("buttonReAddAllViaAddAll"); + buttonReAddAllViaAddAll.addClickListener(new ClickListener() { + + @Override + public void buttonClick(com.vaadin.ui.Button.ClickEvent event) { + List originalItemIds = new ArrayList(cont + .getItemIds()); + cont.removeAllItems(); + cont.addAll(originalItemIds); + } + }); + + Button buttonReplaceByAnotherCollectionViaAddAll = new Button( + "Replace by another items (via addAll())"); + buttonReplaceByAnotherCollectionViaAddAll + .setId("buttonReplaceByAnotherCollectionViaAddAll"); + buttonReplaceByAnotherCollectionViaAddAll + .addClickListener(new ClickListener() { + + @Override + public void buttonClick( + com.vaadin.ui.Button.ClickEvent event) { + cont.removeAllItems(); + // create new collection (of different items) with other + // size + List itemList = new ArrayList(); + for (int i = 0; i < 79; i++) { + TableItem ti = new TableItem(); + ti.setName("AnotherItem1_" + i); + itemList.add(ti); + } + cont.addAll(itemList); + } + }); + + Button buttonReplaceByAnotherCollectionViaAdd = new Button( + "Replace by another items (via add(), add()..)"); + buttonReplaceByAnotherCollectionViaAdd + .setId("buttonReplaceByAnotherCollectionViaAdd"); + buttonReplaceByAnotherCollectionViaAdd + .addClickListener(new ClickListener() { + + @Override + public void buttonClick( + com.vaadin.ui.Button.ClickEvent event) { + cont.removeAllItems(); + for (int i = 0; i < 81; i++) { + TableItem ti = new TableItem(); + ti.setName("AnotherItem2_" + i); + // add one by one in container + cont.addBean(ti); + } + } + }); + + Button buttonReplaceBySubsetOfSmallerSize = new Button( + "Replace rows by sub-set of smaller size (size not enought for restoring scroll position)"); + buttonReplaceBySubsetOfSmallerSize + .setId("buttonReplaceBySubsetOfSmallerSize"); + buttonReplaceBySubsetOfSmallerSize + .addClickListener(new ClickListener() { + + @Override + public void buttonClick( + com.vaadin.ui.Button.ClickEvent event) { + cont.removeAllItems(); + cont.addAll(restoringItemList.subList(0, 20)); + } + }); + + Button buttonReplaceByWholeSubsetPlusOnNew = new Button( + "Replace rows by whole subset plus one new item"); + buttonReplaceByWholeSubsetPlusOnNew + .setId("buttonReplaceByWholeSubsetPlusOnNew"); + buttonReplaceByWholeSubsetPlusOnNew + .addClickListener(new ClickListener() { + + @Override + public void buttonClick( + com.vaadin.ui.Button.ClickEvent event) { + cont.removeAllItems(); + + List list = new ArrayList( + restoringItemList); + TableItem ti = new TableItem(); + ti.setName("AnotherItem3_" + 80); + list.add(ti); + cont.addAll(list); + } + }); + for (int i = 0; i < 80; i++) { TableItem ti = new TableItem(); - ti.setName("Name_" + i); - itemList.add(ti); + ti.setName("Item_" + i); + restoringItemList.add(ti); cont.addBean(ti); } - final Table table = new Table(); - table.setPageLength(-1); - table.setContainerDataSource(cont); - table.setSelectable(true); - - getLayout().addComponent(button1); + getLayout().addComponent(buttonReAddAllViaAddAll); + getLayout().addComponent(buttonReplaceByAnotherCollectionViaAddAll); + getLayout().addComponent(buttonReplaceByAnotherCollectionViaAdd); + getLayout().addComponent(buttonReplaceBySubsetOfSmallerSize); + getLayout().addComponent(buttonReplaceByWholeSubsetPlusOnNew); + getLayout().addComponent(buttonRestore); getLayout().addComponent(table); } diff --git a/uitest/src/com/vaadin/tests/components/table/TableRepairsScrollPositionOnReAddingAllRowsTest.java b/uitest/src/com/vaadin/tests/components/table/TableRepairsScrollPositionOnReAddingAllRowsTest.java index 2fbdd8b581..a3e7f29dfe 100644 --- a/uitest/src/com/vaadin/tests/components/table/TableRepairsScrollPositionOnReAddingAllRowsTest.java +++ b/uitest/src/com/vaadin/tests/components/table/TableRepairsScrollPositionOnReAddingAllRowsTest.java @@ -31,6 +31,12 @@ import com.vaadin.testbench.screenshot.ImageComparison; import com.vaadin.testbench.screenshot.ReferenceNameGenerator; import com.vaadin.tests.tb3.MultiBrowserTest; +/** + * Scroll position should be restored when removing and re-adding all rows in + * Table. + * + * @author Vaadin Ltd + */ public class TableRepairsScrollPositionOnReAddingAllRowsTest extends MultiBrowserTest { @@ -39,29 +45,126 @@ public class TableRepairsScrollPositionOnReAddingAllRowsTest extends throws InterruptedException { openTestURL(); - WebElement buttonReAddRows = findElement(By.id("button1")); + WebElement row0 = $(TableElement.class).first().getCell(0, 0); + int rowLocation0 = row0.getLocation().getY(); + // case 1 scrollUp(); waitUntilNot(new ExpectedCondition() { @Override public Boolean apply(WebDriver input) { - return $(TableElement.class).first().getCell(49, 0) == null; + return $(TableElement.class).first().getCell(48, 0) == null; } }, 10); - WebElement row = $(TableElement.class).first().getCell(49, 0); + WebElement row = $(TableElement.class).first().getCell(48, 0); int rowLocation = row.getLocation().getY(); - buttonReAddRows.click(); + // This button is for re-adding all rows (original itemIds) at once + // (removeAll() + addAll()) + hitButton("buttonReAddAllViaAddAll"); - row = $(TableElement.class).first().getCell(49, 0); + row = $(TableElement.class).first().getCell(48, 0); int newRowLocation = row.getLocation().getY(); // ranged check because IE9 consistently misses the mark by 1 pixel assertThat( - "Scroll position should be the same as before Re-Adding all rows", - (double) newRowLocation, closeTo(rowLocation, 1)); + "Scroll position should be the same as before Re-Adding rows via addAll()", + (double) newRowLocation, + closeTo(rowLocation, row.getSize().height + 1)); + + // case 2 + scrollUp(); + + waitUntilNot(new ExpectedCondition() { + @Override + public Boolean apply(WebDriver input) { + return $(TableElement.class).first().getCell(48, 0) == null; + } + }, 10); + + row = $(TableElement.class).first().getCell(48, 0); + rowLocation = row.getLocation().getY(); + + // This button is for replacing all rows at once (removeAll() + + // addAll()) + hitButton("buttonReplaceByAnotherCollectionViaAddAll"); + + row = $(TableElement.class).first().getCell(48, 0); + newRowLocation = row.getLocation().getY(); + + // ranged check because IE9 consistently misses the mark by 1 pixel + assertThat( + "Scroll position should be the same as before Replacing rows via addAll()", + (double) newRowLocation, + closeTo(rowLocation, row.getSize().height + 1)); + + // case 3 + // This button is for replacing all rows one by one (removeAll() + add() + // + add()..) + hitButton("buttonReplaceByAnotherCollectionViaAdd"); + + row = $(TableElement.class).first().getCell(0, 0); + newRowLocation = row.getLocation().getY(); + + // ranged check because IE9 consistently misses the mark by 1 pixel + assertThat("Scroll position should be 0", (double) newRowLocation, + closeTo(rowLocation0, 1)); + + // case 4 + // This button is for restoring initial list and for scrolling to 0 + // position + hitButton("buttonRestore"); + scrollUp(); + + waitUntilNot(new ExpectedCondition() { + @Override + public Boolean apply(WebDriver input) { + return $(TableElement.class).first().getCell(48, 0) == null; + } + }, 10); + + // This button is for replacing all rows at once but the count of rows + // is less then first index to scroll + hitButton("buttonReplaceBySubsetOfSmallerSize"); + + row = $(TableElement.class).first().getCell(5, 0); + + newRowLocation = row.getLocation().getY(); + + // ranged check because IE9 consistently misses the mark by 1 pixel + assertThat("Scroll position should be 0", (double) newRowLocation, + closeTo(rowLocation0, 1)); + + // case 5 + // This button is for restoring initial list and for scrolling to 0 + // position + hitButton("buttonRestore"); + scrollUp(); + + waitUntilNot(new ExpectedCondition() { + @Override + public Boolean apply(WebDriver input) { + return $(TableElement.class).first().getCell(48, 0) == null; + } + }, 10); + + row = $(TableElement.class).first().getCell(48, 0); + rowLocation = row.getLocation().getY(); + + // This button is for replacing by whole original sub-set of items plus + // one new + hitButton("buttonReplaceByWholeSubsetPlusOnNew"); + + row = $(TableElement.class).first().getCell(48, 0); + newRowLocation = row.getLocation().getY(); + + // ranged check because IE9 consistently misses the mark by 1 pixel + assertThat("Scroll position should be the same as before Replacing", + (double) newRowLocation, + closeTo(rowLocation, row.getSize().height + 1)); + } private void scrollUp() { @@ -69,6 +172,6 @@ public class TableRepairsScrollPositionOnReAddingAllRowsTest extends By.className("v-table-body-wrapper")); JavascriptExecutor js = new TestBenchCommandExecutor(getDriver(), new ImageComparison(), new ReferenceNameGenerator()); - js.executeScript("arguments[0].scrollTop = " + 1200, actualElement); + js.executeScript("arguments[0].scrollTop = " + 1205, actualElement); } }