Bug 183994

(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.

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. :-(

<rdar://problem/38928309>

Created attachment 442824 [details] To check layout test result

Created attachment 442839 [details] Patch

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 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 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.

Created attachment 442853 [details] Patch

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

Created attachment 442854 [details] Patch

Created attachment 442891 [details] Patch

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

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?

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?

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

Created attachment 442910 [details] Patch

I think this change is ready for review now.

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

Created attachment 443238 [details] Patch for landing

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].

Reverted in bug 233142

Created attachment 444348 [details] Patch

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?

(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 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.

Created attachment 444365 [details] Patch

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 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 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.

Created attachment 444450 [details] Patch for landing

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].

Re-opened since this is blocked by bug 234873

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.

*** 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!

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

Committed 257041@main (fd9594cdf418): <https://commits.webkit.org/257041@main> Reviewed commits have been landed. Closing PR #6820 and removing active labels.