Reland "Refactor OverlayPortal semantics (#173005)"#178095
Reland "Refactor OverlayPortal semantics (#173005)"#178095auto-submit[bot] merged 6 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request relands the refactoring of OverlayPortal semantics and includes a fix for an infinite loop. The changes introduce traversalParent and hitTestTransform to correctly handle semantics for overlays, separating traversal order from paint/hit-test order. This is a significant and wide-ranging change across the engine and framework. The implementation is mostly solid, but I have identified a few areas for improvement. I've pointed out a potential race condition in the semantics update logic, a minor bug in aria-owns handling on the web, and a code duplication in the C++ part of the engine.
455d99f to
fb7cc8f
Compare
| required String? identifier, | ||
| required Object? traversalParentIdentifier, | ||
| required Set<Object>? traversalChildIdentifier, | ||
| required Object? traversalChildIdentifier, |
There was a problem hiding this comment.
This should be Object instead of Set<Object>?. This is missed because we haven't directly used it in OverlayPortal. Catched this when adding tests.
fb7cc8f to
87a7eb5
Compare
| // updatedChildren list. | ||
| continue; | ||
| // | ||
| // A corner case is the traversal parent of the traversal child, in paint |
There was a problem hiding this comment.
so something like this?
// assume root node id is 0
Semantics(
// assume semantics node id = 1
traversalParent: 'key',
child: Semantics(
// assume semantics node id = 2
traversalChildren: 'key',
)
)
In this case I assume we want the hittest tree to be
0 -> 1 -> 2
but traversal tree to be
0 -> 2 -> 1
There was a problem hiding this comment.
Talked with @chunhtai offline. In this case we throw exception to indicate wrong parent-child relationship. Also update SemanticsConfiguration.isCompatibleWith to avoid implicit merging.
85c3596 to
ddf67f6
Compare
ddf67f6 to
4e9b287
Compare
7d4f799 to
1c1aa5a
Compare
…78095) Reverts flutter#178007 This PR is to reland flutter#173005 and add a fix to avoid infinite loop. The fix doesn't contain engine changes.
…78095) Reverts flutter#178007 This PR is to reland flutter#173005 and add a fix to avoid infinite loop. The fix doesn't contain engine changes.
…78095) Reverts flutter#178007 This PR is to reland flutter#173005 and add a fix to avoid infinite loop. The fix doesn't contain engine changes.
|
@QuncCccccc Awesome ! Thank you for fixing it 🎉 I was wondering if you'd be aware of a possible workaround while waiting for this fixe to be on the stable channel ? |
|
Hi @ValentinVignal, I actually don't think there's an easy workaround for the original issue. Please wait for the fix on the stable channel:) |
|
Ok noted, thank you for the confirmation :) |
Reverts #178007
This PR is to reland #173005 and add a fix to avoid infinite loop. The fix doesn't contain engine changes.