Stops non-scrolling scrollbars from interfering with pointing events

Some minor scrollbar-related maintenance was also done on the side.

Change-Id: I37d728465e498f586596e1eff14d73d6335e6770

2 files changed, 90 insertions, 54 deletions

diff --git a/client/src/com/vaadin/client/widget/escalator/ScrollbarBundle.java b/client/src/com/vaadin/client/widget/escalator/ScrollbarBundle.java
index be8d11761f..f02ea4eb2d 100644
--- a/client/src/com/vaadin/client/widget/escalator/ScrollbarBundle.java
+++ b/client/src/com/vaadin/client/widget/escalator/ScrollbarBundle.java
@@ -19,6 +19,7 @@ package com.vaadin.client.widget.escalator;
 import com.google.gwt.core.client.Scheduler;
 import com.google.gwt.core.client.Scheduler.ScheduledCommand;
 import com.google.gwt.dom.client.Element;
+import com.google.gwt.dom.client.Style.Display;
 import com.google.gwt.dom.client.Style.Overflow;
 import com.google.gwt.dom.client.Style.Unit;
 import com.google.gwt.event.shared.EventHandler;
@@ -209,8 +210,8 @@ public abstract class ScrollbarBundle implements DeferredWorker {
         }
 
         @Override
-        protected int internalGetScrollSize() {
-            return scrollSizeElement.getOffsetHeight();
+        protected String internalGetScrollSize() {
+            return scrollSizeElement.getStyle().getHeight();
         }
 
         @Override
@@ -219,23 +220,23 @@ public abstract class ScrollbarBundle implements DeferredWorker {
         }
 
         @Override
-        public double getOffsetSize() {
-            return root.getOffsetHeight();
+        public String internalGetOffsetSize() {
+            return root.getStyle().getHeight();
         }
 
         @Override
-        protected void internalSetScrollbarThickness(int px) {
+        protected void internalSetScrollbarThickness(double px) {
             root.getStyle().setWidth(px, Unit.PX);
             scrollSizeElement.getStyle().setWidth(px, Unit.PX);
         }
 
         @Override
-        protected int internalGetScrollbarThickness() {
-            return root.getOffsetWidth();
+        protected String internalGetScrollbarThickness() {
+            return root.getStyle().getWidth();
         }
 
         @Override
-        protected void forceScrollbar(boolean enable) {
+        protected void internalForceScrollbar(boolean enable) {
             if (enable) {
                 root.getStyle().setOverflowY(Overflow.SCROLL);
             } else {
@@ -278,8 +279,8 @@ public abstract class ScrollbarBundle implements DeferredWorker {
         }
 
         @Override
-        protected int internalGetScrollSize() {
-            return scrollSizeElement.getOffsetWidth();
+        protected String internalGetScrollSize() {
+            return scrollSizeElement.getStyle().getWidth();
         }
 
         @Override
@@ -288,23 +289,23 @@ public abstract class ScrollbarBundle implements DeferredWorker {
         }
 
         @Override
-        public double getOffsetSize() {
-            return root.getOffsetWidth();
+        public String internalGetOffsetSize() {
+            return root.getStyle().getWidth();
         }
 
         @Override
-        protected void internalSetScrollbarThickness(int px) {
+        protected void internalSetScrollbarThickness(double px) {
             root.getStyle().setHeight(px, Unit.PX);
             scrollSizeElement.getStyle().setHeight(px, Unit.PX);
         }
 
         @Override
-        protected int internalGetScrollbarThickness() {
-            return root.getOffsetHeight();
+        protected String internalGetScrollbarThickness() {
+            return root.getStyle().getHeight();
         }
 
         @Override
-        protected void forceScrollbar(boolean enable) {
+        protected void internalForceScrollbar(boolean enable) {
             if (enable) {
                 root.getStyle().setOverflowX(Overflow.SCROLL);
             } else {
@@ -342,9 +343,11 @@ public abstract class ScrollbarBundle implements DeferredWorker {
 
     private ScrollbarBundle() {
         root.appendChild(scrollSizeElement);
+        root.getStyle().setDisplay(Display.NONE);
+        root.setTabIndex(-1);
     }
 
-    protected abstract int internalGetScrollSize();
+    protected abstract String internalGetScrollSize();
 
     /**
      * Sets the primary style name
@@ -445,14 +448,27 @@ public abstract class ScrollbarBundle implements DeferredWorker {
      * This is an IE8 workaround, since it doesn't always show scrollbars with
      * <code>overflow: auto</code> enabled.
      */
-    protected abstract void forceScrollbar(boolean enable);
+    protected void forceScrollbar(boolean enable) {
+        if (enable) {
+            root.getStyle().clearDisplay();
+        } else {
+            root.getStyle().setDisplay(Display.NONE);
+        }
+        internalForceScrollbar(enable);
+    }
+
+    protected abstract void internalForceScrollbar(boolean enable);
 
     /**
      * Gets the length of the scrollbar
      * 
      * @return the length of the scrollbar in pixels
      */
-    public abstract double getOffsetSize();
+    public double getOffsetSize() {
+        return parseCssDimensionToPixels(internalGetOffsetSize());
+    }
+
+    public abstract String internalGetOffsetSize();
 
     /**
      * Sets the scroll position of the scrollbar in the axis the scrollbar is
@@ -613,7 +629,7 @@ public abstract class ScrollbarBundle implements DeferredWorker {
      *         through
      */
     public double getScrollSize() {
-        return internalGetScrollSize();
+        return parseCssDimensionToPixels(internalGetScrollSize());
     }
 
     /**
@@ -624,7 +640,7 @@ public abstract class ScrollbarBundle implements DeferredWorker {
      *            the dimension that {@link #scrollSizeElement} should take in
      *            the opposite axis to what the scrollbar is representing
      */
-    protected abstract void internalSetScrollbarThickness(int px);
+    protected abstract void internalSetScrollbarThickness(double px);
 
     /**
      * Sets the scrollbar's thickness.
@@ -637,7 +653,7 @@ public abstract class ScrollbarBundle implements DeferredWorker {
      * @param px
      *            the scrollbar's thickness in pixels
      */
-    public final void setScrollbarThickness(int px) {
+    public final void setScrollbarThickness(double px) {
         isInvisibleScrollbar = (px == 0);
 
         if (isInvisibleScrollbar) {
@@ -653,7 +669,7 @@ public abstract class ScrollbarBundle implements DeferredWorker {
             Event.setEventListener(root, null);
         }
 
-        internalSetScrollbarThickness(Math.max(1, px));
+        internalSetScrollbarThickness(Math.max(1d, px));
     }
 
     /**
@@ -661,7 +677,7 @@ public abstract class ScrollbarBundle implements DeferredWorker {
      * 
      * @return the scrollbar's thickness as defined in the DOM, in pixels
      */
-    protected abstract int internalGetScrollbarThickness();
+    protected abstract String internalGetScrollbarThickness();
 
     /**
      * Gets the scrollbar's thickness.
@@ -672,9 +688,9 @@ public abstract class ScrollbarBundle implements DeferredWorker {
      * 
      * @return the scrollbar's thickness in pixels
      */
-    public final int getScrollbarThickness() {
+    public final double getScrollbarThickness() {
         if (!isInvisibleScrollbar) {
-            return internalGetScrollbarThickness();
+            return parseCssDimensionToPixels(internalGetScrollbarThickness());
         } else {
             return 0;
         }
@@ -808,6 +824,28 @@ public abstract class ScrollbarBundle implements DeferredWorker {
         return getHandlerManager().addHandler(ScrollEvent.TYPE, handler);
     }
 
+    private static double parseCssDimensionToPixels(String size) {
+
+        /*
+         * Sizes of elements are calculated from CSS rather than
+         * element.getOffset*() because those values are 0 whenever display:
+         * none. Because we know that all elements have populated
+         * CSS-dimensions, it's better to do it that way.
+         * 
+         * Another solution would be to make the elements visible while
+         * measuring and then re-hide them, but that would cause unnecessary
+         * reflows that would probably kill the performance dead.
+         */
+
+        if (size.isEmpty()) {
+            return 0;
+        } else {
+            assert size.endsWith("px") : "Can't parse CSS dimension \"" + size
+                    + "\"";
+            return Double.parseDouble(size.substring(0, size.length() - 2));
+        }
+    }
+
     @Override
     public boolean isWorkPending() {
         return scrollSizeTemporaryScrollHandler != null
diff --git a/client/src/com/vaadin/client/widgets/Escalator.java b/client/src/com/vaadin/client/widgets/Escalator.java
index b54c45625b..adbffe178b 100644
--- a/client/src/com/vaadin/client/widgets/Escalator.java
+++ b/client/src/com/vaadin/client/widgets/Escalator.java
@@ -779,16 +779,19 @@ public class Escalator extends Widget implements RequiresResize, DeferredWorker
             // let's fix the table wrapper size, since it's now stable.
             if (verticalScrollNeeded) {
                 tableWrapperWidth -= verticalScrollbar.getScrollbarThickness();
+                tableWrapperWidth = Math.max(0, tableWrapperWidth);
             }
             if (horizontalScrollNeeded) {
                 tableWrapperHeight -= horizontalScrollbar
                         .getScrollbarThickness();
+                tableWrapperHeight = Math.max(0, tableWrapperHeight);
             }
             tableWrapper.getStyle().setHeight(tableWrapperHeight, Unit.PX);
             tableWrapper.getStyle().setWidth(tableWrapperWidth, Unit.PX);
 
-            verticalScrollbar.setOffsetSize(tableWrapperHeight
+            double vScrollbarHeight = Math.max(0, tableWrapperHeight
                     - footer.heightOfSection - header.heightOfSection);
+            verticalScrollbar.setOffsetSize(vScrollbarHeight);
             verticalScrollbar.setScrollSize(scrollContentHeight);
 
             /*
@@ -832,7 +835,7 @@ public class Escalator extends Widget implements RequiresResize, DeferredWorker
                 fCornerStyle.clearDisplay();
 
                 if (horizontalScrollbar.showsScrollHandle()) {
-                    int offset = horizontalScrollbar.getScrollbarThickness();
+                    double offset = horizontalScrollbar.getScrollbarThickness();
                     fCornerStyle.setBottom(offset, Unit.PX);
                 } else {
                     fCornerStyle.clearBottom();
@@ -1149,8 +1152,8 @@ public class Escalator extends Widget implements RequiresResize, DeferredWorker
          */
         protected final TableSectionElement root;
 
-        /** The height of the combined rows in the DOM. */
-        protected double heightOfSection = -1;
+        /** The height of the combined rows in the DOM. Never negative. */
+        protected double heightOfSection = 0;
 
         /**
          * The primary style name of the escalator. Most commonly provided by
@@ -1403,7 +1406,6 @@ public class Escalator extends Widget implements RequiresResize, DeferredWorker
             }
 
             for (int row = visualIndex; row < visualIndex + numberOfRows; row++) {
-                final int rowHeight = getDefaultRowHeight();
                 final TableRowElement tr = TableRowElement.as(DOM.createTR());
                 addedRows.add(tr);
                 tr.addClassName(getStylePrimaryName() + "-row");
@@ -1411,8 +1413,7 @@ public class Escalator extends Widget implements RequiresResize, DeferredWorker
                 for (int col = 0; col < columnConfiguration.getColumnCount(); col++) {
                     final double colWidth = columnConfiguration
                             .getColumnWidthActual(col);
-                    final TableCellElement cellElem = createCellElement(
-                            rowHeight, colWidth);
+                    final TableCellElement cellElem = createCellElement(colWidth);
                     tr.appendChild(cellElem);
 
                     // Set stylename and position if new cell is frozen
@@ -1544,22 +1545,21 @@ public class Escalator extends Widget implements RequiresResize, DeferredWorker
         /**
          * Create and setup an empty cell element.
          * 
-         * @param width
+         * @param colWidth
          *            the width of the cell, in pixels
-         * @param height
-         *            the height of the cell, in pixels
          * 
          * @return a set-up empty cell element
          */
-        public TableCellElement createCellElement(final int height,
-                final double colWidth) {
+        public TableCellElement createCellElement(final double width) {
             final TableCellElement cellElem = TableCellElement.as(DOM
                     .createElement(getCellElementTagName()));
-            if (height >= 0) {
-                cellElem.getStyle().setHeight(height, Unit.PX);
-            }
-            if (colWidth >= 0) {
-                cellElem.getStyle().setWidth(colWidth, Unit.PX);
+
+            final int height = getDefaultRowHeight();
+            assert height >= 0 : "defaultRowHeight was negative. There's a setter leak somewhere.";
+            cellElem.getStyle().setHeight(height, Unit.PX);
+
+            if (width >= 0) {
+                cellElem.getStyle().setWidth(width, Unit.PX);
             }
             cellElem.addClassName(getStylePrimaryName() + "-cell");
             return cellElem;
@@ -1652,12 +1652,10 @@ public class Escalator extends Widget implements RequiresResize, DeferredWorker
             Iterable<FlyweightCell> cells = flyweightRow.getUnattachedCells(
                     offset, numberOfCells);
 
-            final int rowHeight = getDefaultRowHeight();
             for (FlyweightCell cell : cells) {
                 final double colWidth = columnConfiguration
                         .getColumnWidthActual(cell.getColumn());
-                final TableCellElement cellElem = createCellElement(rowHeight,
-                        colWidth);
+                final TableCellElement cellElem = createCellElement(colWidth);
                 cell.setElement(cellElem);
             }
 
@@ -1872,6 +1870,8 @@ public class Escalator extends Widget implements RequiresResize, DeferredWorker
 
         protected void reapplyRowHeight(final TableRowElement tr,
                 final int heightPx) {
+            assert heightPx >= 0 : "Height must not be negative";
+
             Element cellElem = tr.getFirstChildElement();
             while (cellElem != null) {
                 cellElem.getStyle().setHeight(heightPx, Unit.PX);
@@ -4245,9 +4245,9 @@ public class Escalator extends Widget implements RequiresResize, DeferredWorker
     private PositionFunction position;
 
     /** The cached width of the escalator, in pixels. */
-    private double widthOfEscalator;
+    private double widthOfEscalator = 0;
     /** The cached height of the escalator, in pixels. */
-    private double heightOfEscalator;
+    private double heightOfEscalator = 0;
 
     /** The height of Escalator in terms of body rows. */
     private double heightByRows = GridState.DEFAULT_HEIGHT_BY_ROWS;
@@ -4291,12 +4291,10 @@ public class Escalator extends Widget implements RequiresResize, DeferredWorker
 
         root.appendChild(verticalScrollbar.getElement());
         verticalScrollbar.addScrollHandler(scrollHandler);
-        verticalScrollbar.getElement().setTabIndex(-1);
         verticalScrollbar.setScrollbarThickness(Util.getNativeScrollbarSize());
 
         root.appendChild(horizontalScrollbar.getElement());
         horizontalScrollbar.addScrollHandler(scrollHandler);
-        horizontalScrollbar.getElement().setTabIndex(-1);
         horizontalScrollbar
                 .setScrollbarThickness(Util.getNativeScrollbarSize());
         horizontalScrollbar
@@ -4707,10 +4705,10 @@ public class Escalator extends Widget implements RequiresResize, DeferredWorker
         }
 
         Profiler.enter("Escalator.recalculateElementSizes");
-        widthOfEscalator = Util
-                .getRequiredWidthBoundingClientRectDouble(getElement());
-        heightOfEscalator = Util
-                .getRequiredHeightBoundingClientRectDouble(getElement());
+        widthOfEscalator = Math.max(0,
+                Util.getRequiredWidthBoundingClientRectDouble(getElement()));
+        heightOfEscalator = Math.max(0,
+                Util.getRequiredHeightBoundingClientRectDouble(getElement()));
 
         header.recalculateSectionHeight();
         body.recalculateSectionHeight();