From 20114729296ec64c5e0e1b6173cf52be5ece57f5 Mon Sep 17 00:00:00 2001 From: Sam Atkins Date: Mon, 14 Apr 2025 13:53:11 +0100 Subject: [PATCH] LibWeb: Move style sheet parsing into `create_a_css_style_sheet()` The spec is unclear about when exactly we should parse the style sheet. Previously we'd do so before calling this algorithm, which was error-prone, as seen by the bug fixed by the previous commit. The spec for step 1 of "create a CSS style sheet" says: 1. Create a new CSS style sheet object and set its properties as specified. The definitions linked are UA-defined enough that it seems reasonable to put the parsing here. That simplifies the user code a little and makes it harder to mess up. It does raise the question of what to do if parsing fails. I've matched our previous behaviour by just logging and returning in that case. While I'm modifying this method, I've also converted the bool params to enums so they're a little clearer to read. --- Libraries/LibWeb/CSS/StyleSheetList.cpp | 37 ++++++++++++------- Libraries/LibWeb/CSS/StyleSheetList.h | 15 ++++++-- Libraries/LibWeb/DOM/StyleElementUtils.cpp | 42 +++++++++++++--------- Libraries/LibWeb/HTML/HTMLLinkElement.cpp | 30 ++++++---------- 4 files changed, 74 insertions(+), 50 deletions(-) diff --git a/Libraries/LibWeb/CSS/StyleSheetList.cpp b/Libraries/LibWeb/CSS/StyleSheetList.cpp index be0db396e51..0bed4af8a7f 100644 --- a/Libraries/LibWeb/CSS/StyleSheetList.cpp +++ b/Libraries/LibWeb/CSS/StyleSheetList.cpp @@ -1,5 +1,6 @@ /* * Copyright (c) 2020-2024, Andreas Kling + * Copyright (c) 2025, Sam Atkins * * SPDX-License-Identifier: BSD-2-Clause */ @@ -60,23 +61,35 @@ void StyleSheetList::add_a_css_style_sheet(CSS::CSSStyleSheet& sheet) } // https://www.w3.org/TR/cssom/#create-a-css-style-sheet -void StyleSheetList::create_a_css_style_sheet(String type, DOM::Element* owner_node, String media, String title, bool alternate, bool origin_clean, Optional<::URL::URL> location, CSS::CSSStyleSheet* parent_style_sheet, CSS::CSSRule* owner_rule, CSS::CSSStyleSheet& sheet) +GC::Ptr StyleSheetList::create_a_css_style_sheet(String const& css_text, String type, DOM::Element* owner_node, String media, String title, Alternate alternate, OriginClean origin_clean, Optional<::URL::URL> location, CSSStyleSheet* parent_style_sheet, CSSRule* owner_rule) { // 1. Create a new CSS style sheet object and set its properties as specified. - // FIXME: We receive `sheet` from the caller already. This is weird. + // AD-HOC: The spec never tells us when to parse this style sheet, but the most logical place is here. + // AD-HOC: Are we supposed to use the document's URL for the stylesheet's location during parsing? Not doing it breaks things. + auto location_url = location.value_or(document().url()); + auto* sheet = parse_css_stylesheet(Parser::ParsingParams { document(), location_url }, css_text, location_url); - sheet.set_parent_css_style_sheet(parent_style_sheet); - sheet.set_owner_css_rule(owner_rule); - sheet.set_owner_node(owner_node); - sheet.set_type(move(type)); - sheet.set_media(move(media)); - sheet.set_title(move(title)); - sheet.set_alternate(alternate); - sheet.set_origin_clean(origin_clean); - sheet.set_location(move(location)); + // AD-HOC: Exit out if parsing failed. + // FIXME: What should we actually do here? + if (!sheet) { + dbgln_if(CSS_LOADER_DEBUG, "StyleSheetList::create_a_css_style_sheet(): Failed to parse stylesheet at {}", location_url); + return nullptr; + } + + sheet->set_parent_css_style_sheet(parent_style_sheet); + sheet->set_owner_css_rule(owner_rule); + sheet->set_owner_node(owner_node); + sheet->set_type(move(type)); + sheet->set_media(move(media)); + sheet->set_title(move(title)); + sheet->set_alternate(alternate == Alternate::Yes); + sheet->set_origin_clean(origin_clean == OriginClean::Yes); + sheet->set_location(move(location)); // 2. Then run the add a CSS style sheet steps for the newly created CSS style sheet. - add_a_css_style_sheet(sheet); + add_a_css_style_sheet(*sheet); + + return sheet; } void StyleSheetList::add_sheet(CSSStyleSheet& sheet) diff --git a/Libraries/LibWeb/CSS/StyleSheetList.h b/Libraries/LibWeb/CSS/StyleSheetList.h index 935ab4fca11..f757722e5c4 100644 --- a/Libraries/LibWeb/CSS/StyleSheetList.h +++ b/Libraries/LibWeb/CSS/StyleSheetList.h @@ -1,6 +1,7 @@ /* * Copyright (c) 2020-2024, Andreas Kling * Copyright (c) 2023, Luke Wilde + * Copyright (c) 2025, Sam Atkins * * SPDX-License-Identifier: BSD-2-Clause */ @@ -19,9 +20,17 @@ class StyleSheetList final : public Bindings::PlatformObject { public: [[nodiscard]] static GC::Ref create(GC::Ref document_or_shadow_root); - void add_a_css_style_sheet(CSS::CSSStyleSheet&); - void remove_a_css_style_sheet(CSS::CSSStyleSheet&); - void create_a_css_style_sheet(String type, DOM::Element* owner_node, String media, String title, bool alternate, bool origin_clean, Optional<::URL::URL> location, CSS::CSSStyleSheet* parent_style_sheet, CSS::CSSRule* owner_rule, CSS::CSSStyleSheet&); + void add_a_css_style_sheet(CSSStyleSheet&); + void remove_a_css_style_sheet(CSSStyleSheet&); + enum class Alternate : u8 { + No, + Yes, + }; + enum class OriginClean : u8 { + No, + Yes, + }; + GC::Ptr create_a_css_style_sheet(String const& css_text, String type, DOM::Element* owner_node, String media, String title, Alternate, OriginClean, Optional<::URL::URL> location, CSSStyleSheet* parent_style_sheet, CSSRule* owner_rule); Vector> const& sheets() const { return m_sheets; } Vector>& sheets() { return m_sheets; } diff --git a/Libraries/LibWeb/DOM/StyleElementUtils.cpp b/Libraries/LibWeb/DOM/StyleElementUtils.cpp index bd594f58858..9383b6addc1 100644 --- a/Libraries/LibWeb/DOM/StyleElementUtils.cpp +++ b/Libraries/LibWeb/DOM/StyleElementUtils.cpp @@ -52,31 +52,41 @@ void StyleElementUtils::update_a_style_block(DOM::Element& style_element) // FIXME: 5. If the Should element's inline behavior be blocked by Content Security Policy? algorithm returns "Blocked" when executed upon the style element, "style", and the style element's child text content, then return. [CSP] - // FIXME: This is a bit awkward, as the spec doesn't actually tell us when to parse the CSS text, - // so we just do it here and pass the parsed sheet to create_a_css_style_sheet(). - // AD-HOC: Are we supposed to use the document's URL for the stylesheet's location? Not doing it breaks things. - auto* sheet = parse_css_stylesheet(CSS::Parser::ParsingParams(style_element.document()), style_element.text_content().value_or(String {}), style_element.document().url()); - if (!sheet) - return; - - // FIXME: This should probably be handled by StyleSheet::set_owner_node(). - m_associated_css_style_sheet = sheet; - - // 6. Create a CSS style sheet with the following properties... + // 6. Create a CSS style sheet with the following properties: + // type + // text/css + // owner node + // element + // media + // The media attribute of element. + // title + // The title attribute of element, if element is in a document tree, or the empty string otherwise. + // alternate flag + // Unset. + // origin-clean flag + // Set. + // location + // parent CSS style sheet + // owner CSS rule + // null + // disabled flag + // Left at its default value. + // CSS rules + // Left uninitialized. m_style_sheet_list = style_element.document_or_shadow_root_style_sheets(); - m_style_sheet_list->create_a_css_style_sheet( + m_associated_css_style_sheet = m_style_sheet_list->create_a_css_style_sheet( + style_element.text_content().value_or(String {}), "text/css"_string, &style_element, style_element.attribute(HTML::AttributeNames::media).value_or({}), style_element.in_a_document_tree() ? style_element.attribute(HTML::AttributeNames::title).value_or({}) : String {}, - false, - true, + CSS::StyleSheetList::Alternate::No, + CSS::StyleSheetList::OriginClean::Yes, {}, nullptr, - nullptr, - *sheet); + nullptr); } void StyleElementUtils::visit_edges(JS::Cell::Visitor& visitor) diff --git a/Libraries/LibWeb/HTML/HTMLLinkElement.cpp b/Libraries/LibWeb/HTML/HTMLLinkElement.cpp index 2d59eb9fc41..2f52baba346 100644 --- a/Libraries/LibWeb/HTML/HTMLLinkElement.cpp +++ b/Libraries/LibWeb/HTML/HTMLLinkElement.cpp @@ -486,26 +486,18 @@ void HTMLLinkElement::process_stylesheet_resource(bool success, Fetch::Infrastru dbgln("Style sheet {} claimed to be '{}' but decoding failed", response.url().value_or(URL::URL()), encoding); dispatch_event(*DOM::Event::create(realm(), HTML::EventNames::error)); } else { - auto const decoded_string = maybe_decoded_string.release_value(); VERIFY(!response.url_list().is_empty()); - auto location = response.url_list().first(); - m_loaded_style_sheet = parse_css_stylesheet(CSS::Parser::ParsingParams(document(), location), decoded_string, location); - - if (m_loaded_style_sheet) { - document_or_shadow_root_style_sheets().create_a_css_style_sheet( - "text/css"_string, - this, - attribute(HTML::AttributeNames::media).value_or({}), - in_a_document_tree() ? attribute(HTML::AttributeNames::title).value_or({}) : String {}, - m_relationship & Relationship::Alternate && !m_explicitly_enabled, - true, - move(location), - nullptr, - nullptr, - *m_loaded_style_sheet); - } else { - dbgln_if(CSS_LOADER_DEBUG, "HTMLLinkElement: Failed to parse stylesheet: {}", resource()->url()); - } + m_loaded_style_sheet = document_or_shadow_root_style_sheets().create_a_css_style_sheet( + maybe_decoded_string.release_value(), + "text/css"_string, + this, + attribute(HTML::AttributeNames::media).value_or({}), + in_a_document_tree() ? attribute(HTML::AttributeNames::title).value_or({}) : String {}, + (m_relationship & Relationship::Alternate && !m_explicitly_enabled) ? CSS::StyleSheetList::Alternate::Yes : CSS::StyleSheetList::Alternate::No, + CSS::StyleSheetList::OriginClean::Yes, + response.url_list().first(), + nullptr, + nullptr); // 2. Fire an event named load at el. dispatch_event(*DOM::Event::create(realm(), HTML::EventNames::load));