Fixes issue with Table not scrolling completely to the end #12651

Made the Table notice if the user is trying to scroll to an item on the
last "page" and in those cases actually scroll to that item, not just to
the page's first item as it did before.

Change-Id: I47df33c75aa9b7e4f9a5f4bd5daeb301028517e8

client/src/com/vaadin/client/ui/VScrollTable.java

@@ -176,6 +176,8 @@ public class VScrollTable extends FlowPanel implements HasWidgets,
     private int firstRowInViewPort = 0;
     private int pageLength = 15;
     private int lastRequestedFirstvisible = 0; // to detect "serverside scroll"
+    private int firstvisibleOnLastPage = -1; // To detect if the first visible
+                                             // is on the last page
 
     /** For internal use only. May be removed or replaced in the future. */
     public boolean showRowHeaders = false;
@@ -1111,8 +1113,14 @@ public class VScrollTable extends FlowPanel implements HasWidgets,
     private ScheduledCommand lazyScroller = new ScheduledCommand() {
         @Override
         public void execute() {
-            int offsetTop = measureRowHeightOffset(firstvisible);
-            scrollBodyPanel.setScrollPosition(offsetTop);
+            if (firstvisibleOnLastPage > -1) {
+                scrollBodyPanel
+                        .setScrollPosition(measureRowHeightOffset(firstvisibleOnLastPage));
+            } else {
+                scrollBodyPanel
+                        .setScrollPosition(measureRowHeightOffset(firstvisible));
+            }
+            firstRowInViewPort = firstvisible;
         }
     };
 
@@ -1120,6 +1128,8 @@ public class VScrollTable extends FlowPanel implements HasWidgets,
     public void updateFirstVisibleAndScrollIfNeeded(UIDL uidl) {
         firstvisible = uidl.hasVariable("firstvisible") ? uidl
                 .getIntVariable("firstvisible") : 0;
+        firstvisibleOnLastPage = uidl.hasVariable("firstvisibleonlastpage") ? uidl
+                .getIntVariable("firstvisibleonlastpage") : -1;
         if (firstvisible != lastRequestedFirstvisible && scrollBody != null) {
             // received 'surprising' firstvisible from server: scroll there
             firstRowInViewPort = firstvisible;
@@ -2150,18 +2160,7 @@ public class VScrollTable extends FlowPanel implements HasWidgets,
 
         isNewBody = false;
 
-        if (firstvisible > 0) {
-            // Deferred due to some Firefox oddities
-            Scheduler.get().scheduleDeferred(new Command() {
-
-                @Override
-                public void execute() {
-                    scrollBodyPanel
-                            .setScrollPosition(measureRowHeightOffset(firstvisible));
-                    firstRowInViewPort = firstvisible;
-                }
-            });
-        }
+        Scheduler.get().scheduleFinally(lazyScroller);
 
         if (enabled) {
             // Do we need cache rows

server/src/com/vaadin/ui/Table.java

@@ -426,6 +426,12 @@ public class Table extends AbstractSelect implements Action.Container,
      */
     private int currentPageFirstItemIndex = 0;
 
+    /**
+     * Index of the "first" item on the last page if a user has used
+     * setCurrentPageFirstItemIndex to scroll down. -1 if not set.
+     */
+    private int currentPageFirstItemIndexOnLastPage = -1;
+
     /**
      * Holds value of property selectable.
      */
@@ -1477,12 +1483,14 @@ public class Table extends AbstractSelect implements Action.Container,
         }
 
         /*
-         * FIXME #7607 Take somehow into account the case where we want to
-         * scroll to the bottom so that the last row is completely visible even
-         * if (table height) / (row height) is not an integer. Reverted the
-         * original fix because of #8662 regression.
+         * If the new index is on the last page we set the index to be the first
+         * item on that last page and make a note of the real index for the
+         * client side to be able to move the scroll position to the correct
+         * position.
          */
+        int indexOnLastPage = -1;
         if (newIndex > maxIndex) {
+            indexOnLastPage = newIndex;
             newIndex = maxIndex;
         }
 
@@ -1494,6 +1502,20 @@ public class Table extends AbstractSelect implements Action.Container,
                 currentPageFirstItemId = null;
             }
             currentPageFirstItemIndex = newIndex;
+
+            if (needsPageBufferReset) {
+                /*
+                 * The flag currentPageFirstItemIndexOnLastPage denotes a user
+                 * set scrolling position on the last page via
+                 * setCurrentPageFirstItemIndex() and shouldn't be changed by
+                 * the table component internally changing the firstvisible item
+                 * on lazy row fetching. Doing so would make the scrolling
+                 * position not be updated correctly when the lazy rows are
+                 * finally rendered.
+                 */
+                currentPageFirstItemIndexOnLastPage = indexOnLastPage;
+            }
+
         } else {
 
             // For containers not supporting indexes, we must iterate the
@@ -3447,6 +3469,8 @@ public class Table extends AbstractSelect implements Action.Container,
         if (getCurrentPageFirstItemIndex() != 0 || getPageLength() > 0) {
             target.addVariable(this, "firstvisible",
                     getCurrentPageFirstItemIndex());
+            target.addVariable(this, "firstvisibleonlastpage",
+                    currentPageFirstItemIndexOnLastPage);
         }
     }
 

uitest/src/com/vaadin/tests/components/table/ShowLastItem.html

@@ -0,0 +1,36 @@
+<?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" />
+<title>New Test</title>
+</head>
+<body>
+<table cellpadding="1" cellspacing="1" border="1">
+<thead>
+<tr><td rowspan="1" colspan="3">New Test</td></tr>
+</thead><tbody>
+<tr>
+    <td>open</td>
+    <td>/run/com.vaadin.tests.components.table.ShowLastItem?restartApplication</td>
+    <td></td>
+</tr>
+<tr>
+    <td>click</td>
+    <td>vaadin=runcomvaadintestscomponentstableShowLastItem::/VVerticalLayout[0]/ChildComponentContainer[1]/VVerticalLayout[0]/ChildComponentContainer[1]/VButton[0]/domChild[0]/domChild[0]</td>
+    <td></td>
+</tr>
+<tr>
+    <td>pause</td>
+    <td>1000</td>
+    <td></td>
+</tr>
+<tr>
+    <td>screenCapture</td>
+    <td></td>
+    <td>row-20-fully-visible</td>
+</tr>
+
+</tbody></table>
+</body>
+</html>

uitest/src/com/vaadin/tests/components/table/ShowLastItem.java

@@ -0,0 +1,66 @@
+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.Table;
+
+public class ShowLastItem extends AbstractTestUI {
+
+    /*
+     * (non-Javadoc)
+     * 
+     * @see com.vaadin.tests.components.AbstractTestUI#setup(com.vaadin.server.
+     * VaadinRequest)
+     */
+    @Override
+    protected void setup(VaadinRequest request) {
+        final Table table = new Table();
+        table.setHeight("210px");
+
+        table.addContainerProperty("Col", String.class, "");
+
+        for (int i = 0; i < 20; i++) {
+            table.addItem(i).getItemProperty("Col")
+                    .setValue("row " + String.valueOf(i));
+        }
+
+        Button addItemBtn = new Button("Add item", new Button.ClickListener() {
+
+            @Override
+            public void buttonClick(ClickEvent event) {
+                Object itemId = "row " + table.getItemIds().size();
+
+                table.addItem(itemId).getItemProperty("Col")
+                        .setValue(String.valueOf(itemId));
+
+                table.setCurrentPageFirstItemIndex(table.getItemIds().size() - 1);
+            }
+        });
+
+        addComponent(table);
+        addComponent(addItemBtn);
+    }
+
+    /*
+     * (non-Javadoc)
+     * 
+     * @see com.vaadin.tests.components.AbstractTestUI#getTestDescription()
+     */
+    @Override
+    protected String getTestDescription() {
+        return "Show last item in Table by using setCurrentPageFirstItemId";
+    }
+
+    /*
+     * (non-Javadoc)
+     * 
+     * @see com.vaadin.tests.components.AbstractTestUI#getTicketNumber()
+     */
+    @Override
+    protected Integer getTicketNumber() {
+        return 12407;
+    }
+
+}