Positioned 'remembers' things it shouldn't

This patch makes ParentDataNode less general purpose and instead teaches Flex
and Stack how to program the parent data for their children. We used to have
this general system because parent data used to carry CSS styling, but we don't
need it anymore.

Fixes #957

packages/flutter/lib/rendering/flex.dart

@@ -9,10 +9,10 @@ import 'package:sky/rendering/object.dart';
 
 export 'package:sky/rendering/object.dart' show EventDisposition;
 
-class FlexBoxParentData extends BoxParentData with ContainerParentDataMixin<RenderBox> {
+class FlexParentData extends BoxParentData with ContainerParentDataMixin<RenderBox> {
   int flex;
 
-  void merge(FlexBoxParentData other) {
+  void merge(FlexParentData other) {
     if (other.flex != null)
       flex = other.flex;
     super.merge(other);
@@ -41,8 +41,8 @@ enum FlexAlignItems {
 
 typedef double _ChildSizingFunction(RenderBox child, BoxConstraints constraints);
 
-class RenderFlex extends RenderBox with ContainerRenderObjectMixin<RenderBox, FlexBoxParentData>,
-                                        RenderBoxContainerDefaultsMixin<RenderBox, FlexBoxParentData> {
+class RenderFlex extends RenderBox with ContainerRenderObjectMixin<RenderBox, FlexParentData>,
+                                        RenderBoxContainerDefaultsMixin<RenderBox, FlexParentData> {
   // lays out RenderBox children using flexible layout
 
   RenderFlex({
@@ -98,8 +98,8 @@ class RenderFlex extends RenderBox with ContainerRenderObjectMixin<RenderBox, Fl
   double _overflow;
 
   void setupParentData(RenderBox child) {
-    if (child.parentData is! FlexBoxParentData)
-      child.parentData = new FlexBoxParentData();
+    if (child.parentData is! FlexParentData)
+      child.parentData = new FlexParentData();
   }
 
   double _getIntrinsicSize({ BoxConstraints constraints,
@@ -133,7 +133,7 @@ class RenderFlex extends RenderBox with ContainerRenderObjectMixin<RenderBox, Fl
         } else {
           inflexibleSpace += childSize(child, childConstraints);
         }
-        assert(child.parentData is FlexBoxParentData);
+        assert(child.parentData is FlexParentData);
         child = child.parentData.nextSibling;
       }
       double mainSize = maxFlexFractionSoFar * totalFlex + inflexibleSpace;
@@ -196,7 +196,7 @@ class RenderFlex extends RenderBox with ContainerRenderObjectMixin<RenderBox, Fl
           inflexibleSpace += mainSize;
           maxCrossSize = math.max(maxCrossSize, crossSize);
         }
-        assert(child.parentData is FlexBoxParentData);
+        assert(child.parentData is FlexParentData);
         child = child.parentData.nextSibling;
       }
 
@@ -224,7 +224,7 @@ class RenderFlex extends RenderBox with ContainerRenderObjectMixin<RenderBox, Fl
           }
           maxCrossSize = math.max(maxCrossSize, crossSize);
         }
-        assert(child.parentData is FlexBoxParentData);
+        assert(child.parentData is FlexParentData);
         child = child.parentData.nextSibling;
       }
 
@@ -276,7 +276,7 @@ class RenderFlex extends RenderBox with ContainerRenderObjectMixin<RenderBox, Fl
   }
 
   int _getFlex(RenderBox child) {
-    assert(child.parentData is FlexBoxParentData);
+    assert(child.parentData is FlexParentData);
     return child.parentData.flex != null ? child.parentData.flex : 0;
   }
 
@@ -301,7 +301,7 @@ class RenderFlex extends RenderBox with ContainerRenderObjectMixin<RenderBox, Fl
     double freeSpace = canFlex ? mainSize : 0.0;
     RenderBox child = firstChild;
     while (child != null) {
-      assert(child.parentData is FlexBoxParentData);
+      assert(child.parentData is FlexParentData);
       totalChildren++;
       int flex = _getFlex(child);
       if (flex > 0) {
@@ -392,7 +392,7 @@ class RenderFlex extends RenderBox with ContainerRenderObjectMixin<RenderBox, Fl
           if (distance != null)
             maxBaselineDistance = math.max(maxBaselineDistance, distance);
         }
-        assert(child.parentData is FlexBoxParentData);
+        assert(child.parentData is FlexParentData);
         child = child.parentData.nextSibling;
       }
     }
@@ -461,7 +461,7 @@ class RenderFlex extends RenderBox with ContainerRenderObjectMixin<RenderBox, Fl
     double childMainPosition = leadingSpace;
     child = firstChild;
     while (child != null) {
-      assert(child.parentData is FlexBoxParentData);
+      assert(child.parentData is FlexParentData);
       double childCrossPosition;
       switch (_alignItems) {
         case FlexAlignItems.stretch:

packages/flutter/lib/widgets/basic.dart

@@ -485,22 +485,64 @@ class Stack extends MultiChildRenderObjectWrapper {
 
   RenderStack createNode() => new RenderStack();
   RenderStack get renderObject => super.renderObject;
+
+  void updateParentData(RenderObject child, Positioned positioned) {
+    if (positioned == null)
+      _updateParentDataWithValues(child, null, null, null, null);
+    else
+      _updateParentDataWithValues(child, positioned.top, positioned.right, positioned.bottom, positioned.left);
+  }
+
+  void _updateParentDataWithValues(RenderObject child, double top, double right, double bottom, double left) {
+    assert(child.parentData is StackParentData);
+    final StackParentData parentData = child.parentData;
+    bool needsLayout = false;
+    if (parentData.top != top) {
+      parentData.top = top;
+      needsLayout = true;
+    }
+
+    if (parentData.right != right) {
+      parentData.right = right;
+      needsLayout = true;
+    }
+
+    if (parentData.bottom != bottom) {
+      parentData.bottom = bottom;
+      needsLayout = true;
+    }
+
+    if (parentData.left != left) {
+      parentData.left = left;
+      needsLayout = true;
+    }
+
+    if (needsLayout)
+      renderObject.markNeedsLayout();
+  }
 }
 
 class Positioned extends ParentDataNode {
   Positioned({
     Key key,
     Widget child,
-    double top,
-    double right,
-    double bottom,
-    double left
-  }) : super(child,
-             new StackParentData()..top = top
-                                  ..right = right
-                                  ..bottom = bottom
-                                  ..left = left,
-             key: key);
+    this.top,
+    this.right,
+    this.bottom,
+    this.left
+  }) : super(key: key, child: child);
+
+  final double top;
+  final double right;
+  final double bottom;
+  final double left;
+
+   void debugValidateAncestor(Widget ancestor) {
+     assert(() {
+       'Positioned must placed directly inside a Stack';
+       return ancestor is Stack;
+     });
+   }
 }
 
 class Grid extends MultiChildRenderObjectWrapper {
@@ -548,6 +590,19 @@ class Flex extends MultiChildRenderObjectWrapper {
     renderObject.alignItems = alignItems;
     renderObject.textBaseline = textBaseline;
   }
+
+  void updateParentData(RenderObject child, Flexible flexible) {
+    _updateParentDataWithValues(child, flexible == null ? null : flexible.flex);
+  }
+
+  void _updateParentDataWithValues(RenderObject child, int flex) {
+    assert(child.parentData is FlexParentData);
+    final FlexParentData parentData = child.parentData;
+    if (parentData.flex != flex) {
+      parentData.flex = flex;
+      renderObject.markNeedsLayout();
+    }
+  }
 }
 
 class Row extends Flex {
@@ -569,8 +624,17 @@ class Column extends Flex {
 }
 
 class Flexible extends ParentDataNode {
-  Flexible({ Key key, int flex: 1, Widget child })
-    : super(child, new FlexBoxParentData()..flex = flex, key: key);
+  Flexible({ Key key, this.flex: 1, Widget child })
+    : super(key: key, child: child);
+
+  final int flex;
+
+  void debugValidateAncestor(Widget ancestor) {
+    assert(() {
+      'Flexible must placed directly inside a Flex';
+      return ancestor is Flex;
+    });
+  }
 }
 
 class Paragraph extends LeafRenderObjectWrapper {

packages/flutter/lib/widgets/framework.dart

@@ -441,8 +441,10 @@ bool _canSync(Widget a, Widget b) {
 // stylistic information, etc.
 abstract class TagNode extends Widget {
 
-  TagNode(Widget child, { Key key })
-    : this.child = child, super(key: key);
+  TagNode({ Key key, Widget child })
+    : this.child = child, super(key: key) {
+    assert(child != null);
+  }
 
   // TODO(jackson): Remove this workaround for limitation of Dart mixins
   TagNode._withKey(Widget child, Key key)
@@ -480,10 +482,15 @@ abstract class TagNode extends Widget {
 
 }
 
-class ParentDataNode extends TagNode {
-  ParentDataNode(Widget child, this.parentData, { Key key })
-    : super(child, key: key);
-  final ParentData parentData;
+abstract class ParentDataNode extends TagNode {
+  ParentDataNode({ Key key, Widget child })
+    : super(key: key, child: child);
+
+  /// Subclasses should override this function to ensure that they are placed
+  /// inside widgets that expect them.
+  ///
+  /// The given ancestor is the first non-component ancestor of this widget.
+  void debugValidateAncestor(Widget ancestor);
 }
 
 abstract class Inherited extends TagNode {
@@ -553,7 +560,7 @@ class Listener extends TagNode  {
          onPointerUp: onPointerUp,
          custom: custom
        ),
-       super(child, key: key);
+       super(key: key, child: child);
 
   final Map<String, EventListener> listeners;
 
@@ -975,24 +982,30 @@ abstract class RenderObjectWrapper extends Widget {
     // we give them their own slots for them to fit into us.
   }
 
+  /// Override this function if your RenderObjectWrapper uses a [ParentDataNode]
+  /// to program parent data into children.
+  void updateParentData(RenderObject child, ParentDataNode node) { }
+
   void syncRenderObject(RenderObjectWrapper old) {
     assert(old == null || old.renderObject == renderObject);
-    ParentData parentData = null;
-    Widget ancestor = parent;
-    while (ancestor != null && ancestor is! RenderObjectWrapper) {
-      if (ancestor is ParentDataNode && ancestor.parentData != null) {
-        if (parentData != null)
-          parentData.merge(ancestor.parentData); // this will throw if the types aren't the same
-        else
-          parentData = ancestor.parentData;
+    ParentDataNode parentDataNode = null;
+    for (Widget current = parent; current != null; current = current.parent) {
+      assert(() {
+        if (current is ParentDataNode) {
+          Widget ancestor = current.parent;
+          while (ancestor != null && ancestor is Component)
+            ancestor = ancestor.parent;
+          current.debugValidateAncestor(ancestor);
+        }
+        return true;
+      });
+      if (current is ParentDataNode) {
+        assert(parentDataNode == null);
+        parentDataNode = current;
+      } else if (current is RenderObjectWrapper) {
+        current.updateParentData(renderObject, parentDataNode);
+        break;
       }
-      ancestor = ancestor.parent;
-    }
-    if (parentData != null) {
-      assert(renderObject.parentData != null);
-      renderObject.parentData.merge(parentData); // this will throw if the types aren't appropriate
-      if (ancestor != null && ancestor.renderObject != null)
-        ancestor.renderObject.markNeedsLayout();
     }
   }
 

packages/unit/test/widget/stack_test.dart

+import 'package:sky/widgets.dart';
+import 'package:test/test.dart';
+
+import 'widget_tester.dart';
+
+void main() {
+  test('Can change position data', () {
+    WidgetTester tester = new WidgetTester();
+
+    tester.pumpFrame(() {
+      return new Stack([
+        new Positioned(
+          left: 10.0,
+          child: new Container(
+            width: 10.0,
+            height: 10.0
+          )
+        )
+      ]);
+    });
+
+    Container container = tester.findWidget((Widget widget) => widget is Container);
+    expect(container.renderObject.parentData.top, isNull);
+    expect(container.renderObject.parentData.right, isNull);
+    expect(container.renderObject.parentData.bottom, isNull);
+    expect(container.renderObject.parentData.left, equals(10.0));
+
+    tester.pumpFrame(() {
+      return new Stack([
+        new Positioned(
+          right: 10.0,
+          child: new Container(
+            width: 10.0,
+            height: 10.0
+          )
+        )
+      ]);
+    });
+
+    container = tester.findWidget((Widget widget) => widget is Container);
+    expect(container.renderObject.parentData.top, isNull);
+    expect(container.renderObject.parentData.right, equals(10.0));
+    expect(container.renderObject.parentData.bottom, isNull);
+    expect(container.renderObject.parentData.left, isNull);
+  });
+}