LibWeb: Fix insert/delete rule invalidation for adopted style sheets

Invalidaiton for adopted style sheets was broken because we had an assumption that "active" style sheet is always attached to StyleSheetList which is not true for adopted style sheets. This change addresses that by keeping track of all documents/shadow roots that own a style sheet and notifying them about invalidation instead of going through the StyleSheetList.
LadybirdBrowser · Jan 10, 2025 · 9802d5a · 9802d5a
1 parent cf57efd
commit 9802d5a
Show file tree

Hide file tree

Showing 9 changed files with 105 additions and 44 deletions.
diff --git a/Libraries/LibWeb/CSS/CSSStyleRule.cpp b/Libraries/LibWeb/CSS/CSSStyleRule.cpp
@@ -139,10 +139,7 @@ void CSSStyleRule::set_selector_text(StringView selector_text)
 
         m_selectors = parsed_selectors.release_value();
         if (auto* sheet = parent_style_sheet()) {
-            if (auto style_sheet_list = sheet->style_sheet_list()) {
-                style_sheet_list->document().style_computer().invalidate_rule_cache();
-                style_sheet_list->document_or_shadow_root().invalidate_style(DOM::StyleInvalidationReason::SetSelectorText);
-            }
+            sheet->invalidate_owners(DOM::StyleInvalidationReason::SetSelectorText);
         }
     }
 

diff --git a/Libraries/LibWeb/CSS/CSSStyleSheet.cpp b/Libraries/LibWeb/CSS/CSSStyleSheet.cpp
@@ -120,13 +120,14 @@ void CSSStyleSheet::initialize(JS::Realm& realm)
 void CSSStyleSheet::visit_edges(Cell::Visitor& visitor)
 {
     Base::visit_edges(visitor);
-    visitor.visit(m_style_sheet_list);
     visitor.visit(m_rules);
     visitor.visit(m_owner_css_rule);
     visitor.visit(m_default_namespace_rule);
     visitor.visit(m_constructor_document);
     visitor.visit(m_namespace_rules);
     visitor.visit(m_import_rules);
+    for (auto& document_or_shadow_root : m_owning_documents_or_shadow_roots)
+        visitor.visit(document_or_shadow_root);
 }
 
 // https://www.w3.org/TR/cssom/#dom-cssstylesheet-insertrule
@@ -139,7 +140,7 @@ WebIDL::ExceptionOr<unsigned> CSSStyleSheet::insert_rule(StringView rule, unsign
         return WebIDL::NotAllowedError::create(realm(), "Can't call insert_rule() on non-modifiable stylesheets."_string);
 
     // 3. Let parsed rule be the return value of invoking parse a rule with rule.
-    auto context = m_style_sheet_list ? CSS::Parser::ParsingContext { m_style_sheet_list->document() } : CSS::Parser::ParsingContext { realm() };
+    auto context = !m_owning_documents_or_shadow_roots.is_empty() ? Parser::ParsingContext { (*m_owning_documents_or_shadow_roots.begin())->document() } : Parser::ParsingContext { realm() };
     auto parsed_rule = parse_css_rule(context, rule);
 
     // 4. If parsed rule is a syntax error, return parsed rule.
@@ -157,10 +158,7 @@ WebIDL::ExceptionOr<unsigned> CSSStyleSheet::insert_rule(StringView rule, unsign
         // NOTE: The spec doesn't say where to set the parent style sheet, so we'll do it here.
         parsed_rule->set_parent_style_sheet(this);
 
-        if (m_style_sheet_list) {
-            m_style_sheet_list->document().style_computer().invalidate_rule_cache();
-            m_style_sheet_list->document_or_shadow_root().invalidate_style(DOM::StyleInvalidationReason::StyleSheetInsertRule);
-        }
+        invalidate_owners(DOM::StyleInvalidationReason::StyleSheetInsertRule);
     }
 
     return result;
@@ -178,10 +176,7 @@ WebIDL::ExceptionOr<void> CSSStyleSheet::delete_rule(unsigned index)
     // 3. Remove a CSS rule in the CSS rules at index.
     auto result = m_rules->remove_a_css_rule(index);
     if (!result.is_exception()) {
-        if (m_style_sheet_list) {
-            m_style_sheet_list->document().style_computer().invalidate_rule_cache();
-            m_style_sheet_list->document_or_shadow_root().invalidate_style(DOM::StyleInvalidationReason::StyleSheetDeleteRule);
-        }
+        invalidate_owners(DOM::StyleInvalidationReason::StyleSheetDeleteRule);
     }
     return result;
 }
@@ -213,7 +208,7 @@ GC::Ref<WebIDL::Promise> CSSStyleSheet::replace(String text)
         HTML::TemporaryExecutionContext execution_context { realm, HTML::TemporaryExecutionContext::CallbacksEnabled::Yes };
 
         // 1. Let rules be the result of running parse a stylesheet’s contents from text.
-        auto context = m_style_sheet_list ? CSS::Parser::ParsingContext { m_style_sheet_list->document() } : CSS::Parser::ParsingContext { realm };
+        auto context = !m_owning_documents_or_shadow_roots.is_empty() ? Parser::ParsingContext { (*m_owning_documents_or_shadow_roots.begin())->document() } : CSS::Parser::ParsingContext { realm };
         auto* parsed_stylesheet = parse_css_stylesheet(context, text);
         auto& rules = parsed_stylesheet->rules();
 
@@ -247,7 +242,7 @@ WebIDL::ExceptionOr<void> CSSStyleSheet::replace_sync(StringView text)
         return WebIDL::NotAllowedError::create(realm(), "Can't call replaceSync() on non-modifiable stylesheets"_string);
 
     // 2. Let rules be the result of running parse a stylesheet’s contents from text.
-    auto context = m_style_sheet_list ? CSS::Parser::ParsingContext { m_style_sheet_list->document() } : CSS::Parser::ParsingContext { realm() };
+    auto context = !m_owning_documents_or_shadow_roots.is_empty() ? Parser::ParsingContext { (*m_owning_documents_or_shadow_roots.begin())->document() } : CSS::Parser::ParsingContext { realm() };
     auto* parsed_stylesheet = parse_css_stylesheet(context, text);
     auto& rules = parsed_stylesheet->rules();
 
@@ -321,6 +316,25 @@ void CSSStyleSheet::for_each_effective_keyframes_at_rule(Function<void(CSSKeyfra
     });
 }
 
+void CSSStyleSheet::add_owning_document_or_shadow_root(DOM::Node& document_or_shadow_root)
+{
+    VERIFY(document_or_shadow_root.is_document() || document_or_shadow_root.is_shadow_root());
+    m_owning_documents_or_shadow_roots.set(document_or_shadow_root);
+}
+
+void CSSStyleSheet::remove_owning_document_or_shadow_root(DOM::Node& document_or_shadow_root)
+{
+    m_owning_documents_or_shadow_roots.remove(document_or_shadow_root);
+}
+
+void CSSStyleSheet::invalidate_owners(DOM::StyleInvalidationReason reason)
+{
+    for (auto& document_or_shadow_root : m_owning_documents_or_shadow_roots) {
+        document_or_shadow_root->invalidate_style(reason);
+        document_or_shadow_root->document().style_computer().invalidate_rule_cache();
+    }
+}
+
 bool CSSStyleSheet::evaluate_media_queries(HTML::Window const& window)
 {
     bool any_media_queries_changed_match_state = false;
@@ -336,11 +350,6 @@ bool CSSStyleSheet::evaluate_media_queries(HTML::Window const& window)
     return any_media_queries_changed_match_state;
 }
 
-void CSSStyleSheet::set_style_sheet_list(Badge<StyleSheetList>, StyleSheetList* list)
-{
-    m_style_sheet_list = list;
-}
-
 Optional<FlyString> CSSStyleSheet::default_namespace() const
 {
     if (m_default_namespace_rule)

diff --git a/Libraries/LibWeb/CSS/CSSStyleSheet.h b/Libraries/LibWeb/CSS/CSSStyleSheet.h
@@ -13,6 +13,7 @@
 #include <LibWeb/CSS/CSSRuleList.h>
 #include <LibWeb/CSS/CSSStyleRule.h>
 #include <LibWeb/CSS/StyleSheet.h>
+#include <LibWeb/DOM/Node.h>
 #include <LibWeb/WebIDL/Types.h>
 
 namespace Web::CSS {
@@ -62,8 +63,9 @@ class CSSStyleSheet final : public StyleSheet {
     bool evaluate_media_queries(HTML::Window const&);
     void for_each_effective_keyframes_at_rule(Function<void(CSSKeyframesRule const&)> const& callback) const;
 
-    GC::Ptr<StyleSheetList> style_sheet_list() const { return m_style_sheet_list; }
-    void set_style_sheet_list(Badge<StyleSheetList>, StyleSheetList*);
+    void add_owning_document_or_shadow_root(DOM::Node& document_or_shadow_root);
+    void remove_owning_document_or_shadow_root(DOM::Node& document_or_shadow_root);
+    void invalidate_owners(DOM::StyleInvalidationReason);
 
     Optional<FlyString> default_namespace() const;
     GC::Ptr<CSSNamespaceRule> default_namespace_rule() const { return m_default_namespace_rule; }
@@ -109,11 +111,11 @@ class CSSStyleSheet final : public StyleSheet {
     HashMap<FlyString, GC::Ptr<CSSNamespaceRule>> m_namespace_rules;
     Vector<GC::Ref<CSSImportRule>> m_import_rules;
 
-    GC::Ptr<StyleSheetList> m_style_sheet_list;
     GC::Ptr<CSSRule> m_owner_css_rule;
 
     Optional<URL::URL> m_base_url;
     GC::Ptr<DOM::Document const> m_constructor_document;
+    HashTable<GC::Ptr<DOM::Node>> m_owning_documents_or_shadow_roots;
     bool m_constructed { false };
     bool m_disallow_modification { false };
     Optional<bool> m_did_match;

diff --git a/Libraries/LibWeb/CSS/StyleSheetList.cpp b/Libraries/LibWeb/CSS/StyleSheetList.cpp
@@ -81,7 +81,7 @@ void StyleSheetList::create_a_css_style_sheet(String type, DOM::Element* owner_n
 
 void StyleSheetList::add_sheet(CSSStyleSheet& sheet)
 {
-    sheet.set_style_sheet_list({}, this);
+    sheet.add_owning_document_or_shadow_root(document_or_shadow_root());
 
     if (m_sheets.is_empty()) {
         // This is the first sheet, append it to the list.
@@ -114,7 +114,7 @@ void StyleSheetList::add_sheet(CSSStyleSheet& sheet)
 
 void StyleSheetList::remove_sheet(CSSStyleSheet& sheet)
 {
-    sheet.set_style_sheet_list({}, nullptr);
+    sheet.remove_owning_document_or_shadow_root(document_or_shadow_root());
     bool did_remove = m_sheets.remove_first_matching([&](auto& entry) { return entry.ptr() == &sheet; });
     VERIFY(did_remove);
 

diff --git a/Libraries/LibWeb/DOM/AdoptedStyleSheets.cpp b/Libraries/LibWeb/DOM/AdoptedStyleSheets.cpp
@@ -10,11 +10,11 @@
 
 namespace Web::DOM {
 
-GC::Ref<WebIDL::ObservableArray> create_adopted_style_sheets_list(Document& document)
+GC::Ref<WebIDL::ObservableArray> create_adopted_style_sheets_list(Node& document_or_shadow_root)
 {
-    auto adopted_style_sheets = WebIDL::ObservableArray::create(document.realm());
-    adopted_style_sheets->set_on_set_an_indexed_value_callback([&document](JS::Value& value) -> WebIDL::ExceptionOr<void> {
-        auto& vm = document.vm();
+    auto adopted_style_sheets = WebIDL::ObservableArray::create(document_or_shadow_root.realm());
+    adopted_style_sheets->set_on_set_an_indexed_value_callback([&document_or_shadow_root](JS::Value& value) -> WebIDL::ExceptionOr<void> {
+        auto& vm = document_or_shadow_root.vm();
         if (!value.is_object())
             return vm.throw_completion<JS::TypeError>(JS::ErrorType::NotAnObjectOfType, "CSSStyleSheet");
         auto& object = value.as_object();
@@ -26,24 +26,26 @@ GC::Ref<WebIDL::ObservableArray> create_adopted_style_sheets_list(Document& docu
         // 1. If value’s constructed flag is not set, or its constructor document is not equal to this
         //    DocumentOrShadowRoot's node document, throw a "NotAllowedError" DOMException.
         if (!style_sheet.constructed())
-            return WebIDL::NotAllowedError::create(document.realm(), "StyleSheet's constructed flag is not set."_string);
-        if (!style_sheet.constructed() || style_sheet.constructor_document().ptr() != &document)
-            return WebIDL::NotAllowedError::create(document.realm(), "Sharing a StyleSheet between documents is not allowed."_string);
-
-        document.style_computer().load_fonts_from_sheet(style_sheet);
-        document.style_computer().invalidate_rule_cache();
-        document.invalidate_style(DOM::StyleInvalidationReason::AdoptedStyleSheetsList);
+            return WebIDL::NotAllowedError::create(document_or_shadow_root.realm(), "StyleSheet's constructed flag is not set."_string);
+        if (!style_sheet.constructed() || style_sheet.constructor_document().ptr() != &document_or_shadow_root.document())
+            return WebIDL::NotAllowedError::create(document_or_shadow_root.realm(), "Sharing a StyleSheet between documents is not allowed."_string);
+
+        style_sheet.add_owning_document_or_shadow_root(document_or_shadow_root);
+        document_or_shadow_root.document().style_computer().load_fonts_from_sheet(style_sheet);
+        document_or_shadow_root.document().style_computer().invalidate_rule_cache();
+        document_or_shadow_root.invalidate_style(DOM::StyleInvalidationReason::AdoptedStyleSheetsList);
         return {};
     });
-    adopted_style_sheets->set_on_delete_an_indexed_value_callback([&document](JS::Value value) -> WebIDL::ExceptionOr<void> {
+    adopted_style_sheets->set_on_delete_an_indexed_value_callback([&document_or_shadow_root](JS::Value value) -> WebIDL::ExceptionOr<void> {
         VERIFY(value.is_object());
         auto& object = value.as_object();
         VERIFY(is<CSS::CSSStyleSheet>(object));
         auto& style_sheet = static_cast<CSS::CSSStyleSheet&>(object);
 
-        document.style_computer().unload_fonts_from_sheet(style_sheet);
-        document.style_computer().invalidate_rule_cache();
-        document.invalidate_style(DOM::StyleInvalidationReason::AdoptedStyleSheetsList);
+        style_sheet.remove_owning_document_or_shadow_root(document_or_shadow_root);
+        document_or_shadow_root.document().style_computer().unload_fonts_from_sheet(style_sheet);
+        document_or_shadow_root.document().style_computer().invalidate_rule_cache();
+        document_or_shadow_root.invalidate_style(DOM::StyleInvalidationReason::AdoptedStyleSheetsList);
         return {};
     });
 

diff --git a/Libraries/LibWeb/DOM/AdoptedStyleSheets.h b/Libraries/LibWeb/DOM/AdoptedStyleSheets.h
@@ -11,6 +11,6 @@
 
 namespace Web::DOM {
 
-GC::Ref<WebIDL::ObservableArray> create_adopted_style_sheets_list(Document& document);
+GC::Ref<WebIDL::ObservableArray> create_adopted_style_sheets_list(Node& document_or_shadow_root);
 
 }
diff --git a/Libraries/LibWeb/DOM/ShadowRoot.cpp b/Libraries/LibWeb/DOM/ShadowRoot.cpp
@@ -141,14 +141,14 @@ void ShadowRoot::visit_edges(Visitor& visitor)
 GC::Ref<WebIDL::ObservableArray> ShadowRoot::adopted_style_sheets() const
 {
     if (!m_adopted_style_sheets)
-        m_adopted_style_sheets = create_adopted_style_sheets_list(const_cast<Document&>(document()));
+        m_adopted_style_sheets = create_adopted_style_sheets_list(const_cast<ShadowRoot&>(*this));
     return *m_adopted_style_sheets;
 }
 
 WebIDL::ExceptionOr<void> ShadowRoot::set_adopted_style_sheets(JS::Value new_value)
 {
     if (!m_adopted_style_sheets)
-        m_adopted_style_sheets = create_adopted_style_sheets_list(const_cast<Document&>(document()));
+        m_adopted_style_sheets = create_adopted_style_sheets_list(*this);
 
     m_adopted_style_sheets->clear();
     auto iterator_record = TRY(get_iterator(vm(), new_value, JS::IteratorHint::Sync));

diff --git a/Tests/LibWeb/Text/expected/css/insert-rule-in-adopted-style-sheet.txt b/Tests/LibWeb/Text/expected/css/insert-rule-in-adopted-style-sheet.txt
@@ -0,0 +1,4 @@
+color of box 1 before rule insertion: rgb(255, 0, 0)
+color of box 2 before rule insertion: rgb(255, 0, 0)
+color of box 1 after rule insertion: rgb(0, 128, 0)
+color of box 2 after rule insertion: rgb(0, 128, 0)
diff --git a/Tests/LibWeb/Text/input/css/insert-rule-in-adopted-style-sheet.html b/Tests/LibWeb/Text/input/css/insert-rule-in-adopted-style-sheet.html
@@ -0,0 +1,47 @@
+<!DOCTYPE html>
+<script src="../include.js"></script>
+<my-shadow-box id="box1"></my-shadow-box>
+<my-shadow-box id="box2"></my-shadow-box>
+<script>
+    const myStyleSheet = new CSSStyleSheet();
+
+    myStyleSheet.replaceSync(`
+      .box {
+        width: 100px;
+        height: 100px;
+        background-color: red;
+      }
+    `);
+
+    class MyShadowBox extends HTMLElement {
+        constructor() {
+            super();
+
+            const shadowRoot = this.attachShadow({ mode: 'open' });
+            shadowRoot.adoptedStyleSheets = [myStyleSheet];
+
+            this.box = document.createElement('div');
+            this.box.classList.add('box');
+
+            shadowRoot.appendChild(this.box);
+        }
+
+        getBoxColor() {
+            return getComputedStyle(this.box).backgroundColor;
+        }
+    }
+
+
+    test(() => {
+        customElements.define('my-shadow-box', MyShadowBox);
+
+        println("color of box 1 before rule insertion: " + box1.getBoxColor());
+        println("color of box 2 before rule insertion: " + box2.getBoxColor());
+        myStyleSheet.insertRule(
+            '.box { background-color: green; }',
+            myStyleSheet.cssRules.length
+        );
+        println("color of box 1 after rule insertion: " + box1.getBoxColor());
+        println("color of box 2 after rule insertion: " + box2.getBoxColor());
+    });
+</script>