Skip to content

Updates mark needs semantics update logic for overlay portal #151688

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
auto-submit merged 4 commits into from
Jul 24, 2024
Merged

Updates mark needs semantics update logic for overlay portal #151688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9d862cc
Updates mark needs semantics update logic for overlay portal
chunhtai Jul 12, 2024
6d8e3db
update
chunhtai Jul 12, 2024
9ffbf5d
update
chunhtai Jul 23, 2024
2ecffb4
update
chunhtai Jul 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 25 additions & 6 deletions packages/flutter/lib/src/rendering/object.dart
View file
Open in desktop
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!;

@LongCatIsLooong LongCatIsLooong Jul 13, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this line is the only reason that RenderObject.semanticsParent is needed. Is it possible to change the implementation of markNeedsSemanticsUpdate to a recursive one so we can override _RenderDeferredLayoutBox.markNeedsSemanticsUpdate to propagate the call to its surrogate parent? So that we don't have to change the interface of RenderObject.

@chunhtai chunhtai Jul 15, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cant think of a clean way to do this with the mayProduceSiblingNodes. The requirement is the following:

If a node will produce sibling nodes, it must mark two semantics boundaries above to be dirty

If we were to do this, we have to add additional flag either on renderobject or additional parameter to the markNeedsSemanticsUpdate([int howManyBoundaries]).

@chunhtai chunhtai Jul 15, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The semanticsparent is also used for some look up in _getSemanticsForParent.

@LongCatIsLooong LongCatIsLooong Jul 15, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The semanticsparent is also used for some look up in _getSemanticsForParent.

The call from _getSemanticsForParent only wants to check if the parent is null it seems (so it doesn't matter for OverlayPortal).

If a node will produce sibling nodes, it must mark two semantics boundaries above to be dirty

Ah ok that makes sense. (But at the same time that feels a bit weird, not familiar with semantics tree, but why is the closest semantics boundary invalidated if this has child configurations, but it's fine if parent has child configurations?)

@chunhtai chunhtai Jul 16, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the parent is not markNeedsSemanticsUpdate, it won't change its sibling node that get pass on two level up.

If the parent it self is markNeedsSemanticsUpdate it is not the current node's job to mark two level above the parent dirty. It will be when the parent's markNeedsSemanticsUpdate call to mark two level above.

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
View file
Open in desktop
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
View file
Open in desktop
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