Bug 226469 – Aspect ratio from width and height attribute is not compatible to string with invalid ends.

• Component: CSS
• Status: RESOLVED FIXED

[cathiechen]

<img src="/images/green.png" width="100px" height="25px">

The aspectRatio should be as compatible as attributes. The aspectRatio value should be 4 for the example.

[cathiechen]

Created attachment 430243 [details]
Patch

[cathiechen]

Comment on attachment 430243 [details]
Patch

Hi,
I think this patch is ready for review:)

[Rob Buis]

Comment on attachment 430243 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430243&action=review

> Source/WebCore/ChangeLog:3
> +        Aspect ratio from width and height attribute is not compatible to invalid trails

What does "invalid trails" mean?

> Source/WebCore/ChangeLog:8
> +        This patch extracts the invalid checking of length string to getValidPositionForLength(), so it

You probably mean here, "checking of invalid length string". The difference is a bit subtle :)

> Source/WebCore/html/HTMLElement.cpp:1055
> +    if (validPosition != value.length()) {

In modern C++ the above 2 lines can be combined in an if statement IIRC, but not sure our compilers support it.

[cathiechen]

Comment on attachment 430243 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430243&action=review

Hi Rob,
Thanks for the review:)

>> Source/WebCore/ChangeLog:3
>> +        Aspect ratio from width and height attribute is not compatible to invalid trails
> 
> What does "invalid trails" mean?

I mean the length string followed by invalid value, so invalid trailing?

>> Source/WebCore/ChangeLog:8
>> +        This patch extracts the invalid checking of length string to getValidPositionForLength(), so it
> 
> You probably mean here, "checking of invalid length string". The difference is a bit subtle :)

Done, thanks:)

>> Source/WebCore/html/HTMLElement.cpp:1055
>> +    if (validPosition != value.length()) {
> 
> In modern C++ the above 2 lines can be combined in an if statement IIRC, but not sure our compilers support it.

Changes to `if (auto validPosition = getValidPositionForLength(value) && validPosition != value.length())`?
I'll have a try:)

[Rob Buis]

Comment on attachment 430243 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430243&action=review

>>> Source/WebCore/ChangeLog:3
>>> +        Aspect ratio from width and height attribute is not compatible to invalid trails
>> 
>> What does "invalid trails" mean?
> 
> I mean the length string followed by invalid value, so invalid trailing?

Ah right, I understand now you mention trailing :)

[cathiechen]

Created attachment 430244 [details]
Patch

[cathiechen]

Created attachment 430247 [details]
Patch

[cathiechen]

Created attachment 430249 [details]
Patch

[cathiechen]

Created attachment 430250 [details]
Patch

[Darin Adler]

Comment on attachment 430250 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430250&action=review

> Source/WebCore/html/HTMLElement.cpp:149
> +static inline unsigned getValidPositionForLength(const String& value)
> +{
> +    StringImpl* string = value.impl();
> +    if (!string)
> +        return 0;
> +
> +    unsigned parsedLength = 0;
> +    while (parsedLength < string->length() && (*string)[parsedLength] <= ' ')
> +        ++parsedLength;
> +
> +    for (; parsedLength < string->length(); ++parsedLength) {
> +        UChar cc = (*string)[parsedLength];
> +        if (cc > '9')
> +            break;
> +        if (cc < '0') {
> +            if (cc == '%' || cc == '*')
> +                ++parsedLength;
> +            if (cc != '.')
> +                break;
> +        }
> +    }
> +    return parsedLength;
> +}

This is not named in WebKit style. We don’t use get in function names when they have return values. This should take a StringView argument, not a const String&. It’s quite strange that this function used "cc" as its variable name.

But also, I don’t think this strategy of first scanning and then calling the parser is good, so while this *might* be OK for now, it’s actually not a good longer term strategy.

> Source/WebCore/html/HTMLElement.cpp:658
> +        if (auto validPosition = getValidPositionForLength(string); validPosition != string.length())

The strategy of computing a length and doing substring is not the best way to ignore trailing junk. Instead we should add a parseHTMLFloatingPointNumber function that tolerates trailing junk. After reading the HTML specification more carefully, I think we should be changing the functions in HTMLParserIdioms and likely almost never want the algorithm in the current parseValidHTMLFloatingPointNumber function; in some cases we are doing checks for "valid" floating point numbers, when instead we should just be following <https://html.spec.whatwg.org/#rules-for-parsing-floating-point-number-values>.

> Source/WebCore/html/HTMLElement.cpp:659
> +            return parseValidHTMLFloatingPointNumber(string.substring(0, validPosition)).value_or(-1);

This should use the more efficient StringView::substring; doing it with String::substring allocations memory unnecessarily.

[Darin Adler]

I did some more research, and I think the best short term fix here is to use parseToDoubleForNumberType instead of parseValidHTMLFloatingPointNumber. Later we should rename these functions and clarify their relationship with each other.

[cathiechen]

(In reply to Darin Adler from comment #11)
> I did some more research, and I think the best short term fix here is to use
> parseToDoubleForNumberType instead of parseValidHTMLFloatingPointNumber.
> Later we should rename these functions and clarify their relationship with
> each other.

Hi Darin,
Thanks for the review, and sorry for the slow reply!

Yes, I think it makes sense to do a long-term fix. I'll upload a patch with long-term fix later:)

[cathiechen]

Created attachment 430683 [details]
Patch

[Darin Adler]

Comment on attachment 430683 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430683&action=review

Looks good, can be improved.

> Source/WebCore/html/HTMLDimension.h:48
> +class HTMLDimension {
> +    WTF_MAKE_FAST_ALLOCATED;
> +public:
> +    enum class HTMLDimensionType { Percentage, Absolute };
> +
> +    HTMLDimension() { }
> +    HTMLDimension(HTMLDimensionType type, double value)
> +        : m_type(type)
> +        , m_value(value)
> +    { }
> +
> +    bool isAbsolute() const { return m_type == HTMLDimensionType::Absolute; }
> +    bool isPercent() const { return m_type == HTMLDimensionType::Percentage; }
> +    double value() const { return m_value; }
> +
> +private:
> +    HTMLDimensionType m_type { HTMLDimensionType::Absolute };
> +    double m_value { 0 };
> +};

We should use a struct for this. Making it a class with getters and setters doesn’t make things clearer:

    struct HTMLDimension {
        enum class Type: bool ( Percentage, Absolute };
        double value;
        Type type;
    };

Note that you don’t have to repeat the name of the class in the name of the enum member. And you can use a base type to help the compiler optimize better.

This is simple enough that I think it can go into HTMLParserIdioms.h and doesn’t need its own header.

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:481
> +template <typename CharType>

We should spell it out, CharacterType.

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:501
> +    double value = dimensionString.toString().toDouble();

There is no need to call toString().toDouble(). The same operation is available on a StringView. You can call parseDouble from dtoa.h as other functions in this file do.

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:505
> +    HTMLDimension::HTMLDimensionType type = HTMLDimension::HTMLDimensionType::Absolute;

Should use auto here instead of writing out HTMLDimension::HTMLDimensionType twice.

    auto type = ...

> Source/WebCore/html/parser/HTMLParserIdioms.h:91
> +std::optional<HTMLDimension> parseHTMLDimension(const StringView&);

This should take StringView, not const StringView&.

[cathiechen]

Comment on attachment 430683 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430683&action=review

>> Source/WebCore/html/HTMLDimension.h:48
>> +};
> 
> We should use a struct for this. Making it a class with getters and setters doesn’t make things clearer:
> 
>     struct HTMLDimension {
>         enum class Type: bool ( Percentage, Absolute };
>         double value;
>         Type type;
>     };
> 
> Note that you don’t have to repeat the name of the class in the name of the enum member. And you can use a base type to help the compiler optimize better.
> 
> This is simple enough that I think it can go into HTMLParserIdioms.h and doesn’t need its own header.

Done, thanks!

>> Source/WebCore/html/parser/HTMLParserIdioms.cpp:481
>> +template <typename CharType>
> 
> We should spell it out, CharacterType.

Done!

>> Source/WebCore/html/parser/HTMLParserIdioms.cpp:501
>> +    double value = dimensionString.toString().toDouble();
> 
> There is no need to call toString().toDouble(). The same operation is available on a StringView. You can call parseDouble from dtoa.h as other functions in this file do.

Done, thanks!

>> Source/WebCore/html/parser/HTMLParserIdioms.cpp:505
>> +    HTMLDimension::HTMLDimensionType type = HTMLDimension::HTMLDimensionType::Absolute;
> 
> Should use auto here instead of writing out HTMLDimension::HTMLDimensionType twice.
> 
>     auto type = ...

Done!

>> Source/WebCore/html/parser/HTMLParserIdioms.h:91
>> +std::optional<HTMLDimension> parseHTMLDimension(const StringView&);
> 
> This should take StringView, not const StringView&.

I see, got it! Done! Thanks!

[Radar WebKit Bug Importer]

<rdar://problem/78945539>

[cathiechen]

Created attachment 430743 [details]
Patch

[Darin Adler]

Comment on attachment 430743 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430743&action=review

> Source/WebCore/html/HTMLElement.h:156
> +    void addHTMLLengthToStyle(MutableStyleProperties&, CSSPropertyID, const String& value, const AllowPercentageValue = AllowPercentageValue::Yes);

No need for const AllowPercentageValue, just AllowPercentageValue.

> Source/WebCore/html/parser/HTMLParserIdioms.h:92
> +    // Add Relative type to distinguish "number + *".
> +    enum class Type : uint8_t { Percentage, Absolute, Relative };

The HTML specification parsing rules mentions that the result is a number and whether it is a percentage and length. So I would expect the first member of this struct to be "number" to match the wording of the specification and the other to be "type", either percentage or length.

The comment above doesn’t explain to me why we then add the concept of "relative". Can we put the check for the * into a different function?

[Darin Adler]

Comment on attachment 430743 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430743&action=review

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:487
> +    skipWhile<isASCIISpace>(position, end);

This should be isHTMLSpace. When the HTML specification says "ASCII space" they mean isHTMLSpace, sadly, not isASCIISpace.

[cathiechen]

Comment on attachment 430743 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430743&action=review

Hi Darin,
Thanks for the review:)

>> Source/WebCore/html/HTMLElement.h:156
>> +    void addHTMLLengthToStyle(MutableStyleProperties&, CSSPropertyID, const String& value, const AllowPercentageValue = AllowPercentageValue::Yes);
> 
> No need for const AllowPercentageValue, just AllowPercentageValue.

Done.

>> Source/WebCore/html/parser/HTMLParserIdioms.cpp:487
>> +    skipWhile<isASCIISpace>(position, end);
> 
> This should be isHTMLSpace. When the HTML specification says "ASCII space" they mean isHTMLSpace, sadly, not isASCIISpace.

Done, thanks!

>> Source/WebCore/html/parser/HTMLParserIdioms.h:92
>> +    enum class Type : uint8_t { Percentage, Absolute, Relative };
> 
> The HTML specification parsing rules mentions that the result is a number and whether it is a percentage and length. So I would expect the first member of this struct to be "number" to match the wording of the specification and the other to be "type", either percentage or length.
> 
> The comment above doesn’t explain to me why we then add the concept of "relative". Can we put the check for the * into a different function?

Yeah, make sense. I'm a little confused by the "length" type in the spec. Should I use "Pixel" instead?

struct HTMLDimension {
    enum class Type : uint8_t { Percentage, Pixel };
    double number;
    Type type;
};

As to the relative_length, I will add a new function to handle it:)

[cathiechen]

Created attachment 430847 [details]
Patch

[cathiechen]

Created attachment 430849 [details]
Patch

[cathiechen]

Created attachment 430853 [details]
Patch

[cathiechen]

Comment on attachment 430853 [details]
Patch

Hi Darin,
I think the patch is ready for review.
Sorry about that it gets much bigger than expected...

[cathiechen]

(In reply to cathiechen from comment #24)
> Comment on attachment 430853 [details]
> Patch
> 
> Hi Darin,
> I think the patch is ready for review.
> Sorry about that it gets much bigger than expected...

OK, I think it makes sense to split the patch.
One is to introduce HTMLDimension and fix the aspect-ratio trailing junk.
The other one will reuse parseHTMLDimension to other attributes parsing.

[cathiechen]

Created attachment 430950 [details]
Patch

[cathiechen]

Comment on attachment 430950 [details]
Patch

Hi,
I think this patch is getting small and ready for review now:)

[Antti Koivisto]

Comment on attachment 430950 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430950&action=review

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:532
> +std::optional<HTMLDimension> parseHTMLDimension(StringView dimensionString)
> +{
> +    return parseHTMLDimensionInternal(dimensionString);
> +}

This doesn't seem to add anything. Why not just have parseHTMLDimension and get rid of the *Internal version?

[cathiechen]

Comment on attachment 430950 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430950&action=review

Hi Antti,
Thanks for the review!

>> Source/WebCore/html/parser/HTMLParserIdioms.cpp:532
>> +}
> 
> This doesn't seem to add anything. Why not just have parseHTMLDimension and get rid of the *Internal version?

I want to reuse parseHTMLDimensionInternal to parse relative_length (number + *) in the future patch, so that it can take a flag to indicate if it should handle relative_lenght or not.

[cathiechen]

Comment on attachment 430950 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430950&action=review

>>> Source/WebCore/html/parser/HTMLParserIdioms.cpp:532
>>> +}
>> 
>> This doesn't seem to add anything. Why not just have parseHTMLDimension and get rid of the *Internal version?
> 
> I want to reuse parseHTMLDimensionInternal to parse relative_length (number + *) in the future patch, so that it can take a flag to indicate if it should handle relative_lenght or not.

Done!

[cathiechen]

Created attachment 430955 [details]
Patch

[Darin Adler]

Comment on attachment 430955 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430955&action=review

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:481
> +static double parseHTMLDimensionNumber(const CharacterType* position, unsigned length, unsigned& parsedLength)

I would use std::optional instead of using -1 as a magic number to represent failure to parse, and the parsed length should be part of the return value, not an out argument. But neither of those should hold up landing this patch. We can come back and do that later without affecting code outside this file.

[cathiechen]

Comment on attachment 430955 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430955&action=review

Hi Darin,
Thanks for the review!

>> Source/WebCore/html/parser/HTMLParserIdioms.cpp:481
>> +static double parseHTMLDimensionNumber(const CharacterType* position, unsigned length, unsigned& parsedLength)
> 
> I would use std::optional instead of using -1 as a magic number to represent failure to parse, and the parsed length should be part of the return value, not an out argument. But neither of those should hold up landing this patch. We can come back and do that later without affecting code outside this file.

The patch has not landed yet, it seems EWS is slow today. I will upload a new patch to address this:)

[cathiechen]

Created attachment 430978 [details]
Patch

[EWS]

Committed r278689 (238663@main): <https://commits.webkit.org/238663@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430978 [details].