Clean up how we remove nodes from the render tree, and make sure we reinsert...

Clean up how we remove nodes from the render tree, and make sure we reinsert them in the right place.
Fixes weird behaviours when the old and new children of TagNodes can't be synced.

R=abarth@chromium.org

Review URL: https://codereview.chromium.org/1182463006.

sdk/lib/widgets/scaffold.dart

@@ -185,10 +185,12 @@ class Scaffold extends RenderObjectWrapper {
     root[slot] = child != null ? child.root : null;
   }
 
-  void removeChild(Widget node) {
-    assert(node != null);
-    root.remove(node.root);
-    super.removeChild(node);
+  void detachChildRoot(RenderObjectWrapper child) {
+    final root = this.root; // TODO(ianh): Remove this once the analyzer is cleverer
+    assert(root is RenderScaffold);
+    assert(root == child.root.parent);
+    root.remove(child.root);
+    assert(root == this.root); // TODO(ianh): Remove this once the analyzer is cleverer
   }
 
   void remove() {

sdk/lib/widgets/widget.dart

@@ -90,11 +90,6 @@ abstract class Widget {
   // 'slot' is the identifier that the parent RenderObjectWrapper uses to know
   // where to put this descendant
 
-  void remove() {
-    _root = null;
-    setParent(null);
-  }
-
   Widget findAncestor(Type targetType) {
     var ancestor = _parent;
     while (ancestor != null && !reflectClass(ancestor.runtimeType).isSubtypeOf(reflectClass(targetType)))
@@ -102,10 +97,17 @@ abstract class Widget {
     return ancestor;
   }
 
+  void remove() {
+    _root = null;
+    setParent(null);
+  }
+
   void removeChild(Widget node) {
     node.remove();
   }
 
+  void detachRoot();
+
   // Returns the child which should be retained as the child of this node.
   Widget syncChild(Widget node, Widget oldNode, dynamic slot) {
 
@@ -113,12 +115,14 @@ abstract class Widget {
 
     if (node == oldNode) {
       assert(node == null || node.mounted);
+      assert(node is! RenderObjectWrapper || node._ancestor != null);
       return node; // Nothing to do. Subtrees must be identical.
     }
 
     if (node == null) {
       // the child in this slot has gone away
       assert(oldNode.mounted);
+      oldNode.detachRoot();
       removeChild(oldNode);
       assert(!oldNode.mounted);
       return null;
@@ -138,6 +142,7 @@ abstract class Widget {
     if (oldNode != null &&
         (oldNode.runtimeType != node.runtimeType || oldNode.key != node.key)) {
       assert(oldNode.mounted);
+      oldNode.detachRoot();
       removeChild(oldNode);
       oldNode = null;
     }
@@ -148,6 +153,9 @@ abstract class Widget {
     assert(node.root is RenderObject);
     return node;
   }
+
+  String toString() => '$runtimeType($key)';
+
 }
 
 
@@ -175,6 +183,11 @@ abstract class TagNode extends Widget {
     super.remove();
   }
 
+  void detachRoot() {
+    if (content != null)
+      content.detachRoot();
+  }
+
 }
 
 class ParentDataNode extends TagNode {
@@ -305,6 +318,12 @@ abstract class Component extends Widget {
     super.remove();
   }
 
+  void detachRoot() {
+    assert(_built != null);
+    assert(root != null);
+    _built.detachRoot();
+  }
+
   bool _retainStatefulNodeIfPossible(Widget old) {
     assert(!_disqualifiedFromEverAppearingAgain);
 
@@ -466,13 +485,14 @@ abstract class RenderObjectWrapper extends Widget {
 
   RenderObject createNode();
 
-  void insert(RenderObjectWrapper child, dynamic slot);
-
   static final Map<RenderObject, RenderObjectWrapper> _nodeMap =
       new HashMap<RenderObject, RenderObjectWrapper>();
-
   static RenderObjectWrapper _getMounted(RenderObject node) => _nodeMap[node];
 
+  RenderObjectWrapper _ancestor;
+  void insert(RenderObjectWrapper child, dynamic slot);
+  void detachChildRoot(RenderObjectWrapper child);
+
   void _sync(Widget old, dynamic slot) {
     // TODO(abarth): We should split RenderObjectWrapper into two pieces so that
     //               RenderViewObject doesn't need to inherit all this code it
@@ -480,11 +500,14 @@ abstract class RenderObjectWrapper extends Widget {
     assert(parent != null || this is RenderViewWrapper);
     if (old == null) {
       _root = createNode();
-      var ancestor = findAncestor(RenderObjectWrapper);
-      if (ancestor is RenderObjectWrapper)
-        ancestor.insert(this, slot);
+      _ancestor = findAncestor(RenderObjectWrapper);
+      if (_ancestor is RenderObjectWrapper)
+        _ancestor.insert(this, slot);
+      else
+        print("$this has no ancestor");
     } else {
       _root = old.root;
+      _ancestor = old._ancestor;
     }
     assert(_root == root); // in case a subclass reintroduces it
     assert(root != null);
@@ -518,6 +541,13 @@ abstract class RenderObjectWrapper extends Widget {
     _nodeMap.remove(root);
     super.remove();
   }
+
+  void detachRoot() {
+    assert(_ancestor != null);
+    assert(root != null);
+    _ancestor.detachChildRoot(this);
+  }
+
 }
 
 abstract class OneChildRenderObjectWrapper extends RenderObjectWrapper {
@@ -536,17 +566,17 @@ abstract class OneChildRenderObjectWrapper extends RenderObjectWrapper {
 
   void insert(RenderObjectWrapper child, dynamic slot) {
     final root = this.root; // TODO(ianh): Remove this once the analyzer is cleverer
-    assert(slot == null);
     assert(root is RenderObjectWithChildMixin);
+    assert(slot == null);
     root.child = child.root;
     assert(root == this.root); // TODO(ianh): Remove this once the analyzer is cleverer
   }
 
-  void removeChild(Widget node) {
+  void detachChildRoot(RenderObjectWrapper child) {
     final root = this.root; // TODO(ianh): Remove this once the analyzer is cleverer
     assert(root is RenderObjectWithChildMixin);
+    assert(root.child == child.root);
     root.child = null;
-    super.removeChild(node);
     assert(root == this.root); // TODO(ianh): Remove this once the analyzer is cleverer
   }
 
@@ -579,12 +609,11 @@ abstract class MultiChildRenderObjectWrapper extends RenderObjectWrapper {
     assert(root == this.root); // TODO(ianh): Remove this once the analyzer is cleverer
   }
 
-  void removeChild(Widget node) {
+  void detachChildRoot(RenderObjectWrapper child) {
     final root = this.root; // TODO(ianh): Remove this once the analyzer is cleverer
     assert(root is ContainerRenderObjectMixin);
-    assert(node.root.parent == root);
-    root.remove(node.root);
-    super.removeChild(node);
+    assert(child.root.parent == root);
+    root.remove(child.root);
     assert(root == this.root); // TODO(ianh): Remove this once the analyzer is cleverer
   }
 
@@ -647,6 +676,7 @@ abstract class MultiChildRenderObjectWrapper extends RenderObjectWrapper {
       endIndex--;
       oldEndIndex--;
       sync(endIndex);
+      nextSibling = children[endIndex].root;
     }
 
     HashMap<String, Widget> oldNodeIdMap = null;
@@ -729,7 +759,7 @@ abstract class MultiChildRenderObjectWrapper extends RenderObjectWrapper {
     currentNode = null;
     while (oldStartIndex < oldEndIndex) {
       oldNode = oldChildren[oldStartIndex];
-      removeChild(oldNode);
+      syncChild(null, oldNode, null);
       advanceOldStartIndex();
     }
 

tests/widgets/syncs1-expected.txt

+CONSOLE: TestRenderView enabled
+CONSOLE: 
+PAINT FOR FRAME #1 ----------------------------------------------
+1 | TestDisplayList() constructor: 800.0 x 600.0
+------------------------------------------------------------------------
+CONSOLE: 
+PAINT FOR FRAME #2 ----------------------------------------------
+2 | TestDisplayList() constructor: 800.0 x 600.0
+2 | paintChild RenderBlock at Point(0.0, 0.0)
+2 |  | TestDisplayList() constructor: 800.0 x 600.0
+2 |  | paintChild RenderConstrainedBox at Point(0.0, 0.0)
+2 |  |  | TestDisplayList() constructor: 800.0 x 600.0
+2 |  |  | paintChild RenderDecoratedBox at Point(0.0, 0.0)
+2 |  |  |  | TestDisplayList() constructor: 800.0 x 600.0
+2 |  |  |  | drawRect(Rect.fromLTRB(0.0, 0.0, 425.0, 250.0), Paint(Color(0xff00ffff)))
+2 |  |  | paintChild RenderPadding at Point(425.0, 99.0)
+2 |  |  |  | TestDisplayList() constructor: 800.0 x 600.0
+2 |  |  |  | paintChild RenderConstrainedBox at Point(8.0, 8.0)
+2 |  |  |  |  | TestDisplayList() constructor: 800.0 x 600.0
+2 |  |  |  |  | drawRRect(Instance of 'RRect', Paint(Color(0xffe0e0e0)))
+2 |  |  |  |  | paintChild RenderPositionedBox at Point(8.0, 0.0)
+2 |  |  |  |  |  | TestDisplayList() constructor: 800.0 x 600.0
+2 |  |  |  |  |  | paintChild RenderParagraph at Point(0.0, 10.0)
+2 |  |  |  |  |  |  | TestDisplayList() constructor: 800.0 x 600.0
+2 |  | paintChild RenderConstrainedBox at Point(0.0, 250.0)
+2 |  |  | TestDisplayList() constructor: 800.0 x 600.0
+2 |  |  | paintChild RenderDecoratedBox at Point(0.0, 0.0)
+2 |  |  |  | TestDisplayList() constructor: 800.0 x 600.0
+2 |  |  |  | drawRect(Rect.fromLTRB(0.0, 0.0, 800.0, 250.0), Paint(Color(0xff00ffff)))
+2 |  |  | paintChild RenderPadding at Point(0.0, 0.0)
+2 |  |  |  | TestDisplayList() constructor: 800.0 x 600.0
+2 |  |  |  | paintChild RenderConstrainedBox at Point(8.0, 8.0)
+2 |  |  |  |  | TestDisplayList() constructor: 800.0 x 600.0
+2 |  |  |  |  | drawRRect(Instance of 'RRect', Paint(Color(0xffe0e0e0)))
+2 |  |  |  |  | paintChild RenderPositionedBox at Point(8.0, 0.0)
+2 |  |  |  |  |  | TestDisplayList() constructor: 800.0 x 600.0
+2 |  |  |  |  |  | paintChild RenderParagraph at Point(212.5, 109.0)
+2 |  |  |  |  |  |  | TestDisplayList() constructor: 800.0 x 600.0
+------------------------------------------------------------------------
+CONSOLE: 
+PAINT FOR FRAME #3 ----------------------------------------------
+3 | TestDisplayList() constructor: 800.0 x 600.0
+3 | paintChild RenderBlock at Point(0.0, 0.0)
+3 |  | TestDisplayList() constructor: 800.0 x 600.0
+3 |  | paintChild RenderConstrainedBox at Point(0.0, 0.0)
+3 |  |  | TestDisplayList() constructor: 800.0 x 600.0
+3 |  |  | paintChild RenderDecoratedBox at Point(0.0, 0.0)
+3 |  |  |  | TestDisplayList() constructor: 800.0 x 600.0
+3 |  |  |  | drawRect(Rect.fromLTRB(0.0, 0.0, 549.0, 250.0), Paint(Color(0xefff9f00)))
+3 |  |  | paintChild RenderPadding at Point(549.0, 99.0)
+3 |  |  |  | TestDisplayList() constructor: 800.0 x 600.0
+3 |  |  |  | paintChild RenderConstrainedBox at Point(8.0, 8.0)
+3 |  |  |  |  | TestDisplayList() constructor: 800.0 x 600.0
+3 |  |  |  |  | drawRRect(Instance of 'RRect', Paint(Color(0xffe0e0e0)))
+3 |  |  |  |  | paintChild RenderPositionedBox at Point(8.0, 0.0)
+3 |  |  |  |  |  | TestDisplayList() constructor: 800.0 x 600.0
+3 |  |  |  |  |  | paintChild RenderParagraph at Point(0.0, 10.0)
+3 |  |  |  |  |  |  | TestDisplayList() constructor: 800.0 x 600.0
+3 |  | paintChild RenderConstrainedBox at Point(0.0, 250.0)
+3 |  |  | TestDisplayList() constructor: 800.0 x 600.0
+3 |  |  | paintChild RenderDecoratedBox at Point(0.0, 0.0)
+3 |  |  |  | TestDisplayList() constructor: 800.0 x 600.0
+3 |  |  |  | drawRect(Rect.fromLTRB(0.0, 0.0, 800.0, 250.0), Paint(Color(0xefff9f00)))
+3 |  |  | paintChild RenderPadding at Point(0.0, 0.0)
+3 |  |  |  | TestDisplayList() constructor: 800.0 x 600.0
+3 |  |  |  | paintChild RenderConstrainedBox at Point(8.0, 8.0)
+3 |  |  |  |  | TestDisplayList() constructor: 800.0 x 600.0
+3 |  |  |  |  | drawRRect(Instance of 'RRect', Paint(Color(0xffe0e0e0)))
+3 |  |  |  |  | paintChild RenderPositionedBox at Point(8.0, 0.0)
+3 |  |  |  |  |  | TestDisplayList() constructor: 800.0 x 600.0
+3 |  |  |  |  |  | paintChild RenderParagraph at Point(274.5, 109.0)
+3 |  |  |  |  |  |  | TestDisplayList() constructor: 800.0 x 600.0
+------------------------------------------------------------------------
+CONSOLE: 
+PAINT FOR FRAME #4 ----------------------------------------------
+4 | TestDisplayList() constructor: 800.0 x 600.0
+4 | paintChild RenderBlock at Point(0.0, 0.0)
+4 |  | TestDisplayList() constructor: 800.0 x 600.0
+4 |  | paintChild RenderConstrainedBox at Point(0.0, 0.0)
+4 |  |  | TestDisplayList() constructor: 800.0 x 600.0
+4 |  |  | paintChild RenderDecoratedBox at Point(0.0, 0.0)
+4 |  |  |  | TestDisplayList() constructor: 800.0 x 600.0
+4 |  |  |  | drawRect(Rect.fromLTRB(0.0, 0.0, 425.0, 250.0), Paint(Color(0xff00ffff)))
+4 |  |  | paintChild RenderPadding at Point(425.0, 99.0)
+4 |  |  |  | TestDisplayList() constructor: 800.0 x 600.0
+4 |  |  |  | paintChild RenderConstrainedBox at Point(8.0, 8.0)
+4 |  |  |  |  | TestDisplayList() constructor: 800.0 x 600.0
+4 |  |  |  |  | drawRRect(Instance of 'RRect', Paint(Color(0xffe0e0e0)))
+4 |  |  |  |  | paintChild RenderPositionedBox at Point(8.0, 0.0)
+4 |  |  |  |  |  | TestDisplayList() constructor: 800.0 x 600.0
+4 |  |  |  |  |  | paintChild RenderParagraph at Point(0.0, 10.0)
+4 |  |  |  |  |  |  | TestDisplayList() constructor: 800.0 x 600.0
+4 |  | paintChild RenderConstrainedBox at Point(0.0, 250.0)
+4 |  |  | TestDisplayList() constructor: 800.0 x 600.0
+4 |  |  | paintChild RenderDecoratedBox at Point(0.0, 0.0)
+4 |  |  |  | TestDisplayList() constructor: 800.0 x 600.0
+4 |  |  |  | drawRect(Rect.fromLTRB(0.0, 0.0, 800.0, 250.0), Paint(Color(0xff00ffff)))
+4 |  |  | paintChild RenderPadding at Point(0.0, 0.0)
+4 |  |  |  | TestDisplayList() constructor: 800.0 x 600.0
+4 |  |  |  | paintChild RenderConstrainedBox at Point(8.0, 8.0)
+4 |  |  |  |  | TestDisplayList() constructor: 800.0 x 600.0
+4 |  |  |  |  | drawRRect(Instance of 'RRect', Paint(Color(0xffe0e0e0)))
+4 |  |  |  |  | paintChild RenderPositionedBox at Point(8.0, 0.0)
+4 |  |  |  |  |  | TestDisplayList() constructor: 800.0 x 600.0
+4 |  |  |  |  |  | paintChild RenderParagraph at Point(212.5, 109.0)
+4 |  |  |  |  |  |  | TestDisplayList() constructor: 800.0 x 600.0
+------------------------------------------------------------------------
+PAINTED 4 FRAMES

tests/widgets/syncs1.dart

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+import 'dart:async';
+
+import 'package:sky/widgets/basic.dart';
+import 'package:sky/widgets/raised_button.dart';
+
+import '../resources/display_list.dart';
+
+class FiddleApp extends App {
+
+  bool arbitrarySetting = true;
+
+  void toggle() {
+    setState(() {
+      arbitrarySetting = !arbitrarySetting;
+    });
+  }
+
+  Widget buildFlex1() {
+    return new Flex([
+      new Flexible(child: new Container(
+        decoration: new BoxDecoration(
+          backgroundColor: const Color(0xFF00FFFF)
+        )
+      )),
+      new RaisedButton(child: new Text('TAP ME TO CHANGE THE BACKGROUND COLOUR'), onPressed: toggle)
+    ]);
+  }
+
+  Widget buildFlex2() {
+    return new Flex([
+      new Flexible(child: new Container(
+        key: 'something-else',
+        decoration: new BoxDecoration(
+          backgroundColor: const Color(0xEFFF9F00)
+        )
+      )),
+      new RaisedButton(child: new Text('PRESS ME TO CHANGE IT BACK'), onPressed: toggle)
+    ]);
+  }
+
+  Widget buildStack1() {
+    return new Stack([
+      new Positioned(child: new Container(
+        decoration: new BoxDecoration(
+          backgroundColor: const Color(0xFF00FFFF)
+        )
+      )),
+      new RaisedButton(child: new Text('TAP ME TO CHANGE THE BACKGROUND COLOUR'), onPressed: toggle)
+    ]);
+  }
+
+  Widget buildStack2() {
+    return new Stack([
+      new Positioned(child: new Container(
+        key: 'something-else',
+        decoration: new BoxDecoration(
+          backgroundColor: const Color(0xEFFF9F00)
+        )
+      )),
+      new RaisedButton(child: new Text('PRESS ME TO CHANGE IT BACK'), onPressed: toggle)
+    ]);
+  }
+
+  Widget build() {
+    return new Block([
+      new SizedBox(
+        key: 'flex-example',
+        height: 250.0,
+        child: arbitrarySetting ? buildFlex1() : buildFlex2()
+      ),
+      new SizedBox(
+        key: 'stack-example',
+        height: 250.0,
+        child: arbitrarySetting ? buildStack1() : buildStack2()
+      )
+    ]);
+  }
+}
+
+main() {
+  TestRenderView renderViewOverride = new TestRenderView();
+  FiddleApp app = new FiddleApp();
+  runApp(app, renderViewOverride: renderViewOverride);
+  new Future.microtask(() {
+    renderViewOverride.checkFrame();
+    app.toggle();
+    new Future.microtask(() {
+      renderViewOverride.checkFrame();
+      app.toggle();
+      new Future.microtask(() {
+        renderViewOverride.checkFrame();
+        renderViewOverride.endTest();
+      });
+    });
+  });
+}