From 6bb8bf189f2a763bb93cab7076f74627aec8cc82 Mon Sep 17 00:00:00 2001 From: Milo van der Tier Date: Sun, 24 Nov 2024 18:47:15 +0100 Subject: [PATCH] LibWeb: Store CSS color name in CSSRGB When serializing an sRGB color value that originated from a named color, it should return the color name converted to ASCII lowercase. This requires storing the color name (if it has one). This change also requires explicitly removing the color names when computing style, because computed color values do not retain their name. It also requires removing a caching optimization in create_from_color(), because adding the name means that the cached value might be wrong. This fixes some WPT subtests, and also required updating some of our own tests. --- Libraries/LibWeb/CSS/Parser/Parser.cpp | 2 +- Libraries/LibWeb/CSS/StyleComputer.cpp | 11 ++++ .../LibWeb/CSS/StyleValues/CSSColorValue.cpp | 32 +++--------- .../LibWeb/CSS/StyleValues/CSSColorValue.h | 3 +- Libraries/LibWeb/CSS/StyleValues/CSSRGB.cpp | 2 + Libraries/LibWeb/CSS/StyleValues/CSSRGB.h | 11 ++-- .../expected/HTML/background-shorthand.txt | 2 +- ...claration-serialized-custom-properties.txt | 2 +- .../Text/expected/css/keyframes-css-rules.txt | 8 +-- .../css/set-style-declaration-shorthand.txt | 2 +- .../css/style-declaration-parent-rule.txt | 6 +-- .../wpt-import/css/css-nesting/cssom.txt | 24 ++++----- .../css/css-nesting/invalid-inner-rules.txt | 5 +- .../serialize-group-rules-with-decls.txt | 25 +++++---- .../domparsing/style_attribute_html.txt | 15 ++++++ .../domparsing/style_attribute_html.html | 52 +++++++++++++++++++ 16 files changed, 133 insertions(+), 69 deletions(-) create mode 100644 Tests/LibWeb/Text/expected/wpt-import/domparsing/style_attribute_html.txt create mode 100644 Tests/LibWeb/Text/input/wpt-import/domparsing/style_attribute_html.html diff --git a/Libraries/LibWeb/CSS/Parser/Parser.cpp b/Libraries/LibWeb/CSS/Parser/Parser.cpp index cc16c88ff32..5abc10937a8 100644 --- a/Libraries/LibWeb/CSS/Parser/Parser.cpp +++ b/Libraries/LibWeb/CSS/Parser/Parser.cpp @@ -3413,7 +3413,7 @@ RefPtr Parser::parse_color_value(TokenStream& tok auto color = Color::from_string(ident); if (color.has_value()) { transaction.commit(); - return CSSColorValue::create_from_color(color.release_value()); + return CSSColorValue::create_from_color(color.release_value(), ident); } // Otherwise, fall through to the hashless-hex-color case } diff --git a/Libraries/LibWeb/CSS/StyleComputer.cpp b/Libraries/LibWeb/CSS/StyleComputer.cpp index ac8e688d89e..d01b603478c 100644 --- a/Libraries/LibWeb/CSS/StyleComputer.cpp +++ b/Libraries/LibWeb/CSS/StyleComputer.cpp @@ -2425,6 +2425,17 @@ Optional StyleComputer::compute_style_impl(DOM::Element& elemen start_needed_transitions(*previous_style, style, element, pseudo_element); } + // Remove color names from CSS color values. This is needed because computed values cannot be named colors. + for (auto i = to_underlying(CSS::first_property_id); i <= to_underlying(CSS::last_property_id); ++i) { + auto property_id = (CSS::PropertyID)i; + auto* property = style.maybe_null_property(property_id); + if (property && property->is_color()) { + auto& color_value = property->as_color(); + if (color_value.color_type() == CSSColorValue::ColorType::RGB) + style.set_property(property_id, CSSColorValue::create_from_color(color_value.to_color({}))); + } + } + return style; } diff --git a/Libraries/LibWeb/CSS/StyleValues/CSSColorValue.cpp b/Libraries/LibWeb/CSS/StyleValues/CSSColorValue.cpp index 71d2625c648..2f1e4b93a7b 100644 --- a/Libraries/LibWeb/CSS/StyleValues/CSSColorValue.cpp +++ b/Libraries/LibWeb/CSS/StyleValues/CSSColorValue.cpp @@ -17,32 +17,14 @@ namespace Web::CSS { -ValueComparingNonnullRefPtr CSSColorValue::create_from_color(Color color) +ValueComparingNonnullRefPtr CSSColorValue::create_from_color(Color color, Optional name) { - auto make_rgb_color = [](Color const& color) { - return CSSRGB::create( - NumberStyleValue::create(color.red()), - NumberStyleValue::create(color.green()), - NumberStyleValue::create(color.blue()), - NumberStyleValue::create(color.alpha() / 255.0)); - }; - - if (color.value() == 0) { - static auto transparent = make_rgb_color(color); - return transparent; - } - - if (color == Color::from_rgb(0x000000)) { - static auto black = make_rgb_color(color); - return black; - } - - if (color == Color::from_rgb(0xffffff)) { - static auto white = make_rgb_color(color); - return white; - } - - return make_rgb_color(color); + return CSSRGB::create( + NumberStyleValue::create(color.red()), + NumberStyleValue::create(color.green()), + NumberStyleValue::create(color.blue()), + NumberStyleValue::create(color.alpha() / 255.0), + name); } Optional CSSColorValue::resolve_hue(CSSStyleValue const& style_value) diff --git a/Libraries/LibWeb/CSS/StyleValues/CSSColorValue.h b/Libraries/LibWeb/CSS/StyleValues/CSSColorValue.h index 8276dcff90f..c73d006e24e 100644 --- a/Libraries/LibWeb/CSS/StyleValues/CSSColorValue.h +++ b/Libraries/LibWeb/CSS/StyleValues/CSSColorValue.h @@ -9,6 +9,7 @@ #pragma once +#include #include #include @@ -17,7 +18,7 @@ namespace Web::CSS { // https://drafts.css-houdini.org/css-typed-om-1/#csscolorvalue class CSSColorValue : public CSSStyleValue { public: - static ValueComparingNonnullRefPtr create_from_color(Color color); + static ValueComparingNonnullRefPtr create_from_color(Color color, Optional name = {}); virtual ~CSSColorValue() override = default; virtual bool has_color() const override { return true; } diff --git a/Libraries/LibWeb/CSS/StyleValues/CSSRGB.cpp b/Libraries/LibWeb/CSS/StyleValues/CSSRGB.cpp index 1db367101cb..ce3f80be9fb 100644 --- a/Libraries/LibWeb/CSS/StyleValues/CSSRGB.cpp +++ b/Libraries/LibWeb/CSS/StyleValues/CSSRGB.cpp @@ -71,6 +71,8 @@ bool CSSRGB::equals(CSSStyleValue const& other) const String CSSRGB::to_string() const { // FIXME: Do this properly, taking unresolved calculated values into account. + if (m_properties.name.has_value()) + return m_properties.name.value().to_string().to_ascii_lowercase(); return serialize_a_srgb_value(to_color({})); } diff --git a/Libraries/LibWeb/CSS/StyleValues/CSSRGB.h b/Libraries/LibWeb/CSS/StyleValues/CSSRGB.h index 6fcfcdc843d..711fcf73699 100644 --- a/Libraries/LibWeb/CSS/StyleValues/CSSRGB.h +++ b/Libraries/LibWeb/CSS/StyleValues/CSSRGB.h @@ -14,13 +14,13 @@ namespace Web::CSS { // https://drafts.css-houdini.org/css-typed-om-1/#cssrgb class CSSRGB final : public CSSColorValue { public: - static ValueComparingNonnullRefPtr create(ValueComparingNonnullRefPtr r, ValueComparingNonnullRefPtr g, ValueComparingNonnullRefPtr b, ValueComparingRefPtr alpha = {}) + static ValueComparingNonnullRefPtr create(ValueComparingNonnullRefPtr r, ValueComparingNonnullRefPtr g, ValueComparingNonnullRefPtr b, ValueComparingRefPtr alpha = {}, Optional name = {}) { // alpha defaults to 1 if (!alpha) - return adopt_ref(*new (nothrow) CSSRGB(move(r), move(g), move(b), NumberStyleValue::create(1))); + return adopt_ref(*new (nothrow) CSSRGB(move(r), move(g), move(b), NumberStyleValue::create(1), name)); - return adopt_ref(*new (nothrow) CSSRGB(move(r), move(g), move(b), alpha.release_nonnull())); + return adopt_ref(*new (nothrow) CSSRGB(move(r), move(g), move(b), alpha.release_nonnull(), name)); } virtual ~CSSRGB() override = default; @@ -36,9 +36,9 @@ public: virtual bool equals(CSSStyleValue const& other) const override; private: - CSSRGB(ValueComparingNonnullRefPtr r, ValueComparingNonnullRefPtr g, ValueComparingNonnullRefPtr b, ValueComparingNonnullRefPtr alpha) + CSSRGB(ValueComparingNonnullRefPtr r, ValueComparingNonnullRefPtr g, ValueComparingNonnullRefPtr b, ValueComparingNonnullRefPtr alpha, Optional name = {}) : CSSColorValue(ColorType::RGB) - , m_properties { .r = move(r), .g = move(g), .b = move(b), .alpha = move(alpha) } + , m_properties { .r = move(r), .g = move(g), .b = move(b), .alpha = move(alpha), .name = name } { } @@ -47,6 +47,7 @@ private: ValueComparingNonnullRefPtr g; ValueComparingNonnullRefPtr b; ValueComparingNonnullRefPtr alpha; + Optional name; bool operator==(Properties const&) const = default; } m_properties; }; diff --git a/Tests/LibWeb/Text/expected/HTML/background-shorthand.txt b/Tests/LibWeb/Text/expected/HTML/background-shorthand.txt index 18ff6d5e507..f7eb47ea13e 100644 --- a/Tests/LibWeb/Text/expected/HTML/background-shorthand.txt +++ b/Tests/LibWeb/Text/expected/HTML/background-shorthand.txt @@ -1,4 +1,4 @@ -style.cssText = background-color: rgb(255, 255, 0); background-image: none; background-position-x: left 0%; background-position-y: top 0%; background-size: auto auto; background-repeat: repeat repeat; background-attachment: scroll; background-origin: padding-box; background-clip: border-box; +style.cssText = background-color: yellow; background-image: none; background-position-x: left 0%; background-position-y: top 0%; background-size: auto auto; background-repeat: repeat repeat; background-attachment: scroll; background-origin: padding-box; background-clip: border-box; style.length = 9 style[] = 1. background-color diff --git a/Tests/LibWeb/Text/expected/css/PropertyOwningCSSStyleDeclaration-serialized-custom-properties.txt b/Tests/LibWeb/Text/expected/css/PropertyOwningCSSStyleDeclaration-serialized-custom-properties.txt index 7e8b5b60fc3..bf9d863e5e5 100644 --- a/Tests/LibWeb/Text/expected/css/PropertyOwningCSSStyleDeclaration-serialized-custom-properties.txt +++ b/Tests/LibWeb/Text/expected/css/PropertyOwningCSSStyleDeclaration-serialized-custom-properties.txt @@ -1 +1 @@ -test { --color: red; color: rgb(255, 0, 0); } +test { --color: red; color: red; } diff --git a/Tests/LibWeb/Text/expected/css/keyframes-css-rules.txt b/Tests/LibWeb/Text/expected/css/keyframes-css-rules.txt index 6e69d05b20e..a0536981944 100644 --- a/Tests/LibWeb/Text/expected/css/keyframes-css-rules.txt +++ b/Tests/LibWeb/Text/expected/css/keyframes-css-rules.txt @@ -1,7 +1,7 @@ -fooRule: [object CSSKeyframesRule] ~ @keyframes "foo" { 0% { color: rgb(0, 0, 0); } 100% { color: rgb(255, 255, 255); } } +fooRule: [object CSSKeyframesRule] ~ @keyframes "foo" { 0% { color: black; } 100% { color: white; } } fooRule.cssRules: [object CSSRuleList] -fooRule.cssRules[0]: [object CSSKeyframeRule] ~ 0% { color: rgb(0, 0, 0); } +fooRule.cssRules[0]: [object CSSKeyframeRule] ~ 0% { color: black; } fooRule.cssRules[0].parentRule: [object CSSKeyframesRule] -fooRule.cssRules[0].parentRule: [object CSSKeyframesRule] ~ @keyframes "foo" { 0% { color: rgb(0, 0, 0); } 100% { color: rgb(255, 255, 255); } } -fooRule.cssRules[0].style.parentRule: [object CSSKeyframeRule] ~ 0% { color: rgb(0, 0, 0); } +fooRule.cssRules[0].parentRule: [object CSSKeyframesRule] ~ @keyframes "foo" { 0% { color: black; } 100% { color: white; } } +fooRule.cssRules[0].style.parentRule: [object CSSKeyframeRule] ~ 0% { color: black; } fooRule.cssRules[0].style.parentRule === fooRule.cssRules[0]: true diff --git a/Tests/LibWeb/Text/expected/css/set-style-declaration-shorthand.txt b/Tests/LibWeb/Text/expected/css/set-style-declaration-shorthand.txt index b6c856ce170..decee2c3ab3 100644 --- a/Tests/LibWeb/Text/expected/css/set-style-declaration-shorthand.txt +++ b/Tests/LibWeb/Text/expected/css/set-style-declaration-shorthand.txt @@ -28,7 +28,7 @@ Setting flex: ''; becomes... Setting border: '1px solid red'; becomes... border-width: '1px 1px 1px 1px' border-style: 'solid solid solid solid' - border-color: 'rgb(255, 0, 0) rgb(255, 0, 0) rgb(255, 0, 0) rgb(255, 0, 0)' + border-color: 'red red red red' border: '' e.style.length: 12 > [0] border-top-width diff --git a/Tests/LibWeb/Text/expected/css/style-declaration-parent-rule.txt b/Tests/LibWeb/Text/expected/css/style-declaration-parent-rule.txt index 4bce08e27ca..1d118bd8862 100644 --- a/Tests/LibWeb/Text/expected/css/style-declaration-parent-rule.txt +++ b/Tests/LibWeb/Text/expected/css/style-declaration-parent-rule.txt @@ -1,4 +1,4 @@ -spanRule: [object CSSStyleRule] ~ span { color: rgb(128, 0, 128); } -spanRule.style: [object CSSStyleDeclaration] ~ span { color: rgb(128, 0, 128); } -spanRule.style.parentRule: [object CSSStyleRule] ~ span { color: rgb(128, 0, 128); } +spanRule: [object CSSStyleRule] ~ span { color: purple; } +spanRule.style: [object CSSStyleDeclaration] ~ span { color: purple; } +spanRule.style.parentRule: [object CSSStyleRule] ~ span { color: purple; } spanRule.style.parentRule === spanRule: true diff --git a/Tests/LibWeb/Text/expected/wpt-import/css/css-nesting/cssom.txt b/Tests/LibWeb/Text/expected/wpt-import/css/css-nesting/cssom.txt index d18a98339e2..560f8485d1c 100644 --- a/Tests/LibWeb/Text/expected/wpt-import/css/css-nesting/cssom.txt +++ b/Tests/LibWeb/Text/expected/wpt-import/css/css-nesting/cssom.txt @@ -6,19 +6,19 @@ Rerun Found 13 tests -1 Pass -12 Fail +11 Pass +2 Fail Details Result Test Name MessageFail CSSStyleRule is a CSSGroupingRule -Fail Simple CSSOM manipulation of subrules -Fail Simple CSSOM manipulation of subrules 1 -Fail Simple CSSOM manipulation of subrules 2 -Fail Simple CSSOM manipulation of subrules 3 -Fail Simple CSSOM manipulation of subrules 4 -Fail Simple CSSOM manipulation of subrules 5 -Fail Simple CSSOM manipulation of subrules 6 -Fail Simple CSSOM manipulation of subrules 7 +Pass Simple CSSOM manipulation of subrules +Pass Simple CSSOM manipulation of subrules 1 +Pass Simple CSSOM manipulation of subrules 2 +Pass Simple CSSOM manipulation of subrules 3 +Pass Simple CSSOM manipulation of subrules 4 +Pass Simple CSSOM manipulation of subrules 5 +Pass Simple CSSOM manipulation of subrules 6 +Pass Simple CSSOM manipulation of subrules 7 Fail Simple CSSOM manipulation of subrules 8 -Fail Simple CSSOM manipulation of subrules 9 -Fail Simple CSSOM manipulation of subrules 10 +Pass Simple CSSOM manipulation of subrules 9 +Pass Simple CSSOM manipulation of subrules 10 Pass Mutating the selectorText of outer rule invalidates inner rules \ No newline at end of file diff --git a/Tests/LibWeb/Text/expected/wpt-import/css/css-nesting/invalid-inner-rules.txt b/Tests/LibWeb/Text/expected/wpt-import/css/css-nesting/invalid-inner-rules.txt index cf937029dba..ccca4780166 100644 --- a/Tests/LibWeb/Text/expected/wpt-import/css/css-nesting/invalid-inner-rules.txt +++ b/Tests/LibWeb/Text/expected/wpt-import/css/css-nesting/invalid-inner-rules.txt @@ -6,7 +6,8 @@ Rerun Found 2 tests -2 Fail +1 Pass +1 Fail Details -Result Test Name MessageFail Simple CSSOM manipulation of subrules +Result Test Name MessagePass Simple CSSOM manipulation of subrules Fail Simple CSSOM manipulation of subrules 1 \ No newline at end of file diff --git a/Tests/LibWeb/Text/expected/wpt-import/css/css-nesting/serialize-group-rules-with-decls.txt b/Tests/LibWeb/Text/expected/wpt-import/css/css-nesting/serialize-group-rules-with-decls.txt index 6f17ed4a441..07b3d7aca72 100644 --- a/Tests/LibWeb/Text/expected/wpt-import/css/css-nesting/serialize-group-rules-with-decls.txt +++ b/Tests/LibWeb/Text/expected/wpt-import/css/css-nesting/serialize-group-rules-with-decls.txt @@ -6,20 +6,19 @@ Rerun Found 15 tests -4 Pass -11 Fail +15 Pass Details -Result Test Name MessageFail Declarations are serialized on one line, rules on two. -Fail Mixed declarations/rules are on two lines. -Fail Implicit rule is serialized -Fail Implicit rule not removed -Fail Implicit + empty hover rule -Fail Implicit like rule not in first position -Fail Two implicit-like rules -Fail Implicit like rule after decls -Fail Implicit like rule after decls, missing closing braces -Fail Implicit like rule with other selectors -Fail Implicit-like rule in style rule +Result Test Name MessagePass Declarations are serialized on one line, rules on two. +Pass Mixed declarations/rules are on two lines. +Pass Implicit rule is serialized +Pass Implicit rule not removed +Pass Implicit + empty hover rule +Pass Implicit like rule not in first position +Pass Two implicit-like rules +Pass Implicit like rule after decls +Pass Implicit like rule after decls, missing closing braces +Pass Implicit like rule with other selectors +Pass Implicit-like rule in style rule Pass Empty conditional rule Pass Empty style rule Pass Empty conditional inside style rule diff --git a/Tests/LibWeb/Text/expected/wpt-import/domparsing/style_attribute_html.txt b/Tests/LibWeb/Text/expected/wpt-import/domparsing/style_attribute_html.txt new file mode 100644 index 00000000000..349d7ed6d4d --- /dev/null +++ b/Tests/LibWeb/Text/expected/wpt-import/domparsing/style_attribute_html.txt @@ -0,0 +1,15 @@ +Summary + +Harness status: OK + +Rerun + +Found 4 tests + +2 Pass +2 Fail +Details +Result Test Name MessagePass Parsing of initial style attribute +Fail Parsing of invalid style attribute +Fail Parsing of style attribute +Pass Update style.backgroundColor \ No newline at end of file diff --git a/Tests/LibWeb/Text/input/wpt-import/domparsing/style_attribute_html.html b/Tests/LibWeb/Text/input/wpt-import/domparsing/style_attribute_html.html new file mode 100644 index 00000000000..fa34a2cf529 --- /dev/null +++ b/Tests/LibWeb/Text/input/wpt-import/domparsing/style_attribute_html.html @@ -0,0 +1,52 @@ + + +Style attribute in HTML + + +