Fixes two bugs when inserting escalator rows with spacers open (#16644)

If the viewport is not yet filled with escalator rows, it didn't account for
spacers. Also, the scrollbar was updated incorrectly after adding
new rows.

Change-Id: Id9cab71c2c4b82331771d1243143eb9db0883a6c

client/src/com/vaadin/client/widgets/Escalator.java

@@ -820,7 +820,8 @@ public class Escalator extends Widget implements RequiresResize,
          * that the sizes of the scroll handles appear correct in the browser
          */
         public void recalculateScrollbarsForVirtualViewport() {
-            double scrollContentHeight = body.calculateTotalRowHeight();
+            double scrollContentHeight = body.calculateTotalRowHeight()
+                    + body.spacerContainer.getSpacerHeightsSum();
             double scrollContentWidth = columnConfiguration.calculateRowWidth();
 
             double tableWrapperHeight = heightOfEscalator;
@@ -2420,6 +2421,13 @@ public class Escalator extends Widget implements RequiresResize,
                 return;
             }
 
+            /*
+             * TODO This will break the logical index calculation, as it will
+             * try to search for non-
+             */
+            getLogger().warning(
+                    "[[spacers]] scrolling and spacers near the bottom");
+
             boolean rowsWereMoved = false;
 
             final double topElementPosition;
@@ -2587,6 +2595,13 @@ public class Escalator extends Widget implements RequiresResize,
                 return;
             }
 
+            final double pxDiff = numberOfRows * getDefaultRowHeight();
+            for (SpacerContainer.SpacerImpl spacer : spacerContainer
+                    .getSpacersForRowAndAfter(index)) {
+                spacer.setPositionDiff(0, pxDiff);
+                spacer.setRowIndex(spacer.getRow() + numberOfRows);
+            }
+
             /*
              * TODO: this method should probably only add physical rows, and not
              * populate them - let everything be populated as appropriate by the
@@ -2632,13 +2647,6 @@ public class Escalator extends Widget implements RequiresResize,
                 final int visualOffset = getLogicalRowIndex(visualRowOrder
                         .getFirst());
 
-                final double pxDiff = numberOfRows * getDefaultRowHeight();
-                for (SpacerContainer.SpacerImpl spacer : spacerContainer
-                        .getSpacersForRowAndAfter(index)) {
-                    spacer.setPositionDiff(0, pxDiff);
-                    spacer.setRowIndex(spacer.getRow() + numberOfRows);
-                }
-
                 /*
                  * At this point, we have added new escalator rows, if so
                  * needed.
@@ -2648,33 +2656,33 @@ public class Escalator extends Widget implements RequiresResize,
                  * the remaining rows aswell.
                  */
                 final int rowsStillNeeded = numberOfRows - addedRows.size();
-                final Range unupdatedVisual = convertToVisual(Range.withLength(
-                        unupdatedLogicalStart, rowsStillNeeded));
-                final int end = getEscalatorRowCount();
-                final int start = end - unupdatedVisual.length();
-                final int visualTargetIndex = unupdatedLogicalStart
-                        - visualOffset;
-                moveAndUpdateEscalatorRows(Range.between(start, end),
-                        visualTargetIndex, unupdatedLogicalStart);
-
-                // move the surrounding rows to their correct places.
-                double rowTop = (unupdatedLogicalStart + (end - start))
-                        * getDefaultRowHeight();
-                final ListIterator<TableRowElement> i = visualRowOrder
-                        .listIterator(visualTargetIndex + (end - start));
-
-                int logicalRowIndexCursor = unupdatedLogicalStart;
-                while (i.hasNext()) {
-                    SpacerContainer.SpacerImpl spacer = spacerContainer
-                            .getSpacer(logicalRowIndexCursor);
-                    if (spacer != null) {
-                        rowTop += spacer.getHeight();
-                    }
-                    logicalRowIndexCursor++;
 
-                    final TableRowElement tr = i.next();
-                    setRowPosition(tr, 0, rowTop);
-                    rowTop += getDefaultRowHeight();
+                if (rowsStillNeeded > 0) {
+                    final Range unupdatedVisual = convertToVisual(Range
+                            .withLength(unupdatedLogicalStart, rowsStillNeeded));
+                    final int end = getEscalatorRowCount();
+                    final int start = end - unupdatedVisual.length();
+                    final int visualTargetIndex = unupdatedLogicalStart
+                            - visualOffset;
+                    moveAndUpdateEscalatorRows(Range.between(start, end),
+                            visualTargetIndex, unupdatedLogicalStart);
+
+                    // move the surrounding rows to their correct places.
+                    double rowTop = (unupdatedLogicalStart + (end - start))
+                            * getDefaultRowHeight();
+                    final ListIterator<TableRowElement> i = visualRowOrder
+                            .listIterator(visualTargetIndex + (end - start));
+
+                    int logicalRowIndexCursor = unupdatedLogicalStart;
+                    while (i.hasNext()) {
+                        rowTop += spacerContainer
+                                .getSpacerHeight(logicalRowIndexCursor++);
+
+                        final TableRowElement tr = i.next();
+                        getLogger().warning("iterate");
+                        setRowPosition(tr, 0, rowTop);
+                        rowTop += getDefaultRowHeight();
+                    }
                 }
 
                 fireRowVisibilityChangeEvent();
@@ -2786,11 +2794,8 @@ public class Escalator extends Widget implements RequiresResize,
                     setRowPosition(tr, 0, newRowTop);
 
                     newRowTop += getDefaultRowHeight();
-                    SpacerContainer.SpacerImpl spacer = spacerContainer
-                            .getSpacer(logicalTargetIndex + i);
-                    if (spacer != null) {
-                        newRowTop += spacer.getHeight();
-                    }
+                    newRowTop += spacerContainer
+                            .getSpacerHeight(logicalTargetIndex + i);
                 }
             }
         }
@@ -2879,25 +2884,29 @@ public class Escalator extends Widget implements RequiresResize,
                         index, escalatorRowsNeeded);
                 visualRowOrder.addAll(index, addedRows);
 
-                /*
-                 * We need to figure out the top positions for the rows we just
-                 * added.
-                 */
-                for (int i = 0; i < addedRows.size(); i++) {
-                    setRowPosition(addedRows.get(i), 0, (index + i)
-                            * getDefaultRowHeight());
-                }
+                double y = index * getDefaultRowHeight()
+                        + spacerContainer.getSpacerHeightsSumUntilIndex(index);
+                for (int i = index; i < visualRowOrder.size(); i++) {
+
+                    final TableRowElement tr;
+                    if (i - index < addedRows.size()) {
+                        tr = addedRows.get(i - index);
+                    } else {
+                        tr = visualRowOrder.get(i);
+                    }
 
-                /* Move the other rows away from above the added escalator rows */
-                for (int i = index + addedRows.size(); i < visualRowOrder
-                        .size(); i++) {
-                    final TableRowElement tr = visualRowOrder.get(i);
-                    setRowPosition(tr, 0, i * getDefaultRowHeight());
+                    getLogger().warning("y: " + y + ", index: " + i);
+                    setRowPosition(tr, 0, y);
+                    y += getDefaultRowHeight();
+                    double spacerHeight = spacerContainer.getSpacerHeight(i);
+                    getLogger().warning(
+                            "height: " + spacerHeight + ", index: " + i);
+                    y += spacerHeight;
                 }
 
                 return addedRows;
             } else {
-                return new ArrayList<TableRowElement>();
+                return Collections.emptyList();
             }
         }
 
@@ -4519,6 +4528,23 @@ public class Escalator extends Widget implements RequiresResize,
             }
         }
 
+        /**
+         * Calculates the sum of all spacers.
+         * 
+         * @return sum of all spacers, or 0 if no spacers present
+         */
+        public double getSpacerHeightsSum() {
+            return getHeights(rowIndexToSpacer.values());
+        }
+
+        /**
+         * Calculates the sum of all spacers from one row index onwards.
+         * 
+         * @param logicalRowIndex
+         *            the spacer to include as the first calculated spacer
+         * @return the sum of all spacers from {@code logicalRowIndex} and
+         *         onwards, or 0 if no suitable spacers were found
+         */
         @SuppressWarnings("boxing")
         public Collection<SpacerImpl> getSpacersForRowAndAfter(
                 int logicalRowIndex) {
@@ -4738,14 +4764,35 @@ public class Escalator extends Widget implements RequiresResize,
          */
         @SuppressWarnings("boxing")
         public double getSpacerHeightsSumUntilIndex(int logicalIndex) {
+            return getHeights(rowIndexToSpacer.headMap(logicalIndex, false)
+                    .values());
+        }
+
+        private double getHeights(Collection<SpacerImpl> spacers) {
             double heights = 0;
-            for (SpacerImpl spacer : rowIndexToSpacer.headMap(logicalIndex,
-                    false).values()) {
+            for (SpacerImpl spacer : spacers) {
                 heights += spacer.getHeight();
             }
             return heights;
         }
 
+        /**
+         * Gets the height of the spacer for a row index.
+         * 
+         * @param rowIndex
+         *            the index of the row where the spacer should be
+         * @return the height of the spacer at index {@code rowIndex}, or 0 if
+         *         there is no spacer there
+         */
+        public double getSpacerHeight(int rowIndex) {
+            SpacerImpl spacer = getSpacer(rowIndex);
+            if (spacer != null) {
+                return spacer.getHeight();
+            } else {
+                return 0;
+            }
+        }
+
         private boolean spacerExists(int rowIndex) {
             return rowIndexToSpacer.containsKey(Integer.valueOf(rowIndex));
         }

uitest/src/com/vaadin/tests/components/grid/basicfeatures/EscalatorBasicClientFeaturesTest.java

@@ -38,8 +38,10 @@ public abstract class EscalatorBasicClientFeaturesTest extends MultiBrowserTest
     protected static final String COLUMNS = "Columns";
     protected static final String ADD_ONE_COLUMN_TO_BEGINNING = "Add one column to beginning";
     protected static final String ADD_ONE_ROW_TO_BEGINNING = "Add one row to beginning";
+    protected static final String ADD_ONE_ROW_TO_END = "Add one row to end";
     protected static final String REMOVE_ONE_COLUMN_FROM_BEGINNING = "Remove one column from beginning";
     protected static final String REMOVE_ONE_ROW_FROM_BEGINNING = "Remove one row from beginning";
+    protected static final String REMOVE_ALL_ROWS = "Remove all rows";
     protected static final String REMOVE_50_ROWS_FROM_BOTTOM = "Remove 50 rows from bottom";
     protected static final String REMOVE_50_ROWS_FROM_ALMOST_BOTTOM = "Remove 50 rows from almost bottom";
     protected static final String ADD_ONE_OF_EACH_ROW = "Add one of each row";

uitest/src/com/vaadin/tests/components/grid/basicfeatures/escalator/EscalatorSpacerTest.java

@@ -25,10 +25,12 @@ import java.util.regex.Pattern;
 
 import org.junit.Before;
 import org.junit.Test;
+import org.openqa.selenium.WebElement;
 
-import com.vaadin.testbench.TestBenchElement;
+import com.vaadin.client.WidgetUtil;
 import com.vaadin.tests.components.grid.basicfeatures.EscalatorBasicClientFeaturesTest;
 
+@SuppressWarnings("boxing")
 public class EscalatorSpacerTest extends EscalatorBasicClientFeaturesTest {
 
     //@formatter:off
@@ -98,7 +100,6 @@ public class EscalatorSpacerTest extends EscalatorBasicClientFeaturesTest {
     }
 
     @Test
-    @SuppressWarnings("boxing")
     public void spacerPushesVisibleRowsDown() {
         double oldTop = getElementTop(getBodyRow(2));
         selectMenuPath(FEATURES, SPACERS, ROW_1, SET_100PX);
@@ -107,7 +108,51 @@ public class EscalatorSpacerTest extends EscalatorBasicClientFeaturesTest {
         assertGreater("Row below a spacer was not pushed down", newTop, oldTop);
     }
 
-    private static double getElementTop(TestBenchElement element) {
+    @Test
+    public void addingRowAboveSpacerPushesItDown() {
+        selectMenuPath(COLUMNS_AND_ROWS, BODY_ROWS, REMOVE_ALL_ROWS);
+        selectMenuPath(COLUMNS_AND_ROWS, BODY_ROWS, ADD_ONE_ROW_TO_BEGINNING);
+        selectMenuPath(COLUMNS_AND_ROWS, BODY_ROWS, ADD_ONE_ROW_TO_BEGINNING);
+
+        selectMenuPath(FEATURES, SPACERS, ROW_1, SET_100PX);
+        double oldTop = getElementTop(getSpacer(1));
+        selectMenuPath(COLUMNS_AND_ROWS, BODY_ROWS, ADD_ONE_ROW_TO_BEGINNING);
+        double newTop = getElementTop(getSpacer(1));
+
+        assertGreater("Spacer should've been pushed down", newTop, oldTop);
+    }
+
+    @Test
+    public void addingRowBelowSpacerDoesNotPushItDown() {
+        selectMenuPath(COLUMNS_AND_ROWS, BODY_ROWS, REMOVE_ALL_ROWS);
+        selectMenuPath(COLUMNS_AND_ROWS, BODY_ROWS, ADD_ONE_ROW_TO_BEGINNING);
+        selectMenuPath(COLUMNS_AND_ROWS, BODY_ROWS, ADD_ONE_ROW_TO_BEGINNING);
+
+        selectMenuPath(FEATURES, SPACERS, ROW_1, SET_100PX);
+        double oldTop = getElementTop(getSpacer(1));
+        selectMenuPath(COLUMNS_AND_ROWS, BODY_ROWS, ADD_ONE_ROW_TO_END);
+        double newTop = getElementTop(getSpacer(1));
+
+        assertEquals("Spacer should've not been pushed down", newTop, oldTop,
+                WidgetUtil.PIXEL_EPSILON);
+    }
+
+    @Test
+    public void addingRowBelowSpacerIsActuallyRenderedBelowWhenEscalatorIsEmpty() {
+        selectMenuPath(COLUMNS_AND_ROWS, BODY_ROWS, REMOVE_ALL_ROWS);
+        selectMenuPath(COLUMNS_AND_ROWS, BODY_ROWS, ADD_ONE_ROW_TO_BEGINNING);
+        selectMenuPath(COLUMNS_AND_ROWS, BODY_ROWS, ADD_ONE_ROW_TO_BEGINNING);
+
+        selectMenuPath(FEATURES, SPACERS, ROW_1, SET_100PX);
+        double spacerTop = getElementTop(getSpacer(1));
+        selectMenuPath(COLUMNS_AND_ROWS, BODY_ROWS, ADD_ONE_ROW_TO_END);
+        double rowTop = getElementTop(getBodyRow(2));
+
+        assertEquals("Next row should've been rendered below the spacer",
+                spacerTop + 100, rowTop, WidgetUtil.PIXEL_EPSILON);
+    }
+
+    private static double getElementTop(WebElement element) {
         /*
          * we need to parse the style attribute, since using getCssValue gets a
          * normalized value that is harder to parse.