From e8ba1b4fdfbf4a6edc6d4fd5077ac11dd4c74b1c Mon Sep 17 00:00:00 2001 From: Callum Law Date: Sat, 29 Nov 2025 15:25:07 +1300 Subject: [PATCH] LibWeb: Simplify serialization of coordinating value list shorthands We know that longhand values are always `StyleValueList`s Introduces a new fail in the `view-timeline-shorthand.html` WPT test but this is because the test is incorrect and we now correctly don't contract when `view-timeline-inset` has dissimilar cardinality. See web-platform-tests/wpt#56181 --- .../CSS/StyleValues/ShorthandStyleValue.cpp | 56 ++++--------------- .../css/view-timeline-shorthand.txt | 6 +- 2 files changed, 14 insertions(+), 48 deletions(-) diff --git a/Libraries/LibWeb/CSS/StyleValues/ShorthandStyleValue.cpp b/Libraries/LibWeb/CSS/StyleValues/ShorthandStyleValue.cpp index a08ea208e8d..54c07f79622 100644 --- a/Libraries/LibWeb/CSS/StyleValues/ShorthandStyleValue.cpp +++ b/Libraries/LibWeb/CSS/StyleValues/ShorthandStyleValue.cpp @@ -70,24 +70,16 @@ String ShorthandStyleValue::to_string(SerializationMode mode) const return ""_string; } - // FIXME: This is required as parse_comma_separated_value_list() returns a single value directly instead of a list if there's only one. - auto const style_value_as_value_list = [&](RefPtr value) -> StyleValueVector { - if (value->is_value_list()) - return value->as_value_list().values(); - - return { value.release_nonnull() }; - }; - auto const coordinating_value_list_shorthand_to_string = [&](StringView entry_when_all_longhands_initial, Vector const& required_longhands = {}, Vector const& reset_only_longhands = {}) { for (auto reset_only_longhand : reset_only_longhands) { if (!longhand(reset_only_longhand)->equals(property_initial_value(reset_only_longhand))) return ""_string; } - auto entry_count = style_value_as_value_list(longhand(m_properties.sub_properties[0])).size(); + auto entry_count = longhand(m_properties.sub_properties[0])->as_value_list().size(); // If we don't have the same number of values for each non-reset-only longhand, we can't serialize this shorthand. - if (any_of(m_properties.sub_properties, [&](auto longhand_id) { return !reset_only_longhands.contains_slow(longhand_id) && style_value_as_value_list(longhand(longhand_id)).size() != entry_count; })) + if (any_of(m_properties.sub_properties, [&](auto longhand_id) { return !reset_only_longhands.contains_slow(longhand_id) && longhand(longhand_id)->as_value_list().size() != entry_count; })) return ""_string; // We should serialize a longhand if it is not a reset-only longhand and one of the following is true: @@ -103,9 +95,9 @@ String ShorthandStyleValue::to_string(SerializationMode mode) const if (required_longhands.contains_slow(longhand_id)) return true; - auto longhand_value = style_value_as_value_list(longhand(longhand_id))[entry_index]; + auto longhand_value = longhand(longhand_id)->as_value_list().values()[entry_index]; - if (!longhand_value->equals(style_value_as_value_list(property_initial_value(longhand_id))[0])) + if (!longhand_value->equals(property_initial_value(longhand_id)->as_value_list().values()[0])) return true; for (size_t other_longhand_index = longhand_index + 1; other_longhand_index < m_properties.sub_properties.size(); other_longhand_index++) { @@ -114,10 +106,10 @@ String ShorthandStyleValue::to_string(SerializationMode mode) const if (reset_only_longhands.contains_slow(other_longhand_id)) continue; - auto other_longhand_value = style_value_as_value_list(longhand(other_longhand_id))[entry_index]; + auto other_longhand_value = longhand(other_longhand_id)->as_value_list().values()[entry_index]; // FIXME: This should really account for the other longhand being included in the serialization for any reason, not just because it is not the initial value. - if (other_longhand_value->equals(style_value_as_value_list(property_initial_value(other_longhand_id))[0])) + if (other_longhand_value->equals(property_initial_value(other_longhand_id)->as_value_list().values()[0])) continue; if (parse_css_value(Parser::ParsingParams {}, other_longhand_value->to_string(mode), longhand_id)) @@ -140,7 +132,7 @@ String ShorthandStyleValue::to_string(SerializationMode mode) const if (!builder.is_empty() && !first) builder.append(' '); - auto longhand_value = style_value_as_value_list(longhand(longhand_id))[entry_index]; + auto longhand_value = longhand(longhand_id)->as_value_list().values()[entry_index]; builder.append(longhand_value->to_string(mode)); first = false; @@ -862,36 +854,10 @@ String ShorthandStyleValue::to_string(SerializationMode mode) const } case PropertyID::Transition: return coordinating_value_list_shorthand_to_string("all"sv); - case PropertyID::ViewTimeline: { - // FIXME: We can use coordinating_value_list_shorthand_to_string function once parse_comma_separated_value_list - // always returns a list, currently it doesn't properly handle the fact that the entries for - // view-timeline-inset are themselves StyleValueLists - StringBuilder builder; - - auto const& name_values = style_value_as_value_list(longhand(PropertyID::ViewTimelineName)); - auto const& axis_values = style_value_as_value_list(longhand(PropertyID::ViewTimelineAxis)); - auto const& inset_values = style_value_as_value_list(longhand(PropertyID::ViewTimelineInset)); - - if (name_values.size() != axis_values.size()) - return ""_string; - - for (size_t i = 0; i < name_values.size(); i++) { - if (!builder.is_empty()) - builder.append(", "sv); - - builder.append(name_values[i]->to_string(mode)); - - if (axis_values[i]->to_keyword() != Keyword::Block) - builder.appendff(" {}", axis_values[i]->to_string(mode)); - - auto stringified_inset = inset_values[i]->to_string(mode); - - if (stringified_inset != "auto"sv) - builder.appendff(" {}", stringified_inset); - } - - return builder.to_string_without_validation(); - } + case PropertyID::ViewTimeline: + // NB: We don't need to specify a value to use when the entry is empty as all values are initial since + // view-timeline-name is always included + return coordinating_value_list_shorthand_to_string(""sv, { PropertyID::ViewTimelineName }); case PropertyID::WhiteSpace: { auto white_space_collapse_property = longhand(PropertyID::WhiteSpaceCollapse); auto text_wrap_mode_property = longhand(PropertyID::TextWrapMode); diff --git a/Tests/LibWeb/Text/expected/wpt-import/scroll-animations/css/view-timeline-shorthand.txt b/Tests/LibWeb/Text/expected/wpt-import/scroll-animations/css/view-timeline-shorthand.txt index bc59968cbb1..c366a573b6c 100644 --- a/Tests/LibWeb/Text/expected/wpt-import/scroll-animations/css/view-timeline-shorthand.txt +++ b/Tests/LibWeb/Text/expected/wpt-import/scroll-animations/css/view-timeline-shorthand.txt @@ -2,8 +2,8 @@ Harness status: OK Found 83 tests -78 Pass -5 Fail +77 Pass +6 Fail Pass e.style['view-timeline'] = "--abcd" should set the property value Pass e.style['view-timeline'] = "none block" should set the property value Pass e.style['view-timeline'] = "none inline" should set the property value @@ -86,4 +86,4 @@ Pass Shorthand contraction of view-timeline-name:--a, --b:undefined;view-timelin Pass Shorthand contraction of view-timeline-name:none, none:undefined;view-timeline-axis:block, block:undefined;view-timeline-inset:auto auto, auto:undefined Fail Shorthand contraction of view-timeline-name:--a, --b, --c:undefined;view-timeline-axis:inline, inline:undefined;view-timeline-inset:auto, auto:undefined Fail Shorthand contraction of view-timeline-name:--a, --b:undefined;view-timeline-axis:inline, inline, inline:undefined;view-timeline-inset:auto, auto, auto:undefined -Pass Shorthand contraction of view-timeline-name:--a, --b:undefined;view-timeline-axis:inline, inline:undefined;view-timeline-inset:auto, auto, auto:undefined \ No newline at end of file +Fail Shorthand contraction of view-timeline-name:--a, --b:undefined;view-timeline-axis:inline, inline:undefined;view-timeline-inset:auto, auto, auto:undefined \ No newline at end of file