From 7912e6d27ed3312be63dfbb7ffe9f03bc46eed73 Mon Sep 17 00:00:00 2001 From: chunhtai <47866232+chunhtai@users.noreply.github.com> Date: Tue, 23 Jul 2024 17:30:19 -0700 Subject: [PATCH] Updates mark needs semantics update logic for overlay portal (#151688) OverlayPortal attaches its overlaychild's renderobject to overlay directly while keeps its semantics tree under overlayportal. This become a problem when the `overlaychild` markNeedsSemanticsUpdate that it propagate upward the rendering tree. This means instead of marking dirty upward to the OverlayPortal, it directly mark Overlay dirty instead and skip `OverlayPortal`. Currently this does not pose an issue other than unnecessary rebuilds, but it become a problem when I try to optimize the semantics tree compilation https://github.com/flutter/flutter/pull/150394. After the optimization it won't rebuild semantics node unless it is marked dirty. Since the OverlayPortal widget does not get marked dirty, it won't update the subtree. --- .../flutter/lib/src/rendering/object.dart | 31 ++++++++--- packages/flutter/lib/src/widgets/overlay.dart | 5 ++ .../test/widgets/overlay_portal_test.dart | 51 +++++++++++++++++++ 3 files changed, 81 insertions(+), 6 deletions(-) diff --git a/packages/flutter/lib/src/rendering/object.dart b/packages/flutter/lib/src/rendering/object.dart index 6804028b443..8573d006401 100644 --- a/packages/flutter/lib/src/rendering/object.dart +++ b/packages/flutter/lib/src/rendering/object.dart @@ -1825,6 +1825,15 @@ abstract class RenderObject with DiagnosticableTreeMixin implements HitTestTarge RenderObject? get parent => _parent; RenderObject? _parent; + /// The semantics parent of this render object in the semantics tree. + /// + /// This is typically the same as [parent]. + /// + /// [OverlayPortal] overrides this field to change how it forms its + /// semantics sub-tree. + @visibleForOverriding + RenderObject? get semanticsParent => _parent; + /// Called by subclasses when they decide a render object is a child. /// /// Only for use by subclasses when changing their child lists. Calling this @@ -3576,6 +3585,15 @@ abstract class RenderObject with DiagnosticableTreeMixin implements HitTestTarge /// object, for accessibility purposes. Rect get semanticBounds; + /// Whether the semantics of this render object is dirty and await the update. + /// + /// Always returns false in release mode. + bool get debugNeedsSemanticsUpdate { + if (kReleaseMode) { + return false; + } + return _needsSemanticsUpdate; + } bool _needsSemanticsUpdate = true; SemanticsNode? _semantics; @@ -3641,10 +3659,11 @@ abstract class RenderObject with DiagnosticableTreeMixin implements HitTestTarge // node, thus marking this semantics boundary dirty is not enough, it needs // to find the first parent semantics boundary that does not have any // possible sibling node. - while (node.parent != null && (mayProduceSiblingNodes || !isEffectiveSemanticsBoundary)) { + while (node.semanticsParent != null && (mayProduceSiblingNodes || !isEffectiveSemanticsBoundary)) { if (node != this && node._needsSemanticsUpdate) { break; } + node._needsSemanticsUpdate = true; // Since this node is a semantics boundary, the produced sibling nodes will // be attached to the parent semantics boundary. Thus, these sibling nodes @@ -3653,7 +3672,7 @@ abstract class RenderObject with DiagnosticableTreeMixin implements HitTestTarge mayProduceSiblingNodes = false; } - node = node.parent!; + node = node.semanticsParent!; isEffectiveSemanticsBoundary = node._semanticsConfiguration.isSemanticBoundary; if (isEffectiveSemanticsBoundary && node._semantics == null) { // We have reached a semantics boundary that doesn't own a semantics node. @@ -3675,7 +3694,7 @@ abstract class RenderObject with DiagnosticableTreeMixin implements HitTestTarge if (!node._needsSemanticsUpdate) { node._needsSemanticsUpdate = true; if (owner != null) { - assert(node._semanticsConfiguration.isSemanticBoundary || node.parent == null); + assert(node._semanticsConfiguration.isSemanticBoundary || node.semanticsParent == null); owner!._nodesNeedingSemantics.add(node); owner!.requestVisualUpdate(); } @@ -3684,7 +3703,7 @@ abstract class RenderObject with DiagnosticableTreeMixin implements HitTestTarge /// Updates the semantic information of the render object. void _updateSemantics() { - assert(_semanticsConfiguration.isSemanticBoundary || parent == null); + assert(_semanticsConfiguration.isSemanticBoundary || semanticsParent == null); if (_needsLayout) { // There's not enough information in this subtree to compute semantics. // The subtree is probably being kept alive by a viewport but not laid out. @@ -3735,7 +3754,7 @@ abstract class RenderObject with DiagnosticableTreeMixin implements HitTestTarge final bool blockChildInteractions = blockUserActions || config.isBlockingUserActions; final bool childrenMergeIntoParent = mergeIntoParent || config.isMergingSemanticsOfDescendants; final List childConfigurations = []; - final bool explicitChildNode = config.explicitChildNodes || parent == null; + final bool explicitChildNode = config.explicitChildNodes || semanticsParent == null; final ChildSemanticsConfigurationsDelegate? childConfigurationsDelegate = config.childConfigurationsDelegate; final Map configToFragment = {}; final List<_InterestingSemanticsFragment> mergeUpFragments = <_InterestingSemanticsFragment>[]; @@ -3816,7 +3835,7 @@ abstract class RenderObject with DiagnosticableTreeMixin implements HitTestTarge _needsSemanticsUpdate = false; final _SemanticsFragment result; - if (parent == null) { + if (semanticsParent == null) { assert(!config.hasBeenAnnotated); assert(!mergeIntoParent); assert(siblingMergeFragmentGroups.isEmpty); diff --git a/packages/flutter/lib/src/widgets/overlay.dart b/packages/flutter/lib/src/widgets/overlay.dart index 76f092bfd6c..0c0c283613b 100644 --- a/packages/flutter/lib/src/widgets/overlay.dart +++ b/packages/flutter/lib/src/widgets/overlay.dart @@ -2152,6 +2152,7 @@ class _OverlayPortalElement extends RenderObjectElement { if (slot != null) { renderObject._deferredLayoutChild = child as _RenderDeferredLayoutBox; slot._addChild(child); + renderObject.markNeedsSemanticsUpdate(); } else { renderObject.child = child; } @@ -2163,6 +2164,7 @@ class _OverlayPortalElement extends RenderObjectElement { void moveRenderObjectChild(_RenderDeferredLayoutBox child, _OverlayEntryLocation oldSlot, _OverlayEntryLocation newSlot) { assert(newSlot._debugIsLocationValid()); newSlot._moveChild(child, oldSlot); + renderObject.markNeedsSemanticsUpdate(); } @override @@ -2270,6 +2272,9 @@ final class _RenderDeferredLayoutBox extends RenderProxyBox with _RenderTheaterM super.markNeedsLayout(); } + @override + RenderObject? get semanticsParent => _layoutSurrogate; + @override double? computeDryBaseline(BoxConstraints constraints, TextBaseline baseline) { final RenderBox? child = this.child; diff --git a/packages/flutter/test/widgets/overlay_portal_test.dart b/packages/flutter/test/widgets/overlay_portal_test.dart index 436bbf03687..efd35538bf3 100644 --- a/packages/flutter/test/widgets/overlay_portal_test.dart +++ b/packages/flutter/test/widgets/overlay_portal_test.dart @@ -150,6 +150,57 @@ void main() { expect(directionSeenByOverlayChild, textDirection); }); + testWidgets('The overlay portal update semantics does not dirty overlay', (WidgetTester tester) async { + late StateSetter setState; + late final OverlayEntry overlayEntry; + final UniqueKey overlayKey = UniqueKey(); + String msg = 'msg'; + addTearDown(() => overlayEntry..remove()..dispose()); + + await tester.pumpWidget( + Directionality( + textDirection: TextDirection.ltr, + child: Semantics( + container: true, + child: Overlay( + key: overlayKey, + initialEntries: [ + overlayEntry = OverlayEntry( + builder: (BuildContext context) { + return Semantics( + container: true, + explicitChildNodes: true, + child: StatefulBuilder( + builder: (BuildContext context, StateSetter setter) { + setState = setter; + return OverlayPortal( + controller: controller1, + overlayChildBuilder: (BuildContext context) { + return Semantics(label: msg, child: const SizedBox(width: 100, height: 100)); + }, + child: const Text('overlay child'), + ); + } + ), + ); + }, + ), + ], + ), + ), + ), + ); + final RenderObject renderObject = tester.renderObject(find.byKey(overlayKey)); + expect(renderObject.debugNeedsSemanticsUpdate, isFalse); + expect(find.bySemanticsLabel(msg), findsOneWidget); + setState(() { + msg = 'msg2'; + }); + // stop before updating semantics. + await tester.pump(null, EnginePhase.composite); + expect(renderObject.debugNeedsSemanticsUpdate, isFalse); + }); + testWidgets('Safe to deactivate and re-activate OverlayPortal', (WidgetTester tester) async { late final OverlayEntry overlayEntry; addTearDown(() => overlayEntry..remove()..dispose());