Handle dirty bits correctly when render objects are re-attached

Fixes #2855

packages/flutter/lib/src/rendering/object.dart

@@ -107,10 +107,6 @@ class PaintingContext {
       assert(child._layer != null);
       assert(() {
         child.debugRegisterRepaintBoundaryPaint(includedParent: true, includedChild: false);
-        return true;
-      });
-      child._layer.detach();
-      assert(() {
         child._layer.debugCreator = child.debugCreator ?? child.runtimeType;
         return true;
       });
@@ -121,6 +117,7 @@ class PaintingContext {
 
   void _appendLayer(Layer layer) {
     assert(!_isRecording);
+    layer.detach();
     _containerLayer.append(layer);
   }
 
@@ -949,6 +946,30 @@ abstract class RenderObject extends AbstractNode implements HitTestTarget {
   @override
   void attach(PipelineOwner owner) {
     super.attach(owner);
+    // If the node was dirtied in some way while unattached, make sure to add
+    // it to the appropriate dirty list now that an owner is available
+    if (_needsLayout && _relayoutSubtreeRoot != null) {
+      // Don't enter this block if we've never laid out at all;
+      // scheduleInitialLayout() will handle it
+      _needsLayout = false;
+      markNeedsLayout();
+    }
+    if (_needsCompositingBitsUpdate) {
+      _needsCompositingBitsUpdate = false;
+      markNeedsCompositingBitsUpdate();
+    }
+    if (_needsPaint && _layer != null) {
+      // Don't enter this block if we've never painted at all;
+      // scheduleInitialPaint() will handle it
+      _needsPaint = false;
+      markNeedsPaint();
+    }
+    if (_needsSemanticsUpdate && hasSemantics) {
+      // Don't enter this block if we've never updated semantics at all;
+      // scheduleInitialSemantics() will handle it
+      _needsSemanticsUpdate = false;
+      markNeedsSemanticsUpdate();
+    }
   }
 
   bool _needsLayout = true;
@@ -1396,8 +1417,6 @@ abstract class RenderObject extends AbstractNode implements HitTestTarget {
   /// writes are coalesced, removing redundant computation.
   void markNeedsPaint() {
     assert(owner == null || !owner.debugDoingPaint);
-    if (!attached)
-      return; // Don't try painting things that aren't in the hierarchy
     if (_needsPaint)
       return;
     _needsPaint = true;
@@ -1592,7 +1611,7 @@ abstract class RenderObject extends AbstractNode implements HitTestTarget {
   /// tree will be out of date.
   void markNeedsSemanticsUpdate({ bool onlyChanges: false, bool noGeometry: false }) {
     assert(!attached || !owner._debugDoingSemantics);
-    if (!attached || !owner._semanticsEnabled || (_needsSemanticsUpdate && onlyChanges && (_needsSemanticsGeometryUpdate || noGeometry)))
+    if ((attached && !owner._semanticsEnabled) || (_needsSemanticsUpdate && onlyChanges && (_needsSemanticsGeometryUpdate || noGeometry)))
       return;
     if (!noGeometry && (_semantics == null || (_semantics.hasChildren && _semantics.wasAffectedByClip))) {
       // Since the geometry might have changed, we need to make sure to reapply any clips.
@@ -1612,7 +1631,8 @@ abstract class RenderObject extends AbstractNode implements HitTestTarget {
       }
       if (!node._needsSemanticsUpdate) {
         node._needsSemanticsUpdate = true;
-        owner._nodesNeedingSemantics.add(node);
+        if (owner != null)
+          owner._nodesNeedingSemantics.add(node);
       }
     } else {
       // The shape of the semantics tree around us may have changed.
@@ -1631,7 +1651,8 @@ abstract class RenderObject extends AbstractNode implements HitTestTarget {
       node._semantics?.reset();
       if (!node._needsSemanticsUpdate) {
         node._needsSemanticsUpdate = true;
-        owner._nodesNeedingSemantics.add(node);
+        if (owner != null)
+          owner._nodesNeedingSemantics.add(node);
       }
     }
   }

packages/flutter/test/rendering/reattach_test.dart

+// Copyright 2016 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 'package:flutter/material.dart';
+import 'package:flutter/rendering.dart';
+import 'package:sky_services/semantics/semantics.mojom.dart' as mojom;
+import 'package:test/test.dart';
+
+import 'rendering_tester.dart';
+
+class TestTree {
+  TestTree() {
+    // viewport incoming constraints are tight 800x600
+    // viewport is vertical by default
+    root = new RenderViewport(
+      // Place the child to be evaluated within both a repaint boundary and a
+      // layout-root element (in this case a tightly constrained box). Otherwise
+      // the act of transplanting the root into a new container will cause the
+      // relayout/repaint of the new parent node to satisfy the test.
+      child: new RenderRepaintBoundary(
+        child: new RenderConstrainedBox(
+          additionalConstraints: new BoxConstraints.tightFor(height: 20.0, width: 20.0),
+          child: new RenderRepaintBoundary(
+            child: new RenderCustomPaint(
+              painter: new TestCallbackPainter(
+                onPaint: () { painted = true; }
+              ),
+              child: new RenderPositionedBox(
+                child: child = new RenderConstrainedBox(
+                  additionalConstraints: new BoxConstraints.tightFor(height: 20.0, width: 20.0)
+                )
+              )
+            )
+          )
+        )
+      )
+    );
+  }
+  RenderObject root;
+  RenderConstrainedBox child;
+  bool painted = false;
+}
+
+class MutableCompositor extends RenderProxyBox {
+  MutableCompositor({ RenderBox child }) : super(child);
+  bool _alwaysComposite = false;
+  @override
+  bool get alwaysNeedsCompositing => _alwaysComposite;
+}
+
+class TestCompositingBitsTree {
+  TestCompositingBitsTree() {
+    // viewport incoming constraints are tight 800x600
+    // viewport is vertical by default
+    root = new RenderViewport(
+      // Place the child to be evaluated within a repaint boundary. Otherwise
+      // the act of transplanting the root into a new container will cause the
+      // repaint of the new parent node to satisfy the test.
+      child: new RenderRepaintBoundary(
+        child: compositor = new MutableCompositor(
+          child: new RenderCustomPaint(
+            painter: new TestCallbackPainter(
+              onPaint: () { painted = true; }
+            ),
+            child: child = new RenderConstrainedBox(
+              additionalConstraints: new BoxConstraints.tightFor(height: 20.0, width: 20.0)
+            )
+          )
+        )
+      )
+    );
+  }
+  RenderObject root;
+  MutableCompositor compositor;
+  RenderConstrainedBox child;
+  bool painted = false;
+}
+
+class TestSemanticsListener implements mojom.SemanticsListener {
+  final List<mojom.SemanticsNode> updates = <mojom.SemanticsNode>[];
+
+  @override
+  void updateSemanticsTree(List<mojom.SemanticsNode> nodes) {
+    updates.addAll(nodes);
+  }
+}
+
+void main() {
+  test('objects can be detached and re-attached: layout', () {
+    TestTree testTree = new TestTree();
+    // Lay out
+    layout(testTree.root, phase: EnginePhase.layout);
+    expect(testTree.child.size, equals(const Size(20.0, 20.0)));
+    // Remove testTree from the custom render view
+    renderer.renderView.child = null;
+    expect(testTree.child.owner, isNull);
+    // Dirty one of the elements
+    testTree.child.additionalConstraints =
+      new BoxConstraints.tightFor(height: 5.0, width: 5.0);
+    // Lay out again
+    layout(testTree.root, phase: EnginePhase.layout);
+    expect(testTree.child.size, equals(const Size(5.0, 5.0)));
+  });
+  test('objects can be detached and re-attached: compositingBits', () {
+    TestCompositingBitsTree testTree = new TestCompositingBitsTree();
+    // Lay out, composite, and paint
+    layout(testTree.root, phase: EnginePhase.paint);
+    expect(testTree.painted, isTrue);
+    // Remove testTree from the custom render view
+    renderer.renderView.child = null;
+    expect(testTree.child.owner, isNull);
+    // Dirty one of the elements
+    testTree.compositor._alwaysComposite = true;
+    testTree.child.markNeedsCompositingBitsUpdate();
+    testTree.painted = false;
+    // Lay out, composite, and paint again
+    layout(testTree.root, phase: EnginePhase.paint);
+    expect(testTree.painted, isTrue);
+  });
+  test('objects can be detached and re-attached: paint', () {
+    TestTree testTree = new TestTree();
+    // Lay out, composite, and paint
+    layout(testTree.root, phase: EnginePhase.paint);
+    expect(testTree.painted, isTrue);
+    // Remove testTree from the custom render view
+    renderer.renderView.child = null;
+    expect(testTree.child.owner, isNull);
+    // Dirty one of the elements
+    testTree.child.markNeedsPaint();
+    testTree.painted = false;
+    // Lay out, composite, and paint again
+    layout(testTree.root, phase: EnginePhase.paint);
+    expect(testTree.painted, isTrue);
+  });
+  test('objects can be detached and re-attached: semantics', () {
+    TestTree testTree = new TestTree();
+    TestSemanticsListener listener = new TestSemanticsListener();
+    SemanticsNode.addListener(listener);
+    // Lay out, composite, paint, and update semantics
+    layout(testTree.root, phase: EnginePhase.sendSemanticsTree);
+    expect(listener.updates.length, equals(1));
+    // Remove testTree from the custom render view
+    renderer.renderView.child = null;
+    expect(testTree.child.owner, isNull);
+    // Dirty one of the elements
+    listener.updates.clear();
+    testTree.child.markNeedsSemanticsUpdate();
+    expect(listener.updates.length, equals(0));
+    // Lay out, composite, paint, and update semantics again
+    layout(testTree.root, phase: EnginePhase.sendSemanticsTree);
+    expect(listener.updates.length, equals(1));
+  });
+}

packages/flutter/test/rendering/rendering_tester.dart

@@ -25,7 +25,9 @@ enum EnginePhase {
   layout,
   compositingBits,
   paint,
-  composite
+  composite,
+  flushSemantics,
+  sendSemanticsTree
 }
 
 class TestRenderingFlutterBinding extends BindingBase with Scheduler, Services, Renderer, Gesturer {
@@ -51,6 +53,14 @@ class TestRenderingFlutterBinding extends BindingBase with Scheduler, Services,
     if (phase == EnginePhase.paint)
       return;
     renderView.compositeFrame();
+    if (phase == EnginePhase.composite)
+      return;
+    if (SemanticsNode.hasListeners) {
+      pipelineOwner.flushSemantics();
+      if (phase == EnginePhase.flushSemantics)
+        return;
+      SemanticsNode.sendSemanticsTree();
+    }
   }
 }