LibWeb: Filter non-inheriting registered custom properties on inherit

When inheriting custom-property data from a parent element, we were
copying the parent's full CustomPropertyData regardless of whether
each property was registered with `inherits: false`. That caused
non-inheriting registered properties to leak from the parent,
contrary to the @property spec.

Wrap the parent-side lookup so we strip any custom property whose
registration says it should not inherit, and only build a fresh
CustomPropertyData when at least one property was actually filtered.

Key the filtered view's cache on both the destination document's
identity and its custom-property registration generation. The
generation counter is local to each document, so a subtree adopted
into another document (or queried via getComputedStyle from another
window) could otherwise pick up a cached view computed under an
unrelated registration set and silently skip non-inheriting filtering
in the new document.

Libraries/LibWeb/CSS/CSS.cpp

@@ -148,6 +148,7 @@ WebIDL::ExceptionOr<void> register_property(JS::VM& vm, PropertyDefinition defin
     };
     // Append registered property to property set.
     property_set.set(registered_property.property_name, registered_property);
+    document.did_change_custom_property_registrations();
 
     return {};
 }

Libraries/LibWeb/CSS/CustomPropertyData.cpp

@@ -6,6 +6,7 @@
 
 #include <LibWeb/CSS/CustomPropertyData.h>
 #include <LibWeb/CSS/StyleValues/StyleValue.h>
+#include <LibWeb/DOM/Document.h>
 
 namespace Web::CSS {
 
@@ -56,6 +57,53 @@ StyleProperty const* CustomPropertyData::get(FlyString const& name) const
     return nullptr;
 }
 
+RefPtr<CustomPropertyData const> CustomPropertyData::inheritable(DOM::Document const& document) const
+{
+    auto document_identity = reinterpret_cast<FlatPtr>(&document);
+    auto generation = document.custom_property_registration_generation();
+    if (m_cached_inheritable_document_identity == document_identity && m_cached_inheritable_generation == generation) {
+        if (m_cached_inheritable_is_self)
+            return RefPtr<CustomPropertyData const>(this);
+        return m_cached_inheritable_data;
+    }
+
+    RefPtr<CustomPropertyData const> inheritable_parent;
+    if (m_parent)
+        inheritable_parent = m_parent->inheritable(document);
+
+    OrderedHashMap<FlyString, StyleProperty> inheritable_own_values;
+    bool filtered_any_own_values = false;
+    for (auto const& [name, property] : m_own_values) {
+        auto registration = document.get_registered_custom_property(name);
+        if (registration.has_value() && !registration->inherit) {
+            filtered_any_own_values = true;
+            continue;
+        }
+        inheritable_own_values.set(name, property);
+    }
+
+    // Registration generations are local to each document, so the cache has to include the destination document
+    // identity as well. Otherwise a subtree adopted into another document can incorrectly reuse a filtered result
+    // that was computed under a different registration set.
+    m_cached_inheritable_document_identity = document_identity;
+    m_cached_inheritable_generation = generation;
+
+    if (!filtered_any_own_values && inheritable_parent.ptr() == m_parent.ptr()) {
+        m_cached_inheritable_data = nullptr;
+        m_cached_inheritable_is_self = true;
+        return RefPtr<CustomPropertyData const>(this);
+    }
+
+    m_cached_inheritable_is_self = false;
+    if (inheritable_own_values.is_empty() && !inheritable_parent) {
+        m_cached_inheritable_data = nullptr;
+        return nullptr;
+    }
+
+    m_cached_inheritable_data = CustomPropertyData::create(move(inheritable_own_values), move(inheritable_parent));
+    return m_cached_inheritable_data;
+}
+
 void CustomPropertyData::for_each_property(Function<void(FlyString const&, StyleProperty const&)> callback) const
 {
     HashTable<FlyString> seen;

Libraries/LibWeb/CSS/CustomPropertyData.h

@@ -8,8 +8,10 @@
 
 #include <AK/Function.h>
 #include <AK/HashMap.h>
+#include <AK/NumericLimits.h>
 #include <AK/RefCounted.h>
 #include <AK/RefPtr.h>
+#include <AK/Types.h>
 #include <LibWeb/CSS/StyleProperty.h>
 #include <LibWeb/Export.h>
 
@@ -25,6 +27,7 @@ public:
         RefPtr<CustomPropertyData const> parent);
 
     StyleProperty const* get(FlyString const& name) const;
+    RefPtr<CustomPropertyData const> inheritable(DOM::Document const&) const;
 
     OrderedHashMap<FlyString, StyleProperty> const& own_values() const { return m_own_values; }
 
@@ -40,6 +43,10 @@ private:
     OrderedHashMap<FlyString, StyleProperty> m_own_values;
     RefPtr<CustomPropertyData const> m_parent;
     u8 m_ancestor_count { 0 };
+    mutable FlatPtr m_cached_inheritable_document_identity { NumericLimits<FlatPtr>::max() };
+    mutable size_t m_cached_inheritable_generation { NumericLimits<size_t>::max() };
+    mutable RefPtr<CustomPropertyData const> m_cached_inheritable_data;
+    mutable bool m_cached_inheritable_is_self { false };
 };
 
 }

Libraries/LibWeb/CSS/CustomPropertyRegistration.h

@@ -8,6 +8,7 @@
 
 #include <AK/FlyString.h>
 #include <AK/RefPtr.h>
+#include <LibWeb/CSS/StyleValues/StyleValue.h>
 #include <LibWeb/Forward.h>
 
 namespace Web::CSS {
@@ -31,4 +32,19 @@ struct CustomPropertyRegistration {
     RefPtr<StyleValue const> initial_value;
 };
 
+inline bool operator==(CustomPropertyRegistration const& a, CustomPropertyRegistration const& b)
+{
+    if (a.property_name != b.property_name)
+        return false;
+    if (a.syntax != b.syntax)
+        return false;
+    if (a.inherit != b.inherit)
+        return false;
+    if (a.initial_value.ptr() == b.initial_value.ptr())
+        return true;
+    if (!a.initial_value || !b.initial_value)
+        return false;
+    return a.initial_value->equals(*b.initial_value);
+}
+
 }

Libraries/LibWeb/CSS/StyleComputer.cpp

@@ -591,6 +591,14 @@ static void cascade_custom_properties(DOM::AbstractElement abstract_element, Vec
     custom_properties.update(important_custom_properties);
 }
 
+static RefPtr<CustomPropertyData const> inheritable_custom_property_data(DOM::AbstractElement abstract_element)
+{
+    auto data = abstract_element.custom_property_data();
+    if (!data)
+        return nullptr;
+    return data->inheritable(abstract_element.document());
+}
+
 static Optional<CSS::EasingFunction> resolve_keyframe_easing(CSS::StyleValue const& style_value, DOM::AbstractElement abstract_element)
 {
     RefPtr<CSS::StyleValue const> resolved = style_value;
@@ -1855,7 +1863,7 @@ GC::Ptr<ComputedProperties> StyleComputer::compute_style_impl(DOM::AbstractEleme
         RefPtr<CustomPropertyData const> parent_data;
         auto inherit_from = abstract_element.element_to_inherit_style_from();
         if (inherit_from.has_value())
-            parent_data = inherit_from->custom_property_data();
+            parent_data = inheritable_custom_property_data(*inherit_from);
 
         // Build own_values with only properties that differ from the parent.
         // We build a fresh map instead of removing from cascaded_all,
@@ -2266,7 +2274,7 @@ void StyleComputer::compute_custom_properties(ComputedProperties&, DOM::Abstract
     // which would leave an oversized bucket array.
     RefPtr<CustomPropertyData const> parent_data;
     if (inherit_from.has_value())
-        parent_data = inherit_from->custom_property_data();
+        parent_data = inheritable_custom_property_data(*inherit_from);
 
     OrderedHashMap<FlyString, StyleProperty> resolved_own;
     for (auto const& [name, style_property] : data->own_values()) {

Libraries/LibWeb/DOM/Document.cpp

@@ -8128,15 +8128,30 @@ NonnullRefPtr<CSS::StyleValue const> Document::custom_property_initial_value(Fly
     return CSS::GuaranteedInvalidStyleValue::create();
 }
 
+void Document::did_change_custom_property_registrations()
+{
+    ++m_custom_property_registration_generation;
+
+    // Custom property registration changes can alter inheritance and initial values even when no selector matching
+    // changes. Registrations only move when a stylesheet containing an @property rule is added/removed or when
+    // CSS.registerProperty() is called, so a full document restyle is cheap enough in practice.
+    invalidate_style(DOM::StyleInvalidationReason::Other);
+}
+
 void Document::build_registered_properties_cache()
 {
-    m_cached_registered_properties_from_css_property_rules.clear_with_capacity();
+    HashMap<FlyString, CSS::CustomPropertyRegistration> cached_registered_properties_from_css_property_rules;
     for_each_active_css_style_sheet([&](CSS::CSSStyleSheet const& style_sheet) {
         style_sheet.for_each_effective_rule(TraversalOrder::Preorder, [&](CSS::CSSRule const& rule) {
             if (auto* property_rule = as_if<CSS::CSSPropertyRule>(rule))
-                m_cached_registered_properties_from_css_property_rules.set(property_rule->name(), property_rule->to_registration());
+                cached_registered_properties_from_css_property_rules.set(property_rule->name(), property_rule->to_registration());
         });
     });
+
+    if (cached_registered_properties_from_css_property_rules != m_cached_registered_properties_from_css_property_rules)
+        did_change_custom_property_registrations();
+
+    m_cached_registered_properties_from_css_property_rules = move(cached_registered_properties_from_css_property_rules);
 }
 
 void Document::ensure_cookie_version_index(URL::URL const& new_url, URL::URL const& old_url)

Libraries/LibWeb/DOM/Document.h

@@ -1065,6 +1065,8 @@ public:
     HashMap<FlyString, CSS::CustomPropertyRegistration>& registered_property_set();
     Optional<CSS::CustomPropertyRegistration const&> get_registered_custom_property(FlyString const& name) const;
     NonnullRefPtr<CSS::StyleValue const> custom_property_initial_value(FlyString const& name) const;
+    size_t custom_property_registration_generation() const { return m_custom_property_registration_generation; }
+    void did_change_custom_property_registrations();
 
     CSS::StyleScope const& style_scope() const { return m_style_scope; }
     CSS::StyleScope& style_scope() { return m_style_scope; }
@@ -1514,6 +1516,7 @@ private:
     // https://www.w3.org/TR/css-properties-values-api-1/#dom-window-registeredpropertyset-slot
     HashMap<FlyString, CSS::CustomPropertyRegistration> m_registered_property_set;
     HashMap<FlyString, CSS::CustomPropertyRegistration> m_cached_registered_properties_from_css_property_rules;
+    size_t m_custom_property_registration_generation { 0 };
 
     CSS::StyleScope m_style_scope;
 

Tests/LibWeb/Text/expected/css/custom-property-registration-document-adoption.txt

@@ -0,0 +1,3 @@
+before adopt color: rgb(255, 0, 0)
+after adopt color: rgb(0, 128, 0)
+after destination generation change color: rgb(0, 128, 0)

Tests/LibWeb/Text/expected/css/custom-property-registration-invalidation.txt

@@ -0,0 +1,5 @@
+before @property color: rgb(255, 0, 0)
+after @property color: rgb(0, 128, 0)
+after removing @property color: rgb(255, 0, 0)
+before registerProperty color: rgb(255, 0, 0)
+after registerProperty color: rgb(0, 0, 255)

Tests/LibWeb/Text/expected/wpt-import/css/css-properties-values-api/at-property.txt

@@ -2,8 +2,8 @@ Harness status: OK
 
 Found 106 tests
 
-102 Pass
-4 Fail
+103 Pass
+3 Fail
 Pass	Attribute 'syntax' returns expected value for ["<color>"]
 Pass	Attribute 'syntax' returns expected value for ["<color> | none"]
 Pass	Attribute 'syntax' returns expected value for ["<color># | <image> | none"]
@@ -109,4 +109,4 @@ Pass	Non-inherited properties do not inherit
 Pass	Inherited properties inherit
 Pass	Initial values substituted as computed value
 Pass	Non-universal registration are invalid without an initial value
-Fail	Initial value may be omitted for universal registration
+Pass	Initial value may be omitted for universal registration

Tests/LibWeb/Text/expected/wpt-import/css/css-values/inherit-function-invalidation.txt

@@ -2,8 +2,8 @@ Harness status: OK
 
 Found 3 tests
 
-1 Pass
-2 Fail
+2 Pass
+1 Fail
 Pass	inherit() invalidates when values changes on parent element, inherited
 Fail	inherit() invalidates when values changes on parent element, inherited, deeper
-Fail	inherit() invalidates when values changes on parent element, non-inherited
+Pass	inherit() invalidates when values changes on parent element, non-inherited

Tests/LibWeb/Text/input/css/custom-property-registration-document-adoption.html

@@ -0,0 +1,47 @@
+<!DOCTYPE html>
+<script src="../include.js"></script>
+<script>
+    asyncTest(async done => {
+        const parent = document.createElement("div");
+        parent.style.setProperty("--foo", "rgb(255, 0, 0)");
+
+        const target = document.createElement("div");
+        target.textContent = "target";
+        target.style.color = "var(--foo)";
+        parent.appendChild(target);
+        document.body.appendChild(parent);
+
+        // Give the source document a different registration set but the same
+        // generation count as the destination document. That way the test
+        // catches caches that key only on generation and ignore the document.
+        CSS.registerProperty({
+            name: "--source-only",
+            syntax: "*",
+            inherits: true,
+        });
+
+        println(`before adopt color: ${getComputedStyle(target).color}`);
+
+        const iframe = document.createElement("iframe");
+        document.body.appendChild(iframe);
+        iframe.contentWindow.CSS.registerProperty({
+            name: "--foo",
+            syntax: "<color>",
+            inherits: false,
+            initialValue: "rgb(0, 128, 0)",
+        });
+
+        iframe.contentDocument.adoptNode(parent);
+        iframe.contentDocument.body.appendChild(parent);
+
+        println(`after adopt color: ${iframe.contentWindow.getComputedStyle(target).color}`);
+
+        iframe.contentWindow.CSS.registerProperty({
+            name: "--destination-only",
+            syntax: "*",
+            inherits: true,
+        });
+        println(`after destination generation change color: ${iframe.contentWindow.getComputedStyle(target).color}`);
+        done();
+    });
+</script>

Tests/LibWeb/Text/input/css/custom-property-registration-invalidation.html

@@ -0,0 +1,35 @@
+<!DOCTYPE html>
+<script src="../include.js"></script>
+<style id="dynamic-style"></style>
+<div id="parent" style="--foo: rgb(255, 0, 0); --bar: rgb(255, 0, 0);">
+    <div id="foo-target" style="color: var(--foo);"></div>
+    <div id="bar-target" style="color: var(--bar);"></div>
+</div>
+<script>
+    test(() => {
+        const dynamicStyle = document.getElementById("dynamic-style");
+        const fooTarget = document.getElementById("foo-target");
+        const barTarget = document.getElementById("bar-target");
+
+        println(`before @property color: ${getComputedStyle(fooTarget).color}`);
+
+        // This changes only the inheritance semantics of --foo, so it proves
+        // we invalidate inherited custom-property data instead of reusing a
+        // stale cached inheritable view from before the registration existed.
+        dynamicStyle.sheet.insertRule("@property --foo { syntax: '<color>'; inherits: false; initial-value: rgb(0, 128, 0); }");
+        println(`after @property color: ${getComputedStyle(fooTarget).color}`);
+
+        dynamicStyle.sheet.deleteRule(0);
+        println(`after removing @property color: ${getComputedStyle(fooTarget).color}`);
+
+        println(`before registerProperty color: ${getComputedStyle(barTarget).color}`);
+
+        CSS.registerProperty({
+            name: "--bar",
+            syntax: "<color>",
+            inherits: false,
+            initialValue: "rgb(0, 0, 255)",
+        });
+        println(`after registerProperty color: ${getComputedStyle(barTarget).color}`);
+    });
+</script>