Browse Source

LibWeb: Bring parse_as_sizes_attribute() up to date with the spec

The following spec algorithms had changed since we implemented them:
- "parse a sizes attribute"
- "update the source set"
- "create a source set"

This commit brings them up to date, as well as adding some additional
logging when parsing the sizes attribute fails in some way.
Sam Atkins 10 months ago
parent
commit
2a0f6fd23e

+ 57 - 19
Userland/Libraries/LibWeb/CSS/Parser/Parser.cpp

@@ -39,6 +39,7 @@
 #include <LibWeb/CSS/Parser/Parser.h>
 #include <LibWeb/CSS/Parser/Parser.h>
 #include <LibWeb/CSS/Parser/Rule.h>
 #include <LibWeb/CSS/Parser/Rule.h>
 #include <LibWeb/CSS/Selector.h>
 #include <LibWeb/CSS/Selector.h>
+#include <LibWeb/CSS/Sizing.h>
 #include <LibWeb/CSS/StyleValues/AngleStyleValue.h>
 #include <LibWeb/CSS/StyleValues/AngleStyleValue.h>
 #include <LibWeb/CSS/StyleValues/BackgroundRepeatStyleValue.h>
 #include <LibWeb/CSS/StyleValues/BackgroundRepeatStyleValue.h>
 #include <LibWeb/CSS/StyleValues/BackgroundSizeStyleValue.h>
 #include <LibWeb/CSS/StyleValues/BackgroundSizeStyleValue.h>
@@ -86,6 +87,7 @@
 #include <LibWeb/CSS/StyleValues/URLStyleValue.h>
 #include <LibWeb/CSS/StyleValues/URLStyleValue.h>
 #include <LibWeb/CSS/StyleValues/UnresolvedStyleValue.h>
 #include <LibWeb/CSS/StyleValues/UnresolvedStyleValue.h>
 #include <LibWeb/Dump.h>
 #include <LibWeb/Dump.h>
+#include <LibWeb/HTML/HTMLImageElement.h>
 #include <LibWeb/Infra/CharacterTypes.h>
 #include <LibWeb/Infra/CharacterTypes.h>
 #include <LibWeb/Infra/Strings.h>
 #include <LibWeb/Infra/Strings.h>
 
 
@@ -7930,29 +7932,43 @@ private:
 };
 };
 
 
 // https://html.spec.whatwg.org/multipage/images.html#parsing-a-sizes-attribute
 // https://html.spec.whatwg.org/multipage/images.html#parsing-a-sizes-attribute
-LengthOrCalculated Parser::Parser::parse_as_sizes_attribute()
+LengthOrCalculated Parser::Parser::parse_as_sizes_attribute([[maybe_unused]] DOM::Element const& element, HTML::HTMLImageElement const* img)
 {
 {
+    // When asked to parse a sizes attribute from an element element, with an img element or null img:
+
     // 1. Let unparsed sizes list be the result of parsing a comma-separated list of component values
     // 1. Let unparsed sizes list be the result of parsing a comma-separated list of component values
     //    from the value of element's sizes attribute (or the empty string, if the attribute is absent).
     //    from the value of element's sizes attribute (or the empty string, if the attribute is absent).
+    // NOTE: The sizes attribute has already been tokenized into m_token_stream by this point.
     auto unparsed_sizes_list = parse_a_comma_separated_list_of_component_values(m_token_stream);
     auto unparsed_sizes_list = parse_a_comma_separated_list_of_component_values(m_token_stream);
 
 
     // 2. Let size be null.
     // 2. Let size be null.
     Optional<LengthOrCalculated> size;
     Optional<LengthOrCalculated> size;
 
 
+    auto size_is_auto = [&size]() {
+        return !size->is_calculated() && size->value().is_auto();
+    };
+
+    auto remove_all_consecutive_whitespace_tokens_from_the_end_of = [](auto& tokens) {
+        while (!tokens.is_empty() && tokens.last().is_token() && tokens.last().token().is(Token::Type::Whitespace))
+            tokens.take_last();
+    };
+
     // 3. For each unparsed size in unparsed sizes list:
     // 3. For each unparsed size in unparsed sizes list:
-    for (auto& unparsed_size : unparsed_sizes_list) {
+    for (auto i = 0u; i < unparsed_sizes_list.size(); i++) {
+        auto& unparsed_size = unparsed_sizes_list[i];
+
         // 1. Remove all consecutive <whitespace-token>s from the end of unparsed size.
         // 1. Remove all consecutive <whitespace-token>s from the end of unparsed size.
         //    If unparsed size is now empty, that is a parse error; continue.
         //    If unparsed size is now empty, that is a parse error; continue.
-        while (!unparsed_size.is_empty() && unparsed_size.last().is_token() && unparsed_size.last().token().is(Token::Type::Whitespace))
-            unparsed_size.take_last();
+        remove_all_consecutive_whitespace_tokens_from_the_end_of(unparsed_size);
         if (unparsed_size.is_empty()) {
         if (unparsed_size.is_empty()) {
             log_parse_error();
             log_parse_error();
+            dbgln_if(CSS_PARSER_DEBUG, "-> Failed in step 3.1; all whitespace");
             continue;
             continue;
         }
         }
 
 
         // 2. If the last component value in unparsed size is a valid non-negative <source-size-value>,
         // 2. If the last component value in unparsed size is a valid non-negative <source-size-value>,
-        //    let size be its value and remove the component value from unparsed size.
-        //    FIXME: Any CSS function other than the math functions is invalid.
+        //    then set size to its value and remove the component value from unparsed size.
+        //    Any CSS function other than the math functions is invalid.
         //    Otherwise, there is a parse error; continue.
         //    Otherwise, there is a parse error; continue.
         auto last_value_stream = TokenStream<ComponentValue>::of_single_token(unparsed_size.last());
         auto last_value_stream = TokenStream<ComponentValue>::of_single_token(unparsed_size.last());
         if (auto source_size_value = parse_source_size_value(last_value_stream); source_size_value.has_value()) {
         if (auto source_size_value = parse_source_size_value(last_value_stream); source_size_value.has_value()) {
@@ -7960,33 +7976,55 @@ LengthOrCalculated Parser::Parser::parse_as_sizes_attribute()
             unparsed_size.take_last();
             unparsed_size.take_last();
         } else {
         } else {
             log_parse_error();
             log_parse_error();
+            dbgln_if(CSS_PARSER_DEBUG, "-> Failed in step 3.2; couldn't parse {} as a <source-size-value>", unparsed_size.last().to_debug_string());
             continue;
             continue;
         }
         }
 
 
-        // 3. Remove all consecutive <whitespace-token>s from the end of unparsed size.
-        while (!unparsed_size.is_empty() && unparsed_size.last().is_token() && unparsed_size.last().token().is(Token::Type::Whitespace))
-            unparsed_size.take_last();
+        // 3. If size is auto, and img is not null, and img is being rendered, and img allows auto-sizes,
+        //    then set size to the concrete object size width of img, in CSS pixels.
+        // FIXME: "img is being rendered" - we just see if it has a bitmap for now
+        if (size_is_auto() && img && img->immutable_bitmap() && img->allows_auto_sizes()) {
+            // FIXME: The spec doesn't seem to tell us how to determine the concrete size of an <img>, so use the default sizing algorithm.
+            //        Should this use some of the methods from FormattingContext?
+            auto concrete_size = run_default_sizing_algorithm(
+                img->width(), img->height(),
+                img->natural_width(), img->natural_height(), img->intrinsic_aspect_ratio(),
+                // NOTE: https://html.spec.whatwg.org/multipage/rendering.html#img-contain-size
+                CSSPixelSize { 300, 150 });
+            size = Length::make_px(concrete_size.width());
+        }
 
 
-        // If unparsed size is now empty, then return size.
-        if (unparsed_size.is_empty())
-            return size.value();
+        // 4. Remove all consecutive <whitespace-token>s from the end of unparsed size.
+        //    If unparsed size is now empty:
+        remove_all_consecutive_whitespace_tokens_from_the_end_of(unparsed_size);
+        if (unparsed_size.is_empty()) {
+            // 1. If this was not the last item in unparsed sizes list, that is a parse error.
+            if (i != unparsed_sizes_list.size() - 1) {
+                log_parse_error();
+                dbgln_if(CSS_PARSER_DEBUG, "-> Failed in step 3.4.1; is unparsed size #{}, count {}", i, unparsed_sizes_list.size());
+            }
 
 
-        // FIXME: If this was not the keyword auto and it was not the last item in unparsed sizes list, that is a parse error.
+            // 2. If size is not auto, then return size. Otherwise, continue.
+            if (!size_is_auto())
+                return size.release_value();
+            continue;
+        }
 
 
-        // 4. Parse the remaining component values in unparsed size as a <media-condition>.
+        // 5. Parse the remaining component values in unparsed size as a <media-condition>.
         //    If it does not parse correctly, or it does parse correctly but the <media-condition> evaluates to false, continue.
         //    If it does not parse correctly, or it does parse correctly but the <media-condition> evaluates to false, continue.
         TokenStream<ComponentValue> token_stream { unparsed_size };
         TokenStream<ComponentValue> token_stream { unparsed_size };
         auto media_condition = parse_media_condition(token_stream, MediaCondition::AllowOr::Yes);
         auto media_condition = parse_media_condition(token_stream, MediaCondition::AllowOr::Yes);
-        auto context_window = m_context.window();
-        if (context_window && media_condition && media_condition->evaluate(*context_window) == MatchResult::True) {
-            return size.value();
+        auto const* context_window = m_context.window();
+        if (!media_condition || (context_window && media_condition->evaluate(*context_window) == MatchResult::False)) {
+            continue;
         }
         }
 
 
-        // 5. If size is not auto, then return size.
-        if (size.value().is_calculated() || !size.value().value().is_auto())
+        // 5. If size is not auto, then return size. Otherwise, continue.
+        if (!size_is_auto())
             return size.value();
             return size.value();
     }
     }
 
 
+    // 4. Return 100vw.
     return Length(100, Length::Type::Vw);
     return Length(100, Length::Type::Vw);
 }
 }
 
 

+ 1 - 1
Userland/Libraries/LibWeb/CSS/Parser/Parser.h

@@ -75,7 +75,7 @@ public:
 
 
     static NonnullRefPtr<CSSStyleValue> resolve_unresolved_style_value(ParsingContext const&, DOM::Element&, Optional<CSS::Selector::PseudoElement::Type>, PropertyID, UnresolvedStyleValue const&);
     static NonnullRefPtr<CSSStyleValue> resolve_unresolved_style_value(ParsingContext const&, DOM::Element&, Optional<CSS::Selector::PseudoElement::Type>, PropertyID, UnresolvedStyleValue const&);
 
 
-    [[nodiscard]] LengthOrCalculated parse_as_sizes_attribute();
+    [[nodiscard]] LengthOrCalculated parse_as_sizes_attribute(DOM::Element const& element, HTML::HTMLImageElement const* img = nullptr);
 
 
 private:
 private:
     Parser(ParsingContext const&, Vector<Token>);
     Parser(ParsingContext const&, Vector<Token>);

+ 12 - 5
Userland/Libraries/LibWeb/HTML/HTMLImageElement.cpp

@@ -990,7 +990,12 @@ static void update_the_source_set(DOM::Element& element)
         });
         });
     }
     }
 
 
-    // 4. For each child in elements:
+    // 4. Let img be el if el is an img element, otherwise null.
+    HTMLImageElement* img = nullptr;
+    if (is<HTMLImageElement>(element))
+        img = static_cast<HTMLImageElement*>(&element);
+
+    // 5. For each child in elements:
     for (auto child : elements) {
     for (auto child : elements) {
         // 1. If child is el:
         // 1. If child is el:
         if (child == &element) {
         if (child == &element) {
@@ -1039,11 +1044,13 @@ static void update_the_source_set(DOM::Element& element)
                     default_source = href_value.release_value();
                     default_source = href_value.release_value();
             }
             }
 
 
-            // 10. Let el's source set be the result of creating a source set given default source, srcset, and sizes.
+            // 10. Let el's source set be the result of creating a source set given default source, srcset, sizes, and img.
             if (is<HTMLImageElement>(element))
             if (is<HTMLImageElement>(element))
-                static_cast<HTMLImageElement&>(element).set_source_set(SourceSet::create(element, default_source, srcset, sizes));
+                static_cast<HTMLImageElement&>(element).set_source_set(SourceSet::create(element, default_source, srcset, sizes, img));
             else if (is<HTMLLinkElement>(element))
             else if (is<HTMLLinkElement>(element))
                 TODO();
                 TODO();
+
+            // 11. Return.
             return;
             return;
         }
         }
         // 2. If child is not a source element, then continue.
         // 2. If child is not a source element, then continue.
@@ -1070,8 +1077,8 @@ static void update_the_source_set(DOM::Element& element)
             }
             }
         }
         }
 
 
-        // 7. Parse child's sizes attribute, and let source set's source size be the returned value.
-        source_set.m_source_size = parse_a_sizes_attribute(element.document(), child->get_attribute_value(HTML::AttributeNames::sizes));
+        // 7. Parse child's sizes attribute with img, and let source set's source size be the returned value.
+        source_set.m_source_size = parse_a_sizes_attribute(element, child->get_attribute_value(HTML::AttributeNames::sizes), img);
 
 
         // 8. If child has a type attribute, and its value is an unknown or unsupported MIME type, continue to the next child.
         // 8. If child has a type attribute, and its value is an unknown or unsupported MIME type, continue to the next child.
         if (child->has_attribute(HTML::AttributeNames::type)) {
         if (child->has_attribute(HTML::AttributeNames::type)) {

+ 8 - 6
Userland/Libraries/LibWeb/HTML/SourceSet.cpp

@@ -339,15 +339,17 @@ descriptor_parser:
 }
 }
 
 
 // https://html.spec.whatwg.org/multipage/images.html#parse-a-sizes-attribute
 // https://html.spec.whatwg.org/multipage/images.html#parse-a-sizes-attribute
-CSS::LengthOrCalculated parse_a_sizes_attribute(DOM::Document const& document, StringView sizes)
+CSS::LengthOrCalculated parse_a_sizes_attribute(DOM::Element const& element, StringView sizes, HTML::HTMLImageElement const* img)
 {
 {
-    auto css_parser = CSS::Parser::Parser::create(CSS::Parser::ParsingContext { document }, sizes);
-    return css_parser.parse_as_sizes_attribute();
+    auto css_parser = CSS::Parser::Parser::create(CSS::Parser::ParsingContext { element.document() }, sizes);
+    return css_parser.parse_as_sizes_attribute(element, img);
 }
 }
 
 
 // https://html.spec.whatwg.org/multipage/images.html#create-a-source-set
 // https://html.spec.whatwg.org/multipage/images.html#create-a-source-set
-SourceSet SourceSet::create(DOM::Element const& element, String default_source, String srcset, String sizes)
+SourceSet SourceSet::create(DOM::Element const& element, String const& default_source, String const& srcset, String const& sizes, HTML::HTMLImageElement const* img)
 {
 {
+    // When asked to create a source set given a string default source, a string srcset, a string sizes, and an element or null img:
+
     // 1. Let source set be an empty source set.
     // 1. Let source set be an empty source set.
     SourceSet source_set;
     SourceSet source_set;
 
 
@@ -355,8 +357,8 @@ SourceSet SourceSet::create(DOM::Element const& element, String default_source,
     if (!srcset.is_empty())
     if (!srcset.is_empty())
         source_set = parse_a_srcset_attribute(srcset);
         source_set = parse_a_srcset_attribute(srcset);
 
 
-    // 3. Let source size be the result of parsing sizes.
-    source_set.m_source_size = parse_a_sizes_attribute(element.document(), sizes);
+    // 3. Let source size be the result of parsing sizes with img.
+    source_set.m_source_size = parse_a_sizes_attribute(element, sizes, img);
 
 
     // 4. If default source is not the empty string and source set does not contain an image source
     // 4. If default source is not the empty string and source set does not contain an image source
     //    with a pixel density descriptor value of 1, and no image source with a width descriptor,
     //    with a pixel density descriptor value of 1, and no image source with a width descriptor,

+ 2 - 2
Userland/Libraries/LibWeb/HTML/SourceSet.h

@@ -33,7 +33,7 @@ struct ImageSourceAndPixelDensity {
 
 
 // https://html.spec.whatwg.org/multipage/images.html#source-set
 // https://html.spec.whatwg.org/multipage/images.html#source-set
 struct SourceSet {
 struct SourceSet {
-    static SourceSet create(DOM::Element const&, String default_source, String srcset, String sizes);
+    static SourceSet create(DOM::Element const& element, String const& default_source, String const& srcset, String const& sizes, HTML::HTMLImageElement const* img = nullptr);
 
 
     [[nodiscard]] bool is_empty() const;
     [[nodiscard]] bool is_empty() const;
 
 
@@ -50,6 +50,6 @@ struct SourceSet {
 };
 };
 
 
 SourceSet parse_a_srcset_attribute(StringView);
 SourceSet parse_a_srcset_attribute(StringView);
-[[nodiscard]] CSS::LengthOrCalculated parse_a_sizes_attribute(DOM::Document const&, StringView);
+[[nodiscard]] CSS::LengthOrCalculated parse_a_sizes_attribute(DOM::Element const& element, StringView sizes, HTML::HTMLImageElement const* img = nullptr);
 
 
 }
 }