Fixed lost scrollLeft when row count changed in Table (#12652).

Change-Id: I868f56c1e7003c6619859ba46619f4c53ef9744e

4 files changed, 294 insertions, 1 deletions

diff --git a/client/src/com/vaadin/client/Util.java b/client/src/com/vaadin/client/Util.java
index 9cdfa954c6..fd7a354569 100644
--- a/client/src/com/vaadin/client/Util.java
+++ b/client/src/com/vaadin/client/Util.java
@@ -449,6 +449,35 @@ public class Util {
     }
 
     /**
+     * Calculates maximum horizontal scrolling value for the given element.
+     * 
+     * @since 7.1.9
+     * @param element
+     *            which scrollLeft should be calculated
+     * @return maximum value for scrollLeft of the given element
+     */
+    public static int getMaxScrollLeft(final Element element) {
+        int scrollWidth = element.getScrollWidth();
+        int clientWidth = element.getClientWidth();
+        return scrollWidth - clientWidth;
+    }
+
+    /**
+     * Checks if scrollLeft of the element is at its maximum value. Returns
+     * false if the element can't be scrolled horizontally.
+     * 
+     * @since 7.1.9
+     * @param element
+     *            which scrollLeft should be checked
+     * @return true, if scrollLeft is at maximum (false if element can't be
+     *         scrolled horizontally)
+     */
+    public static boolean isScrollLeftAtMax(final Element element) {
+        int scrollLeft = element.getScrollLeft();
+        return scrollLeft != 0 && scrollLeft == getMaxScrollLeft(element);
+    }
+
+    /**
      * Run workaround for webkits overflow auto issue.
      * 
      * See: our bug #2138 and https://bugs.webkit.org/show_bug.cgi?id=21462
@@ -469,6 +498,8 @@ public class Util {
             // check the scrolltop value before hiding the element
             final int scrolltop = elem.getScrollTop();
             final int scrollleft = elem.getScrollLeft();
+            final boolean scrollLeftAtMax = isScrollLeftAtMax(elem);
+
             elem.getStyle().setProperty("overflow", "hidden");
 
             Scheduler.get().scheduleDeferred(new Command() {
@@ -492,6 +523,12 @@ public class Util {
                         elem.setScrollTop(scrollvalue);
                     }
 
+                    // keep horizontal scroll at max if it was before vertical
+                    // scroll bar was added/removed
+                    if (scrollLeftAtMax) {
+                        elem.setScrollLeft(getMaxScrollLeft(elem));
+                    }
+
                     // fix for #6940 : Table horizontal scroll sometimes not
                     // updated when collapsing/expanding columns
                     // Also appeared in Safari 5.1 with webkit 534 (#7667)
diff --git a/client/src/com/vaadin/client/ui/VScrollTable.java b/client/src/com/vaadin/client/ui/VScrollTable.java
index 0304acd590..48fd85144f 100644
--- a/client/src/com/vaadin/client/ui/VScrollTable.java
+++ b/client/src/com/vaadin/client/ui/VScrollTable.java
@@ -276,6 +276,10 @@ public class VScrollTable extends FlowPanel implements HasWidgets,
      */
     private int detachedScrollPosition = 0;
 
+    // fields used in fixing erroneously lost scrollLeft
+    int lastScrollBodyHeight = 0;
+    boolean lastScrollLeftWasAtMax = false;
+
     /**
      * Represents a select range of rows
      */
@@ -1005,6 +1009,15 @@ public class VScrollTable extends FlowPanel implements HasWidgets,
         initialContentReceived = true;
         sizeNeedsInit = true;
         scrollBody.restoreRowVisibility();
+
+        // At least FireFox requires that scrollLeft is restored deferred after
+        // scrollBody is recreated
+        Scheduler.get().scheduleFinally(new ScheduledCommand() {
+            @Override
+            public void execute() {
+                restoreScrollLeft();
+            }
+        });
     }
 
     /** For internal use only. May be removed or replaced in the future. */
@@ -6865,12 +6878,44 @@ public class VScrollTable extends FlowPanel implements HasWidgets,
     }
 
     /**
+     * Tries to restore horizontal scroll position if it was lost due to change
+     * in the height of scrollBody (#12652).
+     */
+    private void restoreScrollLeft() {
+        int upcomingScrollLeft = scrollLeft;
+
+        if (lastScrollLeftWasAtMax) {
+            upcomingScrollLeft = Util.getMaxScrollLeft(scrollBodyPanel
+                    .getElement());
+        }
+        scrollBodyPanel.getElement().setScrollLeft(upcomingScrollLeft);
+    }
+
+    /**
+     * Checks if restore of scrollLeft is needed by checking if height of the
+     * scrollBody has changed.
+     * 
+     * @return true, if restore is required
+     */
+    private boolean isScrollLeftRestoreRequired() {
+        return (scrollBody.getElement().getClientHeight() != lastScrollBodyHeight);
+    }
+
+    /**
      * This method has logic which rows needs to be requested from server when
      * user scrolls
      */
-
     @Override
     public void onScroll(ScrollEvent event) {
+        // restore in initializeRows() doesn't work right with Chrome
+        if (isScrollLeftRestoreRequired()) {
+            restoreScrollLeft();
+        }
+
+        lastScrollBodyHeight = scrollBody.getElement().getClientHeight();
+        lastScrollLeftWasAtMax = Util.isScrollLeftAtMax(scrollBodyPanel
+                .getElement());
+
         scrollLeft = scrollBodyPanel.getElement().getScrollLeft();
         scrollTop = scrollBodyPanel.getScrollPosition();
         /*
diff --git a/uitest/src/com/vaadin/tests/components/table/TableHorizontalScrollPositionOnItemSetChange.html b/uitest/src/com/vaadin/tests/components/table/TableHorizontalScrollPositionOnItemSetChange.html
new file mode 100644
index 0000000000..6fd54ba0ca
--- /dev/null
+++ b/uitest/src/com/vaadin/tests/components/table/TableHorizontalScrollPositionOnItemSetChange.html
@@ -0,0 +1,110 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
+<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
+<head profile="http://selenium-ide.openqa.org/profiles/test-case">
+<meta http-equiv="Content-Type" content="text/html; charset=UTF-8" />
+<link rel="selenium.base" href="http://localhost:8888/" />
+<title>TableHorizontalScrollPositionOnItemSetChange</title>
+</head>
+<body>
+	<table cellpadding="1" cellspacing="1" border="1">
+		<thead>
+			<tr>
+				<td rowspan="1" colspan="3">TableHorizontalScrollPositionOnItemSetChange</td>
+			</tr>
+		</thead>
+		<tbody>
+			<tr>
+				<td>open</td>
+				<td>/run/com.vaadin.tests.components.table.TableHorizontalScrollPositionOnItemSetChange?restartApplication</td>
+				<td></td>
+			</tr>
+			<tr>
+				<td>scrollLeft</td>
+				<td>vaadin=runcomvaadintestscomponentstableTableHorizontalScrollPositionOnItemSetChange::PID_Shorscrolltable/domChild[1]</td>
+				<td>326</td>
+			</tr>
+			<tr>
+				<td>pause</td>
+				<td>500</td>
+				<td></td>
+			</tr>
+			<tr>
+				<td>click</td>
+				<td>vaadin=runcomvaadintestscomponentstableTableHorizontalScrollPositionOnItemSetChange::PID_Slessitems/domChild[0]/domChild[0]</td>
+				<td></td>
+			</tr>
+			<tr>
+				<td>pause</td>
+				<td>500</td>
+				<td></td>
+			</tr>
+			<tr>
+				<td>screenCapture</td>
+				<td></td>
+				<td>left-scroll-position-stays-in-middle-with-fewer-items</td>
+			</tr>
+			<tr>
+				<td>click</td>
+				<td>vaadin=runcomvaadintestscomponentstableTableHorizontalScrollPositionOnItemSetChange::PID_Smoreitems/domChild[0]/domChild[0]</td>
+				<td></td>
+			</tr>
+			<tr>
+				<td>pause</td>
+				<td>500</td>
+				<td></td>
+			</tr>
+			<tr>
+				<td>screenCapture</td>
+				<td></td>
+				<td>left-scroll-position-stays-in-middle-with-more-items</td>
+			</tr>
+			<tr>
+				<td>scrollLeft</td>
+				<td>vaadin=runcomvaadintestscomponentstableTableHorizontalScrollPositionOnItemSetChange::PID_Shorscrolltable/domChild[1]</td>
+				<td>653</td>
+			</tr>
+			<tr>
+				<td>pause</td>
+				<td>500</td>
+				<td></td>
+			</tr>
+			<tr>
+				<td>click</td>
+				<td>vaadin=runcomvaadintestscomponentstableTableHorizontalScrollPositionOnItemSetChange::PID_Slessitems/domChild[0]/domChild[0]</td>
+				<td></td>
+			</tr>
+			<tr>
+				<td>pause</td>
+				<td>500</td>
+				<td></td>
+			</tr>
+			<tr>
+				<td>screenCapture</td>
+				<td></td>
+				<td>left-scroll-position-stays-max-with-fewer-items</td>
+			</tr>
+			<tr>
+				<td>click</td>
+				<td>vaadin=runcomvaadintestscomponentstableTableHorizontalScrollPositionOnItemSetChange::PID_Smoreitems/domChild[0]/domChild[0]</td>
+				<td></td>
+			</tr>
+			<tr>
+				<td>pause</td>
+				<td>500</td>
+				<td></td>
+			</tr>
+			<tr>
+				<td>screenCapture</td>
+				<td></td>
+				<td>left-scroll-position-stays-max-with-more-items</td>
+			</tr>
+			<tr>
+				<td>scrollLeft</td>
+				<td>vaadin=runcomvaadintestscomponentstableTableHorizontalScrollPositionOnItemSetChange::PID_Shorscrolltable/domChild[1]</td>
+				<td>0</td>
+			</tr>
+		</tbody>
+	</table>
+</body>
+</html>
diff --git a/uitest/src/com/vaadin/tests/components/table/TableHorizontalScrollPositionOnItemSetChange.java b/uitest/src/com/vaadin/tests/components/table/TableHorizontalScrollPositionOnItemSetChange.java
new file mode 100644
index 0000000000..1f59a84428
--- /dev/null
+++ b/uitest/src/com/vaadin/tests/components/table/TableHorizontalScrollPositionOnItemSetChange.java
@@ -0,0 +1,101 @@
+/*
+ * Copyright 2000-2013 Vaadin Ltd.
+ * 
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not
+ * use this file except in compliance with the License. You may obtain a copy of
+ * the License at
+ * 
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package com.vaadin.tests.components.table;
+
+import com.vaadin.server.VaadinRequest;
+import com.vaadin.tests.components.AbstractTestUI;
+import com.vaadin.ui.Button;
+import com.vaadin.ui.Button.ClickEvent;
+import com.vaadin.ui.HorizontalLayout;
+import com.vaadin.ui.Table;
+import com.vaadin.ui.VerticalLayout;
+
+public class TableHorizontalScrollPositionOnItemSetChange extends
+        AbstractTestUI {
+
+    @Override
+    protected void setup(VaadinRequest request) {
+        VerticalLayout layout = new VerticalLayout();
+        layout.setMargin(true);
+        layout.setSpacing(true);
+        setContent(layout);
+
+        final Table table = new Table();
+        table.setWidth("640px");
+        table.setHeight("243px");
+        table.setId("horscrolltable");
+        layout.addComponent(table);
+
+        for (int i = 0; i < 15; i++) {
+            table.addContainerProperty("Column " + i, String.class, null);
+        }
+
+        for (int i = 0; i < 60; i++) {
+            table.addItem();
+        }
+
+        Button lessItems = new Button("Less items", new Button.ClickListener() {
+
+            @Override
+            public void buttonClick(Button.ClickEvent event) {
+                table.removeAllItems();
+                for (int i = 0; i < 5; i++) {
+                    table.addItem();
+                }
+            }
+        });
+        lessItems.setId("lessitems");
+
+        Button moreItems = new Button("More items", new Button.ClickListener() {
+
+            @Override
+            public void buttonClick(Button.ClickEvent event) {
+                table.removeAllItems();
+                for (int i = 0; i < 50; i++) {
+                    table.addItem();
+                }
+            }
+        });
+        moreItems.setId("moreitems");
+
+        Button clearItems = new Button("Clear all", new Button.ClickListener() {
+            @Override
+            public void buttonClick(ClickEvent event) {
+                table.removeAllItems();
+            }
+        });
+
+        HorizontalLayout buttonLayout = new HorizontalLayout();
+        buttonLayout.setSpacing(true);
+        layout.addComponent(buttonLayout);
+
+        buttonLayout.addComponent(lessItems);
+        buttonLayout.addComponent(moreItems);
+        buttonLayout.addComponent(clearItems);
+        clearItems.setId("clearitems");
+    }
+
+    @Override
+    protected String getTestDescription() {
+        return "Horizontal scrolling position should not be lost if amount of items changes in Table.";
+    }
+
+    @Override
+    protected Integer getTicketNumber() {
+        return 12652;
+    }
+
+}