Inserting TreeItems into the logical list in the same spot that they are inserted into the physical DOM. This fixes a bug where keyboard navigation occurs out of order, and generally makes the logical and physical structures consistent. Review at http://gwt-code-reviews.appspot.com/645802 Review by: rice@google.com git-svn-id: https://google-web-toolkit.googlecode.com/svn/trunk@8348 8db76d5a-ed1c-0410-87a9-c151d255dfc7

diff --git a/user/src/com/google/gwt/user/client/ui/Tree.java b/user/src/com/google/gwt/user/client/ui/Tree.java
index cd335f7..d5cc644 100644
--- a/user/src/com/google/gwt/user/client/ui/Tree.java
+++ b/user/src/com/google/gwt/user/client/ui/Tree.java

@@ -999,60 +999,7 @@
 
     // The 'root' item is invisible and serves only as a container
     // for all top-level items.
-    root = new TreeItem() {
-      @Override
-      public void insertItem(int beforeIndex, TreeItem item)
-          throws IndexOutOfBoundsException {
-        // Check the index.
-        int childCount = getChildCount();
-        if (beforeIndex < 0 || beforeIndex > childCount) {
-          throw new IndexOutOfBoundsException();
-        }
-
-        // If this element already belongs to a tree or tree item, remove it.
-        if ((item.getParentItem() != null) || (item.getTree() != null)) {
-          item.remove();
-        }
-
-        // Physical attach.
-        Element treeElem = Tree.this.getElement();
-        if (beforeIndex == childCount) {
-          treeElem.appendChild(item.getElement());
-        } else {
-          Element beforeElem = getChild(beforeIndex).getElement();
-          treeElem.insertBefore(item.getElement(), beforeElem);
-        }
-
-        // Logical attach.
-        item.setTree(this.getTree());
-
-        // Explicitly set top-level items' parents to null.
-        item.setParentItem(null);
-        getChildren().add(item);
-
-        // Use no margin on top-most items.
-        if (LocaleInfo.getCurrentLocale().isRTL()) {
-          DOM.setIntStyleAttribute(item.getElement(), "marginRight", 0);
-        } else {
-          DOM.setIntStyleAttribute(item.getElement(), "marginLeft", 0);
-        }
-      }
-
-      @Override
-      public void removeItem(TreeItem item) {
-        if (!getChildren().contains(item)) {
-          return;
-        }
-
-        // Update Item state.
-        item.setTree(null);
-        item.setParentItem(null);
-        getChildren().remove(item);
-
-        DOM.removeChild(Tree.this.getElement(), item.getElement());
-      }
-    };
-    root.initChildren();
+    root = new TreeItem(true);
     root.setTree(this);
     setStyleName("gwt-Tree");
 

diff --git a/user/src/com/google/gwt/user/client/ui/TreeItem.java b/user/src/com/google/gwt/user/client/ui/TreeItem.java
index 4348fff..c2eacfb 100644
--- a/user/src/com/google/gwt/user/client/ui/TreeItem.java
+++ b/user/src/com/google/gwt/user/client/ui/TreeItem.java

@@ -38,6 +38,12 @@
  * </p>
  */
 public class TreeItem extends UIObject implements HasHTML {
+
+  /**
+   * The margin applied to child items.
+   */
+  private static final double CHILD_MARGIN = 16.0;
+
   /**
    * Implementation class for {@link TreeItem}.
    */
@@ -247,6 +253,12 @@
 
   private ArrayList<TreeItem> children;
   private Element contentElem, childSpanElem, imageHolder;
+
+  /**
+   * Indicates that this item is a root item in a tree.
+   */
+  private boolean isRoot;
+
   private boolean open;
   private TreeItem parent;
   private boolean selected;
@@ -261,10 +273,7 @@
    * Creates an empty tree item.
    */
   public TreeItem() {
-    Element elem = DOM.clone(BASE_BARE_ELEM, true);
-    setElement(elem);
-    contentElem = DOM.getFirstChild(elem);
-    DOM.setElementAttribute(contentElem, "id", DOM.createUniqueId());
+    this(false);
   }
 
   /**
@@ -288,6 +297,24 @@
   }
 
   /**
+   * Creates an empty tree item.
+   * 
+   * @param isRoot true if this item is the root of a tree
+   */
+  TreeItem(boolean isRoot) {
+    this.isRoot = isRoot;
+    Element elem = DOM.clone(BASE_BARE_ELEM, true);
+    setElement(elem);
+    contentElem = DOM.getFirstChild(elem);
+    DOM.setElementAttribute(contentElem, "id", DOM.createUniqueId());
+
+    // The root item always has children.
+    if (isRoot) {
+      initChildren();
+    }
+  }
+
+  /**
    * Adds a child tree item containing the specified text.
    * 
    * @param itemText the text to be added
@@ -305,6 +332,9 @@
    * @param item the item to be added
    */
   public void addItem(TreeItem item) {
+    // If this is the item's parent, removing the item will affect the child
+    // count.
+    maybeRemoveItemFromParent(item);
     insertItem(getChildCount(), item);
   }
 
@@ -441,9 +471,7 @@
   public void insertItem(int beforeIndex, TreeItem item)
       throws IndexOutOfBoundsException {
     // Detach item from existing parent.
-    if ((item.getParentItem() != null) || (item.getTree() != null)) {
-      item.remove();
-    }
+    maybeRemoveItemFromParent(item);
 
     // Check the index after detaching in case this item was already the parent.
     int childCount = getChildCount();
@@ -456,28 +484,32 @@
     }
 
     // Set the margin.
+    // Use no margin on top-most items.
+    double margin = isRoot ? 0.0 : CHILD_MARGIN;
     if (LocaleInfo.getCurrentLocale().isRTL()) {
-      item.getElement().getStyle().setMarginRight(16.0, Unit.PX);
+      item.getElement().getStyle().setMarginRight(margin, Unit.PX);
     } else {
-      item.getElement().getStyle().setMarginLeft(16.0, Unit.PX);
+      item.getElement().getStyle().setMarginLeft(margin, Unit.PX);
     }
 
     // Physical attach.
+    Element childContainer = isRoot ? tree.getElement() : childSpanElem;
     if (beforeIndex == childCount) {
-      childSpanElem.appendChild(item.getElement());
+      childContainer.appendChild(item.getElement());
     } else {
       Element beforeElem = getChild(beforeIndex).getElement();
-      childSpanElem.insertBefore(item.getElement(), beforeElem);
+      childContainer.insertBefore(item.getElement(), beforeElem);
     }
 
     // Logical attach.
-    item.setParentItem(this);
-    children.add(item);
+    // Explicitly set top-level items' parents to null if this is root.
+    item.setParentItem(isRoot ? null : this);
+    children.add(beforeIndex, item);
 
     // Adopt.
     item.setTree(tree);
 
-    if (children.size() == 1) {
+    if (!isRoot && children.size() == 1) {
       updateState(false, false);
     }
   }
@@ -534,16 +566,21 @@
     }
 
     // Orphan.
+    Tree oldTree = tree;
     item.setTree(null);
 
     // Physical detach.
-    DOM.removeChild(childSpanElem, item.getElement());
+    if (isRoot) {
+      oldTree.getElement().removeChild(item.getElement());
+    } else {
+      childSpanElem.removeChild(item.getElement());
+    }
 
     // Logical detach.
     item.setParentItem(null);
     children.remove(item);
 
-    if (children.size() == 0) {
+    if (!isRoot && children.size() == 0) {
       updateState(false, false);
     }
   }
@@ -775,6 +812,17 @@
     return imageHolder != null;
   }
 
+  /**
+   * Remove a tree item from its parent if it has one.
+   * 
+   * @param item the tree item to remove from its parent
+   */
+  void maybeRemoveItemFromParent(TreeItem item) {
+    if ((item.getParentItem() != null) || (item.getTree() != null)) {
+      item.remove();
+    }
+  }
+
   void setParentItem(TreeItem parent) {
     this.parent = parent;
   }

diff --git a/user/test/com/google/gwt/user/client/ui/TreeItemTest.java b/user/test/com/google/gwt/user/client/ui/TreeItemTest.java
index 9c3baff..dcd0d0e 100644
--- a/user/test/com/google/gwt/user/client/ui/TreeItemTest.java
+++ b/user/test/com/google/gwt/user/client/ui/TreeItemTest.java

@@ -27,21 +27,52 @@
     return "com.google.gwt.user.User";
   }
 
+  public void testAddIntoSameItem() {
+    TreeItem item = new TreeItem();
+
+    // Add the only child back to its parent.
+    TreeItem a = item.addItem("a");
+    item.addItem(a);
+    assertEquals(1, item.getChildCount());
+    assertEquals(a, item.getChild(0));
+
+    // Add a child back to its parent that has multiple children.
+    TreeItem b = item.addItem("b");
+    item.addItem(a);
+    assertEquals(2, item.getChildCount());
+    assertEquals(b, item.getChild(0));
+    assertEquals(a, item.getChild(1));
+  }
+
   public void testInsert() {
     TreeItem item = new TreeItem();
     TreeItem b = item.addItem("b");
+    assertEquals(1, item.getChildCount());
+    assertEquals(b, item.getChild(0));
 
     // Insert at zero.
     TreeItem a = item.insertItem(0, "a");
+    assertEquals(2, item.getChildCount());
+    assertEquals(a, item.getChild(0));
+    assertEquals(b, item.getChild(1));
     assertEquals(a.getElement().getNextSiblingElement(), b.getElement());
 
     // Insert at end.
     TreeItem d = item.insertItem(2, new Label("b"));
+    assertEquals(3, item.getChildCount());
+    assertEquals(a, item.getChild(0));
+    assertEquals(b, item.getChild(1));
+    assertEquals(d, item.getChild(2));
     assertEquals(b.getElement().getNextSiblingElement(), d.getElement());
 
     // Insert in the middle.
     TreeItem c = new TreeItem("c");
     item.insertItem(2, c);
+    assertEquals(4, item.getChildCount());
+    assertEquals(a, item.getChild(0));
+    assertEquals(b, item.getChild(1));
+    assertEquals(c, item.getChild(2));
+    assertEquals(d, item.getChild(3));
     assertEquals(b.getElement().getNextSiblingElement(), c.getElement());
   }
 

diff --git a/user/test/com/google/gwt/user/client/ui/TreeTest.java b/user/test/com/google/gwt/user/client/ui/TreeTest.java
index f8b27ef..0e04aa0 100644
--- a/user/test/com/google/gwt/user/client/ui/TreeTest.java
+++ b/user/test/com/google/gwt/user/client/ui/TreeTest.java

@@ -180,18 +180,32 @@
   public void testRootInsert() {
     Tree t = new Tree();
     TreeItem b = t.addItem("b");
+    assertEquals(1, t.getItemCount());
+    assertEquals(b, t.getItem(0));
 
     // Insert at zero.
     TreeItem a = t.insertItem(0, "a");
+    assertEquals(2, t.getItemCount());
+    assertEquals(a, t.getItem(0));
+    assertEquals(b, t.getItem(1));
     assertEquals(a.getElement().getNextSiblingElement(), b.getElement());
 
     // Insert at end.
     TreeItem d = t.insertItem(2, new Label("d"));
+    assertEquals(3, t.getItemCount());
+    assertEquals(a, t.getItem(0));
+    assertEquals(b, t.getItem(1));
+    assertEquals(d, t.getItem(2));
     assertEquals(b.getElement().getNextSiblingElement(), d.getElement());
 
     // Insert in the middle.
     TreeItem c = new TreeItem("c");
     t.insertItem(2, c);
+    assertEquals(4, t.getItemCount());
+    assertEquals(a, t.getItem(0));
+    assertEquals(b, t.getItem(1));
+    assertEquals(c, t.getItem(2));
+    assertEquals(d, t.getItem(3));
     assertEquals(b.getElement().getNextSiblingElement(), c.getElement());
   }