Move details row decorators out of spacers td #17423

Change-Id: Ie6c0166ad307b9172acccaa781d6fea316ee6390

4 files changed, 158 insertions, 140 deletions

diff --git a/WebContent/VAADIN/themes/base/grid/grid.scss b/WebContent/VAADIN/themes/base/grid/grid.scss
index 360f020822..0e1dee2b99 100644
--- a/WebContent/VAADIN/themes/base/grid/grid.scss
+++ b/WebContent/VAADIN/themes/base/grid/grid.scss
@@ -1,4 +1,5 @@
-$v-grid-border: 1px solid #ddd !default;
+$v-grid-border-size: 1px !default;
+$v-grid-border: $v-grid-border-size solid #ddd !default;
 $v-grid-cell-vertical-border: $v-grid-border !default;
 $v-grid-cell-horizontal-border: $v-grid-cell-vertical-border !default;
 $v-grid-cell-focused-border: 1px solid !default;
@@ -420,6 +421,10 @@ $v-grid-details-border-bottom-stripe: 1px solid darken($v-grid-row-background-co
     margin-right: 4px;
   }
 
+  .#{$primaryStyleName}-spacer {
+    left: $v-grid-details-marker-width - $v-grid-border-size;
+  }
+  
   .#{$primaryStyleName}-spacer > td {
     display: block;
     padding: 0;
@@ -434,8 +439,14 @@ $v-grid-details-border-bottom-stripe: 1px solid darken($v-grid-row-background-co
     border-top: $v-grid-details-border-top-stripe;
     border-bottom: $v-grid-details-border-bottom-stripe;
   }
+  
+  .#{$primaryStyleName}-spacer-deco-container {
+    position: relative;
+    top: $v-grid-border-size;
+    z-index: 5;
+  }
 
-  .#{$primaryStyleName}-spacer .deco {
+  .#{$primaryStyleName}-spacer-deco {
     top: 0; // this will be overridden by code, but it's a good default. 
     left: 0;
     width: $v-grid-details-marker-width;
@@ -456,10 +467,6 @@ $v-grid-details-border-bottom-stripe: 1px solid darken($v-grid-row-background-co
     }
   }
 
-  .#{$primaryStyleName}-spacer .content {
-    padding-left: $v-grid-details-marker-width;
-  }
-
   // Renderers
 
   .#{$primaryStyleName}-cell > .v-progressbar {
diff --git a/client/src/com/vaadin/client/widget/escalator/Spacer.java b/client/src/com/vaadin/client/widget/escalator/Spacer.java
index 6ccef88f0d..789a64a21e 100644
--- a/client/src/com/vaadin/client/widget/escalator/Spacer.java
+++ b/client/src/com/vaadin/client/widget/escalator/Spacer.java
@@ -34,6 +34,11 @@ public interface Spacer {
     Element getElement();
 
     /**
+     * Gets the decorative element for this spacer.
+     */
+    Element getDecoElement();
+
+    /**
      * Gets the row index.
      * 
      * @return the row index.
diff --git a/client/src/com/vaadin/client/widgets/Escalator.java b/client/src/com/vaadin/client/widgets/Escalator.java
index 01567143dd..eae5789b8a 100644
--- a/client/src/com/vaadin/client/widgets/Escalator.java
+++ b/client/src/com/vaadin/client/widgets/Escalator.java
@@ -962,6 +962,7 @@ public class Escalator extends Widget implements RequiresResize,
 
             lastScrollTop = scrollTop;
             body.updateEscalatorRowsOnScroll();
+            body.spacerContainer.updateSpacerDecosVisibility();
             /*
              * TODO [[optimize]]: Might avoid a reflow by first calculating new
              * scrolltop and scrolleft, then doing the escalator magic based on
@@ -2195,6 +2196,7 @@ public class Escalator extends Widget implements RequiresResize,
                         - footer.getHeightOfSection());
 
                 body.verifyEscalatorCount();
+                body.spacerContainer.updateSpacerDecosVisibility();
             }
 
             Profiler.leave("Escalator.AbstractStaticRowContainer.recalculateSectionHeight");
@@ -2258,10 +2260,13 @@ public class Escalator extends Widget implements RequiresResize,
 
         @Override
         protected void sectionHeightCalculated() {
-            bodyElem.getStyle().setMarginTop(getHeightOfSection(), Unit.PX);
+            double heightOfSection = getHeightOfSection();
+            bodyElem.getStyle().setMarginTop(heightOfSection, Unit.PX);
+            spacerDecoContainer.getStyle().setMarginTop(heightOfSection,
+                    Unit.PX);
             verticalScrollbar.getElement().getStyle()
-                    .setTop(getHeightOfSection(), Unit.PX);
-            headerDeco.getStyle().setHeight(getHeightOfSection(), Unit.PX);
+                    .setTop(heightOfSection, Unit.PX);
+            headerDeco.getStyle().setHeight(heightOfSection, Unit.PX);
         }
 
         @Override
@@ -3473,6 +3478,7 @@ public class Escalator extends Widget implements RequiresResize,
             tBodyScrollLeft = scrollLeft;
             tBodyScrollTop = scrollTop;
             position.set(bodyElem, -tBodyScrollLeft, -tBodyScrollTop);
+            position.set(spacerDecoContainer, 0, -tBodyScrollTop);
         }
 
         /**
@@ -4602,9 +4608,11 @@ public class Escalator extends Widget implements RequiresResize,
         private final class SpacerImpl implements Spacer {
             private TableCellElement spacerElement;
             private TableRowElement root;
+            private DivElement deco;
             private int rowIndex;
             private double height = -1;
             private boolean domHasBeenSetup = false;
+            private double decoHeight;
 
             public SpacerImpl(int rowIndex) {
                 this.rowIndex = rowIndex;
@@ -4613,6 +4621,7 @@ public class Escalator extends Widget implements RequiresResize,
                 spacerElement = TableCellElement.as(DOM.createTD());
                 root.appendChild(spacerElement);
                 root.setPropertyInt(SPACER_LOGICAL_ROW_PROPERTY, rowIndex);
+                deco = DivElement.as(DOM.createDiv());
             }
 
             public void setPositionDiff(double x, double y) {
@@ -4637,12 +4646,19 @@ public class Escalator extends Widget implements RequiresResize,
                 return root;
             }
 
+            @Override
+            public Element getDecoElement() {
+                return deco;
+            }
+
             public void setPosition(double x, double y) {
                 positions.set(getRootElement(), x, y);
+                positions.set(getDecoElement(), 0, y);
             }
 
             public void setStylePrimaryName(String style) {
                 UIObject.setStylePrimaryName(root, style + "-spacer");
+                UIObject.setStylePrimaryName(deco, style + "-spacer-deco");
             }
 
             public void setHeight(double height) {
@@ -4731,8 +4747,42 @@ public class Escalator extends Widget implements RequiresResize,
                     verticalScrollbar.setScrollSize(verticalScrollbar
                             .getScrollSize() + heightDiff);
                 }
+
+                updateDecoratorGeometry(height);
+            }
+
+            /** Resizes and places the decorator. */
+            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;
@@ -4795,6 +4845,35 @@ public class Escalator extends Widget implements RequiresResize,
             public void hide() {
                 getRootElement().getStyle().setDisplay(Display.NONE);
             }
+
+            /**
+             * Crop the decorator element so that it doesn't overlap the header
+             * and footer sections.
+             * 
+             * @param bodyTop
+             *            the top cordinate of the escalator body
+             * @param bodyBottom
+             *            the bottom cordinate of the escalator body
+             * @param decoWidth
+             *            width of the deco
+             */
+            private void updateDecoClip(final double bodyTop,
+                    final double bodyBottom, final double decoWidth) {
+                final int top = deco.getAbsoluteTop();
+                final int bottom = deco.getAbsoluteBottom();
+                if (top < bodyTop || bottom > bodyBottom) {
+                    final double topClip = Math.max(0.0D, bodyTop - top);
+                    final double bottomClip = decoHeight
+                            - Math.max(0.0D, bottom - bodyBottom);
+                    final String clip = new StringBuilder("rect(")
+                            .append(topClip).append("px,").append(decoWidth)
+                            .append("px,").append(bottomClip).append("px,0")
+                            .toString();
+                    deco.getStyle().setProperty("clip", clip);
+                } else {
+                    deco.getStyle().clearProperty("clip");
+                }
+            }
         }
 
         private final TreeMap<Integer, SpacerImpl> rowIndexToSpacer = new TreeMap<Integer, SpacerImpl>();
@@ -4818,6 +4897,9 @@ public class Escalator extends Widget implements RequiresResize,
         };
         private HandlerRegistration spacerScrollerRegistration;
 
+        /** Width of the spacers' decos. Calculated once then cached. */
+        private double spacerDecoWidth = 0.0D;
+
         public void setSpacer(int rowIndex, double height)
                 throws IllegalArgumentException {
 
@@ -4883,9 +4965,10 @@ public class Escalator extends Widget implements RequiresResize,
         }
 
         public void reapplySpacerWidths() {
+            // FIXME #16266 , spacers get couple pixels too much because borders
+            final double width = getInnerWidth() - spacerDecoWidth;
             for (SpacerImpl spacer : rowIndexToSpacer.values()) {
-                spacer.getRootElement().getStyle()
-                        .setWidth(getInnerWidth(), Unit.PX);
+                spacer.getRootElement().getStyle().setWidth(width, Unit.PX);
             }
         }
 
@@ -4916,6 +4999,7 @@ public class Escalator extends Widget implements RequiresResize,
                 destroySpacerContent(spacer);
                 spacer.setHeight(0); // resets row offsets
                 spacer.getRootElement().removeFromParent();
+                spacer.getDecoElement().removeFromParent();
             }
 
             removedSpacers.clear();
@@ -5218,6 +5302,15 @@ public class Escalator extends Widget implements RequiresResize,
             body.getElement().appendChild(spacerRoot);
             spacer.setupDom(height);
 
+            spacerDecoContainer.appendChild(spacer.getDecoElement());
+            if (spacerDecoContainer.getParentElement() == null) {
+                getElement().appendChild(spacerDecoContainer);
+                // calculate the spacer deco width, it won't change
+                spacerDecoWidth = WidgetUtil
+                        .getRequiredWidthBoundingClientRectDouble(spacer
+                                .getDecoElement());
+            }
+
             initSpacerContent(spacer);
 
             body.sortDomElements();
@@ -5340,6 +5433,22 @@ public class Escalator extends Widget implements RequiresResize,
                 spacer.setRowIndex(spacer.getRow() + numberOfRows);
             }
         }
+
+        private void updateSpacerDecosVisibility() {
+            final Range visibleRowRange = getVisibleRowRange();
+            Collection<SpacerImpl> visibleSpacers = rowIndexToSpacer.subMap(
+                    visibleRowRange.getStart() - 1,
+                    visibleRowRange.getEnd() + 1).values();
+            if (!visibleSpacers.isEmpty()) {
+                final double top = tableWrapper.getAbsoluteTop()
+                        + header.getHeightOfSection();
+                final double bottom = tableWrapper.getAbsoluteBottom()
+                        - footer.getHeightOfSection();
+                for (SpacerImpl spacer : visibleSpacers) {
+                    spacer.updateDecoClip(top, bottom, spacerDecoWidth);
+                }
+            }
+        }
     }
 
     private class ElementPositionBookkeeper {
@@ -5495,6 +5604,8 @@ public class Escalator extends Widget implements RequiresResize,
             .createDiv());
     private final DivElement headerDeco = DivElement.as(DOM.createDiv());
     private final DivElement footerDeco = DivElement.as(DOM.createDiv());
+    private final DivElement spacerDecoContainer = DivElement.as(DOM
+            .createDiv());
 
     private PositionFunction position;
 
@@ -5570,6 +5681,8 @@ public class Escalator extends Widget implements RequiresResize,
 
         setStylePrimaryName("v-escalator");
 
+        spacerDecoContainer.setAttribute("aria-hidden", "true");
+
         // init default dimensions
         setHeight(null);
         setWidth(null);
@@ -6294,6 +6407,8 @@ public class Escalator extends Widget implements RequiresResize,
         UIObject.setStylePrimaryName(footerDeco, style + "-footer-deco");
         UIObject.setStylePrimaryName(horizontalScrollbarDeco, style
                 + "-horizontal-scrollbar-deco");
+        UIObject.setStylePrimaryName(spacerDecoContainer, style
+                + "-spacer-deco-container");
 
         header.setStylePrimaryName(style);
         body.setStylePrimaryName(style);
diff --git a/client/src/com/vaadin/client/widgets/Grid.java b/client/src/com/vaadin/client/widgets/Grid.java
index 77bddb281f..f45d8ef3b4 100644
--- a/client/src/com/vaadin/client/widgets/Grid.java
+++ b/client/src/com/vaadin/client/widgets/Grid.java
@@ -2864,21 +2864,13 @@ public class Grid<T> extends ResizeComposite implements
 
     private class GridSpacerUpdater implements SpacerUpdater {
 
-        private static final String DECO_CLASSNAME = "deco";
-        private static final String CONTENT_CLASSNAME = "content";
         private static final String STRIPE_CLASSNAME = "stripe";
 
         private final Map<Element, Widget> elementToWidgetMap = new HashMap<Element, Widget>();
 
         @Override
         public void init(Spacer spacer) {
-            initStructure(spacer);
-            Element root = getDetailsRoot(spacer);
-
-            assert root.getFirstChild() == null : "The spacer's"
-                    + " element should be empty at this point. (row: "
-                    + spacer.getRow() + ", child: " + root.getFirstChild()
-                    + ")";
+            initTheming(spacer);
 
             int rowIndex = spacer.getRow();
 
@@ -2893,12 +2885,13 @@ public class Grid<T> extends ResizeComposite implements
             }
 
             final double spacerHeight;
+            Element spacerElement = spacer.getElement();
             if (detailsWidget == null) {
-                root.removeAllChildren();
+                spacerElement.removeAllChildren();
                 spacerHeight = DETAILS_ROW_INITIAL_HEIGHT;
             } else {
                 Element element = detailsWidget.getElement();
-                root.appendChild(element);
+                spacerElement.appendChild(element);
                 setParent(detailsWidget, Grid.this);
                 Widget previousWidget = elementToWidgetMap.put(element,
                         detailsWidget);
@@ -2911,25 +2904,24 @@ public class Grid<T> extends ResizeComposite implements
                  * re-measure it to make sure that it's the correct height.
                  */
                 double measuredHeight = WidgetUtil
-                        .getRequiredHeightBoundingClientRectDouble(root);
-                assert getElement().isOrHasChild(root) : "The spacer element wasn't in the DOM during measurement, but was assumed to be.";
+                        .getRequiredHeightBoundingClientRectDouble(spacerElement);
+                assert getElement().isOrHasChild(spacerElement) : "The spacer element wasn't in the DOM during measurement, but was assumed to be.";
                 spacerHeight = measuredHeight;
             }
 
             escalator.getBody().setSpacer(rowIndex, spacerHeight);
-            updateDecoratorGeometry(spacerHeight, spacer);
         }
 
         @Override
         public void destroy(Spacer spacer) {
-            Element root = getDetailsRoot(spacer);
+            Element spacerElement = spacer.getElement();
 
-            assert getElement().isOrHasChild(root) : "Trying "
+            assert getElement().isOrHasChild(spacerElement) : "Trying "
                     + "to destroy a spacer that is not connected to this "
                     + "Grid's DOM. (row: " + spacer.getRow() + ", element: "
-                    + root + ")";
+                    + spacerElement + ")";
 
-            Widget detailsWidget = elementToWidgetMap.remove(root
+            Widget detailsWidget = elementToWidgetMap.remove(spacerElement
                     .getFirstChildElement());
 
             if (detailsWidget != null) {
@@ -2938,107 +2930,27 @@ public class Grid<T> extends ResizeComposite implements
                  * returned a null widget.
                  */
 
-                assert root.getFirstChild() != null : "The "
+                assert spacerElement.getFirstChild() != null : "The "
                         + "details row to destroy did not contain a widget - "
                         + "probably removed by something else without "
                         + "permission? (row: " + spacer.getRow()
-                        + ", element: " + root + ")";
+                        + ", element: " + spacerElement + ")";
 
                 setParent(detailsWidget, null);
-                root.removeAllChildren();
+                spacerElement.removeAllChildren();
             }
         }
 
-        /**
-         * Initializes the spacer element into a details structure, containing a
-         * decorator and a slot for the details widget.
-         */
-        private void initStructure(Spacer spacer) {
+        private void initTheming(Spacer spacer) {
             Element spacerRoot = spacer.getElement();
 
-            if (spacerRoot.getChildCount() == 0) {
-                Element deco = DOM.createDiv();
-                deco.setClassName(DECO_CLASSNAME);
-
-                Element detailsContent = DOM.createDiv();
-                detailsContent.setClassName(CONTENT_CLASSNAME);
-
-                if (spacer.getRow() % 2 == 1) {
-                    spacerRoot.getParentElement()
-                            .addClassName(STRIPE_CLASSNAME);
-                }
-
-                spacerRoot.appendChild(deco);
-                spacerRoot.appendChild(detailsContent);
-            }
-
-            else {
-                if (spacer.getRow() % 2 == 1) {
-                    spacerRoot.getParentElement()
-                            .addClassName(STRIPE_CLASSNAME);
-                } else {
-                    spacerRoot.getParentElement().removeClassName(
-                            STRIPE_CLASSNAME);
-                }
-
-                /*
-                 * The only case when we get into this else branch is when the
-                 * previous generated details element was a null Widget. In
-                 * those situations, we don't call destroy on the content, but
-                 * simply reuse it as-is.
-                 */
-                assert getDetailsRoot(spacer).getChildCount() == 0 : "This "
-                        + "code should never be triggered unless the details "
-                        + "root already was empty";
+            if (spacer.getRow() % 2 == 1) {
+                spacerRoot.getParentElement().addClassName(STRIPE_CLASSNAME);
+            } else {
+                spacerRoot.getParentElement().removeClassName(STRIPE_CLASSNAME);
             }
         }
 
-        /** Gets the decorator element from the DOM structure. */
-        private Element getDecorator(Spacer spacer) {
-            TableCellElement td = TableCellElement.as(spacer.getElement());
-            Element decorator = td.getFirstChildElement();
-            return decorator;
-        }
-
-        /** Gets the element for the details widget from the DOM structure. */
-        private Element getDetailsRoot(Spacer spacer) {
-            Element detailsRoot = getDecorator(spacer).getNextSiblingElement();
-            return detailsRoot;
-        }
-
-        /** Resizes and places the decorator. */
-        private void updateDecoratorGeometry(double detailsHeight, Spacer spacer) {
-            Element decorator = getDecorator(spacer);
-            Style style = decorator.getStyle();
-            double rowHeight = escalator.getBody().getDefaultRowHeight();
-            double borderHeight = getBorderHeight(spacer);
-
-            style.setTop(-(rowHeight - borderHeight), Unit.PX);
-            style.setHeight(detailsHeight + rowHeight, Unit.PX);
-        }
-
-        private native double getBorderHeight(Spacer spacer)
-        /*-{
-            var spacerCell = spacer.@com.vaadin.client.widget.escalator.Spacer::getElement()();
-            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);
-                
-                console.log(heightWithBorder+" - "+heightWithoutBorder);
-                return heightWithBorder - heightWithoutBorder;
-            }
-        }-*/;
     }
 
     /**
@@ -6539,18 +6451,14 @@ public class Grid<T> extends ResizeComposite implements
     public com.google.gwt.user.client.Element getSubPartElement(String subPart) {
 
         /*
-         * gandles details[] (translated to spacer[] for Escalator), cell[],
+         * handles details[] (translated to spacer[] for Escalator), cell[],
          * header[] and footer[]
          */
         Element escalatorElement = escalator.getSubPartElement(subPart
                 .replaceFirst("^details\\[", "spacer["));
 
         if (escalatorElement != null) {
-            if (subPart.startsWith("details[")) {
-                return DOM.asOld(parseDetails(escalatorElement));
-            } else {
-                return DOM.asOld(escalatorElement);
-            }
+            return DOM.asOld(escalatorElement);
         }
 
         SubPartArguments args = Escalator.parseSubPartArguments(subPart);
@@ -6563,23 +6471,6 @@ public class Grid<T> extends ResizeComposite implements
         return null;
     }
 
-    @SuppressWarnings("static-method")
-    private Element parseDetails(Element spacer) {
-        assert spacer.getChildCount() == 2 : "Unexpected structure for details ";
-
-        Element decorator = spacer.getFirstChildElement();
-        assert decorator != null : "unexpected spacer DOM structure";
-        assert decorator.getClassName()
-                .equals(GridSpacerUpdater.DECO_CLASSNAME) : "unexpected first details element";
-
-        Element spacerRoot = decorator.getNextSiblingElement();
-        assert spacerRoot != null : "unexpected spacer DOM structure";
-        assert spacerRoot.getClassName().equals(
-                GridSpacerUpdater.CONTENT_CLASSNAME) : "unexpected second details element";
-
-        return DOM.asOld(spacerRoot);
-    }
-
     private Element getSubPartElementEditor(SubPartArguments args) {
 
         if (!args.getType().equalsIgnoreCase("editor")