Refactor OverlayPortal semantics #173005
Refactor OverlayPortal semantics #173005
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
github-actions
bot
added
platform-android
QuncCccccc
force-pushed
the
remove_grafting_for_web_only_copy
branch
from
c365ca5 to
c305e6b
Compare
| @@ -683,6 +683,7 @@ class SemanticsUpdateBuilder { | |||
| required int platformViewId, | |||
| required int scrollChildren, | |||
| required int scrollIndex, | |||
| required int? overlayPortalParent, | |||
There was a problem hiding this comment.
this needs a better name. something like traversalOwner. overlay is a widget layer concept
| @@ -1816,6 +1816,7 @@ abstract class SemanticsUpdateBuilder { | |||
| required int platformViewId, | |||
| required int scrollChildren, | |||
| required int scrollIndex, | |||
| required int overlayPortalParent, | |||
There was a problem hiding this comment.
somewhere in the doc should mention what this paremeter does and why it is here and that is should only be used in web and why
| if (overlayPortalParent != -1) { | ||
| SemanticsObject? parent = owner._semanticsTree[overlayPortalParent!]; | ||
| if (parent != null && parent.semanticRole != null) { | ||
| parent.element.setAttribute('aria-owns', '$kFlutterSemanticNodePrefix${id}'); |
There was a problem hiding this comment.
Does this get cleaned up if the overlayPotalParent change to null?
| /// | ||
| /// If this node indicates an overlay portal child, this is its overlay portal | ||
| /// parent node in traversal order. Otherwise, it is the same as [parent]. | ||
| SemanticsNode? get semanticsParent => _semanticsParent ?? parent; |
There was a problem hiding this comment.
This needs a better name as something along the line of traversalOwner or traversalParent
| @@ -3039,6 +3053,9 @@ class SemanticsNode with DiagnosticableTreeMixin { | |||
| String get identifier => _identifier; | |||
| String _identifier = _kEmptyConfig.identifier; | |||
|
|
|||
| bool get _isOverlayPortalParent => getSemanticsData().identifier.endsWith('parent'); | |||
There was a problem hiding this comment.
This seems a bit easy to collide with real world usage. I think we should create a new property for setting overlay parent of a given semantics node.
There was a problem hiding this comment.
also overlayportal is a widget layer concept, the name here should be more generic. imagine if in the future we have other non-overlay use case that also need to leverage this mechanism. The name here will be confusing
| @@ -3576,8 +3593,60 @@ class SemanticsNode with DiagnosticableTreeMixin { | |||
| static final Int32List _kEmptyCustomSemanticsActionsList = Int32List(0); | |||
| static final Float64List _kIdentityTransform = _initIdentityTransform(); | |||
|
|
|||
| static Matrix4 _computeChildGeometry({ | |||
There was a problem hiding this comment.
since now the grafting only happens in semantics node's hittest transofmr, can you simplify the logic in _SemanticsGeomtry to make sure parent is always an ancestor of child?
also this needs a better name.
| childrenInHitTestOrder = _kEmptyChildList; | ||
| if (_isOverlayPortalParent) { | ||
| final List<SemanticsNode> sortedChildren = _childrenInTraversalOrder(); | ||
| childrenInTraversalOrder = Int32List(sortedChildren.length); |
There was a problem hiding this comment.
consider turn this into a method to be reuse here and below
| for (final SemanticsNode node in visitedNodes) { | ||
| assert(node.parent?._dirty != true); // could be null (no parent) or false (not dirty) | ||
| final String identifier = node.identifier.split(' ')[0]; |
There was a problem hiding this comment.
This is a bit scary given people may have space in their identifier
| @override | ||
| void describeSemanticsConfiguration(SemanticsConfiguration config) { | ||
| super.describeSemanticsConfiguration(config); | ||
| if (childIdentifier != null) { |
There was a problem hiding this comment.
oh nice, so we won't have parentData issue then
Fixes #163576
This PR refactored the grafting part on
OverlayPortal. Originally, the semantics tree ofOverlayPortalwas constructed/grafted in render object phase to make sure the correctness of the traversal order. However this resulted wrong hit-test order and the issue surfaced on web. With the fact that on web we are not able to graft/correct hit-test order tree, this PR:OverlayPortalso the hit-test order is always correct.childrenInTraversalOrderto engine.childrenInTraversalOrdercauses it have different length from the length ofchildrenInHitTestOrderand wrong hit-test transform of theOverlayPortalchildren because when the transform is calculated, it assumes a correct traversal order. To fix these issues, this PR also:OverlayPortalchildren.hitTestTransformproperty and pass it to Android engine.childrenInTraversalOrderandchildrenInHitTestOrder.ARIA-ownsin web engine to fix the traversal order.Pre-launch Checklist
///).