From 6ea5d03f43ab9b0ef58f143a44a6fedd7c36c0e9 Mon Sep 17 00:00:00 2001 From: Sam Atkins Date: Fri, 23 Jul 2021 15:24:33 +0100 Subject: [PATCH] LibWeb: Bring Selector terminology in line with the CSS spec - CompoundSelector -> *deleted* - ComplexSelector -> CompoundSelector - Relation -> Combinator Our Selector is really a ComplexSelector, but only the Parser and SelectorEngine need to know that, so keeping it named Selector makes it more understandable for users. Our CompoundSelector is really a CompoundSelectorAndCombinator. Combining the two makes sense in our codebase, but the accurate name is so long that I think it makes the code less readable. Renamed some Combinators to also match the spec terminology: - AdjacentSibling -> NextSibling - GeneralSibling -> SubsequentSibling The previous names are somewhat ambiguous, so hopefully this is clearer. --- .../LibWeb/CSS/Parser/DeprecatedCSSParser.cpp | 16 +++---- .../Libraries/LibWeb/CSS/Parser/Parser.cpp | 18 ++++---- Userland/Libraries/LibWeb/CSS/Selector.cpp | 8 ++-- Userland/Libraries/LibWeb/CSS/Selector.h | 42 ++++++++++--------- .../Libraries/LibWeb/CSS/SelectorEngine.cpp | 24 +++++------ Userland/Libraries/LibWeb/Dump.cpp | 22 +++++----- 6 files changed, 67 insertions(+), 63 deletions(-) diff --git a/Userland/Libraries/LibWeb/CSS/Parser/DeprecatedCSSParser.cpp b/Userland/Libraries/LibWeb/CSS/Parser/DeprecatedCSSParser.cpp index 0163fd733b..1813f6df62 100644 --- a/Userland/Libraries/LibWeb/CSS/Parser/DeprecatedCSSParser.cpp +++ b/Userland/Libraries/LibWeb/CSS/Parser/DeprecatedCSSParser.cpp @@ -1081,9 +1081,9 @@ public: return simple_selector; } - Optional parse_complex_selector() + Optional parse_complex_selector() { - auto relation = CSS::Selector::ComplexSelector::Relation::Descendant; + auto relation = CSS::Selector::Combinator::Descendant; if (peek() == '{' || peek() == ',') return {}; @@ -1091,13 +1091,13 @@ public: if (is_combinator(peek())) { switch (peek()) { case '>': - relation = CSS::Selector::ComplexSelector::Relation::ImmediateChild; + relation = CSS::Selector::Combinator::ImmediateChild; break; case '+': - relation = CSS::Selector::ComplexSelector::Relation::AdjacentSibling; + relation = CSS::Selector::Combinator::NextSibling; break; case '~': - relation = CSS::Selector::ComplexSelector::Relation::GeneralSibling; + relation = CSS::Selector::Combinator::SubsequentSibling; break; } consume_one(); @@ -1119,12 +1119,12 @@ public: if (simple_selectors.is_empty()) return {}; - return CSS::Selector::ComplexSelector { relation, move(simple_selectors) }; + return CSS::Selector::CompoundSelector { relation, move(simple_selectors) }; } void parse_selector() { - Vector complex_selectors; + Vector complex_selectors; for (;;) { auto index_before = index; @@ -1141,7 +1141,7 @@ public: if (complex_selectors.is_empty()) return; - complex_selectors.first().relation = CSS::Selector::ComplexSelector::Relation::None; + complex_selectors.first().combinator = CSS::Selector::Combinator::None; current_rule.selectors.append(CSS::Selector::create(move(complex_selectors))); } diff --git a/Userland/Libraries/LibWeb/CSS/Parser/Parser.cpp b/Userland/Libraries/LibWeb/CSS/Parser/Parser.cpp index 549b0c3c7a..582d3b2c43 100644 --- a/Userland/Libraries/LibWeb/CSS/Parser/Parser.cpp +++ b/Userland/Libraries/LibWeb/CSS/Parser/Parser.cpp @@ -229,7 +229,7 @@ RefPtr Parser::parse_single_selector(TokenStream& tokens, bool is_r // FIXME: Bring this all in line with the spec. https://www.w3.org/TR/selectors-4/ - Vector selectors; + Vector selectors; auto check_for_eof_or_whitespace = [&](T& current_value) -> bool { if (current_value.is(Token::Type::EndOfFile)) @@ -491,8 +491,8 @@ RefPtr Parser::parse_single_selector(TokenStream& tokens, bool is_r return simple_selector; }; - auto parse_complex_selector = [&]() -> Optional { - auto relation = Selector::ComplexSelector::Relation::Descendant; + auto parse_complex_selector = [&]() -> Optional { + auto combinator = Selector::Combinator::Descendant; tokens.skip_whitespace(); @@ -500,13 +500,13 @@ RefPtr Parser::parse_single_selector(TokenStream& tokens, bool is_r if (current_value.is(Token::Type::Delim)) { auto delim = ((Token)current_value).delim(); if (delim == ">") { - relation = Selector::ComplexSelector::Relation::ImmediateChild; + combinator = Selector::Combinator::ImmediateChild; tokens.next_token(); } else if (delim == "+") { - relation = Selector::ComplexSelector::Relation::AdjacentSibling; + combinator = Selector::Combinator::NextSibling; tokens.next_token(); } else if (delim == "~") { - relation = Selector::ComplexSelector::Relation::GeneralSibling; + combinator = Selector::Combinator::SubsequentSibling; tokens.next_token(); } else if (delim == "|") { tokens.next_token(); @@ -516,7 +516,7 @@ RefPtr Parser::parse_single_selector(TokenStream& tokens, bool is_r return {}; if (next.is(Token::Type::Delim) && next.token().delim() == "|") { - relation = Selector::ComplexSelector::Relation::Column; + combinator = Selector::Combinator::Column; tokens.next_token(); } } @@ -541,7 +541,7 @@ RefPtr Parser::parse_single_selector(TokenStream& tokens, bool is_r if (simple_selectors.is_empty()) return {}; - return Selector::ComplexSelector { relation, move(simple_selectors) }; + return Selector::CompoundSelector { combinator, move(simple_selectors) }; }; for (;;) { @@ -558,7 +558,7 @@ RefPtr Parser::parse_single_selector(TokenStream& tokens, bool is_r return {}; if (!is_relative) - selectors.first().relation = Selector::ComplexSelector::Relation::None; + selectors.first().combinator = Selector::Combinator::None; return Selector::create(move(selectors)); } diff --git a/Userland/Libraries/LibWeb/CSS/Selector.cpp b/Userland/Libraries/LibWeb/CSS/Selector.cpp index aac8481329..0ea5a6834c 100644 --- a/Userland/Libraries/LibWeb/CSS/Selector.cpp +++ b/Userland/Libraries/LibWeb/CSS/Selector.cpp @@ -11,8 +11,8 @@ namespace Web::CSS { -Selector::Selector(Vector&& component_lists) - : m_complex_selectors(move(component_lists)) +Selector::Selector(Vector&& compound_selectors) + : m_compound_selectors(move(compound_selectors)) { } @@ -26,8 +26,8 @@ u32 Selector::specificity() const unsigned tag_names = 0; unsigned classes = 0; - for (auto& list : m_complex_selectors) { - for (auto& simple_selector : list.compound_selector) { + for (auto& list : m_compound_selectors) { + for (auto& simple_selector : list.simple_selectors) { switch (simple_selector.type) { case SimpleSelector::Type::Id: ++ids; diff --git a/Userland/Libraries/LibWeb/CSS/Selector.h b/Userland/Libraries/LibWeb/CSS/Selector.h index 12f5f9f358..981a07ff6e 100644 --- a/Userland/Libraries/LibWeb/CSS/Selector.h +++ b/Userland/Libraries/LibWeb/CSS/Selector.h @@ -15,6 +15,9 @@ namespace Web::CSS { +using SelectorList = NonnullRefPtrVector; + +// This is a in the spec. https://www.w3.org/TR/selectors-4/#complex class Selector : public RefCounted { public: struct SimpleSelector { @@ -65,7 +68,7 @@ public: // Only used when "pseudo_class" is "NthChild" or "NthLastChild". NthChildPattern nth_child_pattern; - NonnullRefPtrVector not_selector {}; + SelectorList not_selector {}; }; PseudoClass pseudo_class; @@ -98,36 +101,37 @@ public: Attribute attribute; }; - struct ComplexSelector { - enum class Relation { - None, - ImmediateChild, - Descendant, - AdjacentSibling, - GeneralSibling, - Column, - }; - Relation relation { Relation::None }; - - using CompoundSelector = Vector; - CompoundSelector compound_selector; + enum class Combinator { + None, + ImmediateChild, // > + Descendant, // + NextSibling, // + + SubsequentSibling, // ~ + Column, // || }; - static NonnullRefPtr create(Vector&& complex_selectors) + struct CompoundSelector { + // Spec-wise, the is not part of a , + // but it is more understandable to put them together. + Combinator combinator { Combinator::None }; + Vector simple_selectors; + }; + + static NonnullRefPtr create(Vector&& compound_selectors) { - return adopt_ref(*new Selector(move(complex_selectors))); + return adopt_ref(*new Selector(move(compound_selectors))); } ~Selector(); - Vector const& complex_selectors() const { return m_complex_selectors; } + Vector const& compound_selectors() const { return m_compound_selectors; } u32 specificity() const; private: - explicit Selector(Vector&&); + explicit Selector(Vector&&); - Vector m_complex_selectors; + Vector m_compound_selectors; }; } diff --git a/Userland/Libraries/LibWeb/CSS/SelectorEngine.cpp b/Userland/Libraries/LibWeb/CSS/SelectorEngine.cpp index 8a93889db4..026dee981b 100644 --- a/Userland/Libraries/LibWeb/CSS/SelectorEngine.cpp +++ b/Userland/Libraries/LibWeb/CSS/SelectorEngine.cpp @@ -195,15 +195,15 @@ static bool matches(CSS::Selector::SimpleSelector const& component, DOM::Element static bool matches(CSS::Selector const& selector, int component_list_index, DOM::Element const& element) { - auto& component_list = selector.complex_selectors()[component_list_index]; - for (auto& component : component_list.compound_selector) { - if (!matches(component, element)) + auto& relative_selector = selector.compound_selectors()[component_list_index]; + for (auto& simple_selector : relative_selector.simple_selectors) { + if (!matches(simple_selector, element)) return false; } - switch (component_list.relation) { - case CSS::Selector::ComplexSelector::Relation::None: + switch (relative_selector.combinator) { + case CSS::Selector::Combinator::None: return true; - case CSS::Selector::ComplexSelector::Relation::Descendant: + case CSS::Selector::Combinator::Descendant: VERIFY(component_list_index != 0); for (auto* ancestor = element.parent(); ancestor; ancestor = ancestor->parent()) { if (!is(*ancestor)) @@ -212,24 +212,24 @@ static bool matches(CSS::Selector const& selector, int component_list_index, DOM return true; } return false; - case CSS::Selector::ComplexSelector::Relation::ImmediateChild: + case CSS::Selector::Combinator::ImmediateChild: VERIFY(component_list_index != 0); if (!element.parent() || !is(*element.parent())) return false; return matches(selector, component_list_index - 1, verify_cast(*element.parent())); - case CSS::Selector::ComplexSelector::Relation::AdjacentSibling: + case CSS::Selector::Combinator::NextSibling: VERIFY(component_list_index != 0); if (auto* sibling = element.previous_element_sibling()) return matches(selector, component_list_index - 1, *sibling); return false; - case CSS::Selector::ComplexSelector::Relation::GeneralSibling: + case CSS::Selector::Combinator::SubsequentSibling: VERIFY(component_list_index != 0); for (auto* sibling = element.previous_element_sibling(); sibling; sibling = sibling->previous_element_sibling()) { if (matches(selector, component_list_index - 1, *sibling)) return true; } return false; - case CSS::Selector::ComplexSelector::Relation::Column: + case CSS::Selector::Combinator::Column: TODO(); } VERIFY_NOT_REACHED(); @@ -237,8 +237,8 @@ static bool matches(CSS::Selector const& selector, int component_list_index, DOM bool matches(CSS::Selector const& selector, DOM::Element const& element) { - VERIFY(!selector.complex_selectors().is_empty()); - return matches(selector, selector.complex_selectors().size() - 1, element); + VERIFY(!selector.compound_selectors().is_empty()); + return matches(selector, selector.compound_selectors().size() - 1, element); } } diff --git a/Userland/Libraries/LibWeb/Dump.cpp b/Userland/Libraries/LibWeb/Dump.cpp index 466c1b62c9..7cb1b4358c 100644 --- a/Userland/Libraries/LibWeb/Dump.cpp +++ b/Userland/Libraries/LibWeb/Dump.cpp @@ -267,27 +267,27 @@ void dump_selector(StringBuilder& builder, CSS::Selector const& selector) { builder.append(" CSS::Selector:\n"); - for (auto& complex_selector : selector.complex_selectors()) { + for (auto& relative_selector : selector.compound_selectors()) { builder.append(" "); char const* relation_description = ""; - switch (complex_selector.relation) { - case CSS::Selector::ComplexSelector::Relation::None: + switch (relative_selector.combinator) { + case CSS::Selector::Combinator::None: relation_description = "None"; break; - case CSS::Selector::ComplexSelector::Relation::ImmediateChild: + case CSS::Selector::Combinator::ImmediateChild: relation_description = "ImmediateChild"; break; - case CSS::Selector::ComplexSelector::Relation::Descendant: + case CSS::Selector::Combinator::Descendant: relation_description = "Descendant"; break; - case CSS::Selector::ComplexSelector::Relation::AdjacentSibling: + case CSS::Selector::Combinator::NextSibling: relation_description = "AdjacentSibling"; break; - case CSS::Selector::ComplexSelector::Relation::GeneralSibling: + case CSS::Selector::Combinator::SubsequentSibling: relation_description = "GeneralSibling"; break; - case CSS::Selector::ComplexSelector::Relation::Column: + case CSS::Selector::Combinator::Column: relation_description = "Column"; break; } @@ -295,8 +295,8 @@ void dump_selector(StringBuilder& builder, CSS::Selector const& selector) if (*relation_description) builder.appendff("{{{}}} ", relation_description); - for (size_t i = 0; i < complex_selector.compound_selector.size(); ++i) { - auto& simple_selector = complex_selector.compound_selector[i]; + for (size_t i = 0; i < relative_selector.simple_selectors.size(); ++i) { + auto& simple_selector = relative_selector.simple_selectors[i]; char const* type_description = "Unknown"; switch (simple_selector.type) { case CSS::Selector::SimpleSelector::Type::Invalid: @@ -459,7 +459,7 @@ void dump_selector(StringBuilder& builder, CSS::Selector const& selector) builder.appendff(" [{}, name='{}', value='{}']", attribute_match_type_description, simple_selector.attribute.name, simple_selector.attribute.value); } - if (i != complex_selector.compound_selector.size() - 1) + if (i != relative_selector.simple_selectors.size() - 1) builder.append(", "); } builder.append("\n");