Bug 183994 - border-radius inline style serializes with invalid syntax
Summary: border-radius inline style serializes with invalid syntax
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: Safari 11
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joonghun Park
URL:
Keywords: FromImplementor, InRadar
Depends on: 233142 234873
Blocks:
  Show dependency treegraph
 
Reported: 2018-03-25 04:46 PDT by Eric Willigers
Modified: 2022-01-07 17:56 PST (History)
19 users (show)

See Also:


Attachments
To check layout test result (5.77 KB, patch)
2021-10-29 09:16 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (9.64 KB, patch)
2021-10-29 10:27 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (8.88 KB, patch)
2021-10-29 12:51 PDT, Joonghun Park
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (8.88 KB, patch)
2021-10-29 13:02 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (10.12 KB, patch)
2021-10-29 18:27 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Add more test cases to border-radius-valid wpt test (10.17 KB, patch)
2021-10-29 19:57 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (12.30 KB, patch)
2021-10-30 06:04 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch for landing (12.33 KB, patch)
2021-11-03 14:15 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (12.90 KB, patch)
2021-11-15 22:37 PST, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (12.18 KB, patch)
2021-11-16 02:48 PST, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch for landing (12.17 KB, patch)
2021-11-16 16:36 PST, Joonghun Park
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Willigers 2018-03-25 04:46:14 PDT
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.)
Comment 1 Emilio Cobos Álvarez (:emilio) 2018-03-25 05:33:13 PDT
(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.
Comment 2 Manuel Rego Casasnovas 2018-03-25 22:02:57 PDT
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. :-(
Comment 3 Radar WebKit Bug Importer 2018-03-27 14:40:33 PDT
<rdar://problem/38928309>
Comment 4 Joonghun Park 2021-10-29 09:16:26 PDT
Created attachment 442824 [details]
To check layout test result
Comment 5 Joonghun Park 2021-10-29 10:27:51 PDT
Created attachment 442839 [details]
Patch
Comment 6 Darin Adler 2021-10-29 10:38:06 PDT
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);
Comment 7 Joonghun Park 2021-10-29 10:58:10 PDT
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.
Comment 8 Joonghun Park 2021-10-29 12:51:19 PDT
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.
Comment 9 Joonghun Park 2021-10-29 12:51:50 PDT
Created attachment 442853 [details]
Patch
Comment 10 Joonghun Park 2021-10-29 12:56:58 PDT
Could you please review this change? I applied the comments of the previous patchset.
Comment 11 Joonghun Park 2021-10-29 13:02:22 PDT
Created attachment 442854 [details]
Patch
Comment 12 Joonghun Park 2021-10-29 18:27:01 PDT
Created attachment 442891 [details]
Patch
Comment 13 EWS Watchlist 2021-10-29 18:27:58 PDT
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
Comment 14 Joonghun Park 2021-10-29 18:32:59 PDT
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?
Comment 15 Joonghun Park 2021-10-29 19:31:03 PDT
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?
Comment 16 Joonghun Park 2021-10-29 19:57:20 PDT
Created attachment 442894 [details]
Add more test cases to border-radius-valid wpt test
Comment 17 Joonghun Park 2021-10-30 06:04:29 PDT
Created attachment 442910 [details]
Patch
Comment 18 Joonghun Park 2021-10-30 07:08:16 PDT
I think this change is ready for review now.
Comment 19 EWS 2021-11-03 14:04:25 PDT
/Volumes/Data/worker/Commit-Queue/build/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
Comment 20 Joonghun Park 2021-11-03 14:15:26 PDT
Created attachment 443238 [details]
Patch for landing
Comment 21 EWS 2021-11-03 15:18:44 PDT
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].
Comment 22 Antti Koivisto 2021-11-15 12:35:00 PST
Reverted in bug 233142
Comment 23 Joonghun Park 2021-11-15 22:37:36 PST
Created attachment 444348 [details]
Patch
Comment 24 Antti Koivisto 2021-11-15 23:55:27 PST
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?
Comment 25 Joonghun Park 2021-11-16 01:45:34 PST
(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.
Comment 26 Joonghun Park 2021-11-16 02:47:13 PST
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.
Comment 27 Joonghun Park 2021-11-16 02:48:05 PST
Created attachment 444365 [details]
Patch
Comment 28 Darin Adler 2021-11-16 10:58:45 PST
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.
Comment 29 Darin Adler 2021-11-16 11:01:01 PST
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.
Comment 30 Joonghun Park 2021-11-16 16:02:28 PST
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.
Comment 31 Joonghun Park 2021-11-16 16:36:06 PST
Created attachment 444450 [details]
Patch for landing
Comment 32 EWS 2021-11-16 21:53:14 PST
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].
Comment 33 WebKit Commit Bot 2022-01-05 01:50:02 PST
Re-opened since this is blocked by bug 234873
Comment 34 Antti Koivisto 2022-01-05 01:55:33 PST
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.