Fixed ordering in ordered layout when inserting components #10154

Change-Id: I025745a4f575ecaece0e897e9b8f90adba7c2d63

4 files changed, 289 insertions, 4 deletions

diff --git a/client/src/com/vaadin/client/ui/orderedlayout/AbstractOrderedLayoutConnector.java b/client/src/com/vaadin/client/ui/orderedlayout/AbstractOrderedLayoutConnector.java
index 3b72af966b..23308e2004 100644
--- a/client/src/com/vaadin/client/ui/orderedlayout/AbstractOrderedLayoutConnector.java
+++ b/client/src/com/vaadin/client/ui/orderedlayout/AbstractOrderedLayoutConnector.java
@@ -301,6 +301,8 @@ public abstract class AbstractOrderedLayoutConnector extends
         int currentIndex = 0;
         VOrderedLayout layout = getWidget();
 
+        layout.setSpacing(getState().spacing);
+
         for (ComponentConnector child : getChildComponents()) {
             Slot slot = layout.getSlot(child.getWidget());
             if (slot.getParent() != layout) {
diff --git a/client/src/com/vaadin/client/ui/orderedlayout/VOrderedLayout.java b/client/src/com/vaadin/client/ui/orderedlayout/VOrderedLayout.java
index 24e3576b49..ad1d83795e 100644
--- a/client/src/com/vaadin/client/ui/orderedlayout/VOrderedLayout.java
+++ b/client/src/com/vaadin/client/ui/orderedlayout/VOrderedLayout.java
@@ -62,12 +62,32 @@ public class VOrderedLayout extends FlowPanel {
     }
 
     /**
-     * Add or move a slot to another index
+     * Add or move a slot to another index.
+     * 
+     * <p>
+     * You should note that the index does not refer to the DOM index if
+     * spacings are used. If spacings are used then the index will be adjusted
+     * to include the spacings when inserted.
+     * </p>
+     * <p>
+     * For instance when using spacing the index converts to DOM index in the
+     * following way:
+     * 
+     * <pre>
+     * index : 0 -> DOM index: 0
+     * index : 1 -> DOM index: 1
+     * index : 2 -> DOM index: 3
+     * index : 3 -> DOM index: 5
+     * index : 4 -> DOM index: 7
+     * </pre>
+     * 
+     * When using this method never account for spacings.
+     * </p>
      * 
      * @param slot
      *            The slot to move or add
      * @param index
-     *            The index where the slot should be placed
+     *            The index where the slot should be placed.
      */
     void addOrMoveSlot(Slot slot, int index) {
         if (slot.getParent() == this) {
@@ -76,7 +96,13 @@ public class VOrderedLayout extends FlowPanel {
                 return;
             }
         }
+
         insert(slot, index);
+
+        /*
+         * We need to confirm spacings are correctly applied after each insert.
+         */
+        setSpacing(spacing);
     }
 
     /**
@@ -98,8 +124,23 @@ public class VOrderedLayout extends FlowPanel {
         // Physical attach.
         container = expandWrapper != null ? expandWrapper : getElement();
         if (domInsert) {
-            DOM.insertChild(container, child.getElement(),
-                    spacing ? beforeIndex * 2 : beforeIndex);
+            if (spacing) {
+                if (beforeIndex != 0) {
+                    /*
+                     * Since the spacing elements are located at the same DOM
+                     * level as the slots we need to take them into account when
+                     * calculating the slot position.
+                     * 
+                     * The spacing elements are always located before the actual
+                     * slot except for the first slot which do not have a
+                     * spacing element like this
+                     * 
+                     * |<slot1><spacing2><slot2><spacing3><slot3>...|
+                     */
+                    beforeIndex = beforeIndex * 2 - 1;
+                }
+            }
+            DOM.insertChild(container, child.getElement(), beforeIndex);
         } else {
             DOM.appendChild(container, child.getElement());
         }
@@ -402,6 +443,12 @@ public class VOrderedLayout extends FlowPanel {
             if (spacing && spacer == null) {
                 spacer = DOM.createDiv();
                 spacer.addClassName("v-spacing");
+                
+                /*
+                 * This has to be done here for the initial render. In other
+                 * cases where the spacer already exists onAttach will handle
+                 * it.
+                 */
                 getElement().getParentElement().insertBefore(spacer,
                         getElement());
             } else if (!spacing && spacer != null) {
@@ -916,6 +963,8 @@ public class VOrderedLayout extends FlowPanel {
         for (Slot slot : widgetToSlot.values()) {
             if (getWidgetIndex(slot) > 0) {
                 slot.setSpacing(spacing);
+            } else {
+                slot.setSpacing(false);
             }
         }
     }
diff --git a/uitest/src/com/vaadin/tests/components/orderedlayout/OrderedLayoutComponentOrdering.html b/uitest/src/com/vaadin/tests/components/orderedlayout/OrderedLayoutComponentOrdering.html
new file mode 100644
index 0000000000..ba75d7e56e
--- /dev/null
+++ b/uitest/src/com/vaadin/tests/components/orderedlayout/OrderedLayoutComponentOrdering.html
@@ -0,0 +1,151 @@
+<?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>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.orderedlayout.OrderedLayoutComponentOrdering?restartApplication</td>
+    <td></td>
+</tr>
+<!--Add two buttons as first-->
+<tr>
+    <td>click</td>
+    <td>vaadin=runcomvaadintestscomponentsorderedlayoutOrderedLayoutComponentOrdering::/VVerticalLayout[0]/VOrderedLayout$Slot[1]/VVerticalLayout[0]/VOrderedLayout$Slot[1]/VButton[0]/domChild[0]</td>
+    <td></td>
+</tr>
+<tr>
+    <td>assertText</td>
+    <td>vaadin=runcomvaadintestscomponentsorderedlayoutOrderedLayoutComponentOrdering::/VVerticalLayout[0]/VOrderedLayout$Slot[1]/VVerticalLayout[0]/VOrderedLayout$Slot[0]/VHorizontalLayout[0]/VOrderedLayout$Slot[0]/VButton[0]/domChild[0]/domChild[0]</td>
+    <td>3</td>
+</tr>
+<tr>
+    <td>assertText</td>
+    <td>vaadin=runcomvaadintestscomponentsorderedlayoutOrderedLayoutComponentOrdering::/VVerticalLayout[0]/VOrderedLayout$Slot[1]/VVerticalLayout[0]/VOrderedLayout$Slot[0]/VHorizontalLayout[0]/VOrderedLayout$Slot[1]/VButton[0]/domChild[0]/domChild[0]</td>
+    <td>4</td>
+</tr>
+<tr>
+    <td>assertText</td>
+    <td>vaadin=runcomvaadintestscomponentsorderedlayoutOrderedLayoutComponentOrdering::/VVerticalLayout[0]/VOrderedLayout$Slot[1]/VVerticalLayout[0]/VOrderedLayout$Slot[0]/VHorizontalLayout[0]/VOrderedLayout$Slot[2]/VButton[0]/domChild[0]/domChild[0]</td>
+    <td>1</td>
+</tr>
+<tr>
+    <td>assertText</td>
+    <td>vaadin=runcomvaadintestscomponentsorderedlayoutOrderedLayoutComponentOrdering::/VVerticalLayout[0]/VOrderedLayout$Slot[1]/VVerticalLayout[0]/VOrderedLayout$Slot[0]/VHorizontalLayout[0]/VOrderedLayout$Slot[3]/VButton[0]/domChild[0]/domChild[0]</td>
+    <td>2</td>
+</tr>
+<!--Add two buttons as second-->
+<tr>
+    <td>click</td>
+    <td>vaadin=runcomvaadintestscomponentsorderedlayoutOrderedLayoutComponentOrdering::/VVerticalLayout[0]/VOrderedLayout$Slot[1]/VVerticalLayout[0]/VOrderedLayout$Slot[2]/VButton[0]/domChild[0]/domChild[0]</td>
+    <td></td>
+</tr>
+<tr>
+    <td>assertText</td>
+    <td>vaadin=runcomvaadintestscomponentsorderedlayoutOrderedLayoutComponentOrdering::/VVerticalLayout[0]/VOrderedLayout$Slot[1]/VVerticalLayout[0]/VOrderedLayout$Slot[0]/VHorizontalLayout[0]/VOrderedLayout$Slot[0]/VButton[0]/domChild[0]/domChild[0]</td>
+    <td>3</td>
+</tr>
+<tr>
+    <td>assertText</td>
+    <td>vaadin=runcomvaadintestscomponentsorderedlayoutOrderedLayoutComponentOrdering::/VVerticalLayout[0]/VOrderedLayout$Slot[1]/VVerticalLayout[0]/VOrderedLayout$Slot[0]/VHorizontalLayout[0]/VOrderedLayout$Slot[1]/VButton[0]/domChild[0]/domChild[0]</td>
+    <td>5</td>
+</tr>
+<tr>
+    <td>assertText</td>
+    <td>vaadin=runcomvaadintestscomponentsorderedlayoutOrderedLayoutComponentOrdering::/VVerticalLayout[0]/VOrderedLayout$Slot[1]/VVerticalLayout[0]/VOrderedLayout$Slot[0]/VHorizontalLayout[0]/VOrderedLayout$Slot[2]/VButton[0]/domChild[0]/domChild[0]</td>
+    <td>6</td>
+</tr>
+<tr>
+    <td>assertText</td>
+    <td>vaadin=runcomvaadintestscomponentsorderedlayoutOrderedLayoutComponentOrdering::/VVerticalLayout[0]/VOrderedLayout$Slot[1]/VVerticalLayout[0]/VOrderedLayout$Slot[0]/VHorizontalLayout[0]/VOrderedLayout$Slot[3]/VButton[0]/domChild[0]/domChild[0]</td>
+    <td>4</td>
+</tr>
+<tr>
+    <td>assertText</td>
+    <td>vaadin=runcomvaadintestscomponentsorderedlayoutOrderedLayoutComponentOrdering::/VVerticalLayout[0]/VOrderedLayout$Slot[1]/VVerticalLayout[0]/VOrderedLayout$Slot[0]/VHorizontalLayout[0]/VOrderedLayout$Slot[4]/VButton[0]/domChild[0]/domChild[0]</td>
+    <td>1</td>
+</tr>
+<tr>
+    <td>assertText</td>
+    <td>vaadin=runcomvaadintestscomponentsorderedlayoutOrderedLayoutComponentOrdering::/VVerticalLayout[0]/VOrderedLayout$Slot[1]/VVerticalLayout[0]/VOrderedLayout$Slot[0]/VHorizontalLayout[0]/VOrderedLayout$Slot[5]/VButton[0]/domChild[0]/domChild[0]</td>
+    <td>2</td>
+</tr>
+<!--Add two buttons as third-->
+<tr>
+    <td>click</td>
+    <td>vaadin=runcomvaadintestscomponentsorderedlayoutOrderedLayoutComponentOrdering::/VVerticalLayout[0]/VOrderedLayout$Slot[1]/VVerticalLayout[0]/VOrderedLayout$Slot[3]/VButton[0]/domChild[0]/domChild[0]</td>
+    <td></td>
+</tr>
+<tr>
+    <td>assertText</td>
+    <td>vaadin=runcomvaadintestscomponentsorderedlayoutOrderedLayoutComponentOrdering::/VVerticalLayout[0]/VOrderedLayout$Slot[1]/VVerticalLayout[0]/VOrderedLayout$Slot[0]/VHorizontalLayout[0]/VOrderedLayout$Slot[0]/VButton[0]/domChild[0]/domChild[0]</td>
+    <td>3</td>
+</tr>
+<tr>
+    <td>assertText</td>
+    <td>vaadin=runcomvaadintestscomponentsorderedlayoutOrderedLayoutComponentOrdering::/VVerticalLayout[0]/VOrderedLayout$Slot[1]/VVerticalLayout[0]/VOrderedLayout$Slot[0]/VHorizontalLayout[0]/VOrderedLayout$Slot[1]/VButton[0]/domChild[0]/domChild[0]</td>
+    <td>5</td>
+</tr>
+<tr>
+    <td>assertText</td>
+    <td>vaadin=runcomvaadintestscomponentsorderedlayoutOrderedLayoutComponentOrdering::/VVerticalLayout[0]/VOrderedLayout$Slot[1]/VVerticalLayout[0]/VOrderedLayout$Slot[0]/VHorizontalLayout[0]/VOrderedLayout$Slot[2]/VButton[0]/domChild[0]/domChild[0]</td>
+    <td>7</td>
+</tr>
+<tr>
+    <td>assertText</td>
+    <td>vaadin=runcomvaadintestscomponentsorderedlayoutOrderedLayoutComponentOrdering::/VVerticalLayout[0]/VOrderedLayout$Slot[1]/VVerticalLayout[0]/VOrderedLayout$Slot[0]/VHorizontalLayout[0]/VOrderedLayout$Slot[3]/VButton[0]/domChild[0]/domChild[0]</td>
+    <td>8</td>
+</tr>
+<tr>
+    <td>assertText</td>
+    <td>vaadin=runcomvaadintestscomponentsorderedlayoutOrderedLayoutComponentOrdering::/VVerticalLayout[0]/VOrderedLayout$Slot[1]/VVerticalLayout[0]/VOrderedLayout$Slot[0]/VHorizontalLayout[0]/VOrderedLayout$Slot[4]/VButton[0]/domChild[0]/domChild[0]</td>
+    <td>6</td>
+</tr>
+<tr>
+    <td>assertText</td>
+    <td>vaadin=runcomvaadintestscomponentsorderedlayoutOrderedLayoutComponentOrdering::/VVerticalLayout[0]/VOrderedLayout$Slot[1]/VVerticalLayout[0]/VOrderedLayout$Slot[0]/VHorizontalLayout[0]/VOrderedLayout$Slot[5]/VButton[0]/domChild[0]/domChild[0]</td>
+    <td>4</td>
+</tr>
+<tr>
+    <td>assertText</td>
+    <td>vaadin=runcomvaadintestscomponentsorderedlayoutOrderedLayoutComponentOrdering::/VVerticalLayout[0]/VOrderedLayout$Slot[1]/VVerticalLayout[0]/VOrderedLayout$Slot[0]/VHorizontalLayout[0]/VOrderedLayout$Slot[6]/VButton[0]/domChild[0]/domChild[0]</td>
+    <td>1</td>
+</tr>
+<tr>
+    <td>assertText</td>
+    <td>vaadin=runcomvaadintestscomponentsorderedlayoutOrderedLayoutComponentOrdering::/VVerticalLayout[0]/VOrderedLayout$Slot[1]/VVerticalLayout[0]/VOrderedLayout$Slot[0]/VHorizontalLayout[0]/VOrderedLayout$Slot[7]/VButton[0]/domChild[0]/domChild[0]</td>
+    <td>2</td>
+</tr>
+<!--Move last to first-->
+<tr>
+    <td>click</td>
+    <td>vaadin=runcomvaadintestscomponentsorderedlayoutOrderedLayoutComponentOrdering::/VVerticalLayout[0]/VOrderedLayout$Slot[1]/VVerticalLayout[0]/VOrderedLayout$Slot[4]/VButton[0]/domChild[0]/domChild[0]</td>
+    <td></td>
+</tr>
+<tr>
+    <td>assertText</td>
+    <td>vaadin=runcomvaadintestscomponentsorderedlayoutOrderedLayoutComponentOrdering::/VVerticalLayout[0]/VOrderedLayout$Slot[1]/VVerticalLayout[0]/VOrderedLayout$Slot[0]/VHorizontalLayout[0]/VOrderedLayout$Slot[0]/VButton[0]/domChild[0]/domChild[0]</td>
+    <td>2</td>
+</tr>
+<!--Move forth to second-->
+<tr>
+    <td>click</td>
+    <td>vaadin=runcomvaadintestscomponentsorderedlayoutOrderedLayoutComponentOrdering::/VVerticalLayout[0]/VOrderedLayout$Slot[1]/VVerticalLayout[0]/VOrderedLayout$Slot[5]/VButton[0]/domChild[0]/domChild[0]</td>
+    <td></td>
+</tr>
+<tr>
+    <td>assertText</td>
+    <td>vaadin=runcomvaadintestscomponentsorderedlayoutOrderedLayoutComponentOrdering::/VVerticalLayout[0]/VOrderedLayout$Slot[1]/VVerticalLayout[0]/VOrderedLayout$Slot[0]/VHorizontalLayout[0]/VOrderedLayout$Slot[1]/VButton[0]/domChild[0]/domChild[0]</td>
+    <td>7</td>
+</tr>
+</tbody></table>
+</body>
+</html>
diff --git a/uitest/src/com/vaadin/tests/components/orderedlayout/OrderedLayoutComponentOrdering.java b/uitest/src/com/vaadin/tests/components/orderedlayout/OrderedLayoutComponentOrdering.java
new file mode 100644
index 0000000000..5857df20b1
--- /dev/null
+++ b/uitest/src/com/vaadin/tests/components/orderedlayout/OrderedLayoutComponentOrdering.java
@@ -0,0 +1,83 @@
+package com.vaadin.tests.components.orderedlayout;
+
+import com.vaadin.tests.components.TestBase;
+import com.vaadin.ui.Button;
+import com.vaadin.ui.Button.ClickEvent;
+import com.vaadin.ui.HorizontalLayout;
+
+public class OrderedLayoutComponentOrdering extends TestBase {
+
+    int counter = 0;
+
+    @Override
+    protected void setup() {
+
+        // Initially horizontal layout has a,b
+        Button a = new Button(String.valueOf(++counter));
+        Button b = new Button(String.valueOf(++counter));
+        final HorizontalLayout hl = new HorizontalLayout(a, b);
+        hl.setCaption("Horizontal layout");
+        hl.setSpacing(true);
+        addComponent(hl);
+
+        Button addFirst = new Button("add first");
+        addFirst.addClickListener(new Button.ClickListener() {
+            @Override
+            public void buttonClick(ClickEvent event) {
+                hl.addComponent(new Button(String.valueOf(++counter)), 0);
+                hl.addComponent(new Button(String.valueOf(++counter)), 1);
+            }
+        });
+        addComponent(addFirst);
+
+        Button add = new Button("add second");
+        add.addClickListener(new Button.ClickListener() {
+            @Override
+            public void buttonClick(ClickEvent event) {
+                hl.addComponent(new Button(String.valueOf(++counter)), 1);
+                hl.addComponent(new Button(String.valueOf(++counter)), 2);
+            }
+        });
+        addComponent(add);
+
+        Button addThird = new Button("add third");
+        addThird.addClickListener(new Button.ClickListener() {
+            @Override
+            public void buttonClick(ClickEvent event) {
+                hl.addComponent(new Button(String.valueOf(++counter)), 2);
+                hl.addComponent(new Button(String.valueOf(++counter)), 3);
+            }
+        });
+        addComponent(addThird);
+
+        Button move = new Button("move last to first");
+        move.addClickListener(new Button.ClickListener() {
+            @Override
+            public void buttonClick(ClickEvent event) {
+                hl.addComponentAsFirst(hl.getComponent(hl.getComponentCount() - 1));
+            }
+        });
+        addComponent(move);
+
+        Button swap = new Button("move forth to second");
+        swap.addClickListener(new Button.ClickListener() {
+            @Override
+            public void buttonClick(ClickEvent event) {
+                hl.addComponent(hl.getComponent(3), 1);
+            }
+        });
+        addComponent(swap);
+
+    }
+
+    @Override
+    protected String getDescription() {
+        return "The order should be 1,3,4,2";
+    }
+
+    @Override
+    protected Integer getTicketNumber() {
+        return 10154;
+    }
+
+}