Skip to content

Commit

Permalink
Apply-order must not matter for grid-template-[areas,rows,columns]
Browse files Browse the repository at this point in the history 
We can currently get two different ComputedStyles depending on which
order we apply certain grid-* properties in. This must not happen in
Blink, even if it doesn't result in a web-facing change, because we
have optimizations that rely on sanity in this area.

The issue is that the Apply functions for grid-template-[columns,rows]
look at the (potentially work-in-progress) computed value of
grid-template-area, and uses this to generate implicit named lines.
However, we also do this implicit named line generation in
GridTemplateAreas::ApplyValue, and store those in a separate field
on ComputedStyle. As far as I can tell, in the all the places we
access ComputedGridTrackList::named_grid_lines, we also already check
ComputedStyle::ImplicitNamedGrid*Lines, which leads me to believe that
adding the implicit named lines during GridTemplate[Columns,Rows]::
Apply is actually redundant.

The only exception is InspectorHighlight, which needs to be extended
to also include the implicit lines.

Fixed: 1320659
Change-Id: I1a36a245559a591cec29b954942c384cf98495b1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3636378
Reviewed-by: Changhao Han <changhaohan@chromium.org>
Reviewed-by: Ana Sollano Kim <ansollan@microsoft.com>
Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1001716}
  • Loading branch information
@andruud
andruud authored and Chromium LUCI CQ committed May 10, 2022
1 parent 1d11d58 commit bcd5354
Show file tree
Hide file tree
Showing 6 changed files with 188 additions and 92 deletions.
5 changes: 0 additions & 5 deletions ...y/blink/renderer/build/scripts/core/css/properties/templates/style_builder_functions.tmpl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -374,11 +374,6 @@ To<Longhand>(resolved_property).{{apply_call}};
{% call(property) apply_value(property) %}
ComputedGridTrackList computed_grid_track_list;
StyleBuilderConverter::ConvertGridTrackList(value, computed_grid_track_list, state);
const NamedGridAreaMap& named_grid_areas = state.Style()->NamedGridArea();
if (!named_grid_areas.IsEmpty()) {
StyleBuilderConverter::CreateImplicitNamedGridLinesFromGridArea(
named_grid_areas, computed_grid_track_list.named_grid_lines, kFor{{type}}s);
}
state.Style()->SetGridTemplate{{type}}s(computed_grid_track_list);
{% endcall %}
{% endif %}
Expand Down
62 changes: 0 additions & 62 deletions third_party/blink/renderer/core/css/css_properties.json5
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2576,9 +2576,6 @@
keywords: ["auto", "min-content", "max-content"],
typedom_types: ["Keyword", "Length", "Percentage", "Flex"],
separator: " ",
// See comment on grid.
supports_incremental_style: false,
idempotent: false,
},
{
name: "grid-auto-flow",
Expand All @@ -2592,9 +2589,6 @@
converter: "ConvertGridAutoFlow",
keywords: ["row", "column"],
typedom_types: ["Keyword"],
// See comment on grid.
supports_incremental_style: false,
idempotent: false,
},
{
name: "grid-auto-rows",
Expand All @@ -2608,9 +2602,6 @@
keywords: ["auto", "min-content", "max-content"],
typedom_types: ["Keyword", "Length", "Percentage", "Flex"],
separator: " ",
// See comment on grid.
supports_incremental_style: false,
idempotent: false,
},
{
name: "grid-column-end",
Expand All @@ -2623,9 +2614,6 @@
keywords: ["auto"],
typedom_types: ["Keyword"],
converter: "ConvertGridPosition",
// See comment on grid.
supports_incremental_style: false,
idempotent: false,
},
{
name: "grid-column-start",
Expand All @@ -2638,9 +2626,6 @@
keywords: ["auto"],
typedom_types: ["Keyword"],
converter: "ConvertGridPosition",
// See comment on grid.
supports_incremental_style: false,
idempotent: false,
},
{
name: "grid-row-end",
Expand All @@ -2653,9 +2638,6 @@
keywords: ["auto"],
typedom_types: ["Keyword"],
converter: "ConvertGridPosition",
// See comment on grid.
supports_incremental_style: false,
idempotent: false,
},
{
name: "grid-row-start",
Expand All @@ -2668,19 +2650,13 @@
keywords: ["auto"],
typedom_types: ["Keyword"],
converter: "ConvertGridPosition",
// See comment on grid.
supports_incremental_style: false,
idempotent: false,
},
{
name: "grid-template-areas",
property_methods: ["ParseSingleValue", "CSSValueFromComputedStyleInternal", "InitialValue"],
style_builder_custom_functions: ["initial", "inherit", "value"],
keywords: ["none"],
typedom_types: ["Keyword"],
// See comment on grid.
supports_incremental_style: false,
idempotent: false,
},
{
name: "grid-template-columns",
Expand All @@ -2697,9 +2673,6 @@
},
keywords: ["none"],
typedom_types: ["Keyword"],
// See comment on grid.
supports_incremental_style: false,
idempotent: false,
},
{
name: "grid-template-rows",
Expand All @@ -2716,9 +2689,6 @@
},
keywords: ["none"],
typedom_types: ["Keyword"],
// See comment on grid.
supports_incremental_style: false,
idempotent: false,
},
{
name: "height",
Expand Down Expand Up @@ -6469,15 +6439,6 @@
],
property_methods: ["ParseShorthand", "CSSValueFromComputedStyleInternal"],
layout_dependent: true,
// The flow with GridTemplateAreas::ApplyValue() and
// GridTemplate{Columns,Rows}::ApplyValue() is not idempotent:
// If you set grids twice, it may find a previous implicit
// named grid area, wrongly assume it was explicit, and thus set named
// grid lines as explicit, too. Since we don't want to track whether
// the grid was implicit or e.g. explicit from inheritance, we simply
// block out anything related to CSS grids from incremental updates.
supports_incremental_style: false,
idempotent: false,
},
{
name: "place-content",
Expand All @@ -6501,56 +6462,36 @@
"grid-column-end"
],
property_methods: ["ParseShorthand", "CSSValueFromComputedStyleInternal"],
// See comment on grid.
supports_incremental_style: false,
idempotent: false,
},
{
name: "grid-column",
longhands: ["grid-column-start", "grid-column-end"],
property_methods: ["ParseShorthand", "CSSValueFromComputedStyleInternal"],
// See comment on grid.
supports_incremental_style: false,
idempotent: false,
},
{
name: "grid-column-gap",
longhands: ["column-gap"],
property_methods: ["ParseShorthand", "CSSValueFromComputedStyleInternal"],
// See comment on grid.
supports_incremental_style: false,
idempotent: false,
},
{
name: "grid-row-gap",
longhands: ["row-gap"],
property_methods: ["ParseShorthand", "CSSValueFromComputedStyleInternal"],
// See comment on grid.
supports_incremental_style: false,
idempotent: false,
},
{
name: "gap",
longhands: ["row-gap", "column-gap"],
property_methods: ["ParseShorthand", "CSSValueFromComputedStyleInternal"],
// See comment on grid.
supports_incremental_style: false,
},
{
name: "grid-gap",
longhands: ["row-gap", "column-gap"],
property_methods: ["ParseShorthand", "CSSValueFromComputedStyleInternal"],
// See comment on grid.
supports_incremental_style: false,
idempotent: false,
},
{
name: "grid-row",
longhands: ["grid-row-start", "grid-row-end"],
property_methods: ["ParseShorthand", "CSSValueFromComputedStyleInternal"],
// See comment on grid.
supports_incremental_style: false,
idempotent: false,
},
{
name: "grid-template",
Expand All @@ -6559,9 +6500,6 @@
],
property_methods: ["ParseShorthand", "CSSValueFromComputedStyleInternal"],
layout_dependent: true,
// See comment on grid.
supports_incremental_style: false,
idempotent: false,
},
{
name: "inset",
Expand Down
59 changes: 59 additions & 0 deletions third_party/blink/renderer/core/css/resolver/style_builder_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include "third_party/blink/renderer/core/css/css_identifier_value.h"
#include "third_party/blink/renderer/core/css/css_inherited_value.h"
#include "third_party/blink/renderer/core/css/css_initial_value.h"
#include "third_party/blink/renderer/core/css/css_test_helpers.h"
#include "third_party/blink/renderer/core/css/resolver/style_resolver.h"
#include "third_party/blink/renderer/core/css/resolver/style_resolver_state.h"
#include "third_party/blink/renderer/core/css/scoped_css_value.h"
Expand Down Expand Up @@ -106,4 +107,62 @@ TEST_F(StyleBuilderTest, HasExplicitInheritance) {
EXPECT_TRUE(style->HasExplicitInheritance());
}

TEST_F(StyleBuilderTest, GridTemplateAreasApplyOrder) {
const CSSProperty& grid_template_areas = GetCSSPropertyGridTemplateAreas();
const CSSProperty& grid_template_rows = GetCSSPropertyGridTemplateRows();
const CSSProperty& grid_template_columns =
GetCSSPropertyGridTemplateColumns();

const CSSValue* grid_template_areas_value = css_test_helpers::ParseLonghand(
GetDocument(), grid_template_areas, "'foo' 'bar' 'baz' 'faz'");
const CSSValue* grid_template_columns_value = css_test_helpers::ParseLonghand(
GetDocument(), grid_template_columns, "50px 50px");
const CSSValue* grid_template_rows_value = css_test_helpers::ParseLonghand(
GetDocument(), grid_template_rows, "50px 50px");

ASSERT_TRUE(grid_template_areas_value);
ASSERT_TRUE(grid_template_columns_value);
ASSERT_TRUE(grid_template_rows_value);

scoped_refptr<ComputedStyle> parent_style =
GetDocument().GetStyleResolver().CreateComputedStyle();
StyleResolverState state(GetDocument(), *GetDocument().body(),
StyleRecalcContext(),
StyleRequest(parent_style.get()));

scoped_refptr<ComputedStyle> style1;
scoped_refptr<ComputedStyle> style2;

// grid-template-areas applied first.
state.SetStyle(ComputedStyle::Clone(*parent_style));
StyleBuilder::ApplyProperty(
grid_template_areas, state,
ScopedCSSValue(*grid_template_areas_value, nullptr));
StyleBuilder::ApplyProperty(
grid_template_columns, state,
ScopedCSSValue(*grid_template_columns_value, nullptr));
StyleBuilder::ApplyProperty(
grid_template_rows, state,
ScopedCSSValue(*grid_template_rows_value, nullptr));
style1 = state.TakeStyle();

// grid-template-areas applied last.
state.SetStyle(ComputedStyle::Clone(*parent_style));
StyleBuilder::ApplyProperty(
grid_template_columns, state,
ScopedCSSValue(*grid_template_columns_value, nullptr));
StyleBuilder::ApplyProperty(
grid_template_rows, state,
ScopedCSSValue(*grid_template_rows_value, nullptr));
StyleBuilder::ApplyProperty(
grid_template_areas, state,
ScopedCSSValue(*grid_template_areas_value, nullptr));
style2 = state.TakeStyle();

ASSERT_TRUE(style1);
ASSERT_TRUE(style2);
EXPECT_EQ(*style1, *style2)
<< "Application order of grid properties does not affect result";
}

} // namespace blink
55 changes: 33 additions & 22 deletions third_party/blink/renderer/core/inspector/inspector_highlight.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -914,41 +914,52 @@ std::unique_ptr<protocol::ListValue> BuildGridLineNames(

std::unique_ptr<protocol::ListValue> lines = protocol::ListValue::create();

const NamedGridLinesMap& named_lines_map =
(direction == kForColumns)
? grid_container_style.GridTemplateColumns().named_grid_lines
: grid_container_style.GridTemplateRows().named_grid_lines;
LayoutUnit gap = grid_interface->GridGap(direction);
LayoutUnit alt_axis_pos = GetPositionForFirstTrack(
layout_object, direction == kForRows ? kForColumns : kForRows,
alt_axis_positions);

for (const auto& item : named_lines_map) {
const String& name = item.key;
auto process_grid_lines_map = [&](const NamedGridLinesMap& named_lines_map) {
for (const auto& item : named_lines_map) {
const String& name = item.key;

for (const wtf_size_t index : item.value) {
LayoutUnit track =
GetPositionForTrackAt(layout_object, index, direction, positions);
for (const wtf_size_t index : item.value) {
LayoutUnit track =
GetPositionForTrackAt(layout_object, index, direction, positions);

LayoutUnit gap_offset =
index > 0 && index < positions.size() - 1 ? gap / 2 : LayoutUnit();
if (is_rtl)
gap_offset *= -1;
LayoutUnit gap_offset =
index > 0 && index < positions.size() - 1 ? gap / 2 : LayoutUnit();
if (is_rtl)
gap_offset *= -1;

LayoutUnit main_axis_pos = track - gap_offset;
PhysicalOffset line_name_pos(main_axis_pos, alt_axis_pos);
LayoutUnit main_axis_pos = track - gap_offset;
PhysicalOffset line_name_pos(main_axis_pos, alt_axis_pos);

if (direction == kForRows)
line_name_pos = Transpose(line_name_pos);
if (direction == kForRows)
line_name_pos = Transpose(line_name_pos);

std::unique_ptr<protocol::DictionaryValue> line =
BuildPosition(LocalToAbsolutePoint(node, line_name_pos, scale));
std::unique_ptr<protocol::DictionaryValue> line =
BuildPosition(LocalToAbsolutePoint(node, line_name_pos, scale));

line->setString("name", name);
line->setString("name", name);

lines->pushValue(std::move(line));
lines->pushValue(std::move(line));
}
}
}
};

const NamedGridLinesMap& explicit_lines_map =
(direction == kForColumns)
? grid_container_style.GridTemplateColumns().named_grid_lines
: grid_container_style.GridTemplateRows().named_grid_lines;

const NamedGridLinesMap& implicit_lines_map =
(direction == kForColumns)
? grid_container_style.ImplicitNamedGridColumnLines()
: grid_container_style.ImplicitNamedGridRowLines();

process_grid_lines_map(explicit_lines_map);
process_grid_lines_map(implicit_lines_map);

return lines;
}
Expand Down

0 comments on commit bcd5354

Please sign in to comment.