From 0bcab6046366892bbcef88e26077b2097af98948 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Sun, 12 Sep 2021 19:24:01 +0200 Subject: [PATCH] LibWeb: Make CSSStyleDeclaration an abstract class This patch moves the CSS property+value storage down to a new subclass of CSSStyleDeclaration called PropertyOwningCSSStyleDeclaration. The JavaScript wrapper for CSSStyleDeclaration now calls virtual functions on the C++ object. This is preparation for supporting computed style CSSStyleDeclaration objects which won't have internal property storage, but rather an internal element pointer. :^) --- .../CSSStyleDeclarationWrapperCustom.cpp | 35 +---------- .../LibWeb/CSS/CSSStyleDeclaration.cpp | 59 +++++++++++++++++-- .../LibWeb/CSS/CSSStyleDeclaration.h | 50 ++++++++++------ .../Libraries/LibWeb/CSS/Parser/Parser.cpp | 12 ++-- Userland/Libraries/LibWeb/CSS/Parser/Parser.h | 8 +-- .../Libraries/LibWeb/CSS/StyleResolver.cpp | 6 +- Userland/Libraries/LibWeb/DOM/Window.cpp | 2 +- Userland/Libraries/LibWeb/Dump.cpp | 2 +- Userland/Libraries/LibWeb/Forward.h | 1 + 9 files changed, 106 insertions(+), 69 deletions(-) diff --git a/Userland/Libraries/LibWeb/Bindings/CSSStyleDeclarationWrapperCustom.cpp b/Userland/Libraries/LibWeb/Bindings/CSSStyleDeclarationWrapperCustom.cpp index 6a20362c08..d5e572fcf3 100644 --- a/Userland/Libraries/LibWeb/Bindings/CSSStyleDeclarationWrapperCustom.cpp +++ b/Userland/Libraries/LibWeb/Bindings/CSSStyleDeclarationWrapperCustom.cpp @@ -4,9 +4,7 @@ * SPDX-License-Identifier: BSD-2-Clause */ -#include #include -#include #include namespace Web::Bindings { @@ -28,10 +26,8 @@ JS::Value CSSStyleDeclarationWrapper::internal_get(JS::PropertyName const& name, auto property_id = CSS::property_id_from_string(name.to_string()); if (property_id == CSS::PropertyID::Invalid) return Base::internal_get(name, receiver); - for (auto& property : impl().properties()) { - if (property.property_id == property_id) - return js_string(vm(), property.value->to_string()); - } + if (auto maybe_property = impl().property(property_id); maybe_property.has_value()) + return js_string(vm(), maybe_property->value->to_string()); return js_string(vm(), String::empty()); } @@ -48,32 +44,7 @@ bool CSSStyleDeclarationWrapper::internal_set(JS::PropertyName const& name, JS:: if (vm().exception()) return false; - auto new_value = parse_css_value(CSS::ParsingContext {}, css_text, property_id); - // FIXME: What are we supposed to do if we can't parse it? - if (!new_value) - return false; - - ScopeGuard style_invalidation_guard = [&] { - auto& declaration = verify_cast(impl()); - if (auto* element = declaration.element()) - element->invalidate_style(); - }; - - // FIXME: I don't think '!important' is being handled correctly here.. - - for (auto& property : impl().m_properties) { - if (property.property_id == property_id) { - property.value = new_value.release_nonnull(); - return true; - } - } - - impl().m_properties.append(CSS::StyleProperty { - .property_id = property_id, - .value = new_value.release_nonnull(), - .important = false, - }); - return true; + return impl().set_property(property_id, css_text); } } diff --git a/Userland/Libraries/LibWeb/CSS/CSSStyleDeclaration.cpp b/Userland/Libraries/LibWeb/CSS/CSSStyleDeclaration.cpp index 1dd5929e45..c5a7b1c50f 100644 --- a/Userland/Libraries/LibWeb/CSS/CSSStyleDeclaration.cpp +++ b/Userland/Libraries/LibWeb/CSS/CSSStyleDeclaration.cpp @@ -5,21 +5,26 @@ */ #include +#include #include namespace Web::CSS { -CSSStyleDeclaration::CSSStyleDeclaration(Vector&& properties, HashMap&& custom_properties) +PropertyOwningCSSStyleDeclaration::PropertyOwningCSSStyleDeclaration(Vector properties, HashMap custom_properties) : m_properties(move(properties)) , m_custom_properties(move(custom_properties)) { } +PropertyOwningCSSStyleDeclaration::~PropertyOwningCSSStyleDeclaration() +{ +} + CSSStyleDeclaration::~CSSStyleDeclaration() { } -String CSSStyleDeclaration::item(size_t index) const +String PropertyOwningCSSStyleDeclaration::item(size_t index) const { if (index >= m_properties.size()) return {}; @@ -27,13 +32,13 @@ String CSSStyleDeclaration::item(size_t index) const } ElementInlineCSSStyleDeclaration::ElementInlineCSSStyleDeclaration(DOM::Element& element) - : CSSStyleDeclaration({}, {}) + : PropertyOwningCSSStyleDeclaration({}, {}) , m_element(element.make_weak_ptr()) { } -ElementInlineCSSStyleDeclaration::ElementInlineCSSStyleDeclaration(DOM::Element& element, CSSStyleDeclaration& declaration) - : CSSStyleDeclaration(move(declaration.m_properties), move(declaration.m_custom_properties)) +ElementInlineCSSStyleDeclaration::ElementInlineCSSStyleDeclaration(DOM::Element& element, PropertyOwningCSSStyleDeclaration& declaration) + : PropertyOwningCSSStyleDeclaration(move(declaration.m_properties), move(declaration.m_custom_properties)) , m_element(element.make_weak_ptr()) { } @@ -42,4 +47,48 @@ ElementInlineCSSStyleDeclaration::~ElementInlineCSSStyleDeclaration() { } +size_t PropertyOwningCSSStyleDeclaration::length() const +{ + return m_properties.size(); +} + +Optional PropertyOwningCSSStyleDeclaration::property(PropertyID property_id) const +{ + for (auto& property : m_properties) { + if (property.property_id == property_id) + return property; + } + return {}; +} + +bool PropertyOwningCSSStyleDeclaration::set_property(PropertyID property_id, StringView css_text) +{ + auto new_value = parse_css_value(CSS::ParsingContext {}, css_text, property_id); + // FIXME: What are we supposed to do if we can't parse it? + if (!new_value) + return false; + + ScopeGuard style_invalidation_guard = [&] { + auto& declaration = verify_cast(*this); + if (auto* element = declaration.element()) + element->invalidate_style(); + }; + + // FIXME: I don't think '!important' is being handled correctly here.. + + for (auto& property : m_properties) { + if (property.property_id == property_id) { + property.value = new_value.release_nonnull(); + return true; + } + } + + m_properties.append(CSS::StyleProperty { + .property_id = property_id, + .value = new_value.release_nonnull(), + .important = false, + }); + return true; +} + } diff --git a/Userland/Libraries/LibWeb/CSS/CSSStyleDeclaration.h b/Userland/Libraries/LibWeb/CSS/CSSStyleDeclaration.h index 37a29d838a..6c8410e3f1 100644 --- a/Userland/Libraries/LibWeb/CSS/CSSStyleDeclaration.h +++ b/Userland/Libraries/LibWeb/CSS/CSSStyleDeclaration.h @@ -26,35 +26,51 @@ class CSSStyleDeclaration public: using WrapperType = Bindings::CSSStyleDeclarationWrapper; - static NonnullRefPtr create(Vector&& properties, HashMap&& custom_properties) - { - return adopt_ref(*new CSSStyleDeclaration(move(properties), move(custom_properties))); - } - virtual ~CSSStyleDeclaration(); - const Vector& properties() const { return m_properties; } - const Optional custom_property(const String& custom_property_name) const { return m_custom_properties.get(custom_property_name); } - size_t custom_property_count() const { return m_custom_properties.size(); }; + virtual size_t length() const = 0; + virtual String item(size_t index) const = 0; - size_t length() const { return m_properties.size(); } - String item(size_t index) const; + virtual Optional property(PropertyID) const = 0; + virtual bool set_property(PropertyID, StringView css_text) = 0; protected: - explicit CSSStyleDeclaration(Vector&&, HashMap&&); + CSSStyleDeclaration() { } +}; + +class PropertyOwningCSSStyleDeclaration : public CSSStyleDeclaration { + friend class ElementInlineCSSStyleDeclaration; + +public: + static NonnullRefPtr create(Vector properties, HashMap custom_properties) + { + return adopt_ref(*new PropertyOwningCSSStyleDeclaration(move(properties), move(custom_properties))); + } + + virtual ~PropertyOwningCSSStyleDeclaration() override; + + virtual size_t length() const override; + virtual String item(size_t index) const override; + + virtual Optional property(PropertyID) const override; + virtual bool set_property(PropertyID, StringView css_text) override; + + const Vector& properties() const { return m_properties; } + Optional custom_property(const String& custom_property_name) const { return m_custom_properties.get(custom_property_name); } + size_t custom_property_count() const { return m_custom_properties.size(); } + +protected: + explicit PropertyOwningCSSStyleDeclaration(Vector, HashMap); private: - friend class ElementInlineCSSStyleDeclaration; - friend class Bindings::CSSStyleDeclarationWrapper; - Vector m_properties; HashMap m_custom_properties; }; -class ElementInlineCSSStyleDeclaration final : public CSSStyleDeclaration { +class ElementInlineCSSStyleDeclaration final : public PropertyOwningCSSStyleDeclaration { public: static NonnullRefPtr create(DOM::Element& element) { return adopt_ref(*new ElementInlineCSSStyleDeclaration(element)); } - static NonnullRefPtr create_and_take_properties_from(DOM::Element& element, CSSStyleDeclaration& declaration) { return adopt_ref(*new ElementInlineCSSStyleDeclaration(element, declaration)); } + static NonnullRefPtr create_and_take_properties_from(DOM::Element& element, PropertyOwningCSSStyleDeclaration& declaration) { return adopt_ref(*new ElementInlineCSSStyleDeclaration(element, declaration)); } virtual ~ElementInlineCSSStyleDeclaration() override; DOM::Element* element() { return m_element.ptr(); } @@ -62,7 +78,7 @@ public: private: explicit ElementInlineCSSStyleDeclaration(DOM::Element&); - explicit ElementInlineCSSStyleDeclaration(DOM::Element&, CSSStyleDeclaration&); + explicit ElementInlineCSSStyleDeclaration(DOM::Element&, PropertyOwningCSSStyleDeclaration&); WeakPtr m_element; }; diff --git a/Userland/Libraries/LibWeb/CSS/Parser/Parser.cpp b/Userland/Libraries/LibWeb/CSS/Parser/Parser.cpp index 2f907ac40d..0fe7343138 100644 --- a/Userland/Libraries/LibWeb/CSS/Parser/Parser.cpp +++ b/Userland/Libraries/LibWeb/CSS/Parser/Parser.cpp @@ -1085,13 +1085,13 @@ Optional Parser::parse_a_declaration(TokenStream& tokens) return {}; } -RefPtr Parser::parse_as_list_of_declarations() +RefPtr Parser::parse_as_list_of_declarations() { return parse_a_list_of_declarations(m_token_stream); } template -RefPtr Parser::parse_a_list_of_declarations(TokenStream& tokens) +RefPtr Parser::parse_a_list_of_declarations(TokenStream& tokens) { dbgln_if(CSS_PARSER_DEBUG, "Parser::parse_as_list_of_declarations"); @@ -1119,7 +1119,7 @@ RefPtr Parser::parse_a_list_of_declarations(TokenStream& } } - return CSSStyleDeclaration::create(move(properties), move(custom_properties)); + return PropertyOwningCSSStyleDeclaration::create(move(properties), move(custom_properties)); } Optional Parser::parse_as_component_value() @@ -1295,7 +1295,7 @@ RefPtr Parser::convert_to_rule(NonnullRefPtr rule) return {}; } -RefPtr Parser::convert_to_declaration(NonnullRefPtr block) +RefPtr Parser::convert_to_declaration(NonnullRefPtr block) { dbgln_if(CSS_PARSER_DEBUG, "Parser::convert_to_declaration"); @@ -3487,10 +3487,10 @@ RefPtr parse_css(CSS::ParsingContext const& context, StringV return parser.parse_as_stylesheet(); } -RefPtr parse_css_declaration(CSS::ParsingContext const& context, StringView const& css) +RefPtr parse_css_declaration(CSS::ParsingContext const& context, StringView const& css) { if (css.is_empty()) - return CSS::CSSStyleDeclaration::create({}, {}); + return CSS::PropertyOwningCSSStyleDeclaration::create({}, {}); CSS::Parser parser(context, css); return parser.parse_as_list_of_declarations(); } diff --git a/Userland/Libraries/LibWeb/CSS/Parser/Parser.h b/Userland/Libraries/LibWeb/CSS/Parser/Parser.h index 062830520e..7b9c566fbc 100644 --- a/Userland/Libraries/LibWeb/CSS/Parser/Parser.h +++ b/Userland/Libraries/LibWeb/CSS/Parser/Parser.h @@ -86,7 +86,7 @@ public: // Used in @supports conditions. [CSS3-CONDITIONAL] Optional parse_as_declaration(); // For the contents of a style attribute, which parses text into the contents of a single style rule. - RefPtr parse_as_list_of_declarations(); + RefPtr parse_as_list_of_declarations(); // For things that need to consume a single value, like the parsing rules for attr(). Optional parse_as_component_value(); // For the contents of presentational attributes, which parse text into a single declaration’s value, or for parsing a stand-alone selector [SELECT] or list of Media Queries [MEDIAQ], as in Selectors API or the media HTML attribute. @@ -109,7 +109,7 @@ private: template Optional parse_a_declaration(TokenStream&); template - RefPtr parse_a_list_of_declarations(TokenStream&); + RefPtr parse_a_list_of_declarations(TokenStream&); template Optional parse_a_component_value(TokenStream&); template @@ -160,7 +160,7 @@ private: [[nodiscard]] NonnullRefPtr consume_a_function(TokenStream&); [[nodiscard]] RefPtr convert_to_rule(NonnullRefPtr); - [[nodiscard]] RefPtr convert_to_declaration(NonnullRefPtr); + [[nodiscard]] RefPtr convert_to_declaration(NonnullRefPtr); [[nodiscard]] Optional convert_to_style_property(StyleDeclarationRule&); static Optional try_parse_float(StringView string); @@ -228,7 +228,7 @@ private: namespace Web { RefPtr parse_css(CSS::ParsingContext const&, StringView const&); -RefPtr parse_css_declaration(CSS::ParsingContext const&, StringView const&); +RefPtr parse_css_declaration(CSS::ParsingContext const&, StringView const&); RefPtr parse_css_value(CSS::ParsingContext const&, StringView const&, CSS::PropertyID property_id = CSS::PropertyID::Invalid); Optional parse_selector(CSS::ParsingContext const&, StringView const&); diff --git a/Userland/Libraries/LibWeb/CSS/StyleResolver.cpp b/Userland/Libraries/LibWeb/CSS/StyleResolver.cpp index ef5a1e38e8..f3fa43e3cf 100644 --- a/Userland/Libraries/LibWeb/CSS/StyleResolver.cpp +++ b/Userland/Libraries/LibWeb/CSS/StyleResolver.cpp @@ -514,7 +514,7 @@ StyleResolver::CustomPropertyResolutionTuple StyleResolver::resolve_custom_prope if (match.specificity < parent_resolved.specificity) continue; - auto custom_property_style = match.rule->declaration().custom_property(custom_property_name); + auto custom_property_style = verify_cast(match.rule->declaration()).custom_property(custom_property_name); if (custom_property_style.has_value()) { element.add_custom_property(custom_property_name, { custom_property_style.value(), match.specificity }); return { custom_property_style.value(), match.specificity }; @@ -548,7 +548,7 @@ NonnullRefPtr StyleResolver::resolve_style(DOM::Element& elemen sort_matching_rules(matching_rules); for (auto& match : matching_rules) { - for (auto& property : match.rule->declaration().properties()) { + for (auto& property : verify_cast(match.rule->declaration()).properties()) { auto property_value = property.value; if (property.value->is_custom_property()) { auto prop = reinterpret_cast(property.value.ptr()); @@ -562,7 +562,7 @@ NonnullRefPtr StyleResolver::resolve_style(DOM::Element& elemen } } - if (auto* inline_style = element.inline_style()) { + if (auto* inline_style = verify_cast(element.inline_style())) { for (auto& property : inline_style->properties()) { set_property_expanding_shorthands(style, property.property_id, property.value, m_document); } diff --git a/Userland/Libraries/LibWeb/DOM/Window.cpp b/Userland/Libraries/LibWeb/DOM/Window.cpp index bf969d811b..e3bbc741d5 100644 --- a/Userland/Libraries/LibWeb/DOM/Window.cpp +++ b/Userland/Libraries/LibWeb/DOM/Window.cpp @@ -192,7 +192,7 @@ NonnullRefPtr Window::get_computed_style(DOM::Element& dbgln("Generating CSS computed style for {} @ {:p}", element.node_name(), &element); Vector properties; HashMap custom_properties; - return CSS::CSSStyleDeclaration::create(move(properties), move(custom_properties)); + return CSS::PropertyOwningCSSStyleDeclaration::create(move(properties), move(custom_properties)); } NonnullRefPtr Window::match_media(String media) diff --git a/Userland/Libraries/LibWeb/Dump.cpp b/Userland/Libraries/LibWeb/Dump.cpp index 6990dd2327..641443dca9 100644 --- a/Userland/Libraries/LibWeb/Dump.cpp +++ b/Userland/Libraries/LibWeb/Dump.cpp @@ -499,7 +499,7 @@ void dump_style_rule(StringBuilder& builder, CSS::CSSStyleRule const& rule) dump_selector(builder, selector); } builder.append(" Declarations:\n"); - for (auto& property : rule.declaration().properties()) { + for (auto& property : verify_cast(rule.declaration()).properties()) { builder.appendff(" {}: '{}'\n", CSS::string_from_property_id(property.property_id), property.value->to_string()); } } diff --git a/Userland/Libraries/LibWeb/Forward.h b/Userland/Libraries/LibWeb/Forward.h index 13114612db..f99f71c7ed 100644 --- a/Userland/Libraries/LibWeb/Forward.h +++ b/Userland/Libraries/LibWeb/Forward.h @@ -22,6 +22,7 @@ class CSSStyleRule; class CSSStyleSheet; class ElementInlineCSSStyleDeclaration; class Length; +class PropertyOwningCSSStyleDeclaration; class Screen; class Selector; class StyleProperties;