Skip to content

Commit

Permalink
Updates mark needs semantics update logic for overlay portal (#151688)
Browse files Browse the repository at this point in the history
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 flutter/flutter#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.
  • Loading branch information
chunhtai authored Jul 24, 2024
1 parent 770c13b commit 7912e6d
Show file tree
Hide file tree
Showing 3 changed files with 81 additions and 6 deletions.
31 changes: 25 additions & 6 deletions packages/flutter/lib/src/rendering/object.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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
Expand All @@ -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.
Expand All @@ -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();
}
Expand All @@ -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.
Expand Down Expand Up @@ -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<SemanticsConfiguration> childConfigurations = <SemanticsConfiguration>[];
final bool explicitChildNode = config.explicitChildNodes || parent == null;
final bool explicitChildNode = config.explicitChildNodes || semanticsParent == null;
final ChildSemanticsConfigurationsDelegate? childConfigurationsDelegate = config.childConfigurationsDelegate;
final Map<SemanticsConfiguration, _InterestingSemanticsFragment> configToFragment = <SemanticsConfiguration, _InterestingSemanticsFragment>{};
final List<_InterestingSemanticsFragment> mergeUpFragments = <_InterestingSemanticsFragment>[];
Expand Down Expand Up @@ -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);
Expand Down
5 changes: 5 additions & 0 deletions packages/flutter/lib/src/widgets/overlay.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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
Expand Down Expand Up @@ -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;
Expand Down
51 changes: 51 additions & 0 deletions packages/flutter/test/widgets/overlay_portal_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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 = 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());
Expand Down

0 comments on commit 7912e6d

Please sign in to comment.