LibWeb: Implement CSSStyleDeclaration.{set,remove}Property close to spec

We already had setProperty() but it was full of ad-hoc idiosyncracies.
This patch aligns setProperty() with the CSSOM spec and also implements
removeProperty() since that's actually needed by setProperty() now.

Some things fixed by this:
- We now support the "priority" parameter to setProperty()
- Element "style" attributes now update to reflect CSSOM mutations

Userland/Libraries/LibWeb/CSS/CSSStyleDeclaration.cpp

@@ -43,35 +43,113 @@ Optional<StyleProperty> PropertyOwningCSSStyleDeclaration::property(PropertyID p
     return {};
 }
 
-bool PropertyOwningCSSStyleDeclaration::set_property(PropertyID property_id, StringView css_text)
-{
-    auto new_value = parse_css_value(CSS::ParsingContext {}, css_text, property_id);
-    if (!new_value) {
-        m_properties.remove_all_matching([&](auto& entry) {
-            return entry.property_id == property_id;
-        });
-        return false;
+// https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-setproperty
+DOM::ExceptionOr<void> PropertyOwningCSSStyleDeclaration::set_property(PropertyID property_id, StringView value, StringView priority)
+{
+    // 1. If the computed flag is set, then throw a NoModificationAllowedError exception.
+    // NOTE: This is handled by the virtual override in ResolvedCSSStyleDeclaration.
+
+    // FIXME: 2. If property is not a custom property, follow these substeps:
+    // FIXME:    1. Let property be property converted to ASCII lowercase.
+    // FIXME:    2. If property is not a case-sensitive match for a supported CSS property, then return.
+    // NOTE: This must be handled before we've turned the property string into a PropertyID.
+
+    // 3. If value is the empty string, invoke removeProperty() with property as argument and return.
+    if (value.is_empty()) {
+        MUST(remove_property(property_id));
+        return {};
     }
 
-    ScopeGuard style_invalidation_guard = [&] {
-        auto& declaration = verify_cast<CSS::ElementInlineCSSStyleDeclaration>(*this);
-        if (auto* element = declaration.element())
-            element->invalidate_style();
-    };
+    // 4. If priority is not the empty string and is not an ASCII case-insensitive match for the string "important", then return.
+    if (!priority.is_empty() && !priority.equals_ignoring_case("important"sv))
+        return {};
+
+    // 5. Let component value list be the result of parsing value for property property.
+    auto component_value_list = parse_css_value(CSS::ParsingContext {}, value, property_id);
+
+    // 6. If component value list is null, then return.
+    if (!component_value_list)
+        return {};
+
+    // 7. Let updated be false.
+    bool updated = false;
+
+    // FIXME: 8. If property is a shorthand property, then for each longhand property longhand that property maps to, in canonical order, follow these substeps:
+    // FIXME:    1. Let longhand result be the result of set the CSS declaration longhand with the appropriate value(s) from component value list,
+    //              with the important flag set if priority is not the empty string, and unset otherwise, and with the list of declarations being the declarations.
+    // FIXME:    2. If longhand result is true, let updated be true.
+
+    // 9. Otherwise, let updated be the result of set the CSS declaration property with value component value list,
+    //    with the important flag set if priority is not the empty string, and unset otherwise,
+    //    and with the list of declarations being the declarations.
+    updated = set_a_css_declaration(property_id, component_value_list.release_nonnull(), !priority.is_empty() ? Important::Yes : Important::No);
+
+    // 10. If updated is true, update style attribute for the CSS declaration block.
+    if (updated)
+        update_style_attribute();
+
+    return {};
+}
+
+// https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-removeproperty
+DOM::ExceptionOr<String> PropertyOwningCSSStyleDeclaration::remove_property(PropertyID property_id)
+{
+    // 1. If the computed flag is set, then throw a NoModificationAllowedError exception.
+    // NOTE: This is handled by the virtual override in ResolvedCSSStyleDeclaration.
+
+    // 2. If property is not a custom property, let property be property converted to ASCII lowercase.
+    // NOTE: We've already converted it to a PropertyID enum value.
+
+    // 3. Let value be the return value of invoking getPropertyValue() with property as argument.
+    // FIXME: The trip through string_from_property_id() here is silly.
+    auto value = get_property_value(string_from_property_id(property_id));
 
-    // FIXME: I don't think '!important' is being handled correctly here..
+    // 4. Let removed be false.
+    bool removed = false;
+
+    // FIXME: 5. If property is a shorthand property, for each longhand property longhand that property maps to:
+    //           1. If longhand is not a property name of a CSS declaration in the declarations, continue.
+    //           2. Remove that CSS declaration and let removed be true.
+
+    // 6. Otherwise, if property is a case-sensitive match for a property name of a CSS declaration in the declarations, remove that CSS declaration and let removed be true.
+    removed = m_properties.remove_first_matching([&](auto& entry) { return entry.property_id == property_id; });
+
+    // 7. If removed is true, Update style attribute for the CSS declaration block.
+    if (removed)
+        update_style_attribute();
+
+    // 8. Return value.
+    return value;
+}
+
+// https://drafts.csswg.org/cssom/#update-style-attribute-for
+void ElementInlineCSSStyleDeclaration::update_style_attribute()
+{
+    if (!m_element)
+        return;
+
+    m_element->set_attribute(HTML::AttributeNames::style, serialized());
+}
+
+// https://drafts.csswg.org/cssom/#set-a-css-declaration
+bool PropertyOwningCSSStyleDeclaration::set_a_css_declaration(PropertyID property_id, NonnullRefPtr<StyleValue> value, Important important)
+{
+    // FIXME: Handle logical property groups.
 
     for (auto& property : m_properties) {
         if (property.property_id == property_id) {
-            property.value = new_value.release_nonnull();
+            if (property.important == important && *property.value == *value)
+                return false;
+            property.value = move(value);
+            property.important = important;
             return true;
         }
     }
 
     m_properties.append(CSS::StyleProperty {
-        .important = Important::No,
+        .important = important,
         .property_id = property_id,
-        .value = new_value.release_nonnull(),
+        .value = move(value),
     });
     return true;
 }
@@ -87,12 +165,20 @@ String CSSStyleDeclaration::get_property_value(StringView property_name) const
     return maybe_property->value->to_string();
 }
 
-void CSSStyleDeclaration::set_property(StringView property_name, StringView css_text)
+DOM::ExceptionOr<void> CSSStyleDeclaration::set_property(StringView property_name, StringView css_text, StringView priority)
 {
     auto property_id = property_id_from_string(property_name);
     if (property_id == CSS::PropertyID::Invalid)
-        return;
-    set_property(property_id, css_text);
+        return {};
+    return set_property(property_id, css_text, priority);
+}
+
+DOM::ExceptionOr<String> CSSStyleDeclaration::remove_property(StringView property_name)
+{
+    auto property_id = property_id_from_string(property_name);
+    if (property_id == CSS::PropertyID::Invalid)
+        return String::empty();
+    return remove_property(property_id);
 }
 
 String CSSStyleDeclaration::css_text() const

Userland/Libraries/LibWeb/CSS/CSSStyleDeclaration.h

@@ -37,9 +37,12 @@ public:
     virtual String item(size_t index) const = 0;
 
     virtual Optional<StyleProperty> property(PropertyID) const = 0;
-    virtual bool set_property(PropertyID, StringView css_text) = 0;
 
-    void set_property(StringView property_name, StringView css_text);
+    virtual DOM::ExceptionOr<void> set_property(PropertyID, StringView css_text, StringView priority = "") = 0;
+    virtual DOM::ExceptionOr<String> remove_property(PropertyID) = 0;
+
+    DOM::ExceptionOr<void> set_property(StringView property_name, StringView css_text, StringView priority);
+    DOM::ExceptionOr<String> remove_property(StringView property_name);
 
     String get_property_value(StringView property) const;
 
@@ -67,7 +70,9 @@ public:
     virtual String item(size_t index) const override;
 
     virtual Optional<StyleProperty> property(PropertyID) const override;
-    virtual bool set_property(PropertyID, StringView css_text) override;
+
+    virtual DOM::ExceptionOr<void> set_property(PropertyID, StringView css_text, StringView priority) override;
+    virtual DOM::ExceptionOr<String> remove_property(PropertyID) override;
 
     Vector<StyleProperty> const& properties() const { return m_properties; }
     HashMap<String, StyleProperty> const& custom_properties() const { return m_custom_properties; }
@@ -79,7 +84,11 @@ public:
 protected:
     explicit PropertyOwningCSSStyleDeclaration(Vector<StyleProperty>, HashMap<String, StyleProperty>);
 
+    virtual void update_style_attribute() { }
+
 private:
+    bool set_a_css_declaration(PropertyID, NonnullRefPtr<StyleValue>, Important);
+
     Vector<StyleProperty> m_properties;
     HashMap<String, StyleProperty> m_custom_properties;
 };
@@ -95,6 +104,8 @@ public:
 private:
     explicit ElementInlineCSSStyleDeclaration(DOM::Element&, Vector<StyleProperty> properties, HashMap<String, StyleProperty> custom_properties);
 
+    virtual void update_style_attribute() override;
+
     WeakPtr<DOM::Element> m_element;
 };
 

Userland/Libraries/LibWeb/CSS/CSSStyleDeclaration.idl

@@ -6,6 +6,7 @@ interface CSSStyleDeclaration {
 
     CSSOMString getPropertyValue(CSSOMString property);
 
-    [CEReactions] undefined setProperty(CSSOMString property, [LegacyNullToEmptyString] CSSOMString value);
+    [CEReactions] undefined setProperty(CSSOMString property, [LegacyNullToEmptyString] CSSOMString value, [LegacyNullToEmptyString] optional CSSOMString priority = "");
+    [CEReactions] CSSOMString removeProperty(CSSOMString property);
 
 };

Userland/Libraries/LibWeb/CSS/ResolvedCSSStyleDeclaration.cpp

@@ -800,9 +800,18 @@ Optional<StyleProperty> ResolvedCSSStyleDeclaration::property(PropertyID propert
     };
 }
 
-bool ResolvedCSSStyleDeclaration::set_property(PropertyID, StringView)
+// https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-setproperty
+DOM::ExceptionOr<void> ResolvedCSSStyleDeclaration::set_property(PropertyID, StringView, StringView)
 {
-    return false;
+    // 1. If the computed flag is set, then throw a NoModificationAllowedError exception.
+    return DOM::NoModificationAllowedError::create("Cannot modify properties in result of getComputedStyle()");
+}
+
+// https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-removeproperty
+DOM::ExceptionOr<String> ResolvedCSSStyleDeclaration::remove_property(PropertyID)
+{
+    // 1. If the computed flag is set, then throw a NoModificationAllowedError exception.
+    return DOM::NoModificationAllowedError::create("Cannot remove properties from result of getComputedStyle()");
 }
 
 String ResolvedCSSStyleDeclaration::serialized() const

Userland/Libraries/LibWeb/CSS/ResolvedCSSStyleDeclaration.h

@@ -22,7 +22,8 @@ public:
     virtual size_t length() const override;
     virtual String item(size_t index) const override;
     virtual Optional<StyleProperty> property(PropertyID) const override;
-    virtual bool set_property(PropertyID, StringView css_text) override;
+    virtual DOM::ExceptionOr<void> set_property(PropertyID, StringView css_text, StringView priority) override;
+    virtual DOM::ExceptionOr<String> remove_property(PropertyID) override;
 
     virtual String serialized() const override;