LibWeb/CSS: Allow descriptors to be shorthands

I was wrong when I added those notes before about this being impossible,
it's *very* possible, for example with the `@page margin` descriptor.
However, until we have a large number of these shorthands and not just a
single example, we can get away with hard-coding support for it.

Libraries/LibWeb/CSS/CSSDescriptors.cpp

@@ -7,6 +7,7 @@
 #include <LibWeb/CSS/CSSDescriptors.h>
 #include <LibWeb/CSS/Parser/Parser.h>
 #include <LibWeb/CSS/Serialize.h>
+#include <LibWeb/CSS/StyleValues/StyleValueList.h>
 #include <LibWeb/Infra/Strings.h>
 
 namespace Web::CSS {
@@ -95,12 +96,27 @@ WebIDL::ExceptionOr<void> CSSDescriptors::set_property(StringView property, Stri
     // 7. Let updated be false.
     auto updated = false;
 
-    // 8. If property is a shorthand property...
+    // 8. If property is a shorthand property, then for each longhand property longhand that property maps to, in canonical order, follow these substeps:
-    // NB: Descriptors can't be shorthands.
+    if (is_shorthand(m_at_rule_id, *descriptor_id)) {
+        for_each_expanded_longhand(m_at_rule_id, *descriptor_id, component_value_list, [this, &updated, priority](DescriptorID longhand_id, auto longhand_value) {
+            VERIFY(longhand_value);
+
+            // 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.
+            auto longhand_result = set_a_css_declaration(longhand_id, longhand_value.release_nonnull(), priority.is_empty() ? Important::No : Important::Yes);
+
+            // 2. If longhand result is true, let updated be true.
+            if (longhand_result)
+                updated = 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(*descriptor_id, *component_value_list, !priority.is_empty() ? Important::Yes : Important::No);
+    else {
+        updated = set_a_css_declaration(*descriptor_id, *component_value_list, !priority.is_empty() ? Important::Yes : Important::No);
+    }
 
     // 10. If updated is true, update style attribute for the CSS declaration block.
     if (updated)
@@ -124,14 +140,21 @@ WebIDL::ExceptionOr<String> CSSDescriptors::remove_property(StringView property)
 
     // 4. Let removed be false.
     bool removed = false;
+    auto descriptor_id = descriptor_id_from_string(m_at_rule_id, property);
 
-    // 5. If property is a shorthand property...
+    // 5. If property is a shorthand property, for each longhand property longhand that property maps to:
-    // NB: Descriptors can't be shorthands.
+    if (descriptor_id.has_value() && is_shorthand(m_at_rule_id, *descriptor_id)) {
+        for_each_expanded_longhand(m_at_rule_id, *descriptor_id, nullptr, [this, &removed](DescriptorID longhand_id, auto const&) {
+            // 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.
+            if (m_descriptors.remove_first_matching([longhand_id](auto& entry) { return entry.descriptor_id == longhand_id; })) {
+                removed = 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.
-    // auto descriptor_id = descriptor_from_string()
+    else if (descriptor_id.has_value()) {
-    auto descriptor_id = descriptor_id_from_string(m_at_rule_id, property);
-    if (descriptor_id.has_value()) {
         removed = m_descriptors.remove_first_matching([descriptor_id](auto& entry) { return entry.descriptor_id == *descriptor_id; });
     }
 
@@ -256,4 +279,56 @@ RefPtr<CSSStyleValue const> CSSDescriptors::descriptor_or_initial_value(Descript
     return descriptor_initial_value(m_at_rule_id, descriptor_id);
 }
 
+bool is_shorthand(AtRuleID at_rule, DescriptorID descriptor)
+{
+    if (at_rule == AtRuleID::Page && descriptor == DescriptorID::Margin)
+        return true;
+
+    return false;
+}
+
+void for_each_expanded_longhand(AtRuleID at_rule, DescriptorID descriptor, RefPtr<CSSStyleValue const> value, Function<void(DescriptorID, RefPtr<CSSStyleValue const>)> callback)
+{
+    if (at_rule == AtRuleID::Page && descriptor == DescriptorID::Margin) {
+        if (!value) {
+            callback(DescriptorID::MarginTop, nullptr);
+            callback(DescriptorID::MarginRight, nullptr);
+            callback(DescriptorID::MarginBottom, nullptr);
+            callback(DescriptorID::MarginLeft, nullptr);
+            return;
+        }
+
+        if (value->is_value_list()) {
+            auto& values = value->as_value_list().values();
+            if (values.size() == 4) {
+                callback(DescriptorID::MarginTop, values[0]);
+                callback(DescriptorID::MarginRight, values[1]);
+                callback(DescriptorID::MarginBottom, values[2]);
+                callback(DescriptorID::MarginLeft, values[3]);
+            } else if (values.size() == 3) {
+                callback(DescriptorID::MarginTop, values[0]);
+                callback(DescriptorID::MarginRight, values[1]);
+                callback(DescriptorID::MarginBottom, values[2]);
+                callback(DescriptorID::MarginLeft, values[1]);
+            } else if (values.size() == 2) {
+                callback(DescriptorID::MarginTop, values[0]);
+                callback(DescriptorID::MarginRight, values[1]);
+                callback(DescriptorID::MarginBottom, values[0]);
+                callback(DescriptorID::MarginLeft, values[1]);
+            } else if (values.size() == 1) {
+                callback(DescriptorID::MarginTop, values[0]);
+                callback(DescriptorID::MarginRight, values[0]);
+                callback(DescriptorID::MarginBottom, values[0]);
+                callback(DescriptorID::MarginLeft, values[0]);
+            }
+
+        } else {
+            callback(DescriptorID::MarginTop, *value);
+            callback(DescriptorID::MarginRight, *value);
+            callback(DescriptorID::MarginBottom, *value);
+            callback(DescriptorID::MarginLeft, *value);
+        }
+    }
+}
+
 }

Libraries/LibWeb/CSS/CSSDescriptors.h

@@ -45,4 +45,7 @@ private:
     Vector<Descriptor> m_descriptors;
 };
 
+bool is_shorthand(AtRuleID, DescriptorID);
+void for_each_expanded_longhand(AtRuleID, DescriptorID, RefPtr<CSSStyleValue const>, Function<void(DescriptorID, RefPtr<CSSStyleValue const>)>);
+
 }

Libraries/LibWeb/CSS/Parser/RuleParsing.cpp

@@ -40,6 +40,49 @@
 
 namespace Web::CSS::Parser {
 
+// A helper that ensures only the last instance of each descriptor is included, while also handling shorthands.
+class DescriptorList {
+public:
+    DescriptorList(AtRuleID at_rule)
+        : m_at_rule(at_rule)
+    {
+    }
+
+    void append(Descriptor&& descriptor)
+    {
+        if (is_shorthand(m_at_rule, descriptor.descriptor_id)) {
+            for_each_expanded_longhand(m_at_rule, descriptor.descriptor_id, descriptor.value, [this](auto longhand_id, auto longhand_value) {
+                append_internal(Descriptor { longhand_id, longhand_value.release_nonnull() });
+            });
+            return;
+        }
+
+        append_internal(move(descriptor));
+    }
+
+    Vector<Descriptor> release_descriptors()
+    {
+        return move(m_descriptors);
+    }
+
+private:
+    void append_internal(Descriptor&& descriptor)
+    {
+        if (m_seen_descriptor_ids.contains(descriptor.descriptor_id)) {
+            m_descriptors.remove_first_matching([&descriptor](Descriptor const& existing) {
+                return existing.descriptor_id == descriptor.descriptor_id;
+            });
+        } else {
+            m_seen_descriptor_ids.set(descriptor.descriptor_id);
+        }
+        m_descriptors.append(move(descriptor));
+    }
+
+    AtRuleID m_at_rule;
+    Vector<Descriptor> m_descriptors;
+    HashTable<DescriptorID> m_seen_descriptor_ids;
+};
+
 GC::Ptr<CSSRule> Parser::convert_to_rule(Rule const& rule, Nested nested)
 {
     return rule.visit(
@@ -582,22 +625,14 @@ GC::Ptr<CSSPropertyRule> Parser::convert_to_property_rule(AtRule const& rule)
 GC::Ptr<CSSFontFaceRule> Parser::convert_to_font_face_rule(AtRule const& rule)
 {
     // https://drafts.csswg.org/css-fonts/#font-face-rule
-    Vector<Descriptor> descriptors;
+    DescriptorList descriptors { AtRuleID::FontFace };
-    HashTable<DescriptorID> seen_descriptor_ids;
     rule.for_each_as_declaration_list([&](auto& declaration) {
         if (auto descriptor = convert_to_descriptor(AtRuleID::FontFace, declaration); descriptor.has_value()) {
-            if (seen_descriptor_ids.contains(descriptor->descriptor_id)) {
-                descriptors.remove_first_matching([&descriptor](Descriptor const& existing) {
-                    return existing.descriptor_id == descriptor->descriptor_id;
-                });
-            } else {
-                seen_descriptor_ids.set(descriptor->descriptor_id);
-            }
             descriptors.append(descriptor.release_value());
         }
     });
 
-    return CSSFontFaceRule::create(realm(), CSSFontFaceDescriptors::create(realm(), move(descriptors)));
+    return CSSFontFaceRule::create(realm(), CSSFontFaceDescriptors::create(realm(), descriptors.release_descriptors()));
 }
 
 static Optional<PageSelectorList> parse_page_selector_list(Vector<ComponentValue> const& component_values)
@@ -671,8 +706,7 @@ GC::Ptr<CSSPageRule> Parser::convert_to_page_rule(AtRule const& rule)
         return nullptr;
 
     GC::RootVector<GC::Ref<CSSRule>> child_rules { realm().heap() };
-    Vector<Descriptor> descriptors;
+    DescriptorList descriptors { AtRuleID::Page };
-    HashTable<DescriptorID> seen_descriptor_ids;
     rule.for_each_as_declaration_rule_list(
         [&](auto& at_rule) {
             // FIXME: Parse margin rules here.
@@ -680,19 +714,12 @@ GC::Ptr<CSSPageRule> Parser::convert_to_page_rule(AtRule const& rule)
         },
         [&](auto& declaration) {
             if (auto descriptor = convert_to_descriptor(AtRuleID::Page, declaration); descriptor.has_value()) {
-                if (seen_descriptor_ids.contains(descriptor->descriptor_id)) {
-                    descriptors.remove_first_matching([&descriptor](Descriptor const& existing) {
-                        return existing.descriptor_id == descriptor->descriptor_id;
-                    });
-                } else {
-                    seen_descriptor_ids.set(descriptor->descriptor_id);
-                }
                 descriptors.append(descriptor.release_value());
             }
         });
 
     auto rule_list = CSSRuleList::create(realm(), child_rules);
-    return CSSPageRule::create(realm(), page_selectors.release_value(), CSSPageDescriptors::create(realm(), move(descriptors)), rule_list);
+    return CSSPageRule::create(realm(), page_selectors.release_value(), CSSPageDescriptors::create(realm(), descriptors.release_descriptors()), rule_list);
 }
 
 }

Tests/LibWeb/Text/expected/wpt-import/css/css-page/cssom/page-001.txt

@@ -2,9 +2,8 @@ Harness status: OK
 
 Found 4 tests
 
-3 Pass
+4 Pass
-1 Fail
 Pass	There should be 3 @page rules.
 Pass	Rule #0
 Pass	Rule #1
-Fail	Rule #2
+Pass	Rule #2

Tests/LibWeb/Text/expected/wpt-import/css/css-page/cssom/page-002.txt

@@ -2,6 +2,6 @@ Harness status: OK
 
 Found 2 tests
 
-2 Fail
+2 Pass
-Fail	Add declarations
+Pass	Add declarations
-Fail	Remove declarations
+Pass	Remove declarations