From a0403ac427785e8bae8c3b61cbba65e7356134d9 Mon Sep 17 00:00:00 2001 From: Sam Atkins Date: Fri, 8 Nov 2024 17:50:38 +0000 Subject: [PATCH] LibWeb: Add & when setting nested style rule's selector text When we first parse a nested CSSStyleRule's selectors, we treat them as relative selectors and then patch them up with an `&` as needed. However, we weren't doing this when assigning the `cssText` attribute. So, let's do that! This gives us a couple of subtest passes. :^) --- .../nested-declarations-matching.txt | 5 +-- .../nested-rule-cssom-invalidation.txt | 4 +-- .../Libraries/LibWeb/CSS/CSSStyleRule.cpp | 28 ++++++++++----- Userland/Libraries/LibWeb/CSS/CSSStyleRule.h | 2 ++ .../Libraries/LibWeb/CSS/Parser/Helpers.cpp | 11 ++++++ Userland/Libraries/LibWeb/CSS/Parser/Parser.h | 1 + .../LibWeb/CSS/Parser/RuleParsing.cpp | 35 ++----------------- Userland/Libraries/LibWeb/CSS/Selector.cpp | 35 +++++++++++++++++++ Userland/Libraries/LibWeb/CSS/Selector.h | 2 ++ 9 files changed, 77 insertions(+), 46 deletions(-) diff --git a/Tests/LibWeb/Text/expected/wpt-import/css/css-nesting/nested-declarations-matching.txt b/Tests/LibWeb/Text/expected/wpt-import/css/css-nesting/nested-declarations-matching.txt index 17b3bb317f2..31f2d4ea680 100644 --- a/Tests/LibWeb/Text/expected/wpt-import/css/css-nesting/nested-declarations-matching.txt +++ b/Tests/LibWeb/Text/expected/wpt-import/css/css-nesting/nested-declarations-matching.txt @@ -6,7 +6,8 @@ Rerun Found 11 tests -11 Fail +1 Pass +10 Fail Details Result Test Name MessageFail Trailing declarations apply after any preceding rules Fail Trailing declarations apply after any preceding rules (no leading) @@ -18,4 +19,4 @@ Fail Bare declartaion in nested grouping rule can match pseudo-element Fail Nested group rules have top-level specificity behavior Fail Nested @scope rules behave like :where(:scope) Fail Nested @scope rules behave like :where(:scope) (trailing) -Fail Nested declarations rule responds to parent selector text change \ No newline at end of file +Pass Nested declarations rule responds to parent selector text change \ No newline at end of file diff --git a/Tests/LibWeb/Text/expected/wpt-import/css/css-nesting/nested-rule-cssom-invalidation.txt b/Tests/LibWeb/Text/expected/wpt-import/css/css-nesting/nested-rule-cssom-invalidation.txt index 0b119b35299..1928733e4c1 100644 --- a/Tests/LibWeb/Text/expected/wpt-import/css/css-nesting/nested-rule-cssom-invalidation.txt +++ b/Tests/LibWeb/Text/expected/wpt-import/css/css-nesting/nested-rule-cssom-invalidation.txt @@ -6,6 +6,6 @@ Rerun Found 1 tests -1 Fail +1 Pass Details -Result Test Name MessageFail Nested rule responds to parent selector text change \ No newline at end of file +Result Test Name MessagePass Nested rule responds to parent selector text change \ No newline at end of file diff --git a/Userland/Libraries/LibWeb/CSS/CSSStyleRule.cpp b/Userland/Libraries/LibWeb/CSS/CSSStyleRule.cpp index 35634bb27a5..e7cfc437045 100644 --- a/Userland/Libraries/LibWeb/CSS/CSSStyleRule.cpp +++ b/Userland/Libraries/LibWeb/CSS/CSSStyleRule.cpp @@ -125,10 +125,18 @@ void CSSStyleRule::set_selector_text(StringView selector_text) clear_caches(); // 1. Run the parse a group of selectors algorithm on the given value. - auto parsed_selectors = parse_selector(Parser::ParsingContext { realm() }, selector_text); + Optional parsed_selectors; + if (parent_style_rule()) { + // AD-HOC: If we're a nested style rule, then we need to parse the selector as relative and then adapt it with implicit &s. + parsed_selectors = parse_selector_for_nested_style_rule(Parser::ParsingContext { realm() }, selector_text); + } else { + parsed_selectors = parse_selector(Parser::ParsingContext { realm() }, selector_text); + } // 2. If the algorithm returns a non-null value replace the associated group of selectors with the returned value. if (parsed_selectors.has_value()) { + // NOTE: If we have a parent style rule, we need to update the selectors to add any implicit `&`s + m_selectors = parsed_selectors.release_value(); if (auto* sheet = parent_style_sheet()) { if (auto style_sheet_list = sheet->style_sheet_list()) { @@ -169,15 +177,8 @@ SelectorList const& CSSStyleRule::absolutized_selectors() const // "When used in the selector of a nested style rule, the nesting selector represents the elements matched by the parent rule. // When used in any other context, it represents the same elements as :scope in that context (unless otherwise defined)." // https://drafts.csswg.org/css-nesting-1/#nest-selector - CSSStyleRule const* parent_style_rule = nullptr; - for (auto* parent = parent_rule(); parent; parent = parent->parent_rule()) { - if (parent->type() == CSSStyleRule::Type::Style) { - parent_style_rule = static_cast(parent); - break; - } - } Selector::SimpleSelector parent_selector; - if (parent_style_rule) { + if (auto const* parent_style_rule = this->parent_style_rule()) { // TODO: If there's only 1, we don't have to use `:is()` for it parent_selector = { .type = Selector::SimpleSelector::Type::PseudoClass, @@ -207,4 +208,13 @@ void CSSStyleRule::clear_caches() m_cached_absolutized_selectors.clear(); } +CSSStyleRule const* CSSStyleRule::parent_style_rule() const +{ + for (auto* parent = parent_rule(); parent; parent = parent->parent_rule()) { + if (parent->type() == CSSStyleRule::Type::Style) + return static_cast(parent); + } + return nullptr; +} + } diff --git a/Userland/Libraries/LibWeb/CSS/CSSStyleRule.h b/Userland/Libraries/LibWeb/CSS/CSSStyleRule.h index 94d143e3c22..f416c2f9705 100644 --- a/Userland/Libraries/LibWeb/CSS/CSSStyleRule.h +++ b/Userland/Libraries/LibWeb/CSS/CSSStyleRule.h @@ -42,6 +42,8 @@ private: virtual void clear_caches() override; virtual String serialized() const override; + CSSStyleRule const* parent_style_rule() const; + SelectorList m_selectors; mutable Optional m_cached_absolutized_selectors; JS::NonnullGCPtr m_declaration; diff --git a/Userland/Libraries/LibWeb/CSS/Parser/Helpers.cpp b/Userland/Libraries/LibWeb/CSS/Parser/Helpers.cpp index 7bc9e6d3a61..ebd84000d0e 100644 --- a/Userland/Libraries/LibWeb/CSS/Parser/Helpers.cpp +++ b/Userland/Libraries/LibWeb/CSS/Parser/Helpers.cpp @@ -54,6 +54,17 @@ Optional parse_selector(CSS::Parser::ParsingContext const& co return CSS::Parser::Parser::create(context, selector_text).parse_as_selector(); } +Optional parse_selector_for_nested_style_rule(CSS::Parser::ParsingContext const& context, StringView selector_text) +{ + auto parser = CSS::Parser::Parser::create(context, selector_text); + + auto maybe_selectors = parser.parse_as_relative_selector(CSS::Parser::Parser::SelectorParsingMode::Standard); + if (!maybe_selectors.has_value()) + return {}; + + return adapt_nested_relative_selector_list(*maybe_selectors); +} + Optional parse_pseudo_element_selector(CSS::Parser::ParsingContext const& context, StringView selector_text) { return CSS::Parser::Parser::create(context, selector_text).parse_as_pseudo_element_selector(); diff --git a/Userland/Libraries/LibWeb/CSS/Parser/Parser.h b/Userland/Libraries/LibWeb/CSS/Parser/Parser.h index eab5a26233a..f737e23a6f9 100644 --- a/Userland/Libraries/LibWeb/CSS/Parser/Parser.h +++ b/Userland/Libraries/LibWeb/CSS/Parser/Parser.h @@ -423,6 +423,7 @@ CSS::CSSStyleSheet* parse_css_stylesheet(CSS::Parser::ParsingContext const&, Str CSS::ElementInlineCSSStyleDeclaration* parse_css_style_attribute(CSS::Parser::ParsingContext const&, StringView, DOM::Element&); RefPtr parse_css_value(CSS::Parser::ParsingContext const&, StringView, CSS::PropertyID property_id = CSS::PropertyID::Invalid); Optional parse_selector(CSS::Parser::ParsingContext const&, StringView); +Optional parse_selector_for_nested_style_rule(CSS::Parser::ParsingContext const&, StringView); Optional parse_pseudo_element_selector(CSS::Parser::ParsingContext const&, StringView); CSS::CSSRule* parse_css_rule(CSS::Parser::ParsingContext const&, StringView); RefPtr parse_media_query(CSS::Parser::ParsingContext const&, StringView); diff --git a/Userland/Libraries/LibWeb/CSS/Parser/RuleParsing.cpp b/Userland/Libraries/LibWeb/CSS/Parser/RuleParsing.cpp index b36b13c2853..dc9f4dcdfd5 100644 --- a/Userland/Libraries/LibWeb/CSS/Parser/RuleParsing.cpp +++ b/Userland/Libraries/LibWeb/CSS/Parser/RuleParsing.cpp @@ -98,39 +98,8 @@ JS::GCPtr Parser::convert_to_style_rule(QualifiedRule const& quali } SelectorList selectors = maybe_selectors.release_value(); - if (nested == Nested::Yes) { - // "Nested style rules differ from non-nested rules in the following ways: - // - A nested style rule accepts a as its prelude (rather than just a ). - // Any relative selectors are relative to the elements represented by the nesting selector. - // - If a selector in the does not start with a combinator but does contain the nesting - // selector, it is interpreted as a non-relative selector." - // https://drafts.csswg.org/css-nesting-1/#syntax - // NOTE: We already parsed the selectors as a - - // Nested relative selectors get a `&` inserted at the beginning. - // This is, handily, how the spec wants them serialized: - // "When serializing a relative selector in a nested style rule, the selector must be absolutized, - // with the implied nesting selector inserted." - // - https://drafts.csswg.org/css-nesting-1/#cssom - - SelectorList new_list; - new_list.ensure_capacity(selectors.size()); - for (auto const& selector : selectors) { - auto first_combinator = selector->compound_selectors().first().combinator; - if (!first_is_one_of(first_combinator, Selector::Combinator::None, Selector::Combinator::Descendant) - || !selector->contains_the_nesting_selector()) { - new_list.append(selector->relative_to(Selector::SimpleSelector { .type = Selector::SimpleSelector::Type::Nesting })); - } else if (first_combinator == Selector::Combinator::Descendant) { - // Replace leading descendant combinator (whitespace) with none, because we're not actually relative. - auto copied_compound_selectors = selector->compound_selectors(); - copied_compound_selectors.first().combinator = Selector::Combinator::None; - new_list.append(Selector::create(move(copied_compound_selectors))); - } else { - new_list.append(selector); - } - } - selectors = move(new_list); - } + if (nested == Nested::Yes) + selectors = adapt_nested_relative_selector_list(selectors); auto* declaration = convert_to_style_declaration(qualified_rule.declarations); if (!declaration) { diff --git a/Userland/Libraries/LibWeb/CSS/Selector.cpp b/Userland/Libraries/LibWeb/CSS/Selector.cpp index e9ccd2c2dd8..17cf972e91e 100644 --- a/Userland/Libraries/LibWeb/CSS/Selector.cpp +++ b/Userland/Libraries/LibWeb/CSS/Selector.cpp @@ -606,4 +606,39 @@ Selector::SimpleSelector Selector::SimpleSelector::absolutized(Selector::SimpleS VERIFY_NOT_REACHED(); } +SelectorList adapt_nested_relative_selector_list(SelectorList const& selectors) +{ + // "Nested style rules differ from non-nested rules in the following ways: + // - A nested style rule accepts a as its prelude (rather than just a ). + // Any relative selectors are relative to the elements represented by the nesting selector. + // - If a selector in the does not start with a combinator but does contain the nesting + // selector, it is interpreted as a non-relative selector." + // https://drafts.csswg.org/css-nesting-1/#syntax + // NOTE: We already parsed the selectors as a + + // Nested relative selectors get a `&` inserted at the beginning. + // This is, handily, how the spec wants them serialized: + // "When serializing a relative selector in a nested style rule, the selector must be absolutized, + // with the implied nesting selector inserted." + // - https://drafts.csswg.org/css-nesting-1/#cssom + + CSS::SelectorList new_list; + new_list.ensure_capacity(selectors.size()); + for (auto const& selector : selectors) { + auto first_combinator = selector->compound_selectors().first().combinator; + if (!first_is_one_of(first_combinator, CSS::Selector::Combinator::None, CSS::Selector::Combinator::Descendant) + || !selector->contains_the_nesting_selector()) { + new_list.append(selector->relative_to(CSS::Selector::SimpleSelector { .type = CSS::Selector::SimpleSelector::Type::Nesting })); + } else if (first_combinator == CSS::Selector::Combinator::Descendant) { + // Replace leading descendant combinator (whitespace) with none, because we're not actually relative. + auto copied_compound_selectors = selector->compound_selectors(); + copied_compound_selectors.first().combinator = CSS::Selector::Combinator::None; + new_list.append(CSS::Selector::create(move(copied_compound_selectors))); + } else { + new_list.append(selector); + } + } + return new_list; +} + } diff --git a/Userland/Libraries/LibWeb/CSS/Selector.h b/Userland/Libraries/LibWeb/CSS/Selector.h index e7c772971e8..3e0a1055c3f 100644 --- a/Userland/Libraries/LibWeb/CSS/Selector.h +++ b/Userland/Libraries/LibWeb/CSS/Selector.h @@ -267,6 +267,8 @@ private: String serialize_a_group_of_selectors(SelectorList const& selectors); +SelectorList adapt_nested_relative_selector_list(SelectorList const&); + } namespace AK {