From d31a58a7d63c3c60a03451cc425f227d5fa9e623 Mon Sep 17 00:00:00 2001 From: Callum Law Date: Tue, 10 Jun 2025 01:31:14 +1200 Subject: [PATCH] LibWeb: Add support for the 'all' CSS property The "longhands" array is populated in the code generator to avoid the overhead of manually maintaining the list in Properties.json There is one subtest that still fails in 'cssstyledeclaration-csstext-all-shorthand', this is related to us not maintaining the relative order of CSS declarations for custom vs non-custom properties. --- .../LibWeb/CSS/Parser/PropertyParsing.cpp | 5 ++ Libraries/LibWeb/CSS/Properties.json | 8 +++ Libraries/LibWeb/CSS/StyleComputer.cpp | 57 ------------------- Libraries/LibWeb/CSS/StyleComputer.h | 19 ------- .../CSS/StyleValues/ShorthandStyleValue.cpp | 5 ++ .../LibWeb/GenerateCSSPropertyID.cpp | 19 ++++++- ...upported-properties-and-default-values.txt | 1 + .../expected/css/all-with-invalid-value.txt | 1 + .../cssstyledeclaration-all-shorthand.txt | 11 ++++ ...styledeclaration-csstext-all-shorthand.txt | 12 ++++ ...cssstyledeclaration-removeProperty-all.txt | 6 ++ .../css/cssom/serialize-all-longhands.txt | 5 +- .../input/css/all-with-invalid-value.html | 14 +++++ .../cssstyledeclaration-all-shorthand.html | 54 ++++++++++++++++++ ...tyledeclaration-csstext-all-shorthand.html | 47 +++++++++++++++ ...ssstyledeclaration-removeProperty-all.html | 17 ++++++ 16 files changed, 199 insertions(+), 82 deletions(-) create mode 100644 Tests/LibWeb/Text/expected/css/all-with-invalid-value.txt create mode 100644 Tests/LibWeb/Text/expected/wpt-import/css/cssom/cssstyledeclaration-all-shorthand.txt create mode 100644 Tests/LibWeb/Text/expected/wpt-import/css/cssom/cssstyledeclaration-csstext-all-shorthand.txt create mode 100644 Tests/LibWeb/Text/expected/wpt-import/css/cssom/cssstyledeclaration-removeProperty-all.txt create mode 100644 Tests/LibWeb/Text/input/css/all-with-invalid-value.html create mode 100644 Tests/LibWeb/Text/input/wpt-import/css/cssom/cssstyledeclaration-all-shorthand.html create mode 100644 Tests/LibWeb/Text/input/wpt-import/css/cssom/cssstyledeclaration-csstext-all-shorthand.html create mode 100644 Tests/LibWeb/Text/input/wpt-import/css/cssom/cssstyledeclaration-removeProperty-all.html diff --git a/Libraries/LibWeb/CSS/Parser/PropertyParsing.cpp b/Libraries/LibWeb/CSS/Parser/PropertyParsing.cpp index 1968320d0b4..95d8c0193bb 100644 --- a/Libraries/LibWeb/CSS/Parser/PropertyParsing.cpp +++ b/Libraries/LibWeb/CSS/Parser/PropertyParsing.cpp @@ -408,6 +408,11 @@ Parser::ParseErrorOr> Parser::parse_css_value // Special-case property handling switch (property_id) { + case PropertyID::All: + // NOTE: The 'all' property, unlike some other shorthands, doesn't support directly listing sub-property + // values, only the CSS-wide keywords - this is handled above, and thus, if we have gotten to here, there + // is an invalid value which is a syntax error. + return ParseError::SyntaxError; case PropertyID::AspectRatio: if (auto parsed_value = parse_aspect_ratio_value(tokens); parsed_value && !tokens.has_next_token()) return parsed_value.release_nonnull(); diff --git a/Libraries/LibWeb/CSS/Properties.json b/Libraries/LibWeb/CSS/Properties.json index 3f5c6229d30..ee199be9d25 100644 --- a/Libraries/LibWeb/CSS/Properties.json +++ b/Libraries/LibWeb/CSS/Properties.json @@ -183,6 +183,14 @@ "align-self" ] }, + "all": { + "affects-layout": true, + "affects-stacking-context": true, + "inherited": false, + "initial": "initial", + "longhands": [], + "_comment": "The 'longhands' array is populated in the code generator to avoid having to maintain it manually" + }, "animation": { "affects-layout": false, "inherited": false, diff --git a/Libraries/LibWeb/CSS/StyleComputer.cpp b/Libraries/LibWeb/CSS/StyleComputer.cpp index 4029c03598f..f8466e8b198 100644 --- a/Libraries/LibWeb/CSS/StyleComputer.cpp +++ b/Libraries/LibWeb/CSS/StyleComputer.cpp @@ -996,57 +996,6 @@ void StyleComputer::for_each_property_expanding_shorthands(PropertyID property_i set_longhand_property(property_id, value); } -void StyleComputer::set_property_expanding_shorthands( - CascadedProperties& cascaded_properties, - PropertyID property_id, - CSSStyleValue const& value, - GC::Ptr declaration, - CascadeOrigin cascade_origin, - Important important, - Optional layer_name) -{ - for_each_property_expanding_shorthands(property_id, value, [&](PropertyID longhand_id, CSSStyleValue const& longhand_value) { - if (longhand_value.is_revert()) { - cascaded_properties.revert_property(longhand_id, important, cascade_origin); - } else if (longhand_value.is_revert_layer()) { - cascaded_properties.revert_layer_property(longhand_id, important, layer_name); - } else { - cascaded_properties.set_property(longhand_id, longhand_value, important, cascade_origin, layer_name, declaration); - } - }); -} - -void StyleComputer::set_all_properties( - CascadedProperties& cascaded_properties, - DOM::Element& element, - Optional pseudo_element, - CSSStyleValue const& value, - DOM::Document& document, - GC::Ptr declaration, - CascadeOrigin cascade_origin, - Important important, - Optional layer_name) const -{ - for (auto i = to_underlying(CSS::first_longhand_property_id); i <= to_underlying(CSS::last_longhand_property_id); ++i) { - auto property_id = (CSS::PropertyID)i; - - if (value.is_revert()) { - cascaded_properties.revert_property(property_id, important, cascade_origin); - continue; - } - - if (value.is_revert_layer()) { - cascaded_properties.revert_layer_property(property_id, important, layer_name); - continue; - } - - NonnullRefPtr property_value = value; - if (property_value->is_unresolved()) - property_value = Parser::Parser::resolve_unresolved_style_value(Parser::ParsingParams { document }, element, pseudo_element, property_id, property_value->as_unresolved()); - set_property_expanding_shorthands(cascaded_properties, property_id, property_value, declaration, cascade_origin, important, layer_name); - } -} - void StyleComputer::cascade_declarations( CascadedProperties& cascaded_properties, DOM::Element& element, @@ -1091,12 +1040,6 @@ void StyleComputer::cascade_declarations( } } - if (property.property_id == PropertyID::All) { - set_all_properties(cascaded_properties, element, pseudo_element, property_value, m_document, &declaration, cascade_origin, important, layer_name); - continue; - } - - // NOTE: This is a duplicate of set_property_expanding_shorthands() with some extra checks. for_each_property_expanding_shorthands(property.property_id, property_value, [&](PropertyID longhand_id, CSSStyleValue const& longhand_value) { // If we're a PSV that's already been seen, that should mean that our shorthand already got // resolved and gave us a value, so we don't want to overwrite it with a PSV. diff --git a/Libraries/LibWeb/CSS/StyleComputer.h b/Libraries/LibWeb/CSS/StyleComputer.h index 9e290fbb12b..7f95e8c1d20 100644 --- a/Libraries/LibWeb/CSS/StyleComputer.h +++ b/Libraries/LibWeb/CSS/StyleComputer.h @@ -129,14 +129,6 @@ class FontLoader; class StyleComputer { public: static void for_each_property_expanding_shorthands(PropertyID, CSSStyleValue const&, Function const& set_longhand_property); - static void set_property_expanding_shorthands( - CascadedProperties&, - PropertyID, - CSSStyleValue const&, - GC::Ptr, - CascadeOrigin, - Important, - Optional layer_name); static NonnullRefPtr get_inherit_value(CSS::PropertyID, DOM::Element const*, Optional = {}); static Optional user_agent_style_sheet_source(StringView name); @@ -222,17 +214,6 @@ private: void compute_defaulted_property_value(ComputedProperties&, DOM::Element const*, CSS::PropertyID, Optional) const; - void set_all_properties( - CascadedProperties&, - DOM::Element&, - Optional, - CSSStyleValue const&, - DOM::Document&, - GC::Ptr, - CascadeOrigin, - Important, - Optional layer_name) const; - template void for_each_stylesheet(CascadeOrigin, Callback) const; diff --git a/Libraries/LibWeb/CSS/StyleValues/ShorthandStyleValue.cpp b/Libraries/LibWeb/CSS/StyleValues/ShorthandStyleValue.cpp index c640628c2dd..16ede89b870 100644 --- a/Libraries/LibWeb/CSS/StyleValues/ShorthandStyleValue.cpp +++ b/Libraries/LibWeb/CSS/StyleValues/ShorthandStyleValue.cpp @@ -95,6 +95,11 @@ String ShorthandStyleValue::to_string(SerializationMode mode) const // Then special cases switch (m_properties.shorthand_property) { + case PropertyID::All: { + // NOTE: 'all' can only be serialized in the case all sub-properties share the same CSS-wide keyword, this is + // handled above, thus, if we get to here that mustn't be the case and we should return the empty string. + return ""_string; + } case PropertyID::Background: { auto color = longhand(PropertyID::BackgroundColor); auto image = longhand(PropertyID::BackgroundImage); diff --git a/Meta/Lagom/Tools/CodeGenerators/LibWeb/GenerateCSSPropertyID.cpp b/Meta/Lagom/Tools/CodeGenerators/LibWeb/GenerateCSSPropertyID.cpp index a87e1f332e4..9c5a573b5e6 100644 --- a/Meta/Lagom/Tools/CodeGenerators/LibWeb/GenerateCSSPropertyID.cpp +++ b/Meta/Lagom/Tools/CodeGenerators/LibWeb/GenerateCSSPropertyID.cpp @@ -15,6 +15,7 @@ #include void replace_logical_aliases(JsonObject& properties); +void populate_all_property_longhands(JsonObject& properties); ErrorOr generate_header_file(JsonObject& properties, Core::File& file); ErrorOr generate_implementation_file(JsonObject& properties, Core::File& file); void generate_bounds_checking_function(JsonObject& properties, SourceGenerator& parent_generator, StringView css_type_name, StringView type_name, Optional default_unit_name = {}, Optional value_getter = {}); @@ -81,6 +82,7 @@ ErrorOr serenity_main(Main::Arguments arguments) }); replace_logical_aliases(properties); + populate_all_property_longhands(properties); auto generated_header_file = TRY(Core::File::open(generated_header_path, Core::File::OpenMode::Write)); auto generated_implementation_file = TRY(Core::File::open(generated_implementation_path, Core::File::OpenMode::Write)); @@ -123,6 +125,20 @@ void replace_logical_aliases(JsonObject& properties) } } +void populate_all_property_longhands(JsonObject& properties) +{ + auto all_entry = properties.get_object("all"sv); + + VERIFY(all_entry.has_value()); + + properties.for_each_member([&](auto name, auto value) { + if (value.as_object().has_array("longhands"sv) || value.as_object().has_array("logical-alias-for"sv) || value.as_object().has_string("legacy-alias-for"sv) || name == "direction" || name == "unicode-bidi") + return; + + MUST(all_entry->get_array("longhands"sv)->append(JsonValue { name })); + }); +} + ErrorOr generate_header_file(JsonObject& properties, Core::File& file) { StringBuilder builder; @@ -142,7 +158,6 @@ namespace Web::CSS { enum class PropertyID : @property_id_underlying_type@ { Invalid, Custom, - All, )~~~"); Vector inherited_shorthand_property_ids; @@ -448,8 +463,6 @@ Optional property_id_from_string(StringView string) if (is_a_custom_property_name_string(string)) return PropertyID::Custom; - if (string.equals_ignoring_ascii_case("all"sv)) - return PropertyID::All; )~~~"); properties.for_each_member([&](auto& name, auto& value) { diff --git a/Tests/LibWeb/Text/expected/css/CSSStyleProperties-all-supported-properties-and-default-values.txt b/Tests/LibWeb/Text/expected/css/CSSStyleProperties-all-supported-properties-and-default-values.txt index e67de32ed0c..b1ed7a9d55a 100644 --- a/Tests/LibWeb/Text/expected/css/CSSStyleProperties-all-supported-properties-and-default-values.txt +++ b/Tests/LibWeb/Text/expected/css/CSSStyleProperties-all-supported-properties-and-default-values.txt @@ -152,6 +152,7 @@ All supported properties and their default values exposed from CSSStylePropertie 'align-items': 'normal' 'alignSelf': 'auto' 'align-self': 'auto' +'all': '' 'animation': 'none' 'animationDelay': '0s' 'animation-delay': '0s' diff --git a/Tests/LibWeb/Text/expected/css/all-with-invalid-value.txt b/Tests/LibWeb/Text/expected/css/all-with-invalid-value.txt new file mode 100644 index 00000000000..ebc5e089293 --- /dev/null +++ b/Tests/LibWeb/Text/expected/css/all-with-invalid-value.txt @@ -0,0 +1 @@ +div { } diff --git a/Tests/LibWeb/Text/expected/wpt-import/css/cssom/cssstyledeclaration-all-shorthand.txt b/Tests/LibWeb/Text/expected/wpt-import/css/cssom/cssstyledeclaration-all-shorthand.txt new file mode 100644 index 00000000000..977b8f064ba --- /dev/null +++ b/Tests/LibWeb/Text/expected/wpt-import/css/cssom/cssstyledeclaration-all-shorthand.txt @@ -0,0 +1,11 @@ +Harness status: OK + +Found 6 tests + +6 Pass +Pass getPropertyValue('all') returns empty string +Pass getPropertyValue('all') returns css-wide keyword if possible +Pass getPropertyValue('all') returns empty string when single property overriden +Pass setProperty('all') sets all property values +Pass removeProperty('all') removes all declarations affected by 'all' +Pass removeProperty('all') removes an 'all' declaration \ No newline at end of file diff --git a/Tests/LibWeb/Text/expected/wpt-import/css/cssom/cssstyledeclaration-csstext-all-shorthand.txt b/Tests/LibWeb/Text/expected/wpt-import/css/cssom/cssstyledeclaration-csstext-all-shorthand.txt new file mode 100644 index 00000000000..113c4cf330a --- /dev/null +++ b/Tests/LibWeb/Text/expected/wpt-import/css/cssom/cssstyledeclaration-csstext-all-shorthand.txt @@ -0,0 +1,12 @@ +Harness status: OK + +Found 6 tests + +5 Pass +1 Fail +Pass 'all' shorthand alone +Pass 'all' shorthand with 'width' and 'height' +Pass 'all' shorthand with 'direction' and 'unicode-bidi' +Fail 'all' shorthand with 'width', 'height' and custom properties +Pass 'all' shorthand with all longhands +Pass all:initial should not exclude custom from cssText \ No newline at end of file diff --git a/Tests/LibWeb/Text/expected/wpt-import/css/cssom/cssstyledeclaration-removeProperty-all.txt b/Tests/LibWeb/Text/expected/wpt-import/css/cssom/cssstyledeclaration-removeProperty-all.txt new file mode 100644 index 00000000000..ccc928c0cb2 --- /dev/null +++ b/Tests/LibWeb/Text/expected/wpt-import/css/cssom/cssstyledeclaration-removeProperty-all.txt @@ -0,0 +1,6 @@ +Harness status: OK + +Found 1 tests + +1 Pass +Pass CSSStyleDeclaration.removeProperty("all") \ No newline at end of file diff --git a/Tests/LibWeb/Text/expected/wpt-import/css/cssom/serialize-all-longhands.txt b/Tests/LibWeb/Text/expected/wpt-import/css/cssom/serialize-all-longhands.txt index 0ef08002f59..5d1401ab107 100644 --- a/Tests/LibWeb/Text/expected/wpt-import/css/cssom/serialize-all-longhands.txt +++ b/Tests/LibWeb/Text/expected/wpt-import/css/cssom/serialize-all-longhands.txt @@ -2,7 +2,6 @@ Harness status: OK Found 2 tests -1 Pass -1 Fail -Fail Specified style +2 Pass +Pass Specified style Pass Computed style \ No newline at end of file diff --git a/Tests/LibWeb/Text/input/css/all-with-invalid-value.html b/Tests/LibWeb/Text/input/css/all-with-invalid-value.html new file mode 100644 index 00000000000..96c4d12a538 --- /dev/null +++ b/Tests/LibWeb/Text/input/css/all-with-invalid-value.html @@ -0,0 +1,14 @@ + + + + + + diff --git a/Tests/LibWeb/Text/input/wpt-import/css/cssom/cssstyledeclaration-all-shorthand.html b/Tests/LibWeb/Text/input/wpt-import/css/cssom/cssstyledeclaration-all-shorthand.html new file mode 100644 index 00000000000..1f7333ab35f --- /dev/null +++ b/Tests/LibWeb/Text/input/wpt-import/css/cssom/cssstyledeclaration-all-shorthand.html @@ -0,0 +1,54 @@ + +CSSOM Test: Passing "all" shorthand to property methods + + + + + diff --git a/Tests/LibWeb/Text/input/wpt-import/css/cssom/cssstyledeclaration-csstext-all-shorthand.html b/Tests/LibWeb/Text/input/wpt-import/css/cssom/cssstyledeclaration-csstext-all-shorthand.html new file mode 100644 index 00000000000..75786b05ec5 --- /dev/null +++ b/Tests/LibWeb/Text/input/wpt-import/css/cssom/cssstyledeclaration-csstext-all-shorthand.html @@ -0,0 +1,47 @@ + +CSSOM test: serialization of the 'all' shorthand in cssText + + + + + diff --git a/Tests/LibWeb/Text/input/wpt-import/css/cssom/cssstyledeclaration-removeProperty-all.html b/Tests/LibWeb/Text/input/wpt-import/css/cssom/cssstyledeclaration-removeProperty-all.html new file mode 100644 index 00000000000..c035a5b2c7b --- /dev/null +++ b/Tests/LibWeb/Text/input/wpt-import/css/cssom/cssstyledeclaration-removeProperty-all.html @@ -0,0 +1,17 @@ + + +CSSStyleDeclaration.removeProperty("all") + + + + + +