Don't explicitly restyle when updating <details> shadow tree (#36769)

I'm not sure why I added these calls in the first place, but they don't
seem to be necessary and seem to be the cause of crashes.

This PR also adds some spec comments that I added while investigating
the crash.


Testing: Covered by existing WPT tests
Fixes https://github.com/servo/servo/issues/36757
Fixes https://github.com/servo/servo/issues/36273

[try
run](https://github.com/simonwuelker/servo/actions/runs/14753023662/job/41415889180)

---------

Signed-off-by: Simon Wülker <simon.wuelker@arcor.de>
This commit is contained in:
Simon Wülker 2025-04-30 14:24:27 +02:00 committed by GitHub
parent af5d665efa
commit c98cf8b306
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 30 additions and 24 deletions

View file

@ -178,8 +178,6 @@ impl HTMLDetailsElement {
} }
} }
shadow_tree.descendants.Assign(slottable_children); shadow_tree.descendants.Assign(slottable_children);
self.upcast::<Node>().dirty(NodeDamage::OtherNodeDamage);
} }
fn update_shadow_tree_styles(&self, can_gc: CanGc) { fn update_shadow_tree_styles(&self, can_gc: CanGc) {
@ -214,8 +212,6 @@ impl HTMLDetailsElement {
.implicit_summary .implicit_summary
.upcast::<Element>() .upcast::<Element>()
.set_string_attribute(&local_name!("style"), implicit_summary_style.into(), can_gc); .set_string_attribute(&local_name!("style"), implicit_summary_style.into(), can_gc);
self.upcast::<Node>().dirty(NodeDamage::OtherNodeDamage);
} }
} }

View file

@ -1007,24 +1007,25 @@ impl Node {
/// <https://dom.spec.whatwg.org/#dom-childnode-replacewith> /// <https://dom.spec.whatwg.org/#dom-childnode-replacewith>
pub(crate) fn replace_with(&self, nodes: Vec<NodeOrString>, can_gc: CanGc) -> ErrorResult { pub(crate) fn replace_with(&self, nodes: Vec<NodeOrString>, can_gc: CanGc) -> ErrorResult {
// Step 1. // Step 1. Let parent be thiss parent.
let parent = if let Some(parent) = self.GetParentNode() { let Some(parent) = self.GetParentNode() else {
parent // Step 2. If parent is null, then return.
} else {
// Step 2.
return Ok(()); return Ok(());
}; };
// Step 3.
// Step 3. Let viableNextSibling be thiss first following sibling not in nodes; otherwise null.
let viable_next_sibling = first_node_not_in(self.following_siblings(), &nodes); let viable_next_sibling = first_node_not_in(self.following_siblings(), &nodes);
// Step 4.
// Step 4. Let node be the result of converting nodes into a node, given nodes and thiss node document.
let node = self let node = self
.owner_doc() .owner_doc()
.node_from_nodes_and_strings(nodes, can_gc)?; .node_from_nodes_and_strings(nodes, can_gc)?;
if self.parent_node == Some(&*parent) { if self.parent_node == Some(&*parent) {
// Step 5. // Step 5. If thiss parent is parent, replace this with node within parent.
parent.ReplaceChild(&node, self, can_gc)?; parent.ReplaceChild(&node, self, can_gc)?;
} else { } else {
// Step 6. // Step 6. Otherwise, pre-insert node into parent before viableNextSibling.
Node::pre_insert(&node, &parent, viable_next_sibling.as_deref(), can_gc)?; Node::pre_insert(&node, &parent, viable_next_sibling.as_deref(), can_gc)?;
} }
Ok(()) Ok(())
@ -3172,24 +3173,29 @@ impl NodeMethods<crate::DomTypeHolder> for Node {
/// <https://dom.spec.whatwg.org/#concept-node-replace> /// <https://dom.spec.whatwg.org/#concept-node-replace>
fn ReplaceChild(&self, node: &Node, child: &Node, can_gc: CanGc) -> Fallible<DomRoot<Node>> { fn ReplaceChild(&self, node: &Node, child: &Node, can_gc: CanGc) -> Fallible<DomRoot<Node>> {
// Step 1. // Step 1. If parent is not a Document, DocumentFragment, or Element node,
// then throw a "HierarchyRequestError" DOMException.
match self.type_id() { match self.type_id() {
NodeTypeId::Document(_) | NodeTypeId::DocumentFragment(_) | NodeTypeId::Element(..) => { NodeTypeId::Document(_) | NodeTypeId::DocumentFragment(_) | NodeTypeId::Element(..) => {
}, },
_ => return Err(Error::HierarchyRequest), _ => return Err(Error::HierarchyRequest),
} }
// Step 2. // Step 2. If node is a host-including inclusive ancestor of parent,
// then throw a "HierarchyRequestError" DOMException.
if node.is_inclusive_ancestor_of(self) { if node.is_inclusive_ancestor_of(self) {
return Err(Error::HierarchyRequest); return Err(Error::HierarchyRequest);
} }
// Step 3. // Step 3. If childs parent is not parent, then throw a "NotFoundError" DOMException.
if !self.is_parent_of(child) { if !self.is_parent_of(child) {
return Err(Error::NotFound); return Err(Error::NotFound);
} }
// Step 4-5. // Step 4. If node is not a DocumentFragment, DocumentType, Element, or CharacterData node,
// then throw a "HierarchyRequestError" DOMException.
// Step 5. If either node is a Text node and parent is a document,
// or node is a doctype and parent is not a document, then throw a "HierarchyRequestError" DOMException.
match node.type_id() { match node.type_id() {
NodeTypeId::CharacterData(CharacterDataTypeId::Text(_)) if self.is::<Document>() => { NodeTypeId::CharacterData(CharacterDataTypeId::Text(_)) if self.is::<Document>() => {
return Err(Error::HierarchyRequest); return Err(Error::HierarchyRequest);
@ -3201,7 +3207,8 @@ impl NodeMethods<crate::DomTypeHolder> for Node {
_ => (), _ => (),
} }
// Step 6. // Step 6. If parent is a document, and any of the statements below, switched on the interface node implements,
// are true, then throw a "HierarchyRequestError" DOMException.
if self.is::<Document>() { if self.is::<Document>() {
match node.type_id() { match node.type_id() {
// Step 6.1 // Step 6.1
@ -3255,7 +3262,8 @@ impl NodeMethods<crate::DomTypeHolder> for Node {
} }
} }
// Step 7-8. // Step 7. Let referenceChild be childs next sibling.
// Step 8. If referenceChild is node, then set referenceChild to nodes next sibling.
let child_next_sibling = child.GetNextSibling(); let child_next_sibling = child.GetNextSibling();
let node_next_sibling = node.GetNextSibling(); let node_next_sibling = node.GetNextSibling();
let reference_child = if child_next_sibling.as_deref() == Some(node) { let reference_child = if child_next_sibling.as_deref() == Some(node) {
@ -3264,7 +3272,7 @@ impl NodeMethods<crate::DomTypeHolder> for Node {
child_next_sibling.as_deref() child_next_sibling.as_deref()
}; };
// Step 9. // Step 9. Let previousSibling be childs previous sibling.
let previous_sibling = child.GetPreviousSibling(); let previous_sibling = child.GetPreviousSibling();
// NOTE: All existing browsers assume that adoption is performed here, which does not follow the DOM spec. // NOTE: All existing browsers assume that adoption is performed here, which does not follow the DOM spec.
@ -3285,7 +3293,7 @@ impl NodeMethods<crate::DomTypeHolder> for Node {
None None
}; };
// Step 12. // Step 12. Let nodes be nodes children if node is a DocumentFragment node; otherwise « node ».
rooted_vec!(let mut nodes); rooted_vec!(let mut nodes);
let nodes = if node.type_id() == let nodes = if node.type_id() ==
NodeTypeId::DocumentFragment(DocumentFragmentTypeId::DocumentFragment) || NodeTypeId::DocumentFragment(DocumentFragmentTypeId::DocumentFragment) ||
@ -3297,7 +3305,7 @@ impl NodeMethods<crate::DomTypeHolder> for Node {
from_ref(&node) from_ref(&node)
}; };
// Step 13. // Step 13. Insert node into parent before referenceChild with the suppress observers flag set.
Node::insert( Node::insert(
node, node,
self, self,
@ -3306,13 +3314,15 @@ impl NodeMethods<crate::DomTypeHolder> for Node {
can_gc, can_gc,
); );
// Step 14.
vtable_for(self).children_changed(&ChildrenMutation::replace( vtable_for(self).children_changed(&ChildrenMutation::replace(
previous_sibling.as_deref(), previous_sibling.as_deref(),
&removed_child, &removed_child,
nodes, nodes,
reference_child, reference_child,
)); ));
// Step 14. Queue a tree mutation record for parent with nodes, removedNodes,
// previousSibling, and referenceChild.
let removed = removed_child.map(|r| [r]); let removed = removed_child.map(|r| [r]);
let mutation = LazyCell::new(|| Mutation::ChildList { let mutation = LazyCell::new(|| Mutation::ChildList {
added: Some(nodes), added: Some(nodes),
@ -3323,7 +3333,7 @@ impl NodeMethods<crate::DomTypeHolder> for Node {
MutationObserver::queue_a_mutation_record(self, mutation); MutationObserver::queue_a_mutation_record(self, mutation);
// Step 15. // Step 15. Return child.
Ok(DomRoot::from_ref(child)) Ok(DomRoot::from_ref(child))
} }