Bug 226469

Summary: Aspect ratio from width and height attribute is not compatible to string with invalid ends.
Product: WebKit Reporter: cathiechen <cathiechen>
Component: CSSAssignee: cathiechen <cathiechen>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, changseok, darin, esprehn+autocc, ews-watchlist, gyuyoung.kim, koivisto, rbuis, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=201641
https://bugs.webkit.org/show_bug.cgi?id=226810
Bug Depends on: 226472    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch none

Description cathiechen 2021-05-31 06:46:05 PDT
<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.
Comment 1 cathiechen 2021-06-01 03:05:42 PDT
Created attachment 430243 [details]
Patch
Comment 2 cathiechen 2021-06-01 03:35:59 PDT
Comment on attachment 430243 [details]
Patch

Hi,
I think this patch is ready for review:)
Comment 3 Rob Buis 2021-06-01 04:07:12 PDT
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.
Comment 4 cathiechen 2021-06-01 04:20:53 PDT
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:)
Comment 5 Rob Buis 2021-06-01 04:39:41 PDT
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 :)
Comment 6 cathiechen 2021-06-01 04:41:06 PDT
Created attachment 430244 [details]
Patch
Comment 7 cathiechen 2021-06-01 04:49:30 PDT
Created attachment 430247 [details]
Patch
Comment 8 cathiechen 2021-06-01 04:54:33 PDT
Created attachment 430249 [details]
Patch
Comment 9 cathiechen 2021-06-01 04:57:07 PDT
Created attachment 430250 [details]
Patch
Comment 10 Darin Adler 2021-06-01 10:01:02 PDT
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.
Comment 11 Darin Adler 2021-06-01 19:13:58 PDT
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.
Comment 12 cathiechen 2021-06-06 08:06:57 PDT
(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:)
Comment 13 cathiechen 2021-06-06 08:42:58 PDT
Created attachment 430683 [details]
Patch
Comment 14 Darin Adler 2021-06-06 15:56:25 PDT
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&.
Comment 15 cathiechen 2021-06-07 06:12:15 PDT
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!
Comment 16 Radar WebKit Bug Importer 2021-06-07 06:47:16 PDT
<rdar://problem/78945539>
Comment 17 cathiechen 2021-06-07 06:58:41 PDT
Created attachment 430743 [details]
Patch
Comment 18 Darin Adler 2021-06-07 16:05:51 PDT
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?
Comment 19 Darin Adler 2021-06-07 16:08:09 PDT
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.
Comment 20 cathiechen 2021-06-08 02:37:11 PDT
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:)
Comment 21 cathiechen 2021-06-08 08:54:52 PDT
Created attachment 430847 [details]
Patch
Comment 22 cathiechen 2021-06-08 09:24:46 PDT
Created attachment 430849 [details]
Patch
Comment 23 cathiechen 2021-06-08 09:49:52 PDT
Created attachment 430853 [details]
Patch
Comment 24 cathiechen 2021-06-08 19:01:19 PDT
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...
Comment 25 cathiechen 2021-06-09 02:56:01 PDT
(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.
Comment 26 cathiechen 2021-06-09 03:13:22 PDT
Created attachment 430950 [details]
Patch
Comment 27 cathiechen 2021-06-09 03:39:43 PDT
Comment on attachment 430950 [details]
Patch

Hi,
I think this patch is getting small and ready for review now:)
Comment 28 Antti Koivisto 2021-06-09 04:05:40 PDT
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?
Comment 29 cathiechen 2021-06-09 04:17:48 PDT
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.
Comment 30 cathiechen 2021-06-09 05:25:33 PDT
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!
Comment 31 cathiechen 2021-06-09 05:30:45 PDT
Created attachment 430955 [details]
Patch
Comment 32 Darin Adler 2021-06-09 08:47:57 PDT
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.
Comment 33 cathiechen 2021-06-09 10:46:14 PDT
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:)
Comment 34 cathiechen 2021-06-09 10:47:40 PDT
Created attachment 430978 [details]
Patch
Comment 35 EWS 2021-06-09 20:17:12 PDT
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].