Details row decorator and border positioning and sizes (#17423)

IE8 still isn't pixel perfect, but you can't have 'em all.

Change-Id: I1780441f130032503d783657103066f502dce570

8 files changed, 246 insertions, 63 deletions

diff --git a/WebContent/VAADIN/themes/base/escalator/escalator.scss b/WebContent/VAADIN/themes/base/escalator/escalator.scss
index bae95b299c..73d45854b9 100644
--- a/WebContent/VAADIN/themes/base/escalator/escalator.scss
+++ b/WebContent/VAADIN/themes/base/escalator/escalator.scss
@@ -136,20 +136,14 @@
   .#{$primaryStyleName}-spacer {
     position: absolute;
     display: block;
-    
+
     background-color: $background-color;
-    
+
     > td {
       width: 100%;
       height: 100%;
+      @include box-sizing(border-box);
     }
     
-    .v-ie8 &, .v-ie9 & {
-      // The inline style of margin-top from the <tbody> to offset the
-      // header's dimension is, for some strange reason, inherited into each
-      // contained <tr>. We need to cancel it:
-
-      margin-top: 0;
-    }
   }
 }
diff --git a/WebContent/VAADIN/themes/base/grid/grid.scss b/WebContent/VAADIN/themes/base/grid/grid.scss
index ee503822b9..531abb1ff1 100644
--- a/WebContent/VAADIN/themes/base/grid/grid.scss
+++ b/WebContent/VAADIN/themes/base/grid/grid.scss
@@ -456,8 +456,9 @@ $v-grid-details-border-bottom-stripe: 1px solid darken($v-grid-row-background-co
   }
   
   .#{$primaryStyleName}-spacer-deco-container {
+    border-top: $v-grid-border-size solid transparent; // same size as table wrapper border 
     position: relative;
-    top: $v-grid-border-size;
+    top: 0; // escalator will override top for scrolling and margin-top for header offset.
     z-index: 5;
   }
 
diff --git a/client/src/com/vaadin/client/WidgetUtil.java b/client/src/com/vaadin/client/WidgetUtil.java
index 5f88f6da46..a9cd23c841 100644
--- a/client/src/com/vaadin/client/WidgetUtil.java
+++ b/client/src/com/vaadin/client/WidgetUtil.java
@@ -1459,4 +1459,124 @@ public class WidgetUtil {
         return Logger.getLogger(WidgetUtil.class.getName());
     }
 
+    /**
+     * Returns the thickness of the given element's top border.
+     * <p>
+     * The value is determined using computed style when available and
+     * calculated otherwise.
+     * 
+     * @since
+     * @param element
+     *            the element to measure
+     * @return the top border thickness
+     */
+    public static double getBorderTopThickness(Element element) {
+        return getBorderThickness(element, new String[] { "borderTopWidth" });
+    }
+
+    /**
+     * Returns the thickness of the given element's bottom border.
+     * <p>
+     * The value is determined using computed style when available and
+     * calculated otherwise.
+     * 
+     * @since
+     * @param element
+     *            the element to measure
+     * @return the bottom border thickness
+     */
+    public static double getBorderBottomThickness(Element element) {
+        return getBorderThickness(element, new String[] { "borderBottomWidth" });
+    }
+
+    /**
+     * Returns the combined thickness of the given element's top and bottom
+     * borders.
+     * <p>
+     * The value is determined using computed style when available and
+     * calculated otherwise.
+     * 
+     * @since
+     * @param element
+     *            the element to measure
+     * @return the top and bottom border thickness
+     */
+    public static double getBorderTopAndBottomThickness(Element element) {
+        return getBorderThickness(element, new String[] { "borderTopWidth",
+                "borderBottomWidth" });
+    }
+
+    /**
+     * Returns the thickness of the given element's left border.
+     * <p>
+     * The value is determined using computed style when available and
+     * calculated otherwise.
+     * 
+     * @since
+     * @param element
+     *            the element to measure
+     * @return the left border thickness
+     */
+    public static double getBorderLeftThickness(Element element) {
+        return getBorderThickness(element, new String[] { "borderLeftWidth" });
+    }
+
+    /**
+     * Returns the thickness of the given element's right border.
+     * <p>
+     * The value is determined using computed style when available and
+     * calculated otherwise.
+     * 
+     * @since
+     * @param element
+     *            the element to measure
+     * @return the right border thickness
+     */
+    public static double getBorderRightThickness(Element element) {
+        return getBorderThickness(element, new String[] { "borderRightWidth" });
+    }
+
+    /**
+     * Returns the thickness of the given element's left and right borders.
+     * <p>
+     * The value is determined using computed style when available and
+     * calculated otherwise.
+     * 
+     * @since
+     * @param element
+     *            the element to measure
+     * @return the top border thickness
+     */
+    public static double getBorderLeftAndRightThickness(Element element) {
+        return getBorderThickness(element, new String[] { "borderLeftWidth",
+                "borderRightWidth" });
+    }
+
+    private static native double getBorderThickness(
+            com.google.gwt.dom.client.Element element, String[] borderNames)
+    /*-{
+        if (typeof $wnd.getComputedStyle === 'function') {
+            var computedStyle = $wnd.getComputedStyle(element);
+            var width = 0;
+            for (i=0; i< borderNames.length; i++) {
+                var borderWidth = computedStyle[borderNames[i]];
+                width += parseFloat(borderWidth);
+            }
+            return width;
+        } else {
+            var parentElement = element.offsetParent;
+            var cloneElement = element.cloneNode(false);
+            cloneElement.style.boxSizing ="content-box";
+            parentElement.appendChild(cloneElement);
+            cloneElement.style.height = "10px"; // IE8 wants the height to be set to something...
+            var heightWithBorder = cloneElement.offsetHeight;
+            for (i=0; i< borderNames.length; i++) {
+                cloneElement.style[borderNames[i]] = "0";
+            }
+            var heightWithoutBorder = cloneElement.offsetHeight;
+            parentElement.removeChild(cloneElement);
+            
+            return heightWithBorder - heightWithoutBorder;
+        }
+    }-*/;
 }
diff --git a/client/src/com/vaadin/client/widgets/Escalator.java b/client/src/com/vaadin/client/widgets/Escalator.java
index 17236c5e30..0cd59ce7ed 100644
--- a/client/src/com/vaadin/client/widgets/Escalator.java
+++ b/client/src/com/vaadin/client/widgets/Escalator.java
@@ -4580,6 +4580,7 @@ public class Escalator extends Widget implements RequiresResize,
             private double height = -1;
             private boolean domHasBeenSetup = false;
             private double decoHeight;
+            private double defaultCellBorderBottomSize = -1;
 
             public SpacerImpl(int rowIndex) {
                 this.rowIndex = rowIndex;
@@ -4620,7 +4621,12 @@ public class Escalator extends Widget implements RequiresResize,
 
             public void setPosition(double x, double y) {
                 positions.set(getRootElement(), x, y);
-                positions.set(getDecoElement(), 0, y);
+                positions
+                        .set(getDecoElement(), 0, y - getSpacerDecoTopOffset());
+            }
+
+            private double getSpacerDecoTopOffset() {
+                return getBody().getDefaultRowHeight();
             }
 
             public void setStylePrimaryName(String style) {
@@ -4637,7 +4643,18 @@ public class Escalator extends Widget implements RequiresResize,
                 final double oldHeight = this.height;
 
                 this.height = height;
-                root.getStyle().setHeight(height, Unit.PX);
+
+                // since the spacer might be rendered on top of the previous
+                // rows border (done with css), need to increase height the
+                // amount of the border thickness
+                if (defaultCellBorderBottomSize < 0) {
+                    defaultCellBorderBottomSize = WidgetUtil
+                            .getBorderBottomThickness(body.getRowElement(
+                                    getVisibleRowRange().getStart())
+                                    .getFirstChildElement());
+                }
+                root.getStyle().setHeight(height + defaultCellBorderBottomSize,
+                        Unit.PX);
 
                 // move the visible spacers getRow row onwards.
                 shiftSpacerPositionsAfterRow(getRow(), heightDiff);
@@ -4722,34 +4739,9 @@ public class Escalator extends Widget implements RequiresResize,
             private void updateDecoratorGeometry(double detailsHeight) {
                 Style style = deco.getStyle();
                 decoHeight = detailsHeight + getBody().getDefaultRowHeight();
-
-                style.setTop(
-                        -(getBody().getDefaultRowHeight() - getBorderTopHeight(getElement())),
-                        Unit.PX);
                 style.setHeight(decoHeight, Unit.PX);
             }
 
-            private native double getBorderTopHeight(Element spacerCell)
-            /*-{
-                if (typeof $wnd.getComputedStyle === 'function') {
-                    var computedStyle = $wnd.getComputedStyle(spacerCell);
-                    var borderTopWidth = computedStyle['borderTopWidth'];
-                    var width = parseFloat(borderTopWidth);
-                    return width;
-                } else {
-                    var spacerRow = spacerCell.offsetParent;
-                    var cloneCell = spacerCell.cloneNode(false);
-                    spacerRow.appendChild(cloneCell);
-                    cloneCell.style.height = "10px"; // IE8 wants the height to be set to something...
-                    var heightWithBorder = cloneCell.offsetHeight;
-                    cloneCell.style.borderTopWidth = "0";
-                    var heightWithoutBorder = cloneCell.offsetHeight;
-                    spacerRow.removeChild(cloneCell);
-                    
-                    return heightWithBorder - heightWithoutBorder;
-                }
-            }-*/;
-
             @Override
             public Element getElement() {
                 return spacerElement;
@@ -4807,10 +4799,12 @@ public class Escalator extends Widget implements RequiresResize,
 
             public void show() {
                 getRootElement().getStyle().clearDisplay();
+                getDecoElement().getStyle().clearDisplay();
             }
 
             public void hide() {
                 getRootElement().getStyle().setDisplay(Display.NONE);
+                getDecoElement().getStyle().setDisplay(Display.NONE);
             }
 
             /**
@@ -4832,9 +4826,10 @@ public class Escalator extends Widget implements RequiresResize,
                     final double topClip = Math.max(0.0D, bodyTop - top);
                     final double bottomClip = decoHeight
                             - Math.max(0.0D, bottom - bodyBottom);
+                    // TODO [optimize] not sure how GWT compiles this
                     final String clip = new StringBuilder("rect(")
                             .append(topClip).append("px,").append(decoWidth)
-                            .append("px,").append(bottomClip).append("px,0")
+                            .append("px,").append(bottomClip).append("px,0)")
                             .toString();
                     deco.getStyle().setProperty("clip", clip);
                 } else {
@@ -5258,16 +5253,21 @@ public class Escalator extends Widget implements RequiresResize,
                 spacerScrollerRegistration = addScrollHandler(spacerScroller);
             }
 
-            SpacerImpl spacer = new SpacerImpl(rowIndex);
+            final SpacerImpl spacer = new SpacerImpl(rowIndex);
 
             rowIndexToSpacer.put(rowIndex, spacer);
-            spacer.setPosition(getScrollLeft(), calculateSpacerTop(rowIndex));
+            // set the position before adding it to DOM
+            positions.set(spacer.getRootElement(), getScrollLeft(),
+                    calculateSpacerTop(rowIndex));
 
             TableRowElement spacerRoot = spacer.getRootElement();
             spacerRoot.getStyle().setWidth(
                     columnConfiguration.calculateRowWidth(), Unit.PX);
             body.getElement().appendChild(spacerRoot);
             spacer.setupDom(height);
+            // set the deco position, requires that spacer is in the DOM
+            positions.set(spacer.getDecoElement(), 0,
+                    spacer.getTop() - spacer.getSpacerDecoTopOffset());
 
             spacerDecoContainer.appendChild(spacer.getDecoElement());
             if (spacerDecoContainer.getParentElement() == null) {
diff --git a/client/src/com/vaadin/client/widgets/Grid.java b/client/src/com/vaadin/client/widgets/Grid.java
index 08a86fe6a7..4951997995 100644
--- a/client/src/com/vaadin/client/widgets/Grid.java
+++ b/client/src/com/vaadin/client/widgets/Grid.java
@@ -2899,9 +2899,17 @@ public class Grid<T> extends ResizeComposite implements
                 /*
                  * Once we have the content properly inside the DOM, we should
                  * re-measure it to make sure that it's the correct height.
+                 * 
+                 * This is rather tricky, since the row (tr) will get the
+                 * height, but the spacer cell (td) has the borders, which
+                 * should go on top of the previous row and next row.
                  */
-                double measuredHeight = WidgetUtil
+                double requiredHeightBoundingClientRectDouble = WidgetUtil
                         .getRequiredHeightBoundingClientRectDouble(element);
+                double borderTopAndBottomHeight = WidgetUtil
+                        .getBorderTopAndBottomThickness(spacerElement);
+                double measuredHeight = requiredHeightBoundingClientRectDouble
+                        + borderTopAndBottomHeight;
                 assert getElement().isOrHasChild(spacerElement) : "The spacer element wasn't in the DOM during measurement, but was assumed to be.";
                 spacerHeight = measuredHeight;
             }
diff --git a/uitest/src/com/vaadin/tests/components/grid/GridDetailsLocationTest.java b/uitest/src/com/vaadin/tests/components/grid/GridDetailsLocationTest.java
index 06e79ac509..c14503fb8d 100644
--- a/uitest/src/com/vaadin/tests/components/grid/GridDetailsLocationTest.java
+++ b/uitest/src/com/vaadin/tests/components/grid/GridDetailsLocationTest.java
@@ -28,6 +28,7 @@ import org.openqa.selenium.Keys;
 import org.openqa.selenium.StaleElementReferenceException;
 import org.openqa.selenium.WebDriver;
 import org.openqa.selenium.WebElement;
+import org.openqa.selenium.remote.DesiredCapabilities;
 import org.openqa.selenium.support.ui.ExpectedCondition;
 
 import com.vaadin.testbench.TestBenchElement;
@@ -35,6 +36,7 @@ import com.vaadin.testbench.elements.ButtonElement;
 import com.vaadin.testbench.elements.CheckBoxElement;
 import com.vaadin.testbench.elements.GridElement.GridRowElement;
 import com.vaadin.testbench.elements.TextFieldElement;
+import com.vaadin.testbench.parallel.Browser;
 import com.vaadin.testbench.parallel.TestCategory;
 import com.vaadin.tests.components.grid.basicfeatures.element.CustomGridElement;
 import com.vaadin.tests.tb3.MultiBrowserTest;
@@ -42,8 +44,9 @@ import com.vaadin.tests.tb3.MultiBrowserTest;
 @TestCategory("grid")
 public class GridDetailsLocationTest extends MultiBrowserTest {
 
-    private static final int decoratorDefaultHeight = 50;
-    private static final int decoratorDefinedHeight = 30;
+    private static final int detailsDefaultHeight = 51;
+    private static final int detailsDefinedHeight = 33;
+    private static final int detailsDefinedHeightIE8 = 31;
 
     private static class Param {
         private final int rowIndex;
@@ -81,16 +84,9 @@ public class GridDetailsLocationTest extends MultiBrowserTest {
     public static Collection<Param> parameters() {
         List<Param> data = new ArrayList<Param>();
 
-        int[][] params = new int[][] {// @formatter:off
-            // row, top+noGen, top+gen
-            { 0, decoratorDefaultHeight, decoratorDefinedHeight }, 
-            { 500, decoratorDefaultHeight, decoratorDefinedHeight },
-            { 999, decoratorDefaultHeight, decoratorDefinedHeight},
-        };
-        // @formatter:on
+        int[] params = new int[] { 0, 500, 999 };
 
-        for (int i[] : params) {
-            int rowIndex = i[0];
+        for (int rowIndex : params) {
 
             data.add(new Param(rowIndex, false, false));
             data.add(new Param(rowIndex, true, false));
@@ -144,29 +140,72 @@ public class GridDetailsLocationTest extends MultiBrowserTest {
     }
 
     @Test
-    public void testDecoratorHeightWithNoGenerator() {
+    public void testDetailsHeightWithNoGenerator() {
         openTestURL();
         toggleAndScroll(5);
 
-        verifyDetailsRowHeight(5, decoratorDefaultHeight);
+        verifyDetailsRowHeight(5, detailsDefaultHeight, 0);
+        verifyDetailsDecoratorLocation(5, 0, 0);
+
+        toggleAndScroll(0);
+
+        verifyDetailsRowHeight(0, detailsDefaultHeight, 0);
+        verifyDetailsDecoratorLocation(0, 0, 1);
+
+        verifyDetailsRowHeight(5, detailsDefaultHeight, 1);
+        verifyDetailsDecoratorLocation(5, 1, 0);
     }
 
     @Test
-    public void testDecoratorHeightWithGenerator() {
+    public void testDetailsHeightWithGenerator() {
         openTestURL();
         useGenerator(true);
         toggleAndScroll(5);
 
-        verifyDetailsRowHeight(5, decoratorDefinedHeight);
+        verifyDetailsRowHeight(5, getDefinedHeight(), 0);
+        verifyDetailsDecoratorLocation(5, 0, 0);
+
+        toggleAndScroll(0);
+
+        verifyDetailsRowHeight(0, getDefinedHeight(), 0);
+        // decorator elements are in DOM in the order they have been added
+        verifyDetailsDecoratorLocation(0, 0, 1);
+
+        verifyDetailsRowHeight(5, getDefinedHeight(), 1);
+        verifyDetailsDecoratorLocation(5, 1, 0);
+    }
+
+    private int getDefinedHeight() {
+        boolean ie8 = isIE8();
+        return ie8 ? detailsDefinedHeightIE8 : detailsDefinedHeight;
     }
 
-    private void verifyDetailsRowHeight(int rowIndex, int expectedHeight) {
+    private void verifyDetailsRowHeight(int rowIndex, int expectedHeight,
+            int visibleIndexOfSpacer) {
         waitForDetailsVisible();
-        WebElement details = getDetailsElement();
+        WebElement details = getDetailsElement(visibleIndexOfSpacer);
         Assert.assertEquals("Wrong details row height", expectedHeight, details
                 .getSize().getHeight());
     }
 
+    private void verifyDetailsDecoratorLocation(int row,
+            int visibleIndexOfSpacer, int visibleIndexOfDeco) {
+        WebElement detailsElement = getDetailsElement(visibleIndexOfSpacer);
+        WebElement detailsDecoElement = getDetailsDecoElement(visibleIndexOfDeco);
+        GridRowElement rowElement = getGrid().getRow(row);
+
+        Assert.assertEquals(
+                "Details deco top position does not match row top pos",
+                rowElement.getLocation().getY(), detailsDecoElement
+                        .getLocation().getY());
+        Assert.assertEquals(
+                "Details deco bottom position does not match details bottom pos",
+                detailsElement.getLocation().getY()
+                        + detailsElement.getSize().getHeight(),
+                detailsDecoElement.getLocation().getY()
+                        + detailsDecoElement.getSize().getHeight());
+    }
+
     private void verifyLocation(Param param) {
         Assert.assertFalse("Notification was present",
                 isElementPresent(By.className("v-Notification")));
@@ -193,6 +232,11 @@ public class GridDetailsLocationTest extends MultiBrowserTest {
                         + bottomBoundary + " decoratorBotton:" + detailsBottom,
                 detailsBottom, bottomBoundary);
 
+        verifyDetailsRowHeight(param.getRowIndex(),
+                param.useGenerator() ? getDefinedHeight()
+                        : detailsDefaultHeight, 0);
+        verifyDetailsDecoratorLocation(param.getRowIndex(), 0, 0);
+
         Assert.assertFalse("Notification was present",
                 isElementPresent(By.className("v-Notification")));
     }
@@ -200,7 +244,15 @@ public class GridDetailsLocationTest extends MultiBrowserTest {
     private final By locator = By.className("v-grid-spacer");
 
     private WebElement getDetailsElement() {
-        return findElement(locator);
+        return getDetailsElement(0);
+    }
+
+    private WebElement getDetailsElement(int index) {
+        return findElements(locator).get(index);
+    }
+
+    private WebElement getDetailsDecoElement(int index) {
+        return findElements(By.className("v-grid-spacer-deco")).get(index);
     }
 
     private void waitForDetailsVisible() {
@@ -242,6 +294,16 @@ public class GridDetailsLocationTest extends MultiBrowserTest {
         }
     }
 
+    private boolean isIE8() {
+        DesiredCapabilities desiredCapabilities = getDesiredCapabilities();
+        DesiredCapabilities ie8Capabilities = Browser.IE8
+                .getDesiredCapabilities();
+        return desiredCapabilities.getBrowserName().equals(
+                ie8Capabilities.getBrowserName())
+                && desiredCapabilities.getVersion().equals(
+                        ie8Capabilities.getVersion());
+    }
+
     @SuppressWarnings("boxing")
     private boolean isCheckedValo(CheckBoxElement checkBoxElement) {
         WebElement checkbox = checkBoxElement.findElement(By.tagName("input"));
@@ -296,5 +358,4 @@ public class GridDetailsLocationTest extends MultiBrowserTest {
                 By.className("v-grid-scroller-horizontal"));
         return scrollBar;
     }
-
-}
+}
\ No newline at end of file
diff --git a/uitest/src/com/vaadin/tests/components/grid/basicfeatures/client/GridDetailsClientTest.java b/uitest/src/com/vaadin/tests/components/grid/basicfeatures/client/GridDetailsClientTest.java
index 619033226c..88158c7f6f 100644
--- a/uitest/src/com/vaadin/tests/components/grid/basicfeatures/client/GridDetailsClientTest.java
+++ b/uitest/src/com/vaadin/tests/components/grid/basicfeatures/client/GridDetailsClientTest.java
@@ -182,7 +182,6 @@ public class GridDetailsClientTest extends GridBasicClientFeaturesTest {
         getGridElement().getDetails(1);
     }
 
-
     @Test
     public void rowElementClassNames() {
         toggleDetailsFor(0);
diff --git a/uitest/src/com/vaadin/tests/components/grid/basicfeatures/escalator/EscalatorSpacerTest.java b/uitest/src/com/vaadin/tests/components/grid/basicfeatures/escalator/EscalatorSpacerTest.java
index e044c192f7..66c131255f 100644
--- a/uitest/src/com/vaadin/tests/components/grid/basicfeatures/escalator/EscalatorSpacerTest.java
+++ b/uitest/src/com/vaadin/tests/components/grid/basicfeatures/escalator/EscalatorSpacerTest.java
@@ -417,7 +417,7 @@ public class EscalatorSpacerTest extends EscalatorBasicClientFeaturesTest {
     public void spacersAreInCorrectDomPositionAfterScroll() {
         selectMenuPath(FEATURES, SPACERS, ROW_1, SET_100PX);
 
-        scrollVerticallyTo(30); // roughly one row's worth
+        scrollVerticallyTo(32); // roughly one row's worth
 
         WebElement tbody = getEscalator().findElement(By.tagName("tbody"));
         WebElement spacer = getChild(tbody, 1);