From dc8377b1d4e5c0ab697215717982cd9a43effa7b Mon Sep 17 00:00:00 2001 From: LongCatIsLooong <31859944+LongCatIsLooong@users.noreply.github.com> Date: Tue, 12 Sep 2023 15:41:45 -0700 Subject: [PATCH] Ensure OverlayPortal.overlayChild's renderObject is reachable via treewalk (#134497) Fixes https://github.com/flutter/flutter/issues/133545 ` child._layoutSurrogate.markNeedsLayout();` was called when `_skipMarkNeedsLayout` is set true so when there's no relayout boundary between the layout surrogate and the RenderTheater, no dirty render objects will be added to the PipelineOwner's dirty list. It's ok to mark the RenderTheater dirty when there's no layout boundary between it and the layout surrogate. --- .../flutter/lib/src/rendering/object.dart | 2 +- packages/flutter/lib/src/widgets/overlay.dart | 34 ++++---- .../test/widgets/overlay_portal_test.dart | 82 +++++++++++++++++++ 3 files changed, 101 insertions(+), 17 deletions(-) diff --git a/packages/flutter/lib/src/rendering/object.dart b/packages/flutter/lib/src/rendering/object.dart index c6fc50a84e3..2683563f8c6 100644 --- a/packages/flutter/lib/src/rendering/object.dart +++ b/packages/flutter/lib/src/rendering/object.dart @@ -1960,7 +1960,7 @@ abstract class RenderObject with DiagnosticableTreeMixin implements HitTestTarge final bool mutationsToDirtySubtreesAllowed = activeLayoutRoot.owner?._debugAllowMutationsToDirtySubtrees ?? false; final bool doingLayoutWithCallback = activeLayoutRoot._doingThisLayoutWithCallback; // Mutations on this subtree is allowed when: - // - the subtree is being mutated in a layout callback. + // - the "activeLayoutRoot" subtree is being mutated in a layout callback. // - a different part of the render tree is doing a layout callback, // and this subtree is being reparented to that subtree, as a result // of global key reparenting. diff --git a/packages/flutter/lib/src/widgets/overlay.dart b/packages/flutter/lib/src/widgets/overlay.dart index e88aafad20b..4a0d6ef2bec 100644 --- a/packages/flutter/lib/src/widgets/overlay.dart +++ b/packages/flutter/lib/src/widgets/overlay.dart @@ -1031,12 +1031,14 @@ class _RenderTheater extends RenderBox with ContainerRenderObjectMixin _layoutSurrogate; void layoutByLayoutSurrogate() { - assert(!_parentDoingLayout); + assert(!_theaterDoingThisLayout); final _RenderTheater? theater = parent as _RenderTheater?; if (theater == null || !attached) { assert(false, '$this is not attached to parent'); @@ -2097,25 +2098,26 @@ final class _RenderDeferredLayoutBox extends RenderProxyBox with _RenderTheaterM super.layout(BoxConstraints.tight(theater.constraints.biggest)); } - bool _parentDoingLayout = false; + bool _theaterDoingThisLayout = false; @override void layout(Constraints constraints, { bool parentUsesSize = false }) { assert(_needsLayout == debugNeedsLayout); // Only _RenderTheater calls this implementation. assert(parent != null); final bool scheduleDeferredLayout = _needsLayout || this.constraints != constraints; - assert(!_parentDoingLayout); - _parentDoingLayout = true; + assert(!_theaterDoingThisLayout); + _theaterDoingThisLayout = true; super.layout(constraints, parentUsesSize: parentUsesSize); - assert(_parentDoingLayout); - _parentDoingLayout = false; + assert(_theaterDoingThisLayout); + _theaterDoingThisLayout = false; _needsLayout = false; assert(!debugNeedsLayout); if (scheduleDeferredLayout) { final _RenderTheater parent = this.parent! as _RenderTheater; // Invoking markNeedsLayout as a layout callback allows this node to be - // merged back to the `PipelineOwner` if it's not already dirty. Otherwise - // this may cause some dirty descendants to performLayout a second time. + // merged back to the `PipelineOwner`'s dirty list in the right order, if + // it's not already dirty. Otherwise this may cause some dirty descendants + // to performLayout a second time. parent.invokeLayoutCallback((BoxConstraints constraints) { markNeedsLayout(); }); } } @@ -2129,7 +2131,7 @@ final class _RenderDeferredLayoutBox extends RenderProxyBox with _RenderTheaterM @override void performLayout() { assert(!_debugMutationsLocked); - if (_parentDoingLayout) { + if (_theaterDoingThisLayout) { _needsLayout = false; return; } diff --git a/packages/flutter/test/widgets/overlay_portal_test.dart b/packages/flutter/test/widgets/overlay_portal_test.dart index 702a5db6405..d361f0c5d6b 100644 --- a/packages/flutter/test/widgets/overlay_portal_test.dart +++ b/packages/flutter/test/widgets/overlay_portal_test.dart @@ -255,6 +255,42 @@ void main() { expect(tester.takeException(), isNull); }); + testWidgets('No relayout boundary between OverlayPortal and Overlay', (WidgetTester tester) async { + // Regression test for https://github.com/flutter/flutter/issues/133545. + final GlobalKey key = GlobalKey(debugLabel: 'key'); + final Widget widget = Directionality( + textDirection: TextDirection.ltr, + child: Overlay( + initialEntries: [ + OverlayEntry( + builder: (BuildContext context) { + // The Positioned widget prevents a relayout boundary from being + // introduced between the Overlay and OverlayPortal. + return Positioned( + top: 0, + left: 0, + child: OverlayPortal( + controller: controller1, + overlayChildBuilder: (BuildContext context) => SizedBox(key: key), + child: const SizedBox(), + ), + ); + }, + ), + ], + ), + ); + + controller1.hide(); + await tester.pumpWidget(widget); + + controller1.show(); + await tester.pump(); + expect(find.byKey(key), findsOneWidget); + expect(tester.takeException(), isNull); + verifyTreeIsClean(); + }); + testWidgets('Throws when the same controller is attached to multiple OverlayPortal', (WidgetTester tester) async { final OverlayPortalController controller = OverlayPortalController(debugLabel: 'local controller'); final Widget widget = Directionality( @@ -516,6 +552,52 @@ void main() { expect(tester.takeException(), isNull); }); + testWidgets('works in a LayoutBuilder 3', (WidgetTester tester) async { + late StateSetter setState; + bool shouldShowChild = false; + + Widget layoutBuilder(BuildContext context, BoxConstraints constraints) { + return OverlayPortal( + controller: controller2, + overlayChildBuilder: (BuildContext context) => const SizedBox(), + child: const SizedBox(), + ); + } + controller1.hide(); + controller2.hide(); + + await tester.pumpWidget( + Directionality( + textDirection: TextDirection.ltr, + child: Overlay( + initialEntries: [ + OverlayStatefulEntry(builder: (BuildContext context, StateSetter setter) { + setState = setter; + // The Positioned widget ensures there's no relayout boundary + // between the Overlay and the OverlayPortal. + return Positioned( + top: 0, + left: 0, + child: OverlayPortal( + controller: controller1, + overlayChildBuilder: (BuildContext context) => const SizedBox(), + child: shouldShowChild ? LayoutBuilder(builder: layoutBuilder) : null, + ), + ); + }), + ], + ), + ), + ); + + controller1.show(); + controller2.show(); + setState(() { shouldShowChild = true; }); + + await tester.pump(); + expect(tester.takeException(), isNull); + }); + testWidgets('throws when no Overlay', (WidgetTester tester) async { await tester.pumpWidget( Directionality(