Escalator now reorders also spacers in DOM (#17334)

Change-Id: I23832565bb1633dca4bac986b1a2e37f99163eb2

4 files changed, 121 insertions, 9 deletions

diff --git a/client/src/com/vaadin/client/widgets/Escalator.java b/client/src/com/vaadin/client/widgets/Escalator.java
index 0ce5aff74e..fe8ba4f67e 100644
--- a/client/src/com/vaadin/client/widgets/Escalator.java
+++ b/client/src/com/vaadin/client/widgets/Escalator.java
@@ -3692,11 +3692,12 @@ public class Escalator extends Widget implements RequiresResize,
              * its parents are) removed from the document. Therefore, we sort
              * everything around that row instead.
              */
-            final TableRowElement focusedRow = getEscalatorRowWithFocus();
+            final TableRowElement focusedRow = getRowWithFocus();
 
             if (focusedRow != null) {
                 assert focusedRow.getParentElement() == root : "Trying to sort around a row that doesn't exist in body";
-                assert visualRowOrder.contains(focusedRow) : "Trying to sort around a row that doesn't exist in visualRowOrder.";
+                assert visualRowOrder.contains(focusedRow)
+                        || body.spacerContainer.isSpacer(focusedRow) : "Trying to sort around a row that doesn't exist in visualRowOrder or is not a spacer.";
             }
 
             /*
@@ -3717,6 +3718,21 @@ public class Escalator extends Widget implements RequiresResize,
              * the first child.
              */
 
+            List<TableRowElement> orderedBodyRows = new ArrayList<TableRowElement>(
+                    visualRowOrder);
+            for (int i = 0; i < visualRowOrder.size(); i++) {
+                SpacerContainer.SpacerImpl spacer = body.spacerContainer
+                        .getSpacer(getTopRowLogicalIndex() + i);
+
+                if (spacer != null) {
+                    orderedBodyRows.add(i + 1, spacer.getRootElement());
+                }
+            }
+            /*
+             * At this point, invisible spacers aren't reordered, so their
+             * position in the DOM is undefined.
+             */
+
             /*
              * If we have a focused row, start in the mode where we put
              * everything underneath that row. Otherwise, all rows are placed as
@@ -3724,8 +3740,8 @@ public class Escalator extends Widget implements RequiresResize,
              */
             boolean insertFirst = (focusedRow == null);
 
-            final ListIterator<TableRowElement> i = visualRowOrder
-                    .listIterator(visualRowOrder.size());
+            final ListIterator<TableRowElement> i = orderedBodyRows
+                    .listIterator(orderedBodyRows.size());
             while (i.hasPrevious()) {
                 TableRowElement tr = i.previous();
 
@@ -3742,12 +3758,13 @@ public class Escalator extends Widget implements RequiresResize,
         }
 
         /**
-         * Get the escalator row that has focus.
+         * Get the {@literal <tbody>} row that contains (or has) focus.
          * 
-         * @return The escalator row that contains a focused DOM element, or
-         *         <code>null</code> if focus is outside of a body row.
+         * @return The {@literal <tbody>} row that contains a focused DOM
+         *         element, or <code>null</code> if focus is outside of a body
+         *         row.
          */
-        private TableRowElement getEscalatorRowWithFocus() {
+        private TableRowElement getRowWithFocus() {
             TableRowElement rowContainingFocus = null;
 
             final Element focusedElement = WidgetUtil.getFocusedElement();
@@ -4781,6 +4798,24 @@ public class Escalator extends Widget implements RequiresResize,
             }
         }
 
+        /** Checks if a given element is a spacer element */
+        public boolean isSpacer(TableRowElement focusedRow) {
+
+            /*
+             * If this needs optimization, we could do a more heuristic check
+             * based on stylenames and stuff, instead of iterating through the
+             * map.
+             */
+
+            for (SpacerImpl spacer : rowIndexToSpacer.values()) {
+                if (spacer.getRootElement().equals(focusedRow)) {
+                    return true;
+                }
+            }
+
+            return false;
+        }
+
         @SuppressWarnings("boxing")
         void scrollToSpacer(int spacerIndex, ScrollDestination destination,
                 int padding) {
@@ -5140,6 +5175,8 @@ public class Escalator extends Widget implements RequiresResize,
             spacer.setupDom(height);
 
             initSpacerContent(spacer);
+
+            body.sortDomElements();
         }
 
         private void updateExistingSpacer(int rowIndex, double newHeight) {
diff --git a/uitest/src/com/vaadin/tests/components/grid/basicfeatures/EscalatorBasicClientFeaturesTest.java b/uitest/src/com/vaadin/tests/components/grid/basicfeatures/EscalatorBasicClientFeaturesTest.java
index 9037b29bb1..e1a3fc0f55 100644
--- a/uitest/src/com/vaadin/tests/components/grid/basicfeatures/EscalatorBasicClientFeaturesTest.java
+++ b/uitest/src/com/vaadin/tests/components/grid/basicfeatures/EscalatorBasicClientFeaturesTest.java
@@ -77,6 +77,7 @@ public abstract class EscalatorBasicClientFeaturesTest extends MultiBrowserTest
     protected static final String COLSPAN_NONE = "Apply no colspan";
     protected static final String SET_100PX = "Set 100px";
     protected static final String SPACERS = "Spacers";
+    protected static final String FOCUSABLE_UPDATER = "Focusable Updater";
     protected static final String SCROLL_HERE_ANY_0PADDING = "Scroll here (ANY, 0)";
     protected static final String SCROLL_HERE_SPACERBELOW_ANY_0PADDING = "Scroll here row+spacer below (ANY, 0)";
     protected static final String REMOVE = "Remove";
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 ce8fcd233e..7905906536 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
@@ -27,11 +27,13 @@ import java.util.regex.Pattern;
 import org.junit.Before;
 import org.junit.ComparisonFailure;
 import org.junit.Test;
+import org.openqa.selenium.By;
 import org.openqa.selenium.WebElement;
 
 import com.vaadin.client.WidgetUtil;
 import com.vaadin.shared.ui.grid.Range;
 import com.vaadin.testbench.elements.NotificationElement;
+import com.vaadin.testbench.parallel.BrowserUtil;
 import com.vaadin.tests.components.grid.basicfeatures.EscalatorBasicClientFeaturesTest;
 
 @SuppressWarnings("boxing")
@@ -96,6 +98,7 @@ public class EscalatorSpacerTest extends EscalatorBasicClientFeaturesTest {
 
     @Before
     public void before() {
+        setDebug(true);
         openTestURL();
         selectMenuPath(COLUMNS_AND_ROWS, BODY_ROWS, "Set 20px default height");
         populate();
@@ -372,6 +375,59 @@ public class EscalatorSpacerTest extends EscalatorBasicClientFeaturesTest {
         assertEquals(getScrollTop(), 950);
     }
 
+    @Test
+    public void domCanBeSortedWithFocusInSpacer() throws InterruptedException {
+
+        // Firefox behaves badly with focus-related tests - skip it.
+        if (BrowserUtil.isFirefox(super.getDesiredCapabilities())) {
+            return;
+        }
+
+        selectMenuPath(FEATURES, SPACERS, FOCUSABLE_UPDATER);
+        selectMenuPath(FEATURES, SPACERS, ROW_1, SET_100PX);
+
+        WebElement inputElement = getEscalator().findElement(
+                By.tagName("input"));
+        inputElement.click();
+        scrollVerticallyTo(30);
+
+        // Sleep needed because of all the JS we're doing, and to let
+        // the DOM reordering to take place.
+        Thread.sleep(500);
+
+        assertFalse("Error message detected", $(NotificationElement.class)
+                .exists());
+    }
+
+    @Test
+    public void spacersAreInsertedInCorrectDomPosition() {
+        selectMenuPath(FEATURES, SPACERS, ROW_1, SET_100PX);
+
+        WebElement tbody = getEscalator().findElement(By.tagName("tbody"));
+        WebElement spacer = getChild(tbody, 2);
+        String cssClass = spacer.getAttribute("class");
+        assertTrue("element index 2 was not a spacer (class=\"" + cssClass
+                + "\")", cssClass.contains("-spacer"));
+    }
+
+    @Test
+    public void spacersAreInCorrectDomPositionAfterScroll() {
+        selectMenuPath(FEATURES, SPACERS, ROW_1, SET_100PX);
+
+        scrollVerticallyTo(30); // roughly one row's worth
+
+        WebElement tbody = getEscalator().findElement(By.tagName("tbody"));
+        WebElement spacer = getChild(tbody, 1);
+        String cssClass = spacer.getAttribute("class");
+        assertTrue("element index 1 was not a spacer (class=\"" + cssClass
+                + "\")", cssClass.contains("-spacer"));
+    }
+
+    private WebElement getChild(WebElement parent, int childIndex) {
+        return (WebElement) executeScript("return arguments[0].children["
+                + childIndex + "];", parent);
+    }
+
     private static double[] getElementDimensions(WebElement element) {
         /*
          * we need to parse the style attribute, since using getCssValue gets a
diff --git a/uitest/src/com/vaadin/tests/widgetset/client/grid/EscalatorBasicClientFeaturesWidget.java b/uitest/src/com/vaadin/tests/widgetset/client/grid/EscalatorBasicClientFeaturesWidget.java
index ec9f214748..c735797731 100644
--- a/uitest/src/com/vaadin/tests/widgetset/client/grid/EscalatorBasicClientFeaturesWidget.java
+++ b/uitest/src/com/vaadin/tests/widgetset/client/grid/EscalatorBasicClientFeaturesWidget.java
@@ -6,6 +6,7 @@ import java.util.List;
 import com.google.gwt.core.client.Duration;
 import com.google.gwt.core.client.Scheduler.ScheduledCommand;
 import com.google.gwt.dom.client.TableCellElement;
+import com.google.gwt.user.client.DOM;
 import com.google.gwt.user.client.ui.Composite;
 import com.google.gwt.user.client.ui.HTML;
 import com.vaadin.client.widget.escalator.EscalatorUpdater;
@@ -658,7 +659,7 @@ public class EscalatorBasicClientFeaturesWidget extends
             public void execute() {
                 BodyRowContainer body = escalator.getBody();
 
-                if (body.getSpacerUpdater().equals(SpacerUpdater.NULL)) {
+                if (SpacerUpdater.NULL.equals(body.getSpacerUpdater())) {
                     body.setSpacerUpdater(CUSTOM);
                 } else {
                     body.setSpacerUpdater(SpacerUpdater.NULL);
@@ -666,6 +667,23 @@ public class EscalatorBasicClientFeaturesWidget extends
             }
         }, menupath);
 
+        addMenuCommand("Focusable Updater", new ScheduledCommand() {
+            @Override
+            public void execute() {
+                escalator.getBody().setSpacerUpdater(new SpacerUpdater() {
+                    @Override
+                    public void init(Spacer spacer) {
+                        spacer.getElement().appendChild(DOM.createInputText());
+                    }
+
+                    @Override
+                    public void destroy(Spacer spacer) {
+                        spacer.getElement().removeAllChildren();
+                    }
+                });
+            }
+        }, menupath);
+
         createSpacersMenuForRow(-1, menupath);
         createSpacersMenuForRow(1, menupath);
         createSpacersMenuForRow(50, menupath);