Do not send traversal child semantics nodes to engine when traversal parent is missing (#193372) fix https://github.com/flutter/flutter/issues/190357 When an `OverlayPortal` (such as the one inside a Material `Slider`) is inside a pushed route, the route's entrance transition (e.g., `FadeTransition`) starts at opacity `0.0` on the initial frame. `RenderAnimatedOpacityMixin.visitChildrenForSemantics` skips visiting the `OverlayPortal` anchor (`traversalParentIdentifier`), while `_RenderTheater.visitChildrenForSemantics` still visits the `OverlayPortal`'s `_RenderDeferredLayoutBox` (`traversalChildIdentifier`) directly on the `OverlayEntry`. Although the traversal child is excluded from its hit-test parent's `childrenInTraversalOrder` and `childrenInHitTestOrder` when its traversal parent is not in `_traversalParentNodes`, `SemanticsOwner.sendSemanticsUpdate` still serialized the traversal child via `_addToUpdate`. This sent an orphan `SemanticsNode` (not referenced by any parent in the update) to the engine, causing issues. This PR: - Skips calling `_addToUpdate` in `SemanticsOwner.sendSemanticsUpdate` for nodes in a traversal child subtree whose traversal parent is missing (`_isSkippedInHitTestTree`). - Marks affected traversal children, their unmerged descendants, and their hit-test `parent`s dirty when a traversal parent is newly registered, detaches, or changes its identifier so the nodes and hit-test child lists are updated in sync when the traversal parent enters or leaves the semantics tree. ## Pre-launch Checklist - [ ] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [ ] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [ ] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [ ] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [ ] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant in-code documentation (doc comments with `///`). - [ ] If this PR introduces a new feature or capability, I created and linked a website documentation issue or PR in [flutter/website] (or verified none is needed). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [ ] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [flutter/website]: https://github.com/flutter/website [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
diff --git a/packages/flutter/lib/src/semantics/semantics.dart b/packages/flutter/lib/src/semantics/semantics.dart index 4b7ee0e..8ecdc33 100644 --- a/packages/flutter/lib/src/semantics/semantics.dart +++ b/packages/flutter/lib/src/semantics/semantics.dart
@@ -3395,6 +3395,16 @@ if (_traversalChildIdentifier case final Object identifier?) { owner!._traversalParentNodes[identifier]?._markDirty(); } + // If a traversal parent detaches while its traversal children remain + // attached, mark their hit-test parents dirty so they drop the orphaned + // traversal children from childrenInHitTestOrder. + if (_traversalParentIdentifier case final Object identifier?) { + if (owner!._traversalChildNodes[identifier] case final Set<SemanticsNode> childNodes?) { + for (final childNode in childNodes) { + childNode.parent?._markDirty(); + } + } + } // Clean up the according entry in owner._traversalParentNodes map. owner!._traversalParentNodes.removeWhere((Object key, SemanticsNode node) => node == this); @@ -3853,6 +3863,12 @@ final mergeAllDescendantsIntoThisNodeValueChanged = _mergeAllDescendantsIntoThisNode != config.isMergingSemanticsOfDescendants; + // Changing whether this node is a traversal child affects whether its + // hit-test parent includes it in traversal and hit-test child lists. + if (_traversalChildIdentifier != config.traversalChildIdentifier) { + parent?._markDirty(); + } + _identifier = config.identifier; _traversalParentIdentifier = config.traversalParentIdentifier; _traversalChildIdentifier = config.traversalChildIdentifier; @@ -4171,6 +4187,43 @@ return childrenInTraversalOrder; } + // Returns true if this node is a grafted traversal child whose traversal + // parent is missing (or itself skipped) in the semantics tree. + bool _shouldSkipInHitTest([Set<SemanticsNode>? visited]) { + if (kIsWeb) { + return false; + } + if (_isTraversalChild && !(parent?._isTraversalParent ?? false)) { + final SemanticsNode? traversalParent = + owner!._traversalParentNodes[_traversalChildIdentifier]; + if (traversalParent == null) { + return true; + } + visited ??= <SemanticsNode>{}; + if (!visited.add(this)) { + return false; + } + return traversalParent._isSkippedInHitTestTree(visited); + } + return false; + } + + // Returns true if this node or any of its hit-test ancestors is skipped due + // to a missing traversal parent. + bool _isSkippedInHitTestTree([Set<SemanticsNode>? visited]) { + if (kIsWeb) { + return false; + } + SemanticsNode? current = this; + while (current != null) { + if (current._shouldSkipInHitTest(visited)) { + return true; + } + current = current.parent; + } + return false; + } + List<SemanticsNode> _childrenInHitTestOrder() { if (_children == null) { return const <SemanticsNode>[]; @@ -4183,16 +4236,7 @@ // dropped from the hit-test tree to keep the two trees in sync. // Otherwise, the user might accidentally hit test a node that cannot // be traversed. - bool shouldNotSkipInHitTest(SemanticsNode child) { - if (child._isTraversalChild) { - final SemanticsNode? traversalParent = - owner!._traversalParentNodes[child.getSemanticsData().traversalChildIdentifier]; - return traversalParent != null; - } - return true; - } - - return _children!.where(shouldNotSkipInHitTest).toList(); + return _children!.where((SemanticsNode child) => !child._shouldSkipInHitTest()).toList(); } Int32List _childrenIdInHitTestOrder() { @@ -4341,7 +4385,7 @@ // order, is the child of the traversal child. In this case, no grafting // needed, otherwise, it will cause infinite loop. SemanticsNode? traversalParent = - owner!._traversalParentNodes[child.getSemanticsData().traversalChildIdentifier]; + owner!._traversalParentNodes[child._traversalChildIdentifier]; final int? traversalParentId = traversalParent?.id; while (traversalParent != null) { if (traversalParent == child) { @@ -5135,9 +5179,31 @@ } } + // Track whether this node was already registered as the traversal + // parent for its current identifier before refreshing the map. + final bool hadSameTraversalParent = + node._isTraversalParent && + _traversalParentNodes[node.traversalParentIdentifier!] == node; + // Clean up the dirty entry in owner._traversalParentNodes map because it - // will be updated later. - _traversalParentNodes.removeWhere((Object key, SemanticsNode oldNode) => node == oldNode); + // will be updated later. If the node's traversalParentIdentifier changed, + // mark the hit-test parents of its former traversal children dirty so + // they can update childrenInHitTestOrder. + _traversalParentNodes.removeWhere((Object key, SemanticsNode oldNode) { + if (node == oldNode) { + if (!kIsWeb && key != node.traversalParentIdentifier) { + if (_traversalChildNodes[key] case final Set<SemanticsNode> childNodes?) { + for (final childNode in childNodes) { + if (childNode.parent != null && !visitedNodes.contains(childNode.parent)) { + childNode.parent!._markDirty(); + } + } + } + } + return true; + } + return false; + }); // Clean up the node from the value set in owner._traversalChildNodes. for (final Set<SemanticsNode> childSet in _traversalChildNodes.values) { childSet.removeWhere((SemanticsNode oldNode) => node == oldNode); @@ -5175,6 +5241,36 @@ parentNode._markDirty(); } } + if (node._isTraversalParent) { + // Mark attached traversal children dirty so they recompute their + // traversal transform. If this traversal parent was newly registered, + // also mark each traversal child's hit-test parent and unmerged + // descendants dirty so previously skipped nodes are included in the + // semantics update. + final Set<SemanticsNode>? childNodes = + _traversalChildNodes[node.traversalParentIdentifier]; + if (childNodes != null) { + for (final SemanticsNode childNode in childNodes) { + if (!childNode.attached) { + continue; + } + if (!visitedNodes.contains(childNode)) { + childNode._markDirty(); + } + if (!hadSameTraversalParent) { + if (childNode.parent != null && !visitedNodes.contains(childNode.parent)) { + childNode.parent!._markDirty(); + } + childNode._visitDescendants((SemanticsNode descendant) { + if (!descendant.isMergedIntoParent && !visitedNodes.contains(descendant)) { + descendant._markDirty(); + } + return true; + }); + } + } + } + } } } } @@ -5200,7 +5296,16 @@ // which happens e.g. when the node is no longer contributing // semantics). if (node._dirty && node.attached) { - node._addToUpdate(builder, customSemanticsActionIds); + if (node._isSkippedInHitTestTree()) { + // Do not serialize nodes that are dropped from the tree due to a + // missing traversal parent, as they would be orphans in the update. + node._dirty = false; + if (node._isTraversalParent) { + _traversalParentNodes.remove(node.traversalParentIdentifier); + } + } else { + node._addToUpdate(builder, customSemanticsActionIds); + } } } _dirtyNodes.clear();
diff --git a/packages/flutter/test/semantics/semantics_update_test.dart b/packages/flutter/test/semantics/semantics_update_test.dart index a1ce409..65c24c1 100644 --- a/packages/flutter/test/semantics/semantics_update_test.dart +++ b/packages/flutter/test/semantics/semantics_update_test.dart
@@ -292,7 +292,9 @@ ), ); - // SemanticsNode#0 (root) should have 0 children in both traversal order and hit-test order. + // SemanticsNode#0 (root) should have 0 children in both traversal order and hit-test order, + // and the orphan traversal child should not be sent in the update. + expect(SemanticsUpdateBuilderSpy.observations.keys, equals(<int>[0])); final SemanticsNodeUpdateObservation? rootObservation = SemanticsUpdateBuilderSpy.observations[0]; expect(rootObservation, isNotNull); @@ -305,6 +307,117 @@ skip: kIsWeb, // [intended] the web engine handles the tree grafting itself. ); + testWidgets( + 'Semantics update skips OverlayPortal traversal child while anchor is hidden by Opacity and sends it when anchor becomes visible', + (WidgetTester tester) async { + final SemanticsHandle handle = tester.ensureSemantics(); + await tester.pumpWidget(const Placeholder(), phase: EnginePhase.build); + SemanticsUpdateBuilderSpy.observations.clear(); + + final controller = OverlayPortalController()..show(); + final opacity = ValueNotifier<double>(0.0); + addTearDown(opacity.dispose); + + final entry = OverlayEntry( + builder: (BuildContext context) { + return ValueListenableBuilder<double>( + valueListenable: opacity, + builder: (BuildContext context, double value, Widget? child) { + return Opacity( + opacity: value, + child: OverlayPortal( + controller: controller, + overlayChildBuilder: (BuildContext context) { + return TestButton(onPressed: () {}, child: const Text('portal child')); + }, + child: TestButton(onPressed: () {}, child: const Text('anchor')), + ), + ); + }, + ); + }, + ); + addTearDown(() { + entry + ..remove() + ..dispose(); + }); + + // Frame 1: opacity is 0.0, so the anchor (traversal parent) is excluded from semantics, + // while the OverlayPortal's deferred layout box (traversal child) is mounted on the Overlay. + // No orphan nodes should be sent in the semantics update. + await tester.pumpWidget( + Directionality( + textDirection: TextDirection.ltr, + child: Overlay(initialEntries: <OverlayEntry>[entry]), + ), + ); + + expect(SemanticsUpdateBuilderSpy.observations.keys, equals(<int>[0])); + expect(SemanticsUpdateBuilderSpy.observations[0]!.childrenInTraversalOrder, isEmpty); + expect(SemanticsUpdateBuilderSpy.observations[0]!.childrenInHitTestOrder, isEmpty); + + // Frame 2: opacity becomes 1.0, so the anchor (traversal parent) enters the semantics tree. + // The root node, anchor, traversal child, and portal child button should all be updated + // and connected without any orphan nodes. + SemanticsUpdateBuilderSpy.observations.clear(); + opacity.value = 1.0; + await tester.pump(); + + final allChildIdsInTraversalOrder = <int>{ + for (final SemanticsNodeUpdateObservation obs + in SemanticsUpdateBuilderSpy.observations.values) + ...obs.childrenInTraversalOrder, + }; + final allChildIdsInHitTestOrder = <int>{ + for (final SemanticsNodeUpdateObservation obs + in SemanticsUpdateBuilderSpy.observations.values) + ...obs.childrenInHitTestOrder, + }; + for (final int id in SemanticsUpdateBuilderSpy.observations.keys) { + if (id != 0) { + expect( + allChildIdsInTraversalOrder, + contains(id), + reason: 'Node $id should be reachable in traversal order', + ); + expect( + allChildIdsInHitTestOrder, + contains(id), + reason: 'Node $id should be reachable in hit-test order', + ); + } + } + expect( + SemanticsUpdateBuilderSpy.observations.values.any( + (SemanticsNodeUpdateObservation obs) => obs.label == 'anchor', + ), + isTrue, + ); + expect( + SemanticsUpdateBuilderSpy.observations.values.any( + (SemanticsNodeUpdateObservation obs) => obs.label == 'portal child', + ), + isTrue, + ); + + // Frame 3: opacity becomes 0.0 again, detaching the traversal parent while the + // OverlayPortal's deferred layout box remains attached to the Overlay. + // The root node should update its hit-test and traversal children to be empty. + SemanticsUpdateBuilderSpy.observations.clear(); + opacity.value = 0.0; + await tester.pump(); + + expect(SemanticsUpdateBuilderSpy.observations.keys, equals(<int>[0])); + expect(SemanticsUpdateBuilderSpy.observations[0]!.childrenInTraversalOrder, isEmpty); + expect(SemanticsUpdateBuilderSpy.observations[0]!.childrenInHitTestOrder, isEmpty); + + SemanticsUpdateBuilderSpy.observations.clear(); + handle.dispose(); + }, + skip: kIsWeb, // [intended] the web engine handles the tree grafting itself. + ); + testWidgets('Semantics update removes detached OverlayPortal traversal child', ( WidgetTester tester, ) async { @@ -380,6 +493,136 @@ SemanticsUpdateBuilderSpy.observations.clear(); handle.dispose(); }, skip: kIsWeb); // intended: the web engine handles the traversal order itself. + + // Regression test for https://github.com/flutter/flutter/issues/190357. + testWidgets( + 'Semantics update does not serialize orphan nodes when pushing and popping a route with OverlayPortal', + (WidgetTester tester) async { + final SemanticsHandle handle = tester.ensureSemantics(); + await tester.pumpWidget(const Placeholder(), phase: EnginePhase.build); + SemanticsUpdateBuilderSpy.observations.clear(); + + final activeTree = <int, SemanticsNodeUpdateObservation>{}; + + void verifyAndCommitUpdate() { + if (SemanticsUpdateBuilderSpy.observations.isEmpty) { + return; + } + activeTree.addAll(SemanticsUpdateBuilderSpy.observations); + + final reachableInTraversal = <int>{}; + void walkTraversal(int id) { + expect( + activeTree.containsKey(id), + isTrue, + reason: 'Node $id referenced in childrenInTraversalOrder was never sent in an update', + ); + if (!reachableInTraversal.add(id)) { + return; + } + activeTree[id]!.childrenInTraversalOrder.forEach(walkTraversal); + } + + final reachableInHitTest = <int>{}; + void walkHitTest(int id) { + expect( + activeTree.containsKey(id), + isTrue, + reason: 'Node $id referenced in childrenInHitTestOrder was never sent in an update', + ); + if (!reachableInHitTest.add(id)) { + return; + } + activeTree[id]!.childrenInHitTestOrder.forEach(walkHitTest); + } + + walkTraversal(0); + walkHitTest(0); + + for (final int updatedId in SemanticsUpdateBuilderSpy.observations.keys) { + expect( + reachableInTraversal, + contains(updatedId), + reason: 'Updated node $updatedId is an orphan in traversal order', + ); + expect( + reachableInHitTest, + contains(updatedId), + reason: 'Updated node $updatedId is an orphan in hit-test order', + ); + } + expect(reachableInTraversal, equals(reachableInHitTest)); + + activeTree.removeWhere((int id, _) => !reachableInTraversal.contains(id)); + SemanticsUpdateBuilderSpy.observations.clear(); + } + + final navigatorKey = GlobalKey<NavigatorState>(); + final portalController = OverlayPortalController()..show(); + + await tester.pumpWidget( + WidgetsApp( + navigatorKey: navigatorKey, + color: const Color(0xFF000000), + onGenerateRoute: (RouteSettings settings) { + return PageRouteBuilder<void>( + pageBuilder: + ( + BuildContext context, + Animation<double> animation, + Animation<double> secondaryAnimation, + ) { + return TestButton(onPressed: () {}, child: const Text('home')); + }, + ); + }, + ), + ); + verifyAndCommitUpdate(); + + // Push a route whose transition starts at opacity 0.0 (like MaterialPageRoute / showDialog) + // and contains an active OverlayPortal (like Material Slider). + navigatorKey.currentState!.push( + PageRouteBuilder<void>( + pageBuilder: + ( + BuildContext context, + Animation<double> animation, + Animation<double> secondaryAnimation, + ) { + return FadeTransition( + opacity: animation, + child: OverlayPortal( + controller: portalController, + overlayChildBuilder: (BuildContext context) => const SizedBox.shrink(), + child: TestButton(onPressed: () {}, child: const Text('pushed slider')), + ), + ); + }, + ), + ); + + // Step through every frame of the push transition. + for (var i = 0; i < 10; i++) { + await tester.pump(const Duration(milliseconds: 50)); + verifyAndCommitUpdate(); + } + expect( + activeTree.values.any((SemanticsNodeUpdateObservation obs) => obs.label == 'pushed slider'), + isTrue, + ); + + // Pop the route and step through every frame of the reverse transition. + navigatorKey.currentState!.pop(); + for (var i = 0; i < 10; i++) { + await tester.pump(const Duration(milliseconds: 50)); + verifyAndCommitUpdate(); + } + + handle.dispose(); + }, + skip: kIsWeb, // [intended] the web engine handles the traversal order itself. + ); } class SemanticsUpdateTestBinding extends AutomatedTestWidgetsFlutterBinding {