Bug 183994 – border-radius inline style serializes with invalid syntax

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Eric Willigers]

https://jsfiddle.net/ericwilligers/49xccjzv/

Spec: https://drafts.csswg.org/css-backgrounds/#border-radius
<length-percentage>{1,4} [ / <length-percentage>{1,4} ]?

Set border-radius inline style to '11px 12px 13px 14px / 25px 26px 27px 28px'.

Reading inline style should give the same value.

Reading inline style gives the invalid value '11px 25px 12px 26px 13px 27px 14px 28px'.
Reading computed style gives correct value '11px 12px 13px 14px / 25px 26px 27px 28px'.

Blink has the same bug.

(Edge and Firefox have the inverse bug: they give correct inline style but give empty string as computed style.)

[Emilio Cobos Álvarez (:emilio)]

(In reply to Eric Willigers from comment #0)
> (Edge and Firefox have the inverse bug: they give correct inline style but
> give empty string as computed style.)

FWIW Firefox's behavior is pretty intentional and long-standing. Gecko never serializes shorthands in computed style, which sort-of makes sense, because shorthands are a specified style concept in general.

[Manuel Rego Casasnovas]

BTW, the following CSSWG issue is somehow related to this:
https://github.com/w3c/csswg-drafts/issues/1041

However it hasn't been solved for more than a year now. :-(

[Radar WebKit Bug Importer]

<rdar://problem/38928309>

[Joonghun Park]

Created attachment 442824 [details]
To check layout test result

[Joonghun Park]

Created attachment 442839 [details]
Patch

[Darin Adler]

Comment on attachment 442839 [details]
Patch

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

Looks OK, but I don’t think there is enough testing for edge cases here. I’m going to say review+ but I think we should refine this more before landing.

> Source/WebCore/css/StyleProperties.cpp:372
> +        bool showBottomLeft = !(topRight == bottomLeft);
> +        bool showBottomRight = !(topLeft == bottomRight) || showBottomLeft;
> +        bool showTopRight = !(topLeft == topRight) || showBottomRight;

Why not use != instead of !(==)?

Why does this compare the pointers instead of comparing the values? I suggest we compute all four strings, and compare equality of the strings. It’s OK to optimize the case where the pointers are identical, but we can’t count on comparing the pointers to see if the values are the same.

> Source/WebCore/css/StyleProperties.cpp:388
> +        StringBuilder result;
> +        result.append(topLeft->cssText());
> +        if (showTopRight) {
> +            result.append(' ');
> +            result.append(topRight->cssText());
> +        }
> +        if (showBottomRight) {
> +            result.append(' ');
> +            result.append(bottomRight->cssText());
> +        }
> +        if (showBottomLeft) {
> +            result.append(' ');
> +            result.append(bottomLeft->cssText());
> +        }
> +        return result.toString();

This can likely be done much more efficiently with makeString.

> Source/WebCore/css/StyleProperties.cpp:398
> +    // All 4 properties must be specified.
> +    if (!topLeft || !topRight || !bottomRight || !bottomLeft)
> +        return String();

I don’t understand this. What does "must be specified" mean here. Is this really an assertion? Why is returning the null string OK?

> Source/WebCore/css/StyleProperties.cpp:400
> +    // FIXME: pair should be replaced with CSSValuePair as specified in CSSPropertyParser's FIXME-NEWPARSER.

Do we really need this FIXME? How will this help us when we do make the change.

> Source/WebCore/css/StyleProperties.cpp:404
> +    const auto* topLeftPair = downcast<CSSPrimitiveValue>(*getPropertyCSSValue(CSSPropertyBorderTopLeftRadius)).pairValue();
> +    const auto* topRightPair = downcast<CSSPrimitiveValue>(*getPropertyCSSValue(CSSPropertyBorderTopRightRadius)).pairValue();
> +    const auto* bottomRightPair = downcast<CSSPrimitiveValue>(*getPropertyCSSValue(CSSPropertyBorderBottomRightRadius)).pairValue();
> +    const auto* bottomLeftPair = downcast<CSSPrimitiveValue>(*getPropertyCSSValue(CSSPropertyBorderBottomLeftRadius)).pairValue();

Should be using the *topLeft, not call getPropertyCSSValue again.

The const here isn’t important. If these are guaranteed to be non-null, how about references instead of pointers?

> Source/WebCore/css/StyleProperties.cpp:414
> +    StringBuilder builder;
> +    builder.append(serialize(topLeftPair->first(), topRightPair->first(), bottomRightPair->first(), bottomLeftPair->first()));
> +
> +    if (!(topLeftPair->first() == topLeftPair->second()) || !(topRightPair->first() == topRightPair->second()) || !(bottomRightPair->first() == bottomRightPair->second() || !(bottomLeftPair->first() == bottomLeftPair->second()))) {
> +        builder.append(" / ");
> +        builder.append(serialize(topLeftPair->second(), topRightPair->second(), bottomRightPair->second(), bottomLeftPair->second()));
> +    }
> +
> +    return builder.toString();

Same comment about comparing pointers instead of strings. We should compare the strings. Also, this shouldn’t be done with StringBuilder. We can just call makeString twice:

    auto first = serialize(topLeftPair->first(), topRightPair->first(), bottomRightPair->first(), bottomLeftPair->first());
    auto second = serialize(topLeftPair->second(), topRightPair->second(), bottomRightPair->second(), bottomLeftPair->second());
    if (first == second)
        return first;
    return makeString(first, " / ", second);

[Joonghun Park]

Comment on attachment 442839 [details]
Patch

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

Thank you for your review, Darin. I will apply the comments in the next patchset.

>> Source/WebCore/css/StyleProperties.cpp:372
>> +        bool showTopRight = !(topLeft == topRight) || showBottomRight;
> 
> Why not use != instead of !(==)?
> 
> Why does this compare the pointers instead of comparing the values? I suggest we compute all four strings, and compare equality of the strings. It’s OK to optimize the case where the pointers are identical, but we can’t count on comparing the pointers to see if the values are the same.

Ok, I will change this to use !== in the next patchset. And we should compare values instead of pointers. That was just my mistake. Thank you for pointing it out.

>> Source/WebCore/css/StyleProperties.cpp:388
>> +        return result.toString();
> 
> This can likely be done much more efficiently with makeString.

Ok, I will use makeString in the next patchset.

>> Source/WebCore/css/StyleProperties.cpp:398
>> +        return String();
> 
> I don’t understand this. What does "must be specified" mean here. Is this really an assertion? Why is returning the null string OK?

This null check is the same one in StyleProperties::get4Values(). Without this, I met a crash regression running LayoutTests/imported/w3c/web-platform-tests/css/css-backgrounds/parsing/border-radius-invalid.html, so I added this check.

Actually, below codes in CSSPropertyParser set all the topLeft, topRight, bottomRight, bottomLeft css property if consumeRadii didn't fail 
this check looks valid, I think.

case CSSPropertyWebkitBorderRadius: {
        RefPtr<CSSPrimitiveValue> horizontalRadii[4];
        RefPtr<CSSPrimitiveValue> verticalRadii[4];
        if (!consumeRadii(horizontalRadii, verticalRadii, m_range, m_context.mode, property == CSSPropertyWebkitBorderRadius))
            return false;
        addProperty(CSSPropertyBorderTopLeftRadius, CSSPropertyBorderRadius, createPrimitiveValuePair(horizontalRadii[0].releaseNonNull(), verticalRadii[0].releaseNonNull(), Pair::IdenticalValueEncoding::Coalesce), important);
        addProperty(CSSPropertyBorderTopRightRadius, CSSPropertyBorderRadius, createPrimitiveValuePair(horizontalRadii[1].releaseNonNull(), verticalRadii[1].releaseNonNull(), Pair::IdenticalValueEncoding::Coalesce), important);
        addProperty(CSSPropertyBorderBottomRightRadius, CSSPropertyBorderRadius, createPrimitiveValuePair(horizontalRadii[2].releaseNonNull(), verticalRadii[2].releaseNonNull(), Pair::IdenticalValueEncoding::Coalesce), important);
        addProperty(CSSPropertyBorderBottomLeftRadius, CSSPropertyBorderRadius, createPrimitiveValuePair(horizontalRadii[3].releaseNonNull(), verticalRadii[3].releaseNonNull(), Pair::IdenticalValueEncoding::Coalesce), important);
        return true;
    }

>> Source/WebCore/css/StyleProperties.cpp:400
>> +    // FIXME: pair should be replaced with CSSValuePair as specified in CSSPropertyParser's FIXME-NEWPARSER.
> 
> Do we really need this FIXME? How will this help us when we do make the change.

Ah, this FIXME doesn't have to be here necessarily. I'm interested in split CSSPrimitiveValue to CSSValue subclasses, so it was just to mark it here.

>> Source/WebCore/css/StyleProperties.cpp:404
>> +    const auto* bottomLeftPair = downcast<CSSPrimitiveValue>(*getPropertyCSSValue(CSSPropertyBorderBottomLeftRadius)).pairValue();
> 
> Should be using the *topLeft, not call getPropertyCSSValue again.
> 
> The const here isn’t important. If these are guaranteed to be non-null, how about references instead of pointers?

Ok, I will use *topLeft and reference in the next patchset.

>> Source/WebCore/css/StyleProperties.cpp:414
>> +    return builder.toString();
> 
> Same comment about comparing pointers instead of strings. We should compare the strings. Also, this shouldn’t be done with StringBuilder. We can just call makeString twice:
> 
>     auto first = serialize(topLeftPair->first(), topRightPair->first(), bottomRightPair->first(), bottomLeftPair->first());
>     auto second = serialize(topLeftPair->second(), topRightPair->second(), bottomRightPair->second(), bottomLeftPair->second());
>     if (first == second)
>         return first;
>     return makeString(first, " / ", second);

Ditto.

[Joonghun Park]

Comment on attachment 442839 [details]
Patch

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

>>> Source/WebCore/css/StyleProperties.cpp:372
>>> +        bool showTopRight = !(topLeft == topRight) || showBottomRight;
>> 
>> Why not use != instead of !(==)?
>> 
>> Why does this compare the pointers instead of comparing the values? I suggest we compute all four strings, and compare equality of the strings. It’s OK to optimize the case where the pointers are identical, but we can’t count on comparing the pointers to see if the values are the same.
> 
> Ok, I will change this to use !== in the next patchset. And we should compare values instead of pointers. That was just my mistake. Thank you for pointing it out.

It seems that in CSSValue.h only operator== is defined. I'm not sure if it is good to add bool operator!=(const CSSValue& other) const { return !equals(other); }.
For now, I will use !( == ) here.

>>> Source/WebCore/css/StyleProperties.cpp:400
>>> +    // FIXME: pair should be replaced with CSSValuePair as specified in CSSPropertyParser's FIXME-NEWPARSER.
>> 
>> Do we really need this FIXME? How will this help us when we do make the change.
> 
> Ah, this FIXME doesn't have to be here necessarily. I'm interested in split CSSPrimitiveValue to CSSValue subclasses, so it was just to mark it here.

I removed unnecessary FIXME comment here.

>>> Source/WebCore/css/StyleProperties.cpp:404
>>> +    const auto* bottomLeftPair = downcast<CSSPrimitiveValue>(*getPropertyCSSValue(CSSPropertyBorderBottomLeftRadius)).pairValue();
>> 
>> Should be using the *topLeft, not call getPropertyCSSValue again.
>> 
>> The const here isn’t important. If these are guaranteed to be non-null, how about references instead of pointers?
> 
> Ok, I will use *topLeft and reference in the next patchset.

Done.

>>> Source/WebCore/css/StyleProperties.cpp:414
>>> +    return builder.toString();
>> 
>> Same comment about comparing pointers instead of strings. We should compare the strings. Also, this shouldn’t be done with StringBuilder. We can just call makeString twice:
>> 
>>     auto first = serialize(topLeftPair->first(), topRightPair->first(), bottomRightPair->first(), bottomLeftPair->first());
>>     auto second = serialize(topLeftPair->second(), topRightPair->second(), bottomRightPair->second(), bottomLeftPair->second());
>>     if (first == second)
>>         return first;
>>     return makeString(first, " / ", second);
> 
> Ditto.

Done.

[Joonghun Park]

Created attachment 442853 [details]
Patch

[Joonghun Park]

Could you please review this change? I applied the comments of the previous patchset.

[Joonghun Park]

Created attachment 442854 [details]
Patch

[Joonghun Park]

Created attachment 442891 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Joonghun Park]

Hi, Darin. I added more test cases to web-platform-tests/css/css-backgrounds/parsing/border-radius-valid.html and applied your comments of the previous patchset.
I'd like to land this patch if you confirm that this patch looks ok to you for safer landing. Could you please review this change again?

[Joonghun Park]

FYI, the corresponding GitHub wpt pr url is https://github.com/web-platform-tests/wpt/pull/31453.

To get it approved, should I get a review flag again in this webkit bug page?

[Joonghun Park]

Created attachment 442894 [details]
Add more test cases to border-radius-valid wpt test

[Joonghun Park]

Created attachment 442910 [details]
Patch

[Joonghun Park]

I think this change is ready for review now.

[EWS]

/Volumes/Data/worker/Commit-Queue/build/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

[Joonghun Park]

Created attachment 443238 [details]
Patch for landing

[EWS]

Committed r285235 (243856@main): <https://commits.webkit.org/243856@main>

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

[Antti Koivisto]

Reverted in bug 233142

[Joonghun Park]

Created attachment 444348 [details]
Patch

[Antti Koivisto]

Comment on attachment 444348 [details]
Patch

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

> Source/WebCore/css/StyleProperties.cpp:374
> +    auto value = getPropertyCSSValue(shorthand.properties()[0]);
> +    if (!value)
> +        return String();
> +
> +    if (value->isCSSWideKeyword())
> +        return value->cssText();

Couldn't this be achieved by testing topLeftValue below?

> Source/WebCore/css/StyleProperties.cpp:411
> +    if (topLeftValue->isInheritValue() && topRightValue->isInheritValue() && bottomRightValue->isInheritValue() && bottomLeftValue->isInheritValue())
> +        return getValueName(CSSValueInherit);

This test is probably now redundant?

> Source/WebCore/css/StyleProperties.cpp:419
> +    if (topLeftValue->isInitialValue() || topRightValue->isInitialValue() || bottomRightValue->isInitialValue() || bottomLeftValue->isInitialValue()) {
> +        if (topLeftValue->isInitialValue() && topRightValue->isInitialValue() && bottomRightValue->isInitialValue() && bottomLeftValue->isInitialValue() && !topLeft.isImplicit()) {
> +            // All components are "initial" and "topLeft" is not implicit.
> +            return getValueName(CSSValueInitial);
> +        }
> +        return String();
> +    }

Is this redundant too?

[Joonghun Park]

(In reply to Antti Koivisto from comment #24)
> Comment on attachment 444348 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=444348&action=review
> 
> > Source/WebCore/css/StyleProperties.cpp:374
> > +    auto value = getPropertyCSSValue(shorthand.properties()[0]);
> > +    if (!value)
> > +        return String();
> > +
> > +    if (value->isCSSWideKeyword())
> > +        return value->cssText();
> 
> Couldn't this be achieved by testing topLeftValue below?
> 
> > Source/WebCore/css/StyleProperties.cpp:411
> > +    if (topLeftValue->isInheritValue() && topRightValue->isInheritValue() && bottomRightValue->isInheritValue() && bottomLeftValue->isInheritValue())
> > +        return getValueName(CSSValueInherit);
> 
> This test is probably now redundant?
> 
> > Source/WebCore/css/StyleProperties.cpp:419
> > +    if (topLeftValue->isInitialValue() || topRightValue->isInitialValue() || bottomRightValue->isInitialValue() || bottomLeftValue->isInitialValue()) {
> > +        if (topLeftValue->isInitialValue() && topRightValue->isInitialValue() && bottomRightValue->isInitialValue() && bottomLeftValue->isInitialValue() && !topLeft.isImplicit()) {
> > +            // All components are "initial" and "topLeft" is not implicit.
> > +            return getValueName(CSSValueInitial);
> > +        }
> > +        return String();
> > +    }
> 
> Is this redundant too?

Thank you for your review, Antti.
I will refine this change with your comments in the next patchset.

[Joonghun Park]

Comment on attachment 444348 [details]
Patch

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

>>> Source/WebCore/css/StyleProperties.cpp:374
>>> +        return value->cssText();
>> 
>> Couldn't this be achieved by testing topLeftValue below?
> 
> Thank you for your review, Antti.
> I will refine this change with your comments in the next patchset.

Done.

>> Source/WebCore/css/StyleProperties.cpp:411
>> +        return getValueName(CSSValueInherit);
> 
> This test is probably now redundant?

Done.

>> Source/WebCore/css/StyleProperties.cpp:419
>> +    }
> 
> Is this redundant too?

Done.

[Joonghun Park]

Created attachment 444365 [details]
Patch

[Darin Adler]

Comment on attachment 444365 [details]
Patch

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

A little sad how repetitive this code is. And it’s kind of sad that to build the single string result we have to allocate 11 strings and destroy 10 of them.

But I think this is done pretty well now and seems ready to land.

> Source/WebCore/css/StyleProperties.cpp:402
> +    if (!topLeftValue || !topRightValue || !bottomRightValue || !bottomLeftValue)
> +        return String();

We’ve already checked !topLeftValue above. No need to check it again here.

[Darin Adler]

Comment on attachment 444365 [details]
Patch

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

> Source/WebCore/css/StyleProperties.cpp:406
> +    // Important flags must be the same
> +    if (topLeft.isImportant() != topRight.isImportant() || topRight.isImportant() != bottomRight.isImportant() || bottomRight.isImportant() != bottomLeft.isImportant())
> +        return String();

I think I would write this differently for clarity:

    bool isImportant = topLeft.isImportant();
    if (topRight.isImportant() != isImportant || bottomRight.isImportant() != isImportant || bottomLeft.isImportant() != isImportant)

The code where we compare them pairwise seems harder to read and verify that it’s correct.

[Joonghun Park]

Comment on attachment 444365 [details]
Patch

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

Thank you for your review, Darin. I applied all the comments you left and will upload the new patch for landing.

>> Source/WebCore/css/StyleProperties.cpp:402
>> +        return String();
> 
> We’ve already checked !topLeftValue above. No need to check it again here.

Done.

>> Source/WebCore/css/StyleProperties.cpp:406
>> +        return String();
> 
> I think I would write this differently for clarity:
> 
>     bool isImportant = topLeft.isImportant();
>     if (topRight.isImportant() != isImportant || bottomRight.isImportant() != isImportant || bottomLeft.isImportant() != isImportant)
> 
> The code where we compare them pairwise seems harder to read and verify that it’s correct.

Done.

[Joonghun Park]

Created attachment 444450 [details]
Patch for landing

[EWS]

Committed r285915 (244324@main): <https://commits.webkit.org/244324@main>

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 234873

[Antti Koivisto]

This crashes with

<style id=s>
div { border-radius:0px; border-bottom-left-radius: inherit; }
</style>
<pre id=l></pre>
<script>
l.textContent = s.sheet.cssRules[0].cssText;
</script>

Even if it didn't crash it would fail to serialize the case where individual properties use global keywords correctly.

[Ahmad Saleem]

*** Safari 16 ***

border-top-left-radius is '11px 25px' ; '11px 25px'

border-top-right-radius is '12px 26px' ; '12px 26px'

border-bottom-right-radius is '13px 27px' ; '13px 27px'

border-bottom-left-radius is '14px 28px' ; '14px 28px'

border-radius is '11px 25px 12px 26px 13px 27px 14px 28px' ; '11px 12px 13px 14px / 25px 26px 27px 28px'

*** Safari Technology Preview 154 ***

border-top-left-radius is '11px 25px' ; '11px 25px'

border-top-right-radius is '12px 26px' ; '12px 26px'

border-bottom-right-radius is '13px 27px' ; '13px 27px'

border-bottom-left-radius is '14px 28px' ; '14px 28px'

border-radius is '11px 25px 12px 26px 13px 27px 14px 28px' ; '11px 12px 13px 14px / 25px 26px 27px 28px'  <<---------------------- PROBLEM

*** Chrome Canary 108 ***

border-top-left-radius is '11px 25px' ; '11px 25px'

border-top-right-radius is '12px 26px' ; '12px 26px'

border-bottom-right-radius is '13px 27px' ; '13px 27px'

border-bottom-left-radius is '14px 28px' ; '14px 28px'

border-radius is '11px 12px 13px 14px / 25px 26px 27px 28px' ; '11px 12px 13px 14px / 25px 26px 27px 28px'

*** Firefox Nightly 107 ***

border-top-left-radius is '11px 25px' ; '11px 25px'

border-top-right-radius is '12px 26px' ; '12px 26px'

border-bottom-right-radius is '13px 27px' ; '13px 27px'

border-bottom-left-radius is '14px 28px' ; '14px 28px'

border-radius is '11px 12px 13px 14px / 25px 26px 27px 28px' ; '11px 12px 13px 14px / 25px 26px 27px 28px'

____

Just wanted to share updated test results. Thanks!

[Oriol Brufau]

Pull request: https://github.com/WebKit/WebKit/pull/6820

[EWS]

Committed 257041@main (fd9594cdf418): <https://commits.webkit.org/257041@main>

Reviewed commits have been landed. Closing PR #6820 and removing active labels.