From 5a1eb9e220bd32ca8c91fd7d49ba1d4f676a21e4 Mon Sep 17 00:00:00 2001 From: Sam Atkins Date: Wed, 13 Nov 2024 15:49:43 +0000 Subject: [PATCH] LibWeb/CSS: Keep invalid parts of ``s around Attempt 2! Reverts 2a5dbedad4e76fbaaab4ba6a6e0e0a740260af05 This time, set up a different combinator when producing a relative invalid selector rather than a standalone one. This fixes the crash. Original description below for simplicity because it still applies. --- Selectors like `:is(.valid, &!?!?!invalid)` need to keep the invalid part around, even though it will never match, for a couple of reasons: - Serialization needs to include them - For nesting, we care if a `&` appeared anywhere in the selector, even in an invalid part. So this patch introduces an `Invalid` simple selector type, which simply holds its original ComponentValues. We search through these looking for `&`, and we dump them out directly when asked to serialize. --- .../LibWeb/CSS/Parser/SelectorParsing.cpp | 29 ++++++++++++- Libraries/LibWeb/CSS/Selector.cpp | 43 +++++++++++++++++++ Libraries/LibWeb/CSS/Selector.h | 8 +++- Libraries/LibWeb/CSS/SelectorEngine.cpp | 3 ++ Libraries/LibWeb/Dump.cpp | 11 +++++ Tests/LibWeb/TestConfig.ini | 1 - .../serialize-invalid-forgiving-selectors.txt | 2 + ...serialize-invalid-forgiving-selectors.html | 17 ++++++++ 8 files changed, 111 insertions(+), 3 deletions(-) create mode 100644 Tests/LibWeb/Text/expected/css/serialize-invalid-forgiving-selectors.txt create mode 100644 Tests/LibWeb/Text/input/css/serialize-invalid-forgiving-selectors.html diff --git a/Libraries/LibWeb/CSS/Parser/SelectorParsing.cpp b/Libraries/LibWeb/CSS/Parser/SelectorParsing.cpp index 16762e45a2f..79734280fe2 100644 --- a/Libraries/LibWeb/CSS/Parser/SelectorParsing.cpp +++ b/Libraries/LibWeb/CSS/Parser/SelectorParsing.cpp @@ -60,6 +60,29 @@ Optional Parser::parse_as_pseudo_element_selector() return simple_selector.pseudo_element(); } +static NonnullRefPtr create_invalid_selector(Selector::Combinator combinator, Vector component_values) +{ + // Trim leading and trailing whitespace + while (!component_values.is_empty() && component_values.first().is(Token::Type::Whitespace)) { + component_values.take_first(); + } + while (!component_values.is_empty() && component_values.last().is(Token::Type::Whitespace)) { + component_values.take_last(); + } + + Selector::SimpleSelector simple { + .type = Selector::SimpleSelector::Type::Invalid, + .value = Selector::SimpleSelector::Invalid { + .component_values = move(component_values), + } + }; + Selector::CompoundSelector compound { + .combinator = combinator, + .simple_selectors = { move(simple) } + }; + return Selector::create({ move(compound) }); +} + template Parser::ParseErrorOr Parser::parse_a_selector_list(TokenStream& tokens, SelectorType mode, SelectorParsingMode parsing_mode) { @@ -70,8 +93,12 @@ Parser::ParseErrorOr Parser::parse_a_selector_list(TokenStream& auto stream = TokenStream(selector_parts); auto selector = parse_complex_selector(stream, mode); if (selector.is_error()) { - if (parsing_mode == SelectorParsingMode::Forgiving) + if (parsing_mode == SelectorParsingMode::Forgiving) { + // Keep the invalid selector around for serialization and nesting + auto combinator = mode == SelectorType::Standalone ? Selector::Combinator::None : Selector::Combinator::Descendant; + selectors.append(create_invalid_selector(combinator, move(selector_parts))); continue; + } return selector.error(); } selectors.append(selector.release_value()); diff --git a/Libraries/LibWeb/CSS/Selector.cpp b/Libraries/LibWeb/CSS/Selector.cpp index 266391e4f8a..2282ff08451 100644 --- a/Libraries/LibWeb/CSS/Selector.cpp +++ b/Libraries/LibWeb/CSS/Selector.cpp @@ -11,6 +11,28 @@ namespace Web::CSS { +static bool component_value_contains_nesting_selector(Parser::ComponentValue const& component_value) +{ + if (component_value.is_delim('&')) + return true; + + if (component_value.is_block()) { + for (auto const& child_value : component_value.block().value) { + if (component_value_contains_nesting_selector(child_value)) + return true; + } + } + + if (component_value.is_function()) { + for (auto const& child_value : component_value.function().value) { + if (component_value_contains_nesting_selector(child_value)) + return true; + } + } + + return false; +} + Selector::Selector(Vector&& compound_selectors) : m_compound_selectors(move(compound_selectors)) { @@ -44,6 +66,17 @@ Selector::Selector(Vector&& compound_selectors) if (m_contains_the_nesting_selector) break; } + if (simple_selector.type == SimpleSelector::Type::Invalid) { + auto& invalid = simple_selector.value.get(); + for (auto& item : invalid.component_values) { + if (component_value_contains_nesting_selector(item)) { + m_contains_the_nesting_selector = true; + break; + } + } + if (m_contains_the_nesting_selector) + break; + } } if (m_contains_the_nesting_selector) break; @@ -184,6 +217,9 @@ u32 Selector::specificity() const // The parented case is handled by replacing & with :is(). // So if we got here, the specificity is 0. break; + case SimpleSelector::Type::Invalid: + // Ignore invalid selectors + break; } } } @@ -360,6 +396,12 @@ String Selector::SimpleSelector::serialize() const // AD-HOC: Not in spec yet. s.append('&'); break; + case Type::Invalid: + // AD-HOC: We're not told how to do these. Just serialize their component values. + auto invalid = value.get(); + for (auto const& component_value : invalid.component_values) + s.append(component_value.to_string()); + break; } return MUST(s.to_string()); } @@ -602,6 +644,7 @@ Selector::SimpleSelector Selector::SimpleSelector::absolutized(Selector::SimpleS case Type::Class: case Type::Attribute: case Type::PseudoElement: + case Type::Invalid: // Everything else isn't affected return *this; } diff --git a/Libraries/LibWeb/CSS/Selector.h b/Libraries/LibWeb/CSS/Selector.h index 3e0a1055c3f..b8d2425861a 100644 --- a/Libraries/LibWeb/CSS/Selector.h +++ b/Libraries/LibWeb/CSS/Selector.h @@ -12,6 +12,7 @@ #include #include #include +#include #include namespace Web::CSS { @@ -92,6 +93,7 @@ public: PseudoClass, PseudoElement, Nesting, + Invalid, }; struct ANPlusBPattern { @@ -195,8 +197,12 @@ public: CaseType case_type; }; + struct Invalid { + Vector component_values; + }; + Type type; - Variant value {}; + Variant value {}; Attribute const& attribute() const { return value.get(); } Attribute& attribute() { return value.get(); } diff --git a/Libraries/LibWeb/CSS/SelectorEngine.cpp b/Libraries/LibWeb/CSS/SelectorEngine.cpp index 6802fa73cc1..1a9f3b30849 100644 --- a/Libraries/LibWeb/CSS/SelectorEngine.cpp +++ b/Libraries/LibWeb/CSS/SelectorEngine.cpp @@ -764,6 +764,9 @@ static inline bool matches(CSS::Selector::SimpleSelector const& component, Optio // :is() is handled already, by us replacing it with :is() directly, so if we // got here, it's :scope. return matches_pseudo_class(CSS::Selector::SimpleSelector::PseudoClassSelector { .type = CSS::PseudoClass::Scope }, style_sheet_for_rule, element, shadow_host, scope, selector_kind); + case CSS::Selector::SimpleSelector::Type::Invalid: + // Invalid selectors never match + return false; } VERIFY_NOT_REACHED(); } diff --git a/Libraries/LibWeb/Dump.cpp b/Libraries/LibWeb/Dump.cpp index 0632bcd734a..7de37a60911 100644 --- a/Libraries/LibWeb/Dump.cpp +++ b/Libraries/LibWeb/Dump.cpp @@ -506,6 +506,9 @@ void dump_selector(StringBuilder& builder, CSS::Selector const& selector, int in case CSS::Selector::SimpleSelector::Type::Nesting: type_description = "Nesting"; break; + case CSS::Selector::SimpleSelector::Type::Invalid: + type_description = "INVALID"; + break; } builder.appendff("{}:", type_description); @@ -599,6 +602,14 @@ void dump_selector(StringBuilder& builder, CSS::Selector const& selector, int in builder.appendff(", value='{}']", attribute.value); } + if (simple_selector.type == CSS::Selector::SimpleSelector::Type::Invalid) { + auto invalid = simple_selector.value.get(); + builder.append(" '"sv); + for (auto const& component_value : invalid.component_values) + builder.append(component_value.to_string()); + builder.append("'"sv); + } + if (i != relative_selector.simple_selectors.size() - 1) builder.append(", "sv); } diff --git a/Tests/LibWeb/TestConfig.ini b/Tests/LibWeb/TestConfig.ini index 63ad0d7874a..15e49763f3d 100644 --- a/Tests/LibWeb/TestConfig.ini +++ b/Tests/LibWeb/TestConfig.ini @@ -61,7 +61,6 @@ Text/input/wpt-import/css/css-flexbox/flex-item-compressible-001.html Ref/input/wpt-import/css/css-nesting/has-nesting.html Ref/input/wpt-import/css/css-nesting/host-nesting-003.html Ref/input/wpt-import/css/css-nesting/host-nesting-004.html -Ref/input/wpt-import/css/css-nesting/nest-containing-forgiving.html Ref/input/wpt-import/css/CSS2/floats/floats-wrap-top-below-bfc-002r.xht Ref/input/wpt-import/css/CSS2/floats/float-nowrap-3.html Ref/input/wpt-import/css/CSS2/floats/overflow-scroll-float-paint-order.html diff --git a/Tests/LibWeb/Text/expected/css/serialize-invalid-forgiving-selectors.txt b/Tests/LibWeb/Text/expected/css/serialize-invalid-forgiving-selectors.txt new file mode 100644 index 00000000000..3a4510100c4 --- /dev/null +++ b/Tests/LibWeb/Text/expected/css/serialize-invalid-forgiving-selectors.txt @@ -0,0 +1,2 @@ +:is(hello, !?) -> :is(hello, !?) +:is(what, :fake(:imaginary), really) -> :is(what, :fake(:imaginary), really) diff --git a/Tests/LibWeb/Text/input/css/serialize-invalid-forgiving-selectors.html b/Tests/LibWeb/Text/input/css/serialize-invalid-forgiving-selectors.html new file mode 100644 index 00000000000..d08b4ae3fb7 --- /dev/null +++ b/Tests/LibWeb/Text/input/css/serialize-invalid-forgiving-selectors.html @@ -0,0 +1,17 @@ + + + +