Skip to content

css: stop rewriting valid positions into ones browsers reject #45197

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.

Sign up for GitHub

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

Closed
robobun wants to merge 1 commit into from
Closed

css: stop rewriting valid positions into ones browsers reject #45197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 32 additions & 3 deletions src/css/values/position.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,21 +149,24 @@ impl Position {

pub(crate) fn to_css(&self, dest: &mut css::Printer) -> Result<(), css::PrintErr> {
if let (PositionComponent::Side(xs), PositionComponent::Length(yl)) = (&self.x, &self.y) {
if xs.side != HorizontalPositionKeyword::Left {
if xs.offset.is_some() && xs.side != HorizontalPositionKeyword::Left {
self.x.to_css(dest)?;
dest.write_str(" top ")?;
return yl.to_css(dest);
}
}
if let PositionComponent::Side(xs) = &self.x {
if xs.side != HorizontalPositionKeyword::Left && self.y.is_center() {
if xs.offset.is_some()
&& xs.side != HorizontalPositionKeyword::Left
&& self.y.is_center()
{
// If there is a side keyword with an offset, "center" must be a keyword not a percentage.
self.x.to_css(dest)?;
return dest.write_str(" center");
}
}
if let (PositionComponent::Length(xl), PositionComponent::Side(ys)) = (&self.x, &self.y) {
if ys.side != VerticalPositionKeyword::Top {
if ys.offset.is_some() && ys.side != VerticalPositionKeyword::Top {
dest.write_str("left ")?;
xl.to_css(dest)?;
dest.write_str(" ")?;
Expand All @@ -180,6 +183,16 @@ impl Position {
return xl.to_css(dest);
}
}
if let PositionComponent::Side(PositionComponentSide {
side: HorizontalPositionKeyword::Left,
offset: Some(xl),
}) = &self.x
{
if self.y.is_center() {
// `left 10px center` => `10px`
return xl.to_css(dest);
}
}
if let PositionComponent::Side(xs) = &self.x {
if xs.offset.is_none() && self.y.is_center() {
let p: LengthPercentage = xs.side.into_length_percentage();
Expand All @@ -191,6 +204,18 @@ impl Position {
return self.y.to_css(dest);
}
}
if let (
PositionComponent::Center,
PositionComponent::Side(PositionComponentSide {
side: VerticalPositionKeyword::Bottom,
offset: Some(_),
}),
) = (&self.x, &self.y)
{
// `center bottom 10px` must keep the keyword: `50% bottom 10px` is not valid.
dest.write_str("center ")?;
return self.y.to_css(dest);
}
if let (PositionComponent::Side(xs), PositionComponent::Side(ys)) = (&self.x, &self.y) {
if xs.offset.is_none() && ys.offset.is_none() {
let x: LengthPercentage = xs.side.into_length_percentage();
Expand Down Expand Up @@ -221,6 +246,8 @@ impl Position {
PositionComponent::Length(len) => {
if len.is_zero() {
break 'x_len Some(&zero);
} else {
break 'x_len Some(len);
}
}
PositionComponent::Center => break 'x_len Some(&fifty),
Expand All @@ -246,6 +273,8 @@ impl Position {
PositionComponent::Length(len) => {
if len.is_zero() {
break 'y_len Some(&zero);
} else {
break 'y_len Some(len);
}
}
PositionComponent::Center => break 'y_len Some(&fifty),
Expand Down
14 changes: 14 additions & 0 deletions test/bundler/css/wpt/background-computed.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,20 @@ describe("background-computed", () => {
"12px 13px, 50% 6px, 30px -10px, -7px 8px",
"12px 13px, 50% 6px, 30px -10px, -7px 8px",
);
runTest("background-position", "right 40%", "right 40%");
runTest("background-position", "10px bottom", "10px bottom");
// A gradient position has no three-value form: `at right top 40%` and `at left 10px bottom` are invalid.
runTest(
"background-image",
"radial-gradient(circle at right 40%, red, blue)",
"radial-gradient(circle at right 40%, red, #00f)",
);
runTest(
"background-image",
"radial-gradient(circle at 10px bottom, red, blue)",
"radial-gradient(circle at 10px bottom, red, #00f)",
);
runTest("background-image", "conic-gradient(at right 40%, red, blue)", "conic-gradient(at right 40%, red, #00f)");
runTest("background-position-x", "0.5em", ".5em");
runTest("background-position-x", "-20%, 10px", "-20%, 10px");
runTest("background-position-x", "center, left, right", "center, left, right");
Expand Down
125 changes: 124 additions & 1 deletion test/js/bun/css/css.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2958,8 +2958,11 @@ describe("css tests", () => {
);

minify_test(".foo { background-position: bottom left }", ".foo{background-position:0 100%}");
minify_test(".foo { background-position: left 10px center }", ".foo{background-position:10px 50%}");
minify_test(".foo { background-position: left 10px center }", ".foo{background-position:10px}");
minify_test(".foo { background-position: right 10px center }", ".foo{background-position:right 10px center}");
minify_test(".foo { background-position: center top 10px }", ".foo{background-position:50% 10px}");
minify_test(".foo { background-position: center bottom 10px }", ".foo{background-position:center bottom 10px}");
minify_test(".foo { background-position: center 10px }", ".foo{background-position:50% 10px}");
minify_test(".foo { background-position: right 10px top 20px }", ".foo{background-position:right 10px top 20px}");
minify_test(".foo { background-position: left 10px top 20px }", ".foo{background-position:10px 20px}");
minify_test(
Expand All @@ -2968,6 +2971,11 @@ describe("css tests", () => {
);
minify_test(".foo { background-position: left 10px top }", ".foo{background-position:10px 0}");
minify_test(".foo { background-position: bottom right }", ".foo{background-position:100% 100%}");
minify_test(".foo { background-position: center top }", ".foo{background-position:top}");
minify_test(".foo { background-position: center bottom }", ".foo{background-position:bottom}");
minify_test(".foo { background-position: left center }", ".foo{background-position:0}");
minify_test(".foo { background-position: right center }", ".foo{background-position:100%}");
minify_test(".foo { background-position: 20px center }", ".foo{background-position:20px}");

minify_test(
".foo { background: url('img-sprite.png') no-repeat bottom right }",
Expand Down Expand Up @@ -3420,6 +3428,121 @@ describe("css tests", () => {
);
});

// One function prints the position of background-position, of the background and mask
// shorthands, and of radial and conic gradients. The expected text is what lightningcss 1.33.0 prints.
describe("position", () => {
// [source, printed, printed with minify]
const sites = [
["background-position: <position>", "background-position: <position>", "background-position:<position>"],
[
"background: url(img.png) <position> no-repeat",
'background: url("img.png") <position> no-repeat',
"background:url(img.png) <position> no-repeat",
],
[
"background: radial-gradient(circle at <position>, red, blue)",
"background: radial-gradient(circle at <position>, red, #00f)",
"background:radial-gradient(circle at <position>,red,#00f)",
],
[
"background: repeating-radial-gradient(circle at <position>, red, blue)",
"background: repeating-radial-gradient(circle at <position>, red, #00f)",
"background:repeating-radial-gradient(circle at <position>,red,#00f)",
],
[
"background: conic-gradient(from 45deg at <position>, red, blue)",
"background: conic-gradient(from 45deg at <position>, red, #00f)",
"background:conic-gradient(from 45deg at <position>,red,#00f)",
],
["mask: url(img.png) <position>", 'mask: url("img.png") <position>', "mask:url(img.png) <position>"],
[
"-webkit-mask: url(img.png) <position>",
'-webkit-mask: url("img.png") <position>',
"-webkit-mask:url(img.png) <position>",
],
];
Comment thread
robobun marked this conversation as resolved.
Resolved
// [as written, as printed]. Only background-position has a three-value form, so a gradient
// with `at right top 40%` or `at left 10px bottom` is invalid and a browser drops it.
const positions = [
["right 40%", "right 40%"],
["10px bottom", "10px bottom"],
["right center", "100%"],
["50% bottom", "bottom"],
];
for (const [source, printed, minified] of sites) {
for (const [written, position] of positions) {
const input = `.foo { ${source.replace("<position>", written)} }`;
cssTest(input, `.foo { ${printed.replace("<position>", position)}; }`);
minify_test(input, `.foo{${minified.replace("<position>", position)}}`);
}
}

// background-position-x and background-position-y each take any axis shape. They merge into
// background-position, or into the background shorthand when the other longhands are known.
// Every cell is a value that background-position accepts.
const xs = ["center", "10px", "left", "left 10px", "right", "right 10px"];
const ys = ["center", "20px", "top", "top 20px", "bottom", "bottom 20px"];
// prettier-ignore
const merged = [
// y: center 20px top top 20px bottom bottom 20px
["center", "50% 20px", "top", "50% 20px", "bottom", "center bottom 20px"], // x: center
["10px", "10px 20px", "10px 0", "10px 20px", "10px bottom", "left 10px bottom 20px"], // x: 10px
["0", "0 20px", "0 0", "0 20px", "0 100%", "left bottom 20px"], // x: left
["10px", "10px 20px", "10px 0", "10px 20px", "left 10px bottom", "left 10px bottom 20px"], // x: left 10px
["100%", "right 20px", "100% 0", "right top 20px", "100% 100%", "right bottom 20px"], // x: right
["right 10px center", "right 10px top 20px", "right 10px top", "right 10px top 20px", "right 10px bottom", "right 10px bottom 20px"], // x: right 10px
];
xs.forEach((x, i) => {
ys.forEach((y, j) => {
const position = merged[i][j];
// `center center` is the only cell that minify changes.
const minified = position === "center" ? "50%" : position;
const axes = `background-position-x: ${x}; background-position-y: ${y}`;
cssTest(`.foo { ${axes} }`, `.foo { background-position: ${position}; }`);
minify_test(`.foo { ${axes} }`, `.foo{background-position:${minified}}`);
cssTest(`.foo { background: url(img.png); ${axes} }`, `.foo { background: url("img.png") ${position}; }`);
minify_test(`.foo { background: url(img.png); ${axes} }`, `.foo{background:url(img.png) ${minified}}`);
});
});

// An axis longhand after background-position replaces that axis.
cssTest(
".foo { background-position: left 10px top 5px; background-position-y: 40% }",
".foo { background-position: 10px 40%; }",
);
minify_test(
".foo { background-position: left 10px top 5px; background-position-y: 40% }",
".foo{background-position:10px 40%}",
);
// The axes merge across adjacent rules with the same selector.
cssTest(
".foo { background-position-x: 10px } .foo { background-position-y: top 5px }",
".foo { background-position: 10px 5px; }",
);
minify_test(
".foo { background-position-x: 10px } .foo { background-position-y: top 5px }",
".foo{background-position:10px 5px}",
);
// background-position after the axis longhands replaces both axes.
cssTest(
".foo { background-position-x: right 10px; background-position-y: 20px; background-position: 10px bottom }",
".foo { background-position: 10px bottom; }",
);
minify_test(
".foo { background-position-x: right 10px; background-position-y: 20px; background-position: 10px bottom }",
".foo{background-position:10px bottom}",
);
// Each layer merges on its own.
cssTest(
".foo { background-position-x: 10px, right, center; background-position-y: top 5px, 40%, bottom 10px }",
".foo { background-position: 10px 5px, right 40%, center bottom 10px; }",
);
minify_test(
".foo { background-position-x: 10px, right, center; background-position-y: top 5px, 40%, bottom 10px }",
".foo{background-position:10px 5px,right 40%,center bottom 10px}",
);
});

describe("flex", () => {
cssTest(
`
Expand Down
Loading