Skip to content

Commit

Permalink
Cherry-pick 82228ed. rdar://problem/109380755
Browse files Browse the repository at this point in the history
    REGRESSION (Safari 16.4, 258767@main): Carcassonne game on boardgamearena.com unplayable (serialization bug affecting background-position)
    https://bugs.webkit.org/show_bug.cgi?id=256814
    rdar://109380755

    Reviewed by Tim Nguyen.

    The code to serialize background-position and mask-position did not correctly handle the case
    where the Y value was 0%. Rearranged the code slightly to resolve this.

    * LayoutTests/fast/css/background-position-serialize-expected.txt: Added test cases.
    * LayoutTests/fast/css/background-position-serialize.html: Ditto.
    * LayoutTests/fast/masking/parsing-webkit-mask-expected.txt: Ditto.
    * LayoutTests/fast/masking/parsing-webkit-mask.html: Ditto.
    * LayoutTests/imported/w3c/web-platform-tests/css/css-backgrounds/parsing/background-position-valid-expected.txt: Ditto.
    * LayoutTests/imported/w3c/web-platform-tests/css/css-backgrounds/parsing/background-position-valid.html: Ditto.

    * LayoutTests/imported/w3c/web-platform-tests/css/css-backgrounds/parsing/background-valid.html:
    Updated expectations since initial value of background-position is "0% 0%", not "0% center".

    * LayoutTests/imported/w3c/web-platform-tests/css/css-masking/parsing/mask-position-valid-expected.txt: Added test cases.
    * LayoutTests/imported/w3c/web-platform-tests/css/css-masking/parsing/mask-position-valid.html: Ditto.

    * LayoutTests/imported/w3c/web-platform-tests/css/css-masking/parsing/mask-valid.sub.html:
    Updated expectations since initial value of mask-position is "0% 0%", not "0% center".

    * Source/WebCore/css/ShorthandSerializer.cpp:
    (WebCore::ShorthandSerializer::serializeLayered const): Rearrange the code to correctly serialize
    background-position-y and mask-position-y as part of a shorthand.

    Canonical link: https://commits.webkit.org/265056@main
Identifier: 259548.861@safari-7615-branch
  • Loading branch information
darinadler authored and MyahCobbs committed Jun 28, 2023
1 parent 383d4ad commit 3f5f0f1
Show file tree
Hide file tree
Showing 11 changed files with 164 additions and 17 deletions.
30 changes: 30 additions & 0 deletions LayoutTests/fast/css/background-position-serialize-expected.txt
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,36 @@ PASS: t.style.backgroundPositionX should be 20% and is.
PASS: t.style.backgroundPositionY should be 20% and is.
PASS: t.style.cssText should be background-position: 20% 20%; and is.
PASS: t.getAttribute('style') should be background-position: 20% 20%; and is.
t.style.backgroundPositionY = '0%';
PASS: t.style.backgroundPosition should be 20% 0% and is.
PASS: t.style.backgroundPositionX should be 20% and is.
PASS: t.style.backgroundPositionY should be 0% and is.
PASS: t.style.cssText should be background-position: 20% 0%; and is.
PASS: t.getAttribute('style') should be background-position: 20% 0%; and is.
t.style.backgroundPositionX = '0%';
PASS: t.style.backgroundPosition should be 0% 0% and is.
PASS: t.style.backgroundPositionX should be 0% and is.
PASS: t.style.backgroundPositionY should be 0% and is.
PASS: t.style.cssText should be background-position: 0% 0%; and is.
PASS: t.getAttribute('style') should be background-position: 0% 0%; and is.
t.style.backgroundPosition = 'center';
PASS: t.style.backgroundPosition should be center and is.
PASS: t.style.backgroundPositionX should be center and is.
PASS: t.style.backgroundPositionY should be center and is.
PASS: t.style.cssText should be background-position: center; and is.
PASS: t.getAttribute('style') should be background-position: center; and is.
t.style.backgroundPosition = '0% center';
PASS: t.style.backgroundPosition should be 0% and is.
PASS: t.style.backgroundPositionX should be 0% and is.
PASS: t.style.backgroundPositionY should be center and is.
PASS: t.style.cssText should be background-position: 0%; and is.
PASS: t.getAttribute('style') should be background-position: 0%; and is.
t.style.backgroundPosition = 'center 0%';
PASS: t.style.backgroundPosition should be center 0% and is.
PASS: t.style.backgroundPositionX should be center and is.
PASS: t.style.backgroundPositionY should be 0% and is.
PASS: t.style.cssText should be background-position: center 0%; and is.
PASS: t.getAttribute('style') should be background-position: center 0%; and is.
t.setAttribute('style', 'background-position: 60% 60% !important;');
PASS: t.style.backgroundPosition should be 60% 60% and is.
PASS: t.style.backgroundPositionX should be 60% and is.
Expand Down
36 changes: 36 additions & 0 deletions LayoutTests/fast/css/background-position-serialize.html
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@

<html>
<body>
<div id="t"></div>
Expand Down Expand Up @@ -99,6 +100,41 @@
shouldBe("t.style.cssText", "background-position: 20% 20%;");
shouldBe("t.getAttribute('style')", "background-position: 20% 20%;");

run("t.style.backgroundPositionY = '0%';");
shouldBe("t.style.backgroundPosition", "20% 0%");
shouldBe("t.style.backgroundPositionX", "20%");
shouldBe("t.style.backgroundPositionY", "0%");
shouldBe("t.style.cssText", "background-position: 20% 0%;");
shouldBe("t.getAttribute('style')", "background-position: 20% 0%;");

run("t.style.backgroundPositionX = '0%';");
shouldBe("t.style.backgroundPosition", "0% 0%");
shouldBe("t.style.backgroundPositionX", "0%");
shouldBe("t.style.backgroundPositionY", "0%");
shouldBe("t.style.cssText", "background-position: 0% 0%;");
shouldBe("t.getAttribute('style')", "background-position: 0% 0%;");

run("t.style.backgroundPosition = 'center';");
shouldBe("t.style.backgroundPosition", "center");
shouldBe("t.style.backgroundPositionX", "center");
shouldBe("t.style.backgroundPositionY", "center");
shouldBe("t.style.cssText", "background-position: center;");
shouldBe("t.getAttribute('style')", "background-position: center;");

run("t.style.backgroundPosition = '0% center';");
shouldBe("t.style.backgroundPosition", "0%");
shouldBe("t.style.backgroundPositionX", "0%");
shouldBe("t.style.backgroundPositionY", "center");
shouldBe("t.style.cssText", "background-position: 0%;");
shouldBe("t.getAttribute('style')", "background-position: 0%;");

run("t.style.backgroundPosition = 'center 0%';");
shouldBe("t.style.backgroundPosition", "center 0%");
shouldBe("t.style.backgroundPositionX", "center");
shouldBe("t.style.backgroundPositionY", "0%");
shouldBe("t.style.cssText", "background-position: center 0%;");
shouldBe("t.getAttribute('style')", "background-position: center 0%;");

run("t.setAttribute('style', 'background-position: 60% 60% !important;');");
shouldBe("t.style.backgroundPosition", "60% 60%");
shouldBe("t.style.backgroundPositionX", "60%");
Expand Down
6 changes: 6 additions & 0 deletions LayoutTests/fast/masking/parsing-webkit-mask-expected.txt
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,12 @@ PASS innerStyle("-webkit-mask-position", "left 10px center") is "left 10px cente
PASS innerStyle("-webkit-mask-position", "center top 20px") is "center top 20px"
PASS innerStyle("-webkit-mask-position", "center left 30px") is "left 30px center"
PASS innerStyle("-webkit-mask-position", "left 20% top") is "left 20% top"
PASS innerStyle("-webkit-mask-position", "0%") is "0%"
PASS innerStyle("-webkit-mask-position", "0% 0%") is "0% 0%"
PASS innerStyle("-webkit-mask-position", "0% center") is "0%"
PASS innerStyle("-webkit-mask-position", "center 0%") is "center 0%"
PASS innerStyle("-webkit-mask-position", "20% 0%") is "20% 0%"
PASS innerStyle("-webkit-mask-position", "0% 20%") is "0% 20%"
PASS innerStyle("-webkit-mask-position-x", "left") is "left"
PASS innerStyle("-webkit-mask-position-y", "top") is "top"
PASS innerStyle("-webkit-mask", "none alpha") is "alpha"
Expand Down
7 changes: 7 additions & 0 deletions LayoutTests/fast/masking/parsing-webkit-mask.html
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,13 @@
testInner("-webkit-mask-position", "center left 30px", "left 30px center");
testInner("-webkit-mask-position", "left 20% top", "left 20% top");

testInner("-webkit-mask-position", "0%", "0%");
testInner("-webkit-mask-position", "0% 0%", "0% 0%");
testInner("-webkit-mask-position", "0% center", "0%");
testInner("-webkit-mask-position", "center 0%", "center 0%");
testInner("-webkit-mask-position", "20% 0%", "20% 0%");
testInner("-webkit-mask-position", "0% 20%", "0% 20%");

testInner("-webkit-mask-position-x", "left", "left");
testInner("-webkit-mask-position-y", "top", "top");

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,9 @@ PASS e.style['background-position'] = "top 17px right 18px" should set the prope
PASS e.style['background-position'] = "bottom center" should set the property value
PASS e.style['background-position'] = "top left" should set the property value
PASS e.style['background-position'] = "bottom right 19%" should set the property value
PASS e.style['background-position'] = "20% 0%" should set the property value
PASS e.style['background-position'] = "0% 0%" should set the property value
PASS e.style['background-position'] = "0%" should set the property value
PASS e.style['background-position'] = "0% center" should set the property value
PASS e.style['background-position'] = "center 0%" should set the property value

Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@
test_valid_value("background-position", "bottom center", ["center bottom", "bottom"]);
test_valid_value("background-position", "top left", "left top");
test_valid_value("background-position", "bottom right 19%", ["right 19% bottom", "right 19% bottom 0%"]); // "right 19% bottom 0%" in Edge
test_valid_value("background-position", "20% 0%");
test_valid_value("background-position", "0% 0%");
test_valid_value("background-position", "0%", ["0%", "0% center"]);
test_valid_value("background-position", "0% center", ["0%", "0% center"]);
test_valid_value("background-position", "center 0%");
</script>
</body>
</html>
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,27 @@
'url(https://example.com/) 1px 2px / 3px 4px space round local padding-box content-box, url(https://example.com/) 1px 2px / 3px 4px space round local padding-box content-box rgb(5, 6, 7)' // WebKit omits quotes - https://bugs.webkit.org/show_bug.cgi?id=28869
]);

test_shorthand_value('background', 'none', {
'background-image': 'none',
'background-position': '0% 0%',
'background-size': 'auto',
'background-repeat': 'repeat',
'background-attachment': 'scroll',
'background-origin': 'padding-box',
'background-clip': 'border-box',
'background-color': 'transparent',
})
test_shorthand_value('background', 'url("https://example.com/") 1px 2px / 3px 4px space round local padding-box content-box, rgb(5, 6, 7) url("https://example.com/") 1px 2px / 3px 4px space round local padding-box content-box', {
'background-image': 'url("https://example.com/"), url("https://example.com/")',
'background-position': '1px 2px, 1px 2px',
'background-size': '3px 4px, 3px 4px',
'background-repeat': 'space round, space round',
'background-attachment': 'local, local',
'background-origin': 'padding-box, padding-box',
'background-clip': 'content-box, content-box',
'background-color': 'rgb(5, 6, 7)',
})

</script>
</body>
</html>
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,9 @@ PASS e.style['mask-position'] = "top center" should set the property value
PASS e.style['mask-position'] = "center" should set the property value
PASS e.style['mask-position'] = "bottom left, right 20%" should set the property value
PASS e.style['mask-position'] = "top, center, left" should set the property value
PASS e.style['mask-position'] = "20% 0%" should set the property value
PASS e.style['mask-position'] = "0% 0%" should set the property value
PASS e.style['mask-position'] = "0%" should set the property value
PASS e.style['mask-position'] = "0% center" should set the property value
PASS e.style['mask-position'] = "center 0%" should set the property value

Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,13 @@
test_valid_value("mask-position", "top");
test_valid_value("mask-position", "top center", "top");
test_valid_value("mask-position", "center", "center");

test_valid_value("mask-position", "bottom left, right 20%", "left bottom, right 20%");
test_valid_value("mask-position", "top, center, left");
test_valid_value("mask-position", "20% 0%");
test_valid_value("mask-position", "0% 0%");
test_valid_value("mask-position", "0%", ["0%", "0% center"]);
test_valid_value("mask-position", "0% center", ["0%", "0% center"]);
test_valid_value("mask-position", "center 0%");
</script>
</body>
</html>
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,27 @@
// To avoid unnecessarily losing test coverage, keep one of the test cases from back then.
test_valid_value('mask', 'none alpha', 'alpha');

test_shorthand_value('mask', 'none', {
'mask-image': 'none',
'mask-position': '0% 0%',
'mask-size': 'auto',
'mask-repeat': 'repeat',
'mask-origin': 'border-box',
'mask-clip': 'border-box',
'mask-composite': 'add',
'mask-mode': 'match-source',
})
test_shorthand_value('mask', 'none, linear-gradient(to left bottom, red, blue) padding-box', {
'mask-image': 'none, linear-gradient(to left bottom, red, blue)',
'mask-position': '0% 0%, 0% 0%',
'mask-size': 'auto, auto',
'mask-repeat': 'repeat, repeat',
'mask-origin': 'border-box, padding-box',
'mask-clip': 'border-box, padding-box',
'mask-composite': 'add, add',
'mask-mode': 'match-source, match-source',
})

</script>
</body>
</html>
39 changes: 23 additions & 16 deletions Source/WebCore/css/ShorthandSerializer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -607,6 +607,29 @@ String ShorthandSerializer::serializeLayered() const
}
}

// A single background-position value (identifier or numeric) sets the other value to center.
// A single mask-position value (identifier or numeric) sets the other value to center.
// Order matters when one is numeric, but not when both are identifiers.
if (longhand == CSSPropertyBackgroundPositionY || longhand == CSSPropertyWebkitMaskPositionY) {
// The previous property is X.
ASSERT(j >= 1);
ASSERT(longhandProperty(j - 1) == CSSPropertyBackgroundPositionX || longhandProperty(j - 1) == CSSPropertyWebkitMaskPositionX);
if (layerValues.valueID(j - 1) == CSSValueCenter && layerValues.isValueID(j)) {
layerValues.skip(j - 1) = true;
layerValues.skip(j) = false;
} else if (layerValues.valueID(j) == CSSValueCenter && !layerValues.isPair(j - 1)) {
layerValues.skip(j - 1) = false;
layerValues.skip(j) = true;
} else if (length() == 2) {
ASSERT(j == 1);
layerValues.skip(0) = false;
layerValues.skip(1) = false;
} else {
if (!layerValues.skip(j - 1))
layerValues.skip(j) = false;
}
}

if (layerValues.skip(j))
continue;

Expand All @@ -623,22 +646,6 @@ String ShorthandSerializer::serializeLayered() const
layerValues.skip(j - 1) = false;
}

// A single background-position value (identifier or numeric) sets the other value to center.
// A single mask-position value (identifier or numeric) sets the other value to center.
// Order matters when one is numeric, but not when both are identifiers.
if (longhand == CSSPropertyBackgroundPositionY || longhand == CSSPropertyWebkitMaskPositionY) {
// The previous property is X.
ASSERT(j >= 1);
ASSERT(longhandProperty(j - 1) == CSSPropertyBackgroundPositionX
|| longhandProperty(j - 1) == CSSPropertyWebkitMaskPositionX);
if (layerValues.valueID(j - 1) == CSSValueCenter && layerValues.isValueID(j))
layerValues.skip(j - 1) = true;
else if (layerValues.valueID(j) == CSSValueCenter && !layerValues.isPair(j - 1)) {
layerValues.skip(j - 1) = false;
layerValues.skip(j) = true;
}
}

// The first value in each animation shorthand that can be parsed as a time is assigned to
// animation-duration, and the second is assigned to animation-delay, so we must serialize
// both if we are serializing animation-delay.
Expand Down

0 comments on commit 3f5f0f1

Please sign in to comment.