Skip to content

Make '-webkit-background-size' a legacy shorthand #3372

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 27, 2022

Conversation

pxlcoder
Copy link
Member

@pxlcoder pxlcoder commented Aug 16, 2022

856140e

Make '-webkit-background-size' a legacy shorthand
https://bugs.webkit.org/show_bug.cgi?id=243562
rdar://98151497

Reviewed by Tim Nguyen.

`background-size` and `-webkit-background-size` are a pair of properties that
share a computed style. `-webkit-background-size` differs from `background-size`
in the interpretation of a single value: `-webkit-background-size: l;` is
equivalent to `background-size: l l;`, whereas `background-size: l;` is
equivalent to `background-size: l auto;`.

Property pairs that share a computed style but differ in syntax should be
implemented as legacy shorthands.

Additionally, always serialize `background-size` and `-webkit-background-size`
as two values, so that values can be roundtripped between the prefixed and
unprefixed variant.

Spec: https://www.w3.org/TR/css-cascade-5/#aliasing

* LayoutTests/TestExpectations:

Remove expectations for a forms test that was unexpectedly setting
`-webkit-background-size`, as it was not a shorthand.

* LayoutTests/fast/backgrounds/background-shorthand-with-backgroundSize-style-expected.txt:
* LayoutTests/fast/backgrounds/background-shorthand-with-backgroundSize-style.html:
* LayoutTests/fast/backgrounds/size/parsing-background-size-values-expected.txt:
* LayoutTests/fast/backgrounds/size/parsing-inherit-expected.txt:
* LayoutTests/fast/backgrounds/size/resources/parsing-background-size-values.js:
* LayoutTests/fast/backgrounds/size/resources/parsing-inherit.js:
* LayoutTests/fast/css/getComputedStyle/computed-style-expected.txt:
* LayoutTests/fast/css/getComputedStyle/computed-style-without-renderer-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-animations/KeyframeEffect-getKeyframes.tentative.html:

Update test to use two-value syntax.

* LayoutTests/imported/w3c/web-platform-tests/css/css-cascade/all-prop-initial-xml-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-backgrounds/background-332-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-backgrounds/background-size-001.html:
* LayoutTests/imported/w3c/web-platform-tests/css/css-backgrounds/parsing/background-size-computed.html:
* LayoutTests/imported/w3c/web-platform-tests/css/cssom/getComputedStyle-detached-subtree-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/cssom/getComputedStyle-pseudo-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/quirks/unitless-length/excluded-properties-002-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/quirks/unitless-length/excluded-properties-002.html:

Include an additional check for `1234px auto`, to avoid a false positive, as `background-size` is serialized to two values.

* LayoutTests/platform/gtk/imported/w3c/web-platform-tests/css/css-cascade/all-prop-initial-xml-expected.txt:
* LayoutTests/platform/gtk/imported/w3c/web-platform-tests/css/cssom/getComputedStyle-detached-subtree-expected.txt:
* LayoutTests/platform/gtk/imported/w3c/web-platform-tests/css/cssom/getComputedStyle-pseudo-expected.txt:
* LayoutTests/platform/ios-wk2/imported/w3c/web-platform-tests/css/css-cascade/all-prop-initial-xml-expected.txt:
* LayoutTests/platform/ios-wk2/imported/w3c/web-platform-tests/css/cssom/getComputedStyle-pseudo-expected.txt:
* LayoutTests/platform/ios/imported/w3c/web-platform-tests/css/cssom/getComputedStyle-detached-subtree-expected.txt:
* LayoutTests/platform/ipad/imported/w3c/web-platform-tests/css/cssom/getComputedStyle-detached-subtree-expected.txt:
* LayoutTests/platform/ipad/imported/w3c/web-platform-tests/css/cssom/getComputedStyle-pseudo-expected.txt:
* LayoutTests/platform/mac-wk1/imported/w3c/web-platform-tests/css/css-cascade/all-prop-initial-xml-expected.txt:
* LayoutTests/platform/wpe/imported/w3c/web-platform-tests/css/css-cascade/all-prop-initial-xml-expected.txt:
* LayoutTests/platform/wpe/imported/w3c/web-platform-tests/css/cssom/getComputedStyle-detached-subtree-expected.txt:
* LayoutTests/platform/wpe/imported/w3c/web-platform-tests/css/cssom/getComputedStyle-pseudo-expected.txt:
* LayoutTests/svg/css/getComputedStyle-basic-expected.txt:
* Source/WebCore/css/CSSProperties.json:
* Source/WebCore/css/ComputedStyleExtractor.cpp:
(WebCore::fillSizeToCSSValue):

Return two values from `getComputedStyle` unless the value is "cover", "contain", or "auto".

(WebCore::ComputedStyleExtractor::valueForPropertyInStyle):
* Source/WebCore/css/StyleProperties.cpp:
(WebCore::StyleProperties::getPropertyValue const):
* Source/WebCore/css/parser/CSSPropertyParser.cpp:
(WebCore::consumeBackgroundSize):

Always serialize `background-size` and `-webkit-background-size` as two values.

(WebCore::CSSPropertyParser::parseSingleValue):
(WebCore::CSSPropertyParser::parseShorthand):

Canonical link: https://commits.webkit.org/254925@main

5325011

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 🧪 win
✅ 🛠 ios-sim ✅ 🛠 mac-debug ✅ 🛠 gtk ✅ 🛠 wincairo
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🛠 mac-AS-debug ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 api-mac ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-wk1
✅ 🛠 tv-sim ✅ 🧪 mac-wk2
✅ 🛠 🧪 merge ✅ 🛠 watch ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 watch-sim ✅ 🧪 mac-wk2-stress

@pxlcoder pxlcoder self-assigned this Aug 16, 2022
@pxlcoder pxlcoder added Forms For bugs specific to form elements (checkboxes, buttons, text fields, etc.) WebKit Nightly Build labels Aug 16, 2022
@pxlcoder pxlcoder marked this pull request as draft August 16, 2022 20:27
@nt1m nt1m requested a review from Loirooriol August 16, 2022 21:22
@@ -376,6 +376,8 @@ String StyleProperties::getPropertyValue(CSSPropertyID propertyID, Document* doc
return get2Values(scrollPaddingInlineShorthand());
case CSSPropertyWebkitTextOrientation:
return getPropertyValue(CSSPropertyTextOrientation);
case CSSPropertyWebkitBackgroundSize:
return getPropertyValue(CSSPropertyBackgroundSize);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not good, there is no round-trip:

element.style.backgroundSize = "100px"; // behaves as "100px auto"
element.style.webkitBackgroundSize; // "100px", oops!
element.style.webkitBackgroundSize += ""; // changes behavior to "100px 100px"
element.style.webkitBackgroundSize = "100px"; // behaves as "100px 100px"
element.style.backgroundSize; // "100px", oops!
element.style.backgroundSize += ""; // changes behavior to "100px auto"

And I guess this was already wrong, but please fix gCS while you are at it:

element.style.backgroundSize = "100px";
getComputedStyle(element).webkitBackgroundSize; // "100px", oops!

This is OK:

element.style.webkitBackgroundSize = "100px";
getComputedStyle(element).backgroundSize; // "100px 100px"

See whatwg/compat#28, either serialize background-size and -webkit-background-size differently, or always use 2 values. People there seemed to prefer 2 values.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and make the unprefixed property parse like -webkit-background-size if it's web-compatible. whatwg/compat#28 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the parsing is out-of-scope for this change, and would carry more web compat risk.

I have changed serialization to always use two values.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Aug 17, 2022
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Sep 23, 2022
@pxlcoder pxlcoder removed the merging-blocked Applied to prevent a change from being merged label Sep 23, 2022
@webkit-early-warning-system
Copy link
Collaborator

Starting EWS tests for 50a2514. Live statuses available at the PR page, #3372

Copy link
Contributor

@Loirooriol Loirooriol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Informal LGTM.
It would be good to fix getComputedStyle while you are at it, but it's already broken so not blocking on that.

@nt1m
Copy link
Member

nt1m commented Sep 23, 2022

It would be good to fix getComputedStyle while you are at it, but it's already broken so not blocking on that.

I think Aditya's patch makes sure to always serialize to 2 values, so that would also get fixed, I would think.

We could add a test for it.

@Loirooriol
Copy link
Contributor

No, getComputedStyle still omits the 2nd value if it's auto, which is wrong for -webkit-background-size. See https://searchfox.org/wubkat/rev/7b1a4e7e5fb469b0eea41fe131f2f190d08f2383/Source/WebCore/css/ComputedStyleExtractor.cpp#2115-2116

    if (fillSize.size.height.isAuto())
        return ComputedStyleExtractor::zoomAdjustedPixelValueForLength(fillSize.size.width, style);

@nt1m
Copy link
Member

nt1m commented Sep 24, 2022

No, getComputedStyle still omits the 2nd value if it's auto, which is wrong for -webkit-background-size. See https://searchfox.org/wubkat/rev/7b1a4e7e5fb469b0eea41fe131f2f190d08f2383/Source/WebCore/css/ComputedStyleExtractor.cpp#2115-2116

    if (fillSize.size.height.isAuto())
        return ComputedStyleExtractor::zoomAdjustedPixelValueForLength(fillSize.size.width, style);

Good catch! Seems easily fixable, we could pass in a CSSPropertyID for fillSizeToCSSValue and only do this adjustment for the gCS for mask-size

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Sep 24, 2022
Copy link
Member

@nt1m nt1m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me with tests on EWS changed to match the new expected results (change the expected results to keep them passing)

Also, please do a WPT export.

@pxlcoder
Copy link
Member Author

Following the change to gCS, the following WPTs are failing:

imported/w3c/web-platform-tests/css/css-backgrounds/background-332.html
imported/w3c/web-platform-tests/css/css-backgrounds/background-size-001.html
imported/w3c/web-platform-tests/css/css-backgrounds/parsing/background-size-computed.html
imported/w3c/web-platform-tests/css/cssom/getComputedStyle-pseudo.html

They expect a single value for background-size, when a single value is specified. If I were to update the tests to match our new behavior, they would fail on other browsers. Using assert_any would fix that issue, but also seems a little strange.

@nt1m @zcorpan Any suggestions on how to proceed with gCS / these WPT?

@nt1m
Copy link
Member

nt1m commented Sep 25, 2022

@pxlcoder You can either change the expected result, or use assert_in_array() to accept both forms. I slightly favor the first option to make other browsers follow suit (or participate in the discussion for whatwg/compat#28), and getting interoperable behavior.

@pxlcoder pxlcoder removed the merging-blocked Applied to prevent a change from being merged label Sep 25, 2022
@pxlcoder pxlcoder marked this pull request as ready for review September 25, 2022 01:51
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Sep 25, 2022
Copy link
Member

@nt1m nt1m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a GTK specific baseline for:
LayoutTests/imported/w3c/web-platform-tests/css/cssom/getComputedStyle-pseudo.html ?

This is actually not a regression of your patch but of a3ee3aa , but would be nice to fix anyway here since your patch touches the baseline.

@@ -2110,15 +2110,15 @@ static Ref<CSSValue> maskModeToCSSValue(MaskMode type)
return CSSValuePool::singleton().createValue(CSSValueMatchSource);
}

static Ref<CSSValue> fillSizeToCSSValue(const FillSize& fillSize, const RenderStyle& style)
static Ref<CSSValue> fillSizeToCSSValue(const FillSize& fillSize, const RenderStyle& style, CSSPropertyID propertyID)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make the propertyID the first argument to be consistent with createPositionListForLayer and createSingleAxisPositionValueForLayer?

@pxlcoder pxlcoder removed the merging-blocked Applied to prevent a change from being merged label Sep 26, 2022
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Sep 26, 2022
@nt1m
Copy link
Member

nt1m commented Sep 26, 2022

iOS needs a similar rebaseline for LayoutTests/imported/w3c/web-platform-tests/css/cssom/getComputedStyle-pseudo.html

@pxlcoder
Copy link
Member Author

iOS needs a similar rebaseline for LayoutTests/imported/w3c/web-platform-tests/css/cssom/getComputedStyle-pseudo.html

Not sure what's going on here – the patch contains a baseline in LayoutTests/platform/ios/imported/w3c/web-platform-tests/css/cssom/getComputedStyle-pseudo-expected.txt, and the test is passing locally 🤔

@pxlcoder
Copy link
Member Author

(turns out there's already a baseline in ios-wk2, will update that one instead of adding a new one)

@pxlcoder pxlcoder removed the merging-blocked Applied to prevent a change from being merged label Sep 26, 2022
@pxlcoder pxlcoder added the merge-queue Applied to send a pull request to merge-queue label Sep 27, 2022
https://bugs.webkit.org/show_bug.cgi?id=243562
rdar://98151497

Reviewed by Tim Nguyen.

`background-size` and `-webkit-background-size` are a pair of properties that
share a computed style. `-webkit-background-size` differs from `background-size`
in the interpretation of a single value: `-webkit-background-size: l;` is
equivalent to `background-size: l l;`, whereas `background-size: l;` is
equivalent to `background-size: l auto;`.

Property pairs that share a computed style but differ in syntax should be
implemented as legacy shorthands.

Additionally, always serialize `background-size` and `-webkit-background-size`
as two values, so that values can be roundtripped between the prefixed and
unprefixed variant.

Spec: https://www.w3.org/TR/css-cascade-5/#aliasing

* LayoutTests/TestExpectations:

Remove expectations for a forms test that was unexpectedly setting
`-webkit-background-size`, as it was not a shorthand.

* LayoutTests/fast/backgrounds/background-shorthand-with-backgroundSize-style-expected.txt:
* LayoutTests/fast/backgrounds/background-shorthand-with-backgroundSize-style.html:
* LayoutTests/fast/backgrounds/size/parsing-background-size-values-expected.txt:
* LayoutTests/fast/backgrounds/size/parsing-inherit-expected.txt:
* LayoutTests/fast/backgrounds/size/resources/parsing-background-size-values.js:
* LayoutTests/fast/backgrounds/size/resources/parsing-inherit.js:
* LayoutTests/fast/css/getComputedStyle/computed-style-expected.txt:
* LayoutTests/fast/css/getComputedStyle/computed-style-without-renderer-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-animations/KeyframeEffect-getKeyframes.tentative.html:

Update test to use two-value syntax.

* LayoutTests/imported/w3c/web-platform-tests/css/css-cascade/all-prop-initial-xml-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-backgrounds/background-332-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-backgrounds/background-size-001.html:
* LayoutTests/imported/w3c/web-platform-tests/css/css-backgrounds/parsing/background-size-computed.html:
* LayoutTests/imported/w3c/web-platform-tests/css/cssom/getComputedStyle-detached-subtree-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/cssom/getComputedStyle-pseudo-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/quirks/unitless-length/excluded-properties-002-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/quirks/unitless-length/excluded-properties-002.html:

Include an additional check for `1234px auto`, to avoid a false positive, as `background-size` is serialized to two values.

* LayoutTests/platform/gtk/imported/w3c/web-platform-tests/css/css-cascade/all-prop-initial-xml-expected.txt:
* LayoutTests/platform/gtk/imported/w3c/web-platform-tests/css/cssom/getComputedStyle-detached-subtree-expected.txt:
* LayoutTests/platform/gtk/imported/w3c/web-platform-tests/css/cssom/getComputedStyle-pseudo-expected.txt:
* LayoutTests/platform/ios-wk2/imported/w3c/web-platform-tests/css/css-cascade/all-prop-initial-xml-expected.txt:
* LayoutTests/platform/ios-wk2/imported/w3c/web-platform-tests/css/cssom/getComputedStyle-pseudo-expected.txt:
* LayoutTests/platform/ios/imported/w3c/web-platform-tests/css/cssom/getComputedStyle-detached-subtree-expected.txt:
* LayoutTests/platform/ipad/imported/w3c/web-platform-tests/css/cssom/getComputedStyle-detached-subtree-expected.txt:
* LayoutTests/platform/ipad/imported/w3c/web-platform-tests/css/cssom/getComputedStyle-pseudo-expected.txt:
* LayoutTests/platform/mac-wk1/imported/w3c/web-platform-tests/css/css-cascade/all-prop-initial-xml-expected.txt:
* LayoutTests/platform/wpe/imported/w3c/web-platform-tests/css/css-cascade/all-prop-initial-xml-expected.txt:
* LayoutTests/platform/wpe/imported/w3c/web-platform-tests/css/cssom/getComputedStyle-detached-subtree-expected.txt:
* LayoutTests/platform/wpe/imported/w3c/web-platform-tests/css/cssom/getComputedStyle-pseudo-expected.txt:
* LayoutTests/svg/css/getComputedStyle-basic-expected.txt:
* Source/WebCore/css/CSSProperties.json:
* Source/WebCore/css/ComputedStyleExtractor.cpp:
(WebCore::fillSizeToCSSValue):

Return two values from `getComputedStyle` unless the value is "cover", "contain", or "auto".

(WebCore::ComputedStyleExtractor::valueForPropertyInStyle):
* Source/WebCore/css/StyleProperties.cpp:
(WebCore::StyleProperties::getPropertyValue const):
* Source/WebCore/css/parser/CSSPropertyParser.cpp:
(WebCore::consumeBackgroundSize):

Always serialize `background-size` and `-webkit-background-size` as two values.

(WebCore::CSSPropertyParser::parseSingleValue):
(WebCore::CSSPropertyParser::parseShorthand):

Canonical link: https://commits.webkit.org/254925@main
@webkit-commit-queue
Copy link
Collaborator

Committed 254925@main (856140e): https://commits.webkit.org/254925@main

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Forms For bugs specific to form elements (checkboxes, buttons, text fields, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants