Hiding/Unhiding Grid column when details row is open (#17691)

Fixes paintRemoveColumns and paintInsertColumns in Escalator.AbstractStaticRowContainer
to not include spacers in row count.
Fixes couple ColumnHidingTests for IE8.
Change-Id: I283ee9fcdf0f3a7d0019948a700225c27a25d701

2 files changed, 62 insertions, 24 deletions

diff --git a/client/src/com/vaadin/client/widgets/Escalator.java b/client/src/com/vaadin/client/widgets/Escalator.java
index 0cd59ce7ed..a8797123f6 100644
--- a/client/src/com/vaadin/client/widgets/Escalator.java
+++ b/client/src/com/vaadin/client/widgets/Escalator.java
@@ -1345,6 +1345,20 @@ public class Escalator extends Widget implements RequiresResize,
         }
 
         /**
+         * This method calculates the current row count directly from the DOM.
+         * <p>
+         * While Escalator is stable, this value should equal to
+         * {@link #getRowCount()}, but while row counts are being updated, these
+         * two values might differ for a short while.
+         * <p>
+         * Any extra content, such as spacers for the body, should not be
+         * included in this count.
+         * 
+         * @return the actual DOM count of rows
+         */
+        public abstract int getDomRowCount();
+
+        /**
          * {@inheritDoc}
          * <p>
          * <em>Implementation detail:</em> This method does no DOM modifications
@@ -1603,7 +1617,7 @@ public class Escalator extends Widget implements RequiresResize,
 
         protected void paintRemoveColumns(final int offset,
                 final int numberOfColumns) {
-            for (int i = 0; i < root.getChildCount(); i++) {
+            for (int i = 0; i < getDomRowCount(); i++) {
                 TableRowElement row = getTrByVisualIndex(i);
                 flyweightRow.setup(row, i,
                         columnConfiguration.getCalculatedColumnWidths());
@@ -1627,7 +1641,7 @@ public class Escalator extends Widget implements RequiresResize,
         protected void paintInsertColumns(final int offset,
                 final int numberOfColumns, boolean frozen) {
 
-            for (int row = 0; row < root.getChildCount(); row++) {
+            for (int row = 0; row < getDomRowCount(); row++) {
                 final TableRowElement tr = getTrByVisualIndex(row);
                 paintInsertCells(tr, row, offset, numberOfColumns);
             }
@@ -2120,6 +2134,11 @@ public class Escalator extends Widget implements RequiresResize,
         }
 
         @Override
+        public int getDomRowCount() {
+            return root.getChildCount();
+        }
+
+        @Override
         protected void paintRemoveRows(final int index, final int numberOfRows) {
             for (int i = index; i < index + numberOfRows; i++) {
                 final TableRowElement tr = root.getRows().getItem(index);
@@ -2697,7 +2716,7 @@ public class Escalator extends Widget implements RequiresResize,
                 if (rowsStillNeeded > 0) {
                     final Range unupdatedVisual = convertToVisual(Range
                             .withLength(unupdatedLogicalStart, rowsStillNeeded));
-                    final int end = getEscalatorRowCount();
+                    final int end = getDomRowCount();
                     final int start = end - unupdatedVisual.length();
                     final int visualTargetIndex = unupdatedLogicalStart
                             - visualOffset;
@@ -2755,11 +2774,10 @@ public class Escalator extends Widget implements RequiresResize,
             assert visualTargetIndex >= 0 : "Visual target must be 0 or greater (was "
                     + visualTargetIndex + ")";
 
-            assert visualTargetIndex <= getEscalatorRowCount() : "Visual target "
+            assert visualTargetIndex <= getDomRowCount() : "Visual target "
                     + "must not be greater than the number of escalator rows (was "
-                    + visualTargetIndex
-                    + ", escalator rows "
-                    + getEscalatorRowCount() + ")";
+                    + visualTargetIndex + ", escalator rows "
+                    + getDomRowCount() + ")";
 
             assert logicalTargetIndex + visualSourceRange.length() <= getRowCount() : "Logical "
                     + "target leads to rows outside of the data range ("
@@ -2910,7 +2928,7 @@ public class Escalator extends Widget implements RequiresResize,
                 final int index, final int numberOfRows) {
 
             final int escalatorRowsStillFit = getMaxEscalatorRowCapacity()
-                    - getEscalatorRowCount();
+                    - getDomRowCount();
             final int escalatorRowsNeeded = Math.min(numberOfRows,
                     escalatorRowsStillFit);
 
@@ -3036,7 +3054,7 @@ public class Escalator extends Widget implements RequiresResize,
 
             // ranges evaluated, let's do things.
             if (!removedVisualInside.isEmpty()) {
-                int escalatorRowCount = body.getEscalatorRowCount();
+                int escalatorRowCount = body.getDomRowCount();
 
                 /*
                  * remember: the rows have already been subtracted from the row
@@ -3899,17 +3917,8 @@ public class Escalator extends Widget implements RequiresResize,
             }
         }
 
-        /**
-         * This method calculates the current escalator row count directly from
-         * the DOM.
-         * <p>
-         * While Escalator is stable, this value should equal to
-         * {@link #visualRowOrder}.size(), but while row counts are being
-         * updated, these two values might differ for a short while.
-         * 
-         * @return the actual DOM count of escalator rows
-         */
-        public int getEscalatorRowCount() {
+        @Override
+        public int getDomRowCount() {
             return root.getChildCount()
                     - spacerContainer.getSpacersInDom().size();
         }
@@ -5814,7 +5823,7 @@ public class Escalator extends Widget implements RequiresResize,
          * updated correctly. Since it isn't, we'll simply and brutally rip out
          * the DOM elements (in an elegant way, of course).
          */
-        int rowsToRemove = body.getEscalatorRowCount();
+        int rowsToRemove = body.getDomRowCount();
         for (int i = 0; i < rowsToRemove; i++) {
             int index = rowsToRemove - i - 1;
             TableRowElement tr = bodyElem.getRows().getItem(index);
diff --git a/uitest/src/com/vaadin/tests/components/grid/basicfeatures/GridColumnHidingTest.java b/uitest/src/com/vaadin/tests/components/grid/basicfeatures/GridColumnHidingTest.java
index b446bdef48..c4ad2ea347 100644
--- a/uitest/src/com/vaadin/tests/components/grid/basicfeatures/GridColumnHidingTest.java
+++ b/uitest/src/com/vaadin/tests/components/grid/basicfeatures/GridColumnHidingTest.java
@@ -23,6 +23,7 @@ import static org.junit.Assert.assertTrue;
 
 import java.util.List;
 
+import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Ignore;
 import org.junit.Test;
@@ -818,6 +819,27 @@ public class GridColumnHidingTest extends GridBasicClientFeaturesTest {
         verifyHeaderCellColspan(1, 4, 1);
     }
 
+    @Test
+    public void testColumnHiding_detailsRowIsOpen_renderedCorrectly() {
+        selectMenuPath("Component", "Row details", "Toggle details for...",
+                "Row 1");
+        assertColumnHeaderOrder(0, 1, 2, 3, 4);
+        Assert.assertNotNull("Details not found", getGridElement()
+                .getDetails(1));
+
+        toggleHideColumnAPI(0);
+
+        assertColumnHeaderOrder(1, 2, 3, 4);
+        Assert.assertNotNull("Details not found", getGridElement()
+                .getDetails(1));
+
+        toggleHideColumnAPI(0);
+
+        assertColumnHeaderOrder(0, 1, 2, 3, 4);
+        Assert.assertNotNull("Details not found", getGridElement()
+                .getDetails(1));
+    }
+
     private void loadSpannedCellsFixture() {
         selectMenuPath("Component", "State", "Width", "1000px");
         appendHeaderRow();
@@ -854,9 +876,16 @@ public class GridColumnHidingTest extends GridBasicClientFeaturesTest {
     }
 
     private void verifyHeaderCellColspan(int row, int column, int colspan) {
-        assertEquals(Integer.valueOf(colspan), Integer.valueOf(Integer
-                .parseInt(getGridElement().getHeaderCell(row, column)
-                        .getAttribute("colspan"))));
+        try {
+            assertEquals(Integer.valueOf(colspan), Integer.valueOf(Integer
+                    .parseInt(getGridElement().getHeaderCell(row, column)
+                            .getAttribute("colspan"))));
+        } catch (NumberFormatException nfe) {
+            // IE8 has colSpan
+            assertEquals(Integer.valueOf(colspan), Integer.valueOf(Integer
+                    .parseInt(getGridElement().getHeaderCell(row, column)
+                            .getAttribute("colSpan"))));
+        }
     }
 
     private void verifyNumberOfCellsInHeader(int row, int numberOfCells) {