Bug 223892 – Omit default position/angle when serializing radial & conic gradients

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

[Tim Nguyen (:ntim)]

Seems like positions aren't omitted when they are at their default value (50% 50%/center center).

https://webkit-search.igalia.com/webkit/source/LayoutTests/imported/w3c/web-platform-tests/css/css-backgrounds/parsing/background-image-computed.sub-expected.txt#21-22

[Radar WebKit Bug Importer]

<rdar://problem/76228290>

[Tim Nguyen (:ntim)]

Created attachment 425257 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 425259 [details]
Patch

[Tyler Wilcock]

Comment on attachment 425259 [details]
Patch

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

> Source/WebCore/css/CSSPrimitiveValue.cpp:866
> +bool CSSPrimitiveValue::isCenterPosition() const
> +{
> +    switch (primitiveUnitType()) {
> +    case CSSUnitType::CSS_VALUE_ID:
> +        return valueID() == CSSValueCenter;
> +    case CSSUnitType::CSS_PERCENTAGE:
> +        return m_value.num == 50;
> +    default:
> +        return false;
> +    }
> +}

Being "center position", or at least this definition of it, seems specific to gradient values, no?  Would it make sense to add a static bool method to CSSGradientValue.cpp instead?

static bool isCenterPosition(const CSSPrimitiveValue& primitiveValue)
{

}

Also, I don't think you need to switch on unit type.  If the primitive value is not a value ID and not a percent, it will return CSSValueInvalid and 0 respectively, meaning neither bool (== CSSValueCenter, == 50) would evaluate to true.  I think this method could become:

return primitiveValue.valueID() == CSSValueCenter || primitiveValue.doubleValue(CSSUnitType::CSS_PERCENTAGE) == 50;

> LayoutTests/fast/gradients/unprefixed-gradient-parsing.html:106
> +var computedStr = s.replace(" at 50% 50%", "");

Can you explain why this is added?

[Tim Nguyen (:ntim)]

(In reply to Tyler Wilcock from comment #4)
> Comment on attachment 425259 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425259&action=review
> 
> > Source/WebCore/css/CSSPrimitiveValue.cpp:866
> > +bool CSSPrimitiveValue::isCenterPosition() const
> > +{
> > +    switch (primitiveUnitType()) {
> > +    case CSSUnitType::CSS_VALUE_ID:
> > +        return valueID() == CSSValueCenter;
> > +    case CSSUnitType::CSS_PERCENTAGE:
> > +        return m_value.num == 50;
> > +    default:
> > +        return false;
> > +    }
> > +}
> 
> Being "center position", or at least this definition of it, seems specific
> to gradient values, no?  Would it make sense to add a static bool method to
> CSSGradientValue.cpp instead?
>
> static bool isCenterPosition(const CSSPrimitiveValue& primitiveValue)
> {
> 
> }
> 
> Also, I don't think you need to switch on unit type.  If the primitive value
> is not a value ID and not a percent, it will return CSSValueInvalid and 0
> respectively, meaning neither bool (== CSSValueCenter, == 50) would evaluate
> to true.  I think this method could become:
> 
> return primitiveValue.valueID() == CSSValueCenter ||
> primitiveValue.doubleValue(CSSUnitType::CSS_PERCENTAGE) == 50;

I don't mind either way. center also makes sense for `background-position` or `object-position`, or anything using the `<position>` CSS type. Ideally, we'd have a `Position` type reflected in the style code as well, with a `isCenter` checking for both coordinates being center, rather than doing each one individually outside.


> > LayoutTests/fast/gradients/unprefixed-gradient-parsing.html:106
> > +var computedStr = s.replace(" at 50% 50%", "");
> 
> Can you explain why this is added?

I'll add a comment, but this is needed, since this patch strips the default center position when serializing the gradient, so it reflects this in the test as well.

[Tim Nguyen (:ntim)]

Created attachment 425980 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 426009 [details]
Patch

[Tim Nguyen (:ntim)]

Comment on attachment 426009 [details]
Patch

Antti reviewed this, thanks Antti!

[EWS]

Committed r276002 (236554@main): <https://commits.webkit.org/236554@main>

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