layout: Accessibility code cleanups - #46691
Conversation
mukilan
left a comment
There was a problem hiding this comment.
There are few minor issues in the doc comments, but looks good to me!
| update.unresolved_local_damage.remove(&self.id); | ||
| } | ||
|
|
||
| /// Update the given AccessibilityNode from its corresponding DOM node and |
There was a problem hiding this comment.
| /// Update the given AccessibilityNode from its corresponding DOM node and | |
| /// Update the given [`AccessibilityNode`] from its corresponding DOM node and |
There was a problem hiding this comment.
This is not changed in this PR, but the comment is outdated.
| /// [`LocalAccessibilityDamage::SubtreeChanged`]. |
| } | ||
|
|
||
| /// Update the given AccessibilityNode from its corresponding DOM node and | ||
| /// ['AccessibilityDamage']. |
There was a problem hiding this comment.
This is currently not a valid link since we are not using backticks.
| /// ['AccessibilityDamage']. | |
| /// [`AccessibilityDamage`]. |
|
|
||
| // Update self.child_nodes in place. | ||
| self.child_nodes.push(new_child.clone()); | ||
| self.child_nodes.push(strong_child.clone()); |
There was a problem hiding this comment.
The clone is not necessary as this is the last use of strong_child.
| self.child_nodes.push(strong_child.clone()); | |
| self.child_nodes.push(strong_child); |
There was a problem hiding this comment.
I moved this call earlier, so it still needs a .clone(), but I removed an extra clone() by not making a copy of new_child.
| // same position as previously. | ||
| let weak_self = ref_self.downgrade(); | ||
| for dom_child in remaining_dom_children { | ||
| let (new_id, new_child) = tree.get_or_create_node(&dom_child, update); |
There was a problem hiding this comment.
This is not something related to this PR. I am wondering if it makes sense to pass the parent handle to get_or_create_node here, rather than set it at the end of the loop. get_or_create_node already borrows the node to set the tag name, so potentially we could avoid an additional borrow if the node is not new.
There was a problem hiding this comment.
I like this idea, but I think we need to give it some more thought since we also call update_node_from_dom_node() on the new child, and it might be a bit odd to move that into get_or_create_node() since it's potentially recursive.
That also makes me reconsider my renaming of that method here!
There was a problem hiding this comment.
Oh, I meant we can move only the new_child.parent_node = Some(weak_self.clone()) part, which is currently at the end of the loop, into get_or_create_node. I agree that moving update_node_from_dom_node might not be a good idea.
There was a problem hiding this comment.
How would that avoid a borrow? We need to set the parent on the new child whether it's newly-created or not, so we need to borrow it somewhere either way. We need to borrow the newly-created child to call update_node_and_populate_new_descendants_from_dom_node() here, so we can't only borrow it in get_or_create_node() unless we also move that call in there as well.
There was a problem hiding this comment.
I was referring to the additional borrow in the case of an existing node, not the newly-created node. For the former, if we set the parent in get_or_create_node, then we don't need to borrow again in this method.
There was a problem hiding this comment.
Looks like in the recent push get_or_create_node we only borrow the node for newly-created case, so I guess that now addresses the extra borrow for existing nodes.
There was a problem hiding this comment.
Ohhh now I finally get what you were suggesting 🤦 sorry for being obtuse!
- Move `resolve_local_damage_for_node_and_subtree()` and `update_node_and_descendants_from_dom_node()` on to `AccessibilityNode`; - Rename `update_node_from_dom_node()` to `update_properties_from_dom_node()`; - Rename `update_node_and_descendants_from_dom_node()` to `update_node_from_dom_node()`; - Rename `update_descendants_from_dom_node()` to `update_children_from_dom_node()`, and change the `weak_self` argument to be an `ArcRefCell`, only downgrading if necessary; - Add an `ancestors()` iterator on `AccessibilityNode`; - Make `AccessibilityNode::children()` return an Iterator; - Make `test_accessibility_partial_subtree_move_and_delete()` less confusing by using more different types of elements. Signed-off-by: Alice Boxhall <alice@igalia.com>
Review comments: - Fix out-of-date references to `LocalDamage`; - Fix broken doc comment references. Extra changes: - Rename counter names to match the names in `ReflowStatistics` and `ServoTestUtils`; - Rename `update_node_from_dom_node()` to `update_node_and_populate_new_descendants_from_dom_node()`; - Rename `update_children_from_dom_node()` to `update_children_and_populate_new_descendants_from_dom_node()`. Signed-off-by: Alice Boxhall <alice@igalia.com>
19410ce to
c57150c
Compare
resolve_local_damage_for_node_and_subtree()andupdate_node_and_descendants_from_dom_node()on toAccessibilityNode;update_node_from_dom_node()toupdate_properties_from_dom_node();update_node_and_descendants_from_dom_node()toupdate_node_and_populate_new_descendants_from_dom_node();update_descendants_from_dom_node()toupdate_children_and_populate_new_descendants_from_dom_node(), and change theweak_selfargument to be anArcRefCell, only downgrading if necessary;ancestors()iterator onAccessibilityNode;AccessibilityNode::children()return an Iterator;test_accessibility_partial_subtree_move_and_delete()less confusing by using more different types of elements;UpdateCountersto match their corresponding values inReflowStatisticsandServoTestUtils.Testing: No behaviour changes.
Fixes: part of #4344