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
<rdar://problem/76228290>
Created attachment 425257 [details] Patch
Created attachment 425259 [details] Patch
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?
(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.
Created attachment 425980 [details] Patch
Created attachment 426009 [details] Patch
Comment on attachment 426009 [details] Patch Antti reviewed this, thanks Antti!
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].