From 663596bc544277a43e4a676a473f73e0b3a79b68 Mon Sep 17 00:00:00 2001 From: Ian Hickson Date: Thu, 1 Sep 2016 11:54:42 -0700 Subject: [PATCH] Handle GlobalKey reparenting inside LayoutBuilder (#5673) --- .../lib/src/material/flexible_space_bar.dart | 1 + .../flutter/lib/src/rendering/object.dart | 42 ++++++++++- .../lib/src/widgets/layout_builder.dart | 41 +++++++---- .../lib/src/widgets/scroll_behavior.dart | 2 +- .../layout_builder_and_parent_data_test.dart | 73 +++++++++++++++++++ .../widget/layout_builder_mutations_test.dart | 62 ++++++++++++++++ 6 files changed, 202 insertions(+), 19 deletions(-) create mode 100644 packages/flutter/test/widget/layout_builder_and_parent_data_test.dart create mode 100644 packages/flutter/test/widget/layout_builder_mutations_test.dart diff --git a/packages/flutter/lib/src/material/flexible_space_bar.dart b/packages/flutter/lib/src/material/flexible_space_bar.dart index 54d3fb67ff..51ad8e70cd 100644 --- a/packages/flutter/lib/src/material/flexible_space_bar.dart +++ b/packages/flutter/lib/src/material/flexible_space_bar.dart @@ -91,6 +91,7 @@ class _FlexibleSpaceBarState extends State { if (config.background != null) { final double fadeStart = math.max(0.0, 1.0 - kToolBarHeight / deltaHeight); final double fadeEnd = 1.0; + assert(fadeStart <= fadeEnd); final double opacity = 1.0 - new Interval(fadeStart, fadeEnd).transform(t); final double parallax = new Tween(begin: 0.0, end: deltaHeight / 4.0).lerp(t); if (opacity > 0.0) { diff --git a/packages/flutter/lib/src/rendering/object.dart b/packages/flutter/lib/src/rendering/object.dart index 4cb7e8af80..fc49aaa4e7 100644 --- a/packages/flutter/lib/src/rendering/object.dart +++ b/packages/flutter/lib/src/rendering/object.dart @@ -880,6 +880,25 @@ class PipelineOwner { } } + // This flag is used to allow the kinds of mutations performed by GlobalKey + // reparenting while a LayoutBuilder is being rebuilt and in so doing tries to + // move a node from another LayoutBuilder subtree that hasn't been updated + // yet. To set this, call [_enableMutationsToDirtySubtrees], which is called + // by [RenderObject.invokeLayoutCallback]. + bool _debugAllowMutationsToDirtySubtrees = false; + + // See [RenderObject.invokeLayoutCallback]. + void _enableMutationsToDirtySubtrees(VoidCallback callback) { + assert(_debugDoingLayout); + bool oldState = _debugAllowMutationsToDirtySubtrees; + _debugAllowMutationsToDirtySubtrees = true; + try { + callback(); + } finally { + _debugAllowMutationsToDirtySubtrees = oldState; + } + } + List _nodesNeedingCompositingBitsUpdate = []; /// Updates the [needsCompositing] bits. /// @@ -1174,6 +1193,10 @@ abstract class RenderObject extends AbstractNode implements HitTestTarget { result = true; break; } + if (owner != null && owner._debugAllowMutationsToDirtySubtrees && node._needsLayout) { + result = true; + break; + } if (node._debugMutationsLocked) { result = false; break; @@ -1584,8 +1607,21 @@ abstract class RenderObject extends AbstractNode implements HitTestTarget { // this field always holds a closure. VoidCallback _performLayout = _doNothing; - /// Allows this render object to mutate its child list during layout and - /// calls callback. + /// Allows mutations to be made to this object's child list (and any + /// descendants) as well as to any other dirty nodes in the render tree owned + /// by the same [PipelineOwner] as this object. The `callback` argument is + /// invoked synchronously, and the mutations are allowed only during that + /// callback's execution. + /// + /// This exists to allow child lists to be built on-demand during layout (e.g. + /// based on the object's size), and to enable nodes to be moved around the + /// tree as this happens (e.g. to handle [GlobalKey] reparenting), while still + /// ensuring that any particular node is only laid out once per frame. + /// + /// Calling this function disables a number of assertions that are intended to + /// catch likely bugs. As such, using this function is generally discouraged. + /// + /// This function can only be called during layout. @protected void invokeLayoutCallback(LayoutCallback callback) { assert(_debugMutationsLocked); @@ -1593,7 +1629,7 @@ abstract class RenderObject extends AbstractNode implements HitTestTarget { assert(!_doingThisLayoutWithCallback); _doingThisLayoutWithCallback = true; try { - callback(constraints); + owner._enableMutationsToDirtySubtrees(() { callback(constraints); }); } finally { _doingThisLayoutWithCallback = false; } diff --git a/packages/flutter/lib/src/widgets/layout_builder.dart b/packages/flutter/lib/src/widgets/layout_builder.dart index 5b15eb729b..f41cff68c8 100644 --- a/packages/flutter/lib/src/widgets/layout_builder.dart +++ b/packages/flutter/lib/src/widgets/layout_builder.dart @@ -48,10 +48,14 @@ class LayoutBuilder extends RenderObjectWidget { @override _RenderLayoutBuilder createRenderObject(BuildContext context) => new _RenderLayoutBuilder(); + + // updateRenderObject is redundant with the logic in the LayoutBuilderElement below. } class _RenderLayoutBuilder extends RenderBox with RenderObjectWithChildMixin { - _RenderLayoutBuilder({ LayoutCallback callback }) : _callback = callback; + _RenderLayoutBuilder({ + LayoutCallback callback, + }) : _callback = callback; LayoutCallback get callback => _callback; LayoutCallback _callback; @@ -102,8 +106,8 @@ class _RenderLayoutBuilder extends RenderBox with RenderObjectWithChildMixin { /// from the given offset by the given delta. T applyCurve(T scrollOffset, T scrollDelta) => scrollOffset; - /// Whether this scroll behavior currently permits scrolling + /// Whether this scroll behavior currently permits scrolling. bool get isScrollable => true; /// The scroll drag constant to use for physics simulations created by this diff --git a/packages/flutter/test/widget/layout_builder_and_parent_data_test.dart b/packages/flutter/test/widget/layout_builder_and_parent_data_test.dart new file mode 100644 index 0000000000..2fabba5909 --- /dev/null +++ b/packages/flutter/test/widget/layout_builder_and_parent_data_test.dart @@ -0,0 +1,73 @@ +// 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_test/flutter_test.dart' hide TypeMatcher; +import 'package:flutter/widgets.dart'; + +class SizeChanger extends StatefulWidget { + SizeChanger({ + Key key, + this.child, + }) : super(key: key); + + final Widget child; + + @override + SizeChangerState createState() => new SizeChangerState(); +} + +class SizeChangerState extends State { + bool _flag = false; + + void trigger() { + setState(() { + _flag = true; + }); + } + + @override + Widget build(BuildContext context) { + return new Row( + children: [ + new SizedBox( + height: _flag ? 50.0 : 100.0, + width: 100.0, + child: config.child + ) + ], + ); + } +} + +class Wrapper extends StatelessWidget { + Wrapper({ + Key key, + this.child, + }) : super(key: key); + + final Widget child; + + @override + Widget build(BuildContext context) { + return child; + } +} + +void main() { + testWidgets('Applying parent data inside a LayoutBuilder', (WidgetTester tester) async { + int frame = 0; + await tester.pumpWidget(new SizeChanger( // when this is triggered, the child LayoutBuilder will build again + child: new LayoutBuilder(builder: (BuildContext context, BoxConstraints constraints) { + return new Column(children: [new Flexible( + flex: frame, // this is different after the next pump, so that the parentData has to be applied again + child: new Container(height: 100.0), + )]); + }) + )); + frame += 1; + SizeChangerState state = tester.state(find.byType(SizeChanger)); + state.trigger(); + await tester.pump(); + }); +} diff --git a/packages/flutter/test/widget/layout_builder_mutations_test.dart b/packages/flutter/test/widget/layout_builder_mutations_test.dart new file mode 100644 index 0000000000..48e4dca4e1 --- /dev/null +++ b/packages/flutter/test/widget/layout_builder_mutations_test.dart @@ -0,0 +1,62 @@ +// 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/src/widgets/basic.dart'; +import 'package:flutter/src/widgets/framework.dart'; +import 'package:flutter/src/widgets/layout_builder.dart'; +import 'package:flutter_test/flutter_test.dart' hide TypeMatcher; + +class Wrapper extends StatelessWidget { + Wrapper({ + Key key, + this.child + }) : super(key: key) { + assert(child != null); + } + + final Widget child; + + @override + Widget build(BuildContext context) => child; +} + +void main() { + testWidgets('Moving a global key from another LayoutBuilder at layout time', (WidgetTester tester) async { + final GlobalKey victimKey = new GlobalKey(); + + await tester.pumpWidget(new Row(children: [ + new Wrapper( + child: new LayoutBuilder(builder: (BuildContext context, BoxConstraints constraints) { + return new SizedBox(); + }), + ), + new Wrapper( + child: new Wrapper( + child: new LayoutBuilder(builder: (BuildContext context, BoxConstraints constraints) { + return new Wrapper( + child: new SizedBox(key: victimKey) + ); + }) + ) + ), + ])); + + await tester.pumpWidget(new Row(children: [ + new Wrapper( + child: new LayoutBuilder(builder: (BuildContext context, BoxConstraints constraints) { + return new Wrapper( + child: new SizedBox(key: victimKey) + ); + }) + ), + new Wrapper( + child: new Wrapper( + child: new LayoutBuilder(builder: (BuildContext context, BoxConstraints constraints) { + return new SizedBox(); + }) + ) + ), + ])); + }); +}