Detached traversal child should update its traversal parent#186118
Conversation
github-actions
Bot
added
framework
QuncCccccc
force-pushed
the
invisible_items_traversal_fix
branch
from
20d1562 to
5ffe0b5
Compare
QuncCccccc
changed the title
QuncCccccc
force-pushed
the
invisible_items_traversal_fix
branch
from
5ffe0b5 to
e16cafb
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to track and update traversal parent nodes in SemanticsOwner when a child node is detached, and adds a corresponding test case. The reviewer suggests refactoring the implementation to avoid introducing a new _dirtyTraversalParentNodes set, recommending instead that parentNode._markDirty() be used to leverage the existing _dirtyNodes infrastructure for better consistency and robustness.
| if (_traversalChildIdentifier case final Object identifier?) { | ||
| if (owner!._traversalParentNodes[identifier] case final SemanticsNode parentNode? | ||
| when parentNode.attached && parentNode._isTraversalParent) { | ||
| owner!._dirtyTraversalParentNodes.add(parentNode); |
There was a problem hiding this comment.
why not use parentNode._markDirty() ?
There was a problem hiding this comment.
I tried that first but marking the traversal parent node dirty will fail a case when its traversal node is dirty. Because when we send a node to _addToUpdate, we have an assertion to check node.parent?._dirty != true.
So I'm thinking if the traversal parent node itself is not "dirty" and we just wanted to update its children lists, we can keep this in _dirtyTraversalParentNodes? What do you think?
There was a problem hiding this comment.
Done! Talking with @chunhtai offline and seems sorting the updated list to make sure parent is always sent to update first would avoid the error. Thank you!
| final Set<SemanticsNode> _dirtyNodes = <SemanticsNode>{}; | ||
| final Map<int, SemanticsNode> _nodes = <int, SemanticsNode>{}; | ||
| final Set<SemanticsNode> _detachedNodes = <SemanticsNode>{}; | ||
| final Set<SemanticsNode> _dirtyTraversalParentNodes = <SemanticsNode>{}; |
There was a problem hiding this comment.
why do we need a separate list? can we just use the _dirtyNodes?
There was a problem hiding this comment.
Removed. Thanks!
QuncCccccc
force-pushed
the
invisible_items_traversal_fix
branch
from
e16cafb to
ec56744
Compare
QuncCccccc
force-pushed
the
invisible_items_traversal_fix
branch
from
2bebc18 to
3711637
Compare
QuncCccccc
force-pushed
the
invisible_items_traversal_fix
branch
from
a76c7ce to
e00cc76
Compare
QuncCccccc
force-pushed
the
invisible_items_traversal_fix
branch
from
e00cc76 to
65c0c25
Compare
| } | ||
| } | ||
|
|
||
| updatedVisitedNodes.sort((SemanticsNode a, SemanticsNode b) => a.depth - b.depth); |
There was a problem hiding this comment.
updatedVisitedNodes cames from visitedNodes which is already sorted above, do you know why we need to sort it again?
There was a problem hiding this comment.
it also feel like we are sorting excessively
localDirtyNodes is sorted to be added to visitedNodes
and visitedNodes is sorted to be added updatedVisitedNodes
and here is another point of sorting for updatedVisitedNodes. I felt we could eliminate some of them and just sort once at the end. Also maybe they should be stored in a set before the sorting
There was a problem hiding this comment.
I see, it makes sense. Seems we sort the list 3 times which is unnecessary. I'll update the logic a bit!
chunhtai
left a comment
chunhtai
left a comment
There was a problem hiding this comment.
code LGTM, just left some nit
| _traversalChildNodes[node.traversalChildIdentifier!] ??= <SemanticsNode>{}; | ||
| _traversalChildNodes[node.traversalChildIdentifier!]!.add(node); |
There was a problem hiding this comment.
_traversalChildNodes.putIfAbsent(node.traversalChildIdentifier!, <SemanticsNode>() =><SemanticsNode>{}).add(node)
flutter-dashboard
Bot
removed
the
autosubmit
…186118) Fixes flutter#185784 When a traversal child is detached, its traversal parent was not marked dirty. So the framework didn't send an updated childrenInTraversalOrder for traversal parent. On the engine side, the out-of-dated subtree is still reachable, so Talkback still read invisible items after the menu closed. This PR is to update the _dirtyNodes list when a traversal child is detached. When a traversal child is detached, its traversal parent will be marked dirty and added to the _dirtyNodes list to update its childrenInTraversalOrder. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing.
3 participants
Fixes #185784
When a traversal child is detached, its traversal parent was not marked dirty. So the framework didn't send an updated childrenInTraversalOrder for traversal parent. On the engine side, the out-of-dated subtree is still reachable, so Talkback still read invisible items after the menu closed.
This PR is to update the _dirtyNodes list when a traversal child is detached. When a traversal child is detached, its traversal parent will be marked dirty and added to the _dirtyNodes list to update its childrenInTraversalOrder.
Pre-launch Checklist
///).