layout: Avoid extra tree walks when removing accessibility nodes. - #46348
Merged
Conversation
delan
approved these changes
Jul 8, 2026
Comment on lines
+404
to
+406
| if !update.tree_changes.contains_key(&child_id) { | ||
| ids_to_remove.push(child_id); | ||
| } |
Member
There was a problem hiding this comment.
the contains_key() check can be removed, because tree_changes has been drained by this point
| self.opaque_node_to_id.remove(&opaque_node); | ||
| } | ||
| for child in node.borrow().children() { | ||
| let child_id = child.borrow().id; |
Member
There was a problem hiding this comment.
this can be gotten with node.child_ids(), instead of borrowing the child
Comment on lines
+410
to
+414
| debug_assert!( | ||
| update.unresolved_local_damage.is_empty(), | ||
| "Damage not empty: {:?}", | ||
| update.unresolved_local_damage | ||
| ); |
Member
There was a problem hiding this comment.
does this need to be debug_assert!()? seems fast?
Member
There was a problem hiding this comment.
also would be good to write a comment explaining why this is the case
Contributor
Author
There was a problem hiding this comment.
My reasoning was that I think it might not be worth crashing the tab over if there is pending damage, but it might actually be better to crash the tab in that case in case there are some subtle errors in the accessibility tree.
Comment added.
| tree_id: tree.tree_id, | ||
| }; | ||
|
|
||
| tree.remove_stale_nodes(self); |
Member
There was a problem hiding this comment.
was this reordering across the TreeUpdate initialiser intentional?
Contributor
Author
There was a problem hiding this comment.
Ah - I figured it out: we need to drop removed nodes before we filter_map() on line 902 in case we have nodes which were both changed and removed in the same update.
mrobinson
approved these changes
Jul 8, 2026
| } | ||
|
|
||
| for id in update | ||
| let mut ids_to_remove: Vec<NodeId> = update |
Member
There was a problem hiding this comment.
Suggested change
| let mut ids_to_remove: Vec<NodeId> = update | |
| let mut ids_to_remove: Vec<_> = update |
| } | ||
|
|
||
| // We should have resolved all damage in nodes still in the tree by this point, and any | ||
| // nodes not in the tree should have been removed from this map in the loop above. |
Member
There was a problem hiding this comment.
Suggested change
| // nodes not in the tree should have been removed from this map in the loop above. | |
| // nodes not in the tree should have been removed from this map in the loop above. |
alice
force-pushed
the
accessibility-subtree-moves
branch
from
July 9, 2026 13:34
4aeb8f7 to
60cbe13
Compare
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Jul 10, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to no response for status checks
Jul 10, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Jul 13, 2026
Instead of setting the TreeChange for each node in a removed subtree, just set the TreeChange for the subtree root. Also only root the subtree root in AccessibilityData. Signed-off-by: Alice Boxhall <alice@igalia.com>
- Avoid unnecessary borrow when recursively removing children of removed nodes - Avoid completely pointless check of empty map (!) - Comment why we can assert that `unresolved_local_damage` is empty at the end of `drop_removed_nodes()`, and make it an `assert!()`. Signed-off-by: Alice Boxhall <alice@igalia.com>
Signed-off-by: Alice Boxhall <alice@igalia.com>
alice
force-pushed
the
accessibility-subtree-moves
branch
from
July 14, 2026 09:35
60cbe13 to
3eb0c7a
Compare
Contributor
Author
|
Marked WPT test as flaky in #46497. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Instead of setting the TreeChange for each node in a removed subtree, just set the TreeChange for the subtree root.
Also only root the subtree root in AccessibilityData.
Testing: Adds a new test with more involved sequences of moving and removing nodes.
Fixes: #46347