Skip to content

Commit

Permalink
LibWeb: Let style elements remember which StyleSheetList they live in
Browse files Browse the repository at this point in the history
Instead of trying to locate the relevant StyleSheetList on style element
removal from the DOM, we now simply keep a pointer to the list instead.

This fixes an issue where using attachShadow() on an element that had
a declarative shadow DOM would cause any style elements present to use
the wrong StyleSheetList when removing themselves from the tree.
  • Loading branch information
awesomekling committed Sep 21, 2024
1 parent 99e82b8 commit f812d5d
Show file tree
Hide file tree
Showing 6 changed files with 30 additions and 25 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
PASS (didn't crash)
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<div id="hmmst"><template shadowrootmode="open"><div><style></style></div></template></div>
<script src="../include.js"></script>
<script>
test(() => {
hmmst.attachShadow({ mode: "open" });
println("PASS (didn't crash)");
hmmst.remove();
});
</script>
29 changes: 11 additions & 18 deletions Userland/Libraries/LibWeb/DOM/StyleElementUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,6 @@

namespace Web::DOM {

static CSS::StyleSheetList& relevant_style_sheet_list_for_node(DOM::Node& node)
{
auto& root_node = node.root();
if (is<DOM::ShadowRoot>(root_node))
return static_cast<DOM::ShadowRoot&>(root_node).style_sheets();

return node.document().style_sheets();
}

// The user agent must run the "update a style block" algorithm whenever one of the following conditions occur:
// FIXME: The element is popped off the stack of open elements of an HTML parser or XML parser.
//
Expand All @@ -32,7 +23,7 @@ static CSS::StyleSheetList& relevant_style_sheet_list_for_node(DOM::Node& node)
// The element is not on the stack of open elements of an HTML parser or XML parser, and it becomes connected or disconnected.
//
// https://html.spec.whatwg.org/multipage/semantics.html#update-a-style-block
void StyleElementUtils::update_a_style_block(DOM::Element& style_element, JS::GCPtr<DOM::Node> old_parent_if_removed_from)
void StyleElementUtils::update_a_style_block(DOM::Element& style_element)
{
// OPTIMIZATION: Skip parsing CSS if we're in the middle of parsing a HTML fragment.
// The style block will be parsed upon insertion into a proper document.
Expand All @@ -43,13 +34,8 @@ void StyleElementUtils::update_a_style_block(DOM::Element& style_element, JS::GC
// 2. If element has an associated CSS style sheet, remove the CSS style sheet in question.

if (m_associated_css_style_sheet) {
// NOTE: If we're here in response to a node being removed from the tree, we need to remove the stylesheet from the style scope
// of the old parent, not the style scope of the node itself, since it's too late to find it that way!
if (old_parent_if_removed_from) {
relevant_style_sheet_list_for_node(*old_parent_if_removed_from).remove_a_css_style_sheet(*m_associated_css_style_sheet);
} else {
style_element.document_or_shadow_root_style_sheets().remove_a_css_style_sheet(*m_associated_css_style_sheet);
}
m_style_sheet_list->remove_a_css_style_sheet(*m_associated_css_style_sheet);
m_style_sheet_list = nullptr;

// FIXME: This should probably be handled by StyleSheet::set_owner_node().
m_associated_css_style_sheet = nullptr;
Expand All @@ -76,7 +62,8 @@ void StyleElementUtils::update_a_style_block(DOM::Element& style_element, JS::GC
m_associated_css_style_sheet = sheet;

// 6. Create a CSS style sheet with the following properties...
style_element.document_or_shadow_root_style_sheets().create_a_css_style_sheet(
m_style_sheet_list = style_element.document_or_shadow_root_style_sheets();
m_style_sheet_list->create_a_css_style_sheet(
"text/css"_string,
&style_element,
style_element.attribute(HTML::AttributeNames::media).value_or({}),
Expand All @@ -91,4 +78,10 @@ void StyleElementUtils::update_a_style_block(DOM::Element& style_element, JS::GC
*sheet);
}

void StyleElementUtils::visit_edges(JS::Cell::Visitor& visitor)
{
visitor.visit(m_associated_css_style_sheet);
visitor.visit(m_style_sheet_list);
}

}
12 changes: 7 additions & 5 deletions Userland/Libraries/LibWeb/DOM/StyleElementUtils.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,19 +14,21 @@ namespace Web::DOM {

class StyleElementUtils {
public:
void update_a_style_block(DOM::Element& style_element, JS::GCPtr<DOM::Node> old_parent_if_removed_from = nullptr);
void update_a_style_block(DOM::Element& style_element);

CSS::CSSStyleSheet* sheet() { return m_associated_css_style_sheet; }
CSS::CSSStyleSheet const* sheet() const { return m_associated_css_style_sheet; }

void visit_edges(JS::Cell::Visitor& visitor)
{
visitor.visit(m_associated_css_style_sheet);
}
[[nodiscard]] JS::GCPtr<CSS::StyleSheetList> style_sheet_list() { return m_style_sheet_list; }
[[nodiscard]] JS::GCPtr<CSS::StyleSheetList const> style_sheet_list() const { return m_style_sheet_list; }

void visit_edges(JS::Cell::Visitor&);

private:
// https://www.w3.org/TR/cssom/#associated-css-style-sheet
JS::GCPtr<CSS::CSSStyleSheet> m_associated_css_style_sheet;

JS::GCPtr<CSS::StyleSheetList> m_style_sheet_list;
};

}
2 changes: 1 addition & 1 deletion Userland/Libraries/LibWeb/HTML/HTMLStyleElement.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ void HTMLStyleElement::inserted()

void HTMLStyleElement::removed_from(Node* old_parent)
{
m_style_element_utils.update_a_style_block(*this, old_parent);
m_style_element_utils.update_a_style_block(*this);
Base::removed_from(old_parent);
}

Expand Down
2 changes: 1 addition & 1 deletion Userland/Libraries/LibWeb/SVG/SVGStyleElement.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ void SVGStyleElement::inserted()

void SVGStyleElement::removed_from(Node* old_parent)
{
m_style_element_utils.update_a_style_block(*this, old_parent);
m_style_element_utils.update_a_style_block(*this);
Base::removed_from(old_parent);
}

Expand Down

0 comments on commit f812d5d

Please sign in to comment.