From 407bdb39f7d87acaf15c399e46baf6d6a858fa6d Mon Sep 17 00:00:00 2001 From: Jarno Rantala Date: Fri, 3 Jan 2014 14:39:58 +0200 Subject: [PATCH] Ignores scroll events while update from server is in progress (#11454) When ItemSetChange event occurs, it will recreate the rows in client side. This will mess up the scroll position in scrollBodyPanel when its content is removed. This why the onScroll events should be ignored until the scroll position is reset by lazyScroller. Change-Id: Ib70e0dd7b730d4745a84742509145658e35d517e --- .../com/vaadin/client/ui/VScrollTable.java | 38 +- .../components/table/ExpandingContainer.java | 429 ++++++++++++++++++ ...ndingContainerVisibleRowRaceCondition.java | 67 +++ ...gContainerVisibleRowRaceConditionTest.java | 90 ++++ 4 files changed, 622 insertions(+), 2 deletions(-) create mode 100644 uitest/src/com/vaadin/tests/components/table/ExpandingContainer.java create mode 100644 uitest/src/com/vaadin/tests/components/table/ExpandingContainerVisibleRowRaceCondition.java create mode 100644 uitest/src/com/vaadin/tests/components/table/ExpandingContainerVisibleRowRaceConditionTest.java diff --git a/client/src/com/vaadin/client/ui/VScrollTable.java b/client/src/com/vaadin/client/ui/VScrollTable.java index c56a2a8772..bb431bf132 100644 --- a/client/src/com/vaadin/client/ui/VScrollTable.java +++ b/client/src/com/vaadin/client/ui/VScrollTable.java @@ -991,6 +991,12 @@ public class VScrollTable extends FlowPanel implements HasWidgets, if (scrollBody != null) { scrollBody.removeFromParent(); } + + // Without this call the scroll position is messed up in IE even after + // the lazy scroller has set the scroll position to the first visible + // item + scrollBodyPanel.getScrollPosition(); + scrollBody = createScrollBody(); scrollBody.renderInitialRows(rowData, uidl.getIntAttribute("firstrow"), @@ -1121,7 +1127,28 @@ public class VScrollTable extends FlowPanel implements HasWidgets, } } + private boolean lazyScrollerIsActive; + + private void disableLazyScroller() { + lazyScrollerIsActive = false; + scrollBodyPanel.getElement().getStyle().clearOverflowX(); + scrollBodyPanel.getElement().getStyle().clearOverflowY(); + } + + private void enableLazyScroller() { + Scheduler.get().scheduleDeferred(lazyScroller); + lazyScrollerIsActive = true; + // prevent scrolling to jump in IE11 + scrollBodyPanel.getElement().getStyle().setOverflowX(Overflow.HIDDEN); + scrollBodyPanel.getElement().getStyle().setOverflowY(Overflow.HIDDEN); + } + + private boolean isLazyScrollerActive() { + return lazyScrollerIsActive; + } + private ScheduledCommand lazyScroller = new ScheduledCommand() { + @Override public void execute() { if (firstvisible > 0) { @@ -1134,6 +1161,7 @@ public class VScrollTable extends FlowPanel implements HasWidgets, .setScrollPosition(measureRowHeightOffset(firstvisible)); } } + disableLazyScroller(); } }; @@ -1152,7 +1180,7 @@ public class VScrollTable extends FlowPanel implements HasWidgets, // Only scroll if the first visible changes from the server side. // Else we might unintentionally scroll even when the scroll // position has not changed. - Scheduler.get().scheduleDeferred(lazyScroller); + enableLazyScroller(); } } @@ -2172,7 +2200,7 @@ public class VScrollTable extends FlowPanel implements HasWidgets, isNewBody = false; if (firstvisible > 0) { - Scheduler.get().scheduleDeferred(lazyScroller); + enableLazyScroller(); } if (enabled) { @@ -6871,6 +6899,12 @@ public class VScrollTable extends FlowPanel implements HasWidgets, @Override public void onScroll(ScrollEvent event) { + // Do not handle scroll events while there is scroll initiated from + // server side which is not yet executed (#11454) + if (isLazyScrollerActive()) { + return; + } + scrollLeft = scrollBodyPanel.getElement().getScrollLeft(); scrollTop = scrollBodyPanel.getScrollPosition(); /* diff --git a/uitest/src/com/vaadin/tests/components/table/ExpandingContainer.java b/uitest/src/com/vaadin/tests/components/table/ExpandingContainer.java new file mode 100644 index 0000000000..829c29b95b --- /dev/null +++ b/uitest/src/com/vaadin/tests/components/table/ExpandingContainer.java @@ -0,0 +1,429 @@ +package com.vaadin.tests.components.table; + +import java.util.AbstractList; +import java.util.Arrays; +import java.util.Collection; +import java.util.List; +import java.util.logging.Logger; + +import com.vaadin.data.Container; +import com.vaadin.data.Item; +import com.vaadin.data.Property; +import com.vaadin.data.util.AbstractContainer; +import com.vaadin.data.util.BeanItem; +import com.vaadin.server.VaadinSession; +import com.vaadin.ui.Component; +import com.vaadin.ui.Label; + +@SuppressWarnings("serial") +public class ExpandingContainer extends AbstractContainer implements + Container.Ordered, Container.Indexed, Container.ItemSetChangeNotifier { + + public static final List PROPERTY_IDS = Arrays.asList("id", + "column1", "column2"); + + private final Label sizeLabel; + private final Logger log = Logger.getLogger(this.getClass().getName()); + + private int currentSize = 300; + + private boolean loggingEnabled; + + public ExpandingContainer(Label sizeLabel) { + this.sizeLabel = sizeLabel; + updateLabel(); + } + + private void log(String message) { + if (loggingEnabled) { + log.info(message); + } + } + + // Expand container if we scroll past 85% + public int checkExpand(int index) { + log("checkExpand(" + index + ")"); + if (index >= currentSize * 0.85) { + final int oldsize = currentSize; + currentSize = (int) (oldsize * 1.3333); + log("*** getSizeWithHint(" + index + "): went past 85% of size=" + + oldsize + ", new size=" + currentSize); + updateLabel(); + } + return currentSize; + }; + + @Override + public void fireItemSetChange() { + super.fireItemSetChange(); + } + + private void updateLabel() { + sizeLabel.setValue("Container size: " + currentSize); + } + + public void triggerItemSetChange() { + log("*** triggerItemSetChange(): scheduling item set change event"); + final VaadinSession session = VaadinSession.getCurrent(); + new Thread() { + @Override + public void run() { + ExpandingContainer.this.invoke(session, new Runnable() { + @Override + public void run() { + log("*** Firing item set change event"); + ExpandingContainer.this.fireItemSetChange(); + } + }); + } + }.start(); + } + + private void invoke(VaadinSession session, Runnable action) { + session.lock(); + VaadinSession previousSession = VaadinSession.getCurrent(); + VaadinSession.setCurrent(session); + try { + action.run(); + } finally { + session.unlock(); + VaadinSession.setCurrent(previousSession); + } + } + + // Container + + @Override + public BeanItem getItem(Object itemId) { + if (!(itemId instanceof Integer)) { + return null; + } + final int index = ((Integer) itemId).intValue(); + return new BeanItem(new MyBean(index)); + } + + @Override + public Collection getItemIds() { + return new IntList(size()); + } + + @Override + public List getContainerPropertyIds() { + return PROPERTY_IDS; + } + + @Override + @SuppressWarnings("rawtypes") + public Property/* */getContainerProperty(Object itemId, + Object propertyId) { + BeanItem item = getItem(itemId); + return item != null ? item.getItemProperty(propertyId) : null; + } + + @Override + public Class getType(Object propertyId) { + return Component.class; + } + + @Override + public int size() { + return currentSize; + } + + @Override + public boolean containsId(Object itemId) { + if (!(itemId instanceof Integer)) { + return false; + } + int index = ((Integer) itemId).intValue(); + checkExpand(index); + return index >= 0 && index < currentSize; + } + + /** + * @throws UnsupportedOperationException + * always + */ + @Override + public Item addItem(Object itemId) { + throw new UnsupportedOperationException(); + } + + /** + * @throws UnsupportedOperationException + * always + */ + @Override + public Item addItem() { + throw new UnsupportedOperationException(); + } + + /** + * @throws UnsupportedOperationException + * always + */ + @Override + public boolean removeItem(Object itemId) { + throw new UnsupportedOperationException(); + } + + /** + * @throws UnsupportedOperationException + * always + */ + @Override + public boolean addContainerProperty(Object propertyId, Class type, + Object defaultValue) { + throw new UnsupportedOperationException(); + } + + /** + * @throws UnsupportedOperationException + * always + */ + @Override + public boolean removeContainerProperty(Object propertyId) { + throw new UnsupportedOperationException(); + } + + /** + * @throws UnsupportedOperationException + * always + */ + @Override + public boolean removeAllItems() { + throw new UnsupportedOperationException(); + } + + // Container.Indexed + + /** + * @throws UnsupportedOperationException + * always + */ + @Override + public Object addItemAt(int index) { + throw new UnsupportedOperationException(); + } + + /** + * @throws UnsupportedOperationException + * always + */ + @Override + public Item addItemAt(int index, Object newItemId) { + throw new UnsupportedOperationException(); + } + + @Override + public Integer getIdByIndex(int index) { + if (index < 0) { + throw new IndexOutOfBoundsException("index < " + index); + } + final int size = currentSize; + if (index >= size) { + throw new IndexOutOfBoundsException("index=" + index + " but size=" + + size); + } + checkExpand(index); + return index; + } + + @Override + public List getItemIds(int startIndex, int numberOfItems) { + if (numberOfItems < 0) { + throw new IllegalArgumentException("numberOfItems < 0"); + } + final int size = currentSize; + checkExpand(startIndex); + if (startIndex < 0 || startIndex > size) { + throw new IndexOutOfBoundsException("startIndex=" + startIndex + + " but size=" + size); + } + if (startIndex + numberOfItems > size) { + numberOfItems = size - startIndex; + } + return new IntList(startIndex, numberOfItems); + } + + @Override + public int indexOfId(Object itemId) { + if (!(itemId instanceof Integer)) { + return -1; + } + final int index = ((Integer) itemId).intValue(); + checkExpand(index); + if (index < 0 || index >= currentSize) { + return -1; + } + return index; + } + + // Container.Ordered + + @Override + public Integer nextItemId(Object itemId) { + if (!(itemId instanceof Integer)) { + return null; + } + int index = ((Integer) itemId).intValue(); + checkExpand(index); + if (index < 0 || index + 1 >= currentSize) { + return null; + } + return index + 1; + } + + @Override + public Integer prevItemId(Object itemId) { + if (!(itemId instanceof Integer)) { + return null; + } + int index = ((Integer) itemId).intValue(); + checkExpand(index); + if (index - 1 < 0 || index >= currentSize) { + return null; + } + return index - 1; + } + + @Override + public Integer firstItemId() { + return currentSize == 0 ? null : 0; + } + + @Override + public Integer lastItemId() { + final int size = currentSize; + return size == 0 ? null : size - 1; + } + + @Override + public boolean isFirstId(Object itemId) { + if (!(itemId instanceof Integer)) { + return false; + } + final int index = ((Integer) itemId).intValue(); + checkExpand(index); + final int size = currentSize; + return size > 0 && index == 0; + } + + @Override + public boolean isLastId(Object itemId) { + if (!(itemId instanceof Integer)) { + return false; + } + int index = ((Integer) itemId).intValue(); + checkExpand(index); + int size = currentSize; + return size > 0 && index == size - 1; + } + + /** + * @throws UnsupportedOperationException + * always + */ + @Override + public Item addItemAfter(Object previousItemId) { + throw new UnsupportedOperationException(); + } + + /** + * @throws UnsupportedOperationException + * always + */ + @Override + public Item addItemAfter(Object previousItemId, Object newItemId) { + throw new UnsupportedOperationException(); + } + + // Container.ItemSetChangeNotifier + + @Override + @SuppressWarnings("deprecation") + public void addListener(Container.ItemSetChangeListener listener) { + super.addListener(listener); + } + + @Override + public void addItemSetChangeListener( + Container.ItemSetChangeListener listener) { + super.addItemSetChangeListener(listener); + } + + @Override + @SuppressWarnings("deprecation") + public void removeListener(Container.ItemSetChangeListener listener) { + super.removeListener(listener); + } + + @Override + public void removeItemSetChangeListener( + Container.ItemSetChangeListener listener) { + super.removeItemSetChangeListener(listener); + } + + // IntList + + private static class IntList extends AbstractList { + + private final int min; + private final int size; + + public IntList(int size) { + this(0, size); + } + + public IntList(int min, int size) { + if (size < 0) { + throw new IllegalArgumentException("size < 0"); + } + this.min = min; + this.size = size; + } + + @Override + public int size() { + return size; + } + + @Override + public Integer get(int index) { + if (index < 0 || index >= size) { + throw new IndexOutOfBoundsException(); + } + return min + index; + } + } + + // MyBean + public class MyBean { + + private final int index; + + public MyBean(int index) { + this.index = index; + } + + public String getId() { + return "ROW #" + index; + } + + public String getColumn1() { + return genText(); + } + + public String getColumn2() { + return genText(); + } + + private String genText() { + return "this is a line of text in row #" + index; + } + } + + public void logDetails(boolean enabled) { + loggingEnabled = enabled; + } +} diff --git a/uitest/src/com/vaadin/tests/components/table/ExpandingContainerVisibleRowRaceCondition.java b/uitest/src/com/vaadin/tests/components/table/ExpandingContainerVisibleRowRaceCondition.java new file mode 100644 index 0000000000..8ad2d7f9c7 --- /dev/null +++ b/uitest/src/com/vaadin/tests/components/table/ExpandingContainerVisibleRowRaceCondition.java @@ -0,0 +1,67 @@ +package com.vaadin.tests.components.table; + +import java.util.Map; + +import com.vaadin.server.VaadinRequest; +import com.vaadin.ui.Label; +import com.vaadin.ui.Table; +import com.vaadin.ui.UI; +import com.vaadin.ui.VerticalLayout; + +@SuppressWarnings("serial") +public class ExpandingContainerVisibleRowRaceCondition extends UI { + + static final String TABLE = "table"; + + @Override + public void init(VaadinRequest request) { + final VerticalLayout rootLayout = new VerticalLayout(); + + rootLayout.setSpacing(true); + rootLayout.setSizeFull(); + rootLayout.setMargin(true); + + final Label sizeLabel = new Label(); + final ExpandingContainer container = new ExpandingContainer(sizeLabel); + container.logDetails(false); + + Table table = new Table(null, container) { + @Override + public void changeVariables(Object source, + Map variables) { + if (variables.containsKey("firstvisible")) { + int index = (Integer) variables.get("firstvisible"); + container.checkExpand(index); + } + if (variables.containsKey("reqfirstrow") + || variables.containsKey("reqrows")) { + try { + int index = ((Integer) variables + .get("lastToBeRendered")).intValue(); + container.checkExpand(index); + } catch (Exception e) { + // do nothing + } + } + super.changeVariables(source, variables); + } + }; + table.setId(TABLE); + table.setCacheRate(0); + table.setSizeFull(); + table.setVisibleColumns(ExpandingContainer.PROPERTY_IDS + .toArray(new String[ExpandingContainer.PROPERTY_IDS.size()])); + + table.setCurrentPageFirstItemIndex(120); + + rootLayout.addComponent(table); + rootLayout.setExpandRatio(table, 1); + + rootLayout.addComponent(sizeLabel); + + setContent(rootLayout); + + container.checkExpand(300); + } + +} diff --git a/uitest/src/com/vaadin/tests/components/table/ExpandingContainerVisibleRowRaceConditionTest.java b/uitest/src/com/vaadin/tests/components/table/ExpandingContainerVisibleRowRaceConditionTest.java new file mode 100644 index 0000000000..73dd7b1f81 --- /dev/null +++ b/uitest/src/com/vaadin/tests/components/table/ExpandingContainerVisibleRowRaceConditionTest.java @@ -0,0 +1,90 @@ +/* + * Copyright 2000-2013 Vaadin Ltd. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ +package com.vaadin.tests.components.table; + +import static com.vaadin.tests.components.table.ExpandingContainerVisibleRowRaceCondition.TABLE; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.fail; + +import java.util.List; + +import org.junit.Test; +import org.openqa.selenium.By; +import org.openqa.selenium.WebElement; + +import com.vaadin.tests.tb3.MultiBrowserTest; + +public class ExpandingContainerVisibleRowRaceConditionTest extends + MultiBrowserTest { + + private static final int ROW_HEIGHT = 20; + + @Test + public void testScrollingWorksWithoutJumpingWhenItemSetChangeOccurs() { + openTestURL(); + sleep(1000); + + WebElement table = vaadinElementById(TABLE); + assertFirstRowIdIs("ROW #120"); + + testBenchElement(table.findElement(By.className("v-scrollable"))) + .scroll(320 * ROW_HEIGHT); + sleep(1000); + + assertRowIdIsInThePage("ROW #330"); + assertScrollPositionIsNotVisible(); + } + + @Override + protected void sleep(int milliseconds) { + try { + super.sleep(milliseconds); + } catch (InterruptedException e) { + e.printStackTrace(); + } + } + + private void assertFirstRowIdIs(String expected) { + List cellsOfFirstColumn = getCellsOfFirstColumn(); + WebElement first = cellsOfFirstColumn.get(0); + assertEquals(expected, first.getText()); + } + + private void assertRowIdIsInThePage(String expected) { + List cellsOfFirstColumn = getCellsOfFirstColumn(); + for (WebElement rowId : cellsOfFirstColumn) { + if (expected.equals(rowId.getText())) { + return; + } + } + fail("Expected row was not found"); + } + + private void assertScrollPositionIsNotVisible() { + WebElement table = vaadinElementById(TABLE); + WebElement scrollPosition = table.findElement(By + .className("v-table-scrollposition")); + assertFalse(scrollPosition.isDisplayed()); + } + + private List getCellsOfFirstColumn() { + WebElement table = vaadinElementById(TABLE); + List firstCellOfRows = table.findElements(By + .cssSelector(".v-table-table tr > td")); + return firstCellOfRows; + } +} -- 2.39.5