Fix issues in Grid with undefined height (#9104)

4 files changed, 183 insertions, 64 deletions

diff --git a/client/src/main/java/com/vaadin/client/widgets/Escalator.java b/client/src/main/java/com/vaadin/client/widgets/Escalator.java
index 11bfa113cb..8801987104 100644
--- a/client/src/main/java/com/vaadin/client/widgets/Escalator.java
+++ b/client/src/main/java/com/vaadin/client/widgets/Escalator.java
@@ -195,7 +195,6 @@ abstract class JsniWorkaround {
      * to Java code.
      *
      * @see #createScrollListenerFunction(Escalator)
-     * @see Escalator#onScroll()
      * @see Escalator.Scroller#onScroll()
      */
     protected final JavaScriptObject scrollListenerFunction;
@@ -205,7 +204,6 @@ abstract class JsniWorkaround {
      * it on to Java code.
      *
      * @see #createMousewheelListenerFunction(Escalator)
-     * @see Escalator#onScroll()
      * @see Escalator.Scroller#onScroll()
      */
     protected final JavaScriptObject mousewheelListenerFunction;
@@ -253,7 +251,7 @@ abstract class JsniWorkaround {
      *
      * @param esc
      *            a reference to the current instance of {@link Escalator}
-     * @see Escalator#onScroll()
+     * @see Escalator.Scroller#onScroll()
      */
     protected abstract JavaScriptObject createScrollListenerFunction(
             Escalator esc);
@@ -264,7 +262,7 @@ abstract class JsniWorkaround {
      *
      * @param esc
      *            a reference to the current instance of {@link Escalator}
-     * @see Escalator#onScroll()
+     * @see Escalator.Scroller#onScroll()
      */
     protected abstract JavaScriptObject createMousewheelListenerFunction(
             Escalator esc);
@@ -1139,10 +1137,10 @@ public class Escalator extends Widget
          * Usually {@code "th"} or {@code "td"}.
          * <p>
          * <em>Note:</em> To actually <em>create</em> such an element, use
-         * {@link #createCellElement(int, int)} instead.
+         * {@link #createCellElement(double)} instead.
          *
          * @return the tag name for the element to represent cells as
-         * @see #createCellElement(int, int)
+         * @see #createCellElement(double)
          */
         protected abstract String getCellElementTagName();
 
@@ -1189,9 +1187,6 @@ public class Escalator extends Widget
             assertArgumentsAreValidAndWithinRange(index, numberOfRows);
 
             rows -= numberOfRows;
-            if (heightMode == HeightMode.UNDEFINED) {
-                heightByRows = rows;
-            }
 
             if (!isAttached()) {
                 return;
@@ -1207,8 +1202,9 @@ public class Escalator extends Widget
          * range of logical indices. This may be fewer than {@code numberOfRows}
          * , even zero, if not all the removed rows are actually visible.
          * <p>
-         * The implementation must call {@link #paintRemoveRow(Element, int)}
-         * for each row that is removed from the DOM.
+         * The implementation must call
+         * {@link #paintRemoveRow(TableRowElement, int)} for each row that is
+         * removed from the DOM.
          *
          * @param index
          *            the logical index of the first removed row
@@ -1315,10 +1311,6 @@ public class Escalator extends Widget
             }
 
             rows += numberOfRows;
-            if (heightMode == HeightMode.UNDEFINED) {
-                heightByRows = rows;
-            }
-
             /*
              * only add items in the DOM if the widget itself is attached to the
              * DOM. We can't calculate sizes otherwise.
@@ -2671,6 +2663,24 @@ public class Escalator extends Widget
         }
 
         @Override
+        public void insertRows(int index, int numberOfRows) {
+            super.insertRows(index, numberOfRows);
+
+            if (heightMode == HeightMode.UNDEFINED) {
+                setHeightByRows(getRowCount());
+            }
+        }
+
+        @Override
+        public void removeRows(int index, int numberOfRows) {
+            super.removeRows(index, numberOfRows);
+
+            if (heightMode == HeightMode.UNDEFINED) {
+                setHeightByRows(getRowCount());
+            }
+        }
+
+        @Override
         protected void paintInsertRows(final int index,
                 final int numberOfRows) {
             if (numberOfRows == 0) {
@@ -3111,44 +3121,27 @@ public class Escalator extends Widget
                  */
                 int rowsLeft = getRowCount();
                 if (rowsLeft < escalatorRowCount) {
-                    int escalatorRowsToRemove = escalatorRowCount - rowsLeft;
-                    for (int i = 0; i < escalatorRowsToRemove; i++) {
-                        final TableRowElement tr = visualRowOrder
-                                .remove(removedVisualInside.getStart());
-
-                        paintRemoveRow(tr, index);
+                    /*
+                     * Remove extra DOM rows and refresh contents.
+                     */
+                    for (int i = escalatorRowCount - 1; i >= rowsLeft; i--) {
+                        final TableRowElement tr = visualRowOrder.remove(i);
+                        paintRemoveRow(tr, i);
                         removeRowPosition(tr);
                     }
-                    escalatorRowCount -= escalatorRowsToRemove;
 
-                    /*
-                     * Because we're removing escalator rows, we don't have
-                     * anything to scroll by. Let's make sure the viewport is
-                     * scrolled to top, to render any rows possibly left above.
-                     */
-                    body.setBodyScrollPosition(tBodyScrollLeft, 0);
+                    // Move rest of the rows to the Escalator's top
+                    Range visualRange = Range.withLength(0,
+                            visualRowOrder.size());
+                    moveAndUpdateEscalatorRows(visualRange, 0, 0);
 
-                    /*
-                     * We might have removed some rows from the middle, so let's
-                     * make sure we're not left with any holes. Also remember:
-                     * visualIndex == logicalIndex applies now.
-                     */
-                    final int dirtyRowsStart = removedLogicalInside.getStart();
-                    double y = getRowTop(dirtyRowsStart);
-                    for (int i = dirtyRowsStart; i < escalatorRowCount; i++) {
-                        final TableRowElement tr = visualRowOrder.get(i);
-                        setRowPosition(tr, 0, y);
-                        y += getDefaultRowHeight();
-                        y += spacerContainer.getSpacerHeight(i);
-                    }
+                    sortDomElements();
+                    setTopRowLogicalIndex(0);
 
-                    // #8825 update data starting from the first moved row
-                    final int start = dirtyRowsStart;
-                    final int end = escalatorRowCount;
-                    for (int i = start; i < end; i++) {
-                        final TableRowElement tr = visualRowOrder.get(i);
-                        refreshRow(tr, i);
-                    }
+                    scroller.recalculateScrollbarsForVirtualViewport();
+
+                    fireRowVisibilityChangeEvent();
+                    return;
                 }
 
                 else {
@@ -6444,11 +6437,12 @@ public class Escalator extends Widget
      * @param rows
      *            the number of rows that should be visible in Escalator's body
      * @throws IllegalArgumentException
-     *             if {@code rows} is &leq; 0, {@link Double#isInifinite(double)
+     *             if {@code rows} is &leq; 0, {@link Double#isInfinite(double)
      *             infinite} or {@link Double#isNaN(double) NaN}.
      * @see #setHeightMode(HeightMode)
      */
     public void setHeightByRows(double rows) throws IllegalArgumentException {
+        getLogger().warning("HeightByRows: " + rows);
         if (rows <= 0) {
             throw new IllegalArgumentException(
                     "The number of rows must be a positive number.");
diff --git a/compatibility-client/src/main/java/com/vaadin/v7/client/widgets/Escalator.java b/compatibility-client/src/main/java/com/vaadin/v7/client/widgets/Escalator.java
index f022d691b7..4eea41c0ff 100644
--- a/compatibility-client/src/main/java/com/vaadin/v7/client/widgets/Escalator.java
+++ b/compatibility-client/src/main/java/com/vaadin/v7/client/widgets/Escalator.java
@@ -703,13 +703,13 @@ public class Escalator extends Widget
         /*-{
             var vScroll = esc.@com.vaadin.v7.client.widgets.Escalator::verticalScrollbar;
             var vScrollElem = vScroll.@com.vaadin.v7.client.widget.escalator.ScrollbarBundle::getElement()();
-        
+
             var hScroll = esc.@com.vaadin.v7.client.widgets.Escalator::horizontalScrollbar;
             var hScrollElem = hScroll.@com.vaadin.v7.client.widget.escalator.ScrollbarBundle::getElement()();
-        
+
             return $entry(function(e) {
                 var target = e.target;
-        
+
                 // in case the scroll event was native (i.e. scrollbars were dragged, or
                 // the scrollTop/Left was manually modified), the bundles have old cache
                 // values. We need to make sure that the caches are kept up to date.
@@ -730,29 +730,29 @@ public class Escalator extends Widget
             return $entry(function(e) {
                 var deltaX = e.deltaX ? e.deltaX : -0.5*e.wheelDeltaX;
                 var deltaY = e.deltaY ? e.deltaY : -0.5*e.wheelDeltaY;
-        
+
                 // Delta mode 0 is in pixels; we don't need to do anything...
-        
+
                 // A delta mode of 1 means we're scrolling by lines instead of pixels
                 // We need to scale the number of lines by the default line height
                 if(e.deltaMode === 1) {
                     var brc = esc.@com.vaadin.v7.client.widgets.Escalator::body;
                     deltaY *= brc.@com.vaadin.v7.client.widgets.Escalator.AbstractRowContainer::getDefaultRowHeight()();
                 }
-        
+
                 // Other delta modes aren't supported
                 if((e.deltaMode !== undefined) && (e.deltaMode >= 2 || e.deltaMode < 0)) {
                     var msg = "Unsupported wheel delta mode \"" + e.deltaMode + "\"";
-        
+
                     // Print warning message
                     esc.@com.vaadin.v7.client.widgets.Escalator::logWarning(*)(msg);
                 }
-        
+
                 // IE8 has only delta y
                 if (isNaN(deltaY)) {
                     deltaY = -0.5*e.wheelDelta;
                 }
-        
+
                 @com.vaadin.v7.client.widgets.Escalator.JsniUtil::moveScrollFromEvent(*)(esc, deltaX, deltaY, e);
             });
         }-*/;
@@ -1208,9 +1208,6 @@ public class Escalator extends Widget
             assertArgumentsAreValidAndWithinRange(index, numberOfRows);
 
             rows -= numberOfRows;
-            if (heightMode == HeightMode.UNDEFINED) {
-                heightByRows = rows;
-            }
 
             if (!isAttached()) {
                 return;
@@ -1334,9 +1331,6 @@ public class Escalator extends Widget
             }
 
             rows += numberOfRows;
-            if (heightMode == HeightMode.UNDEFINED) {
-                heightByRows = rows;
-            }
 
             /*
              * only add items in the DOM if the widget itself is attached to the
@@ -2514,6 +2508,24 @@ public class Escalator extends Widget
         }
 
         @Override
+        public void insertRows(int index, int numberOfRows) {
+            super.insertRows(index, numberOfRows);
+
+            if (heightMode == HeightMode.UNDEFINED) {
+                heightByRows = getRowCount();
+            }
+        }
+
+        @Override
+        public void removeRows(int index, int numberOfRows) {
+            super.removeRows(index, numberOfRows);
+
+            if (heightMode == HeightMode.UNDEFINED) {
+                heightByRows = getRowCount();
+            }
+        }
+
+        @Override
         public void setStylePrimaryName(String primaryStyleName) {
             super.setStylePrimaryName(primaryStyleName);
             UIObject.setStylePrimaryName(root, primaryStyleName + "-body");
diff --git a/uitest/src/main/java/com/vaadin/tests/components/grid/GridUndefinedHeight.java b/uitest/src/main/java/com/vaadin/tests/components/grid/GridUndefinedHeight.java
new file mode 100644
index 0000000000..77480b3eef
--- /dev/null
+++ b/uitest/src/main/java/com/vaadin/tests/components/grid/GridUndefinedHeight.java
@@ -0,0 +1,39 @@
+package com.vaadin.tests.components.grid;
+
+import com.vaadin.server.VaadinRequest;
+import com.vaadin.shared.ui.grid.HeightMode;
+import com.vaadin.tests.components.AbstractTestUI;
+import com.vaadin.ui.Button;
+import com.vaadin.ui.Grid;
+import com.vaadin.ui.VerticalLayout;
+
+public class GridUndefinedHeight extends AbstractTestUI {
+
+    @Override
+    protected void setup(VaadinRequest request) {
+        VerticalLayout layout = new VerticalLayout();
+
+        Grid<String> grid = new Grid<>();
+        grid.setItems("Foo", "Bar", "Baz");
+        grid.setHeightMode(HeightMode.UNDEFINED);
+        grid.addColumn(Object::toString).setCaption("toString()");
+
+        com.vaadin.v7.ui.Grid oldGrid = new com.vaadin.v7.ui.Grid();
+        oldGrid.addColumn("toString", String.class);
+        oldGrid.addRow("Foo");
+        oldGrid.addRow("Bar");
+        oldGrid.addRow("Baz");
+        oldGrid.setHeightMode(
+                com.vaadin.v7.shared.ui.grid.HeightMode.UNDEFINED);
+
+        layout.addComponents(grid, oldGrid, new Button("Add header row", e -> {
+            grid.appendHeaderRow();
+            oldGrid.appendHeaderRow();
+        }));
+        layout.setHeight("600px");
+        layout.setExpandRatio(grid, 1.0f);
+        layout.setExpandRatio(oldGrid, 1.0f);
+        addComponent(layout);
+    }
+
+}
diff --git a/uitest/src/test/java/com/vaadin/tests/components/grid/GridUndefinedHeightTest.java b/uitest/src/test/java/com/vaadin/tests/components/grid/GridUndefinedHeightTest.java
new file mode 100644
index 0000000000..07ccf6a95b
--- /dev/null
+++ b/uitest/src/test/java/com/vaadin/tests/components/grid/GridUndefinedHeightTest.java
@@ -0,0 +1,74 @@
+package com.vaadin.tests.components.grid;
+
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+import com.vaadin.testbench.elements.ButtonElement;
+import com.vaadin.testbench.elements.GridElement;
+import com.vaadin.testbench.parallel.TestCategory;
+import com.vaadin.tests.tb3.SingleBrowserTest;
+
+@TestCategory("grid")
+public class GridUndefinedHeightTest extends SingleBrowserTest {
+
+    @Before
+    public void before() {
+        setDebug(true);
+        openTestURL();
+    }
+
+    @Test
+    public void grid_undefined_height() {
+        GridElement grid = $(GridElement.class).first();
+        int oneRow = grid.getRow(0).getSize().getHeight();
+        int gridHeight = grid.getSize().getHeight();
+        int rows = 4; // Header Row + 3 Body Rows
+
+        Assert.assertEquals("Grid height mismatch", oneRow * rows, gridHeight);
+
+        assertNoErrorNotifications();
+    }
+
+    @Test
+    public void gridv7_undefined_height() {
+        GridElement grid = $(GridElement.class).get(1);
+        int oneRow = grid.getRow(0).getSize().getHeight();
+        int gridHeight = grid.getSize().getHeight();
+        int rows = 4; // Header Row + 3 Body Rows
+
+        Assert.assertEquals("Grid height mismatch", oneRow * rows, gridHeight);
+
+        assertNoErrorNotifications();
+    }
+
+    @Test
+    public void grid_undefined_height_add_header() {
+        // Add header row to Grid
+        $(ButtonElement.class).first().click();
+
+        GridElement grid = $(GridElement.class).first();
+        int oneRow = grid.getRow(0).getSize().getHeight();
+        int gridHeight = grid.getSize().getHeight();
+        int rows = 5; // 2 Header Rows + 3 Body Rows
+
+        Assert.assertEquals("Grid height mismatch", oneRow * rows, gridHeight);
+
+        assertNoErrorNotifications();
+    }
+
+    @Test
+    public void gridv7_undefined_height_add_header() {
+        // Add header row to Grid
+        $(ButtonElement.class).first().click();
+
+        GridElement grid = $(GridElement.class).get(1);
+        int oneRow = grid.getRow(0).getSize().getHeight();
+        int gridHeight = grid.getSize().getHeight();
+        int rows = 5; // 2 Header Rows + 3 Body Rows
+
+        Assert.assertEquals("Grid height mismatch", oneRow * rows, gridHeight);
+
+        assertNoErrorNotifications();
+    }
+}