Skip to content

Commit

Permalink
Fix grid and grid-template shorthand serialization
Browse files Browse the repository at this point in the history
There were two issues affecting html-to-image: we were serializing the
`grid` shorthand too aggressively. The way the syntax is defined you
cannot have both grid-template-areas and grid-auto-flow for the grid
syntax. In these cases we'll now just not serialize grid, falling back
to other shorthands (grid-template) or just longhands.

We also were not serializing 'auto' grid track size values even when
they were explicitly specified. this also can break roundtripping.
Note that adding these back causes some new test failures in
grid-template-shorthand-value.html and grid-shorthand-value.html. This
is tracked by crbug.com/1311457 and will be addressed in a follow up
CL.

R=ansollan@microsoft.com

Bug: 1305997
Change-Id: I6200e0647ded31103245d48d9269d3abfdd19fb2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3558286
Reviewed-by: Ana Sollano Kim <ansollan@microsoft.com>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Daniel Libby <dlibby@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#987069}
  • Loading branch information
dlibby- authored and Chromium LUCI CQ committed Mar 30, 2022
1 parent 69faded commit 3ccbdca
Show file tree
Hide file tree
Showing 6 changed files with 141 additions and 7 deletions.
18 changes: 13 additions & 5 deletions third_party/blink/renderer/core/css/style_property_serializer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1258,6 +1258,16 @@ String StylePropertySerializer::GetShorthandValueForGrid(
CSSValueID::kAuto) {
return GetShorthandValueForGridTemplate(shorthand);
}

// If we have grid-auto-{flow,row,column} along with named lines, we can't
// serialize as a "grid" shorthand, as a syntax that combines the two is not
// valid per the grammar.
const CSSValue* template_area_value =
property_set_.GetPropertyCSSValue(*shorthand.properties()[2]);
if (*template_area_value !=
*(To<Longhand>(GetCSSPropertyGridTemplateAreas()).InitialValue()))
return String();

const CSSValue* template_row_values =
property_set_.GetPropertyCSSValue(*shorthand.properties()[0]);
const CSSValue* template_column_values =
Expand Down Expand Up @@ -1365,11 +1375,9 @@ String StylePropertySerializer::GetShorthandValueForGridTemplate(
result.Append('"');
++grid_area_index;
}
if (row_value_text != "auto") {
if (!result.IsEmpty())
result.Append(' ');
result.Append(row_value_text);
}
if (!result.IsEmpty())
result.Append(' ');
result.Append(row_value_text);
}
}
if (!(IsA<CSSIdentifierValue>(template_column_values) &&
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ FAIL grid-template.initial assert_equals: initial value of grid-template should
FAIL grid-template.none assert_equals: none expected "50px 50px 50px / 150px" but got "50px 50px 50px / 150px / none"
FAIL grid-template.<grid-template-rows> / <grid-template-columns> assert_equals: <grid-template-rows> / <grid-template-columns> expected "100px 100px / 200px 200px" but got "100px 100px / 200px 200px / none"
FAIL grid-template.<line-names> assert_equals: <line-names> expected "[a] auto [b] auto [c] / [d] auto [e] auto [f]" but got "[a] 50px [b] 50px [c] / [d] 150px [e] 100px [f] / none"
FAIL grid-template.<string>+ assert_equals: <string>+ expected "\"a b\" \"a b\"" but got "50px 50px / 150px 100px / \"a b\" \"a b\""
FAIL grid-template.<string><track-size>+ assert_equals: <string><track-size>+ expected "100px / \"a b\" 50px" but got "\"a b\" \"a b\""
FAIL grid-template.<string>+ assert_equals: <string>+ expected "\"a b\" \"a b\"" but got "\"a b\" auto \"a b\" auto"
FAIL grid-template.<string><track-size>+ assert_equals: <string><track-size>+ expected "100px / \"a b\" 50px" but got "\"a b\" auto \"a b\" auto"
FAIL grid-template.reset assert_equals: reset expected "50px 50px 50px / 150px" but got "50px 50px 50px / 150px / none"
PASS grid-auto-columns
PASS grid-auto-columns.initial
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<title>CSS Grid Layout Test: only serialize 'grid' when the value can roundtrip</title>
<link rel="author" title="Daniel Libby" href="mailto:dlibby@microsoft.com">
<link rel="help" href="https://drafts.csswg.org/css-grid/#propdef-grid">
<meta name="assert" content="grid shorthand must not serialize when the value cannot roundtrip.">
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
</head>
<body>
<script>
let element = document.createElement('div');
const autoFlowRows = "auto-flow auto / 100px 100px";
element.style.grid = autoFlowRows;
test(() => {
assert_equals(element.style.grid, autoFlowRows, autoFlowRows + " must be serialized via `grid` property");
assert_equals(element.style.cssText, "grid: " + autoFlowRows + ";", autoFlowRows + " must be serialized via `cssText`");
assert_equals(element.style.gridTemplateAreas, "none");
}, "Serialization without grid-template-areas");

const gridTemplateAreasValue = '"one two" "three four"';
element.style.gridTemplateAreas = gridTemplateAreasValue;
test(() => {
assert_equals(element.style.grid, "", "grid shorthand must not be serialized when grid-template-areas and grid-auto-flow/rows are both set");
assert_equals(element.style.gridTemplateAreas, gridTemplateAreasValue);
assert_equals(element.style.gridAutoFlow, "row");
assert_equals(element.style.gridAutoRows, "auto");
}, "Serialization with grid-template-areas");
</script>
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
This is a testharness.js-based test.
PASS e.style['grid'] = "none" should set the property value
PASS e.style['grid'] = "none / none" should set the property value
PASS e.style['grid'] = "auto / auto" should set the property value
PASS e.style['grid'] = "none / [a] 0px" should set the property value
PASS e.style['grid'] = "none / [] 0px" should set the property value
PASS e.style['grid'] = "[a] 10px / auto" should set the property value
PASS e.style['grid'] = "[a] 10px / none" should set the property value
PASS e.style['grid'] = "[] 10px [] / [] auto []" should set the property value
PASS e.style['grid'] = "[a] \"a\" 10px" should set the property value
PASS e.style['grid'] = "[a] \"a\" 10px []" should set the property value
PASS e.style['grid'] = "\"a\" 10px" should set the property value
PASS e.style['grid'] = "[] \"a\" 10px" should set the property value
PASS e.style['grid'] = "[a] \"a\" 10px [a]" should set the property value
PASS e.style['grid'] = "\"a\"" should set the property value
PASS e.style['grid'] = "\"a\" auto" should set the property value
PASS e.style['grid'] = "\"a a a\"" should set the property value
PASS e.style['grid'] = "\"a\" / 10px" should set the property value
PASS e.style['grid'] = "\"a\" / 20%" should set the property value
PASS e.style['grid'] = "\"a\" / 5fr" should set the property value
FAIL e.style['grid'] = "[a] \"a\"" should set the property value assert_equals: serialization should be canonical expected "[a] \"a\"" but got "[a] \"a\" auto"
FAIL e.style['grid'] = "[a] \"a\" [a]" should set the property value assert_equals: serialization should be canonical expected "[a] \"a\" [a]" but got "[a] \"a\" auto [a]"
PASS e.style['grid'] = "[] \"a\"" should set the property value
FAIL e.style['grid'] = "\"a\" [] [] \"b\"" should set the property value assert_equals: serialization should be canonical expected "\"a\" \"b\"" but got "\"a\" auto \"b\" auto"
FAIL e.style['grid'] = "\"a\" [] \"b\"" should set the property value assert_equals: serialization should be canonical expected "\"a\" \"b\"" but got "\"a\" auto \"b\" auto"
FAIL e.style['grid'] = "\"a\" [a] [b] \"b\"" should set the property value assert_equals: serialization should be canonical expected "\"a\" [a b] \"b\"" but got "\"a\" auto [a b] \"b\" auto"
FAIL e.style['grid'] = "\"a\" [a] \"b\"" should set the property value assert_equals: serialization should be canonical expected "\"a\" [a] \"b\"" but got "\"a\" auto [a] \"b\" auto"
PASS e.style['grid'] = "\"a\" / 0" should set the property value
PASS e.style['grid'] = "\"a\" 10px / 10px" should set the property value
FAIL e.style['grid'] = "\"a\" [a] \"b\" 10px" should set the property value assert_equals: serialization should be canonical expected "\"a\" [a] \"b\" 10px" but got "\"a\" auto [a] \"b\" 10px"
FAIL e.style['grid'] = "\"a\" [a] \"b\" 10px [a]" should set the property value assert_equals: serialization should be canonical expected "\"a\" [a] \"b\" 10px [a]" but got "\"a\" auto [a] \"b\" 10px [a]"
FAIL e.style['grid'] = "\"a\" [a] [a] \"b\" 10px" should set the property value assert_equals: serialization should be canonical expected "\"a\" [a a] \"b\" 10px" but got "\"a\" auto [a a] \"b\" 10px"
FAIL e.style['grid'] = "\"a\" [a] [] \"b\" 10px" should set the property value assert_equals: serialization should be canonical expected "\"a\" [a] \"b\" 10px" but got "\"a\" auto [a] \"b\" 10px"
FAIL e.style['grid'] = "\"a\" 10px [a] \"b\" [a]" should set the property value assert_equals: serialization should be canonical expected "\"a\" 10px [a] \"b\" [a]" but got "\"a\" 10px [a] \"b\" auto [a]"
FAIL e.style['grid'] = "\"a\" [a] \"b\" [a]" should set the property value assert_equals: serialization should be canonical expected "\"a\" [a] \"b\" [a]" but got "\"a\" auto [a] \"b\" auto [a]"
FAIL e.style['grid'] = "[a] \"a\" [a] \"b\" [a]" should set the property value assert_equals: serialization should be canonical expected "[a] \"a\" [a] \"b\" [a]" but got "[a] \"a\" auto [a] \"b\" auto [a]"
FAIL e.style['grid'] = "\"a\" \"a\" [a] \"b\" [a]" should set the property value assert_equals: serialization should be canonical expected "\"a\" \"a\" [a] \"b\" [a]" but got "\"a\" auto \"a\" auto [a] \"b\" auto [a]"
FAIL e.style['grid'] = "\"a\" [a] \"b\" [a] / 0" should set the property value assert_equals: serialization should be canonical expected "\"a\" [a] \"b\" [a] / 0px" but got "\"a\" auto [a] \"b\" auto [a] / 0px"
FAIL e.style['grid'] = "\"a\" \"a\" [a] [a] \"b\" / auto" should set the property value assert_equals: serialization should be canonical expected "\"a\" \"a\" [a a] \"b\" / auto" but got "\"a\" auto \"a\" auto [a a] \"b\" auto / auto"
PASS e.style['grid'] = "100px / auto-flow dense 100px" should set the property value
PASS e.style['grid'] = "auto-flow dense 1fr / 100px" should set the property value
PASS e.style['grid'] = "100px / dense auto-flow 100px" should set the property value
PASS e.style['grid'] = "dense auto-flow 1fr / 100px" should set the property value
PASS e.style['grid'] = "100px / auto-flow 100px" should set the property value
PASS e.style['grid'] = "auto-flow 1fr / 100px" should set the property value
PASS e.style['grid'] = "none / auto-flow 100px" should set the property value
PASS e.style['grid'] = "auto-flow 1fr / none" should set the property value
PASS e.style['grid'] = "auto / auto-flow 100px" should set the property value
PASS e.style['grid'] = "auto-flow 1fr / auto" should set the property value
PASS e.style['grid'] = "1fr / 1fr" should set the property value
Harness: the test ran to completion.

Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
This is a testharness.js-based test.
PASS e.style['grid-template'] = "none" should set the property value
PASS e.style['grid-template'] = "none / none" should set the property value
PASS e.style['grid-template'] = "auto / auto" should set the property value
PASS e.style['grid-template'] = "none / [a] 0px" should set the property value
PASS e.style['grid-template'] = "none / [] 0px" should set the property value
PASS e.style['grid-template'] = "[a] 10px / auto" should set the property value
PASS e.style['grid-template'] = "[a] 10px / none" should set the property value
PASS e.style['grid-template'] = "[] 10px [] / [] auto []" should set the property value
PASS e.style['grid-template'] = "[a] \"a\" 10px" should set the property value
PASS e.style['grid-template'] = "[a] \"a\" 10px []" should set the property value
PASS e.style['grid-template'] = "\"a\" 10px" should set the property value
PASS e.style['grid-template'] = "[] \"a\" 10px" should set the property value
PASS e.style['grid-template'] = "[a] \"a\" 10px [a]" should set the property value
PASS e.style['grid-template'] = "\"a\"" should set the property value
PASS e.style['grid-template'] = "\"a\" auto" should set the property value
PASS e.style['grid-template'] = "\"a a a\"" should set the property value
PASS e.style['grid-template'] = "\"a\" / 10px" should set the property value
PASS e.style['grid-template'] = "\"a\" / 20%" should set the property value
PASS e.style['grid-template'] = "\"a\" / 5fr" should set the property value
FAIL e.style['grid-template'] = "[a] \"a\"" should set the property value assert_equals: serialization should be canonical expected "[a] \"a\"" but got "[a] \"a\" auto"
FAIL e.style['grid-template'] = "[a] \"a\" [a]" should set the property value assert_equals: serialization should be canonical expected "[a] \"a\" [a]" but got "[a] \"a\" auto [a]"
PASS e.style['grid-template'] = "[] \"a\"" should set the property value
FAIL e.style['grid-template'] = "\"a\" [] [] \"b\"" should set the property value assert_equals: serialization should be canonical expected "\"a\" \"b\"" but got "\"a\" auto \"b\" auto"
FAIL e.style['grid-template'] = "\"a\" [] \"b\"" should set the property value assert_equals: serialization should be canonical expected "\"a\" \"b\"" but got "\"a\" auto \"b\" auto"
FAIL e.style['grid-template'] = "\"a\" [a] [b] \"b\"" should set the property value assert_equals: serialization should be canonical expected "\"a\" [a b] \"b\"" but got "\"a\" auto [a b] \"b\" auto"
FAIL e.style['grid-template'] = "\"a\" [a] \"b\"" should set the property value assert_equals: serialization should be canonical expected "\"a\" [a] \"b\"" but got "\"a\" auto [a] \"b\" auto"
PASS e.style['grid-template'] = "\"a\" / 0" should set the property value
PASS e.style['grid-template'] = "\"a\" 10px / 10px" should set the property value
FAIL e.style['grid-template'] = "\"a\" [a] \"b\" 10px" should set the property value assert_equals: serialization should be canonical expected "\"a\" [a] \"b\" 10px" but got "\"a\" auto [a] \"b\" 10px"
FAIL e.style['grid-template'] = "\"a\" [a] \"b\" 10px [a]" should set the property value assert_equals: serialization should be canonical expected "\"a\" [a] \"b\" 10px [a]" but got "\"a\" auto [a] \"b\" 10px [a]"
FAIL e.style['grid-template'] = "\"a\" [a] [a] \"b\" 10px" should set the property value assert_equals: serialization should be canonical expected "\"a\" [a a] \"b\" 10px" but got "\"a\" auto [a a] \"b\" 10px"
FAIL e.style['grid-template'] = "\"a\" [a] [] \"b\" 10px" should set the property value assert_equals: serialization should be canonical expected "\"a\" [a] \"b\" 10px" but got "\"a\" auto [a] \"b\" 10px"
FAIL e.style['grid-template'] = "\"a\" 10px [a] \"b\" [a]" should set the property value assert_equals: serialization should be canonical expected "\"a\" 10px [a] \"b\" [a]" but got "\"a\" 10px [a] \"b\" auto [a]"
FAIL e.style['grid-template'] = "\"a\" [a] \"b\" [a]" should set the property value assert_equals: serialization should be canonical expected "\"a\" [a] \"b\" [a]" but got "\"a\" auto [a] \"b\" auto [a]"
FAIL e.style['grid-template'] = "[a] \"a\" [a] \"b\" [a]" should set the property value assert_equals: serialization should be canonical expected "[a] \"a\" [a] \"b\" [a]" but got "[a] \"a\" auto [a] \"b\" auto [a]"
FAIL e.style['grid-template'] = "\"a\" \"a\" [a] \"b\" [a]" should set the property value assert_equals: serialization should be canonical expected "\"a\" \"a\" [a] \"b\" [a]" but got "\"a\" auto \"a\" auto [a] \"b\" auto [a]"
FAIL e.style['grid-template'] = "\"a\" [a] \"b\" [a] / 0" should set the property value assert_equals: serialization should be canonical expected "\"a\" [a] \"b\" [a] / 0px" but got "\"a\" auto [a] \"b\" auto [a] / 0px"
FAIL e.style['grid-template'] = "\"a\" \"a\" [a] [a] \"b\" / auto" should set the property value assert_equals: serialization should be canonical expected "\"a\" \"a\" [a a] \"b\" / auto" but got "\"a\" auto \"a\" auto [a a] \"b\" auto / auto"
PASS e.style['grid-template'] = "\"a\" auto [a] \"b\" auto [b] / 10px" should set the property value
Harness: the test ran to completion.

Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
test_valid_value("grid-template", '"a" "a" [a] "b" [a]');
test_valid_value("grid-template", '"a" [a] "b" [a] / 0', '"a" [a] "b" [a] / 0px');
test_valid_value("grid-template", '"a" "a" [a] [a] "b" / auto', '"a" "a" [a a] "b" / auto');
test_valid_value("grid-template", '"a" auto [a] "b" auto [b] / 10px');

// FIXME: add more values to test full syntax

Expand Down

0 comments on commit 3ccbdca

Please sign in to comment.