Skip to content

Commit

Permalink
[GridNG] Fix querying %-margins on grid-items.
Browse files Browse the repository at this point in the history 
This patch uses the "StoreMargins" to write the margins into the
appropriate field. Eventually we'll remove this and instead store the
margins inside the NGLink, but today is not that day.

Also remove this GridItemData::margins field as this isn't required
anymore.
Removes NGGridLayoutAlgorithmMeasuring due to the GridItemData::margins
removal.

Bug: 1045599
Change-Id: I80fffae2f90420123e8b071d0096d00b99612e35
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2720345
Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org>
Reviewed-by: Christian Biesinger <cbiesinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#858447}
  • Loading branch information
@bfgeek
bfgeek authored and Chromium LUCI CQ committed Feb 28, 2021
1 parent 8176650 commit cae822c
Show file tree
Hide file tree
Showing 4 changed files with 13 additions and 190 deletions.
28 changes: 13 additions & 15 deletions third_party/blink/renderer/core/layout/ng/grid/ng_grid_layout_algorithm.cc
View file
Expand Up @@ -592,10 +592,12 @@ LayoutUnit NGGridLayoutAlgorithm::ContributionSizeForGridItem(
break;
}

const auto margins =
ComputeMarginsFor(space, node.Style(), ConstraintSpace());

DCHECK_NE(contribution, kIndefiniteSize);
return contribution + ((track_direction == kForColumns)
? grid_item.margins.InlineSum()
: grid_item.margins.BlockSum());
return contribution + ((track_direction == kForColumns) ? margins.InlineSum()
: margins.BlockSum());
}

void NGGridLayoutAlgorithm::ConstructAndAppendGridItems(
Expand Down Expand Up @@ -850,12 +852,6 @@ NGGridLayoutAlgorithm::GridItemData NGGridLayoutAlgorithm::MeasureGridItem(
.GetPosition(),
/* is_inline_axis */ false, &grid_item.is_block_axis_stretched);

// TODO(ikilpatrick): This is likely incorrect for margins in the
// ComputeMinMaxSizes phase.
grid_item.margins =
ComputePhysicalMargins(item_style, ChildAvailableSize().inline_size)
.ConvertToLogical(ConstraintSpace().GetWritingDirection());

grid_item.item_type = node.IsOutOfFlowPositioned() ? ItemType::kOutOfFlow
: ItemType::kInGridFlow;

Expand Down Expand Up @@ -2116,20 +2112,22 @@ void NGGridLayoutAlgorithm::PlaceGridItems(const GridItems& grid_items,
const auto& physical_fragment =
To<NGPhysicalBoxFragment>(result->PhysicalFragment());

const auto margins =
ComputeMarginsFor(space, grid_item.node.Style(), ConstraintSpace());

// Apply the grid-item's alignment (if any).
NGBoxFragment fragment(ConstraintSpace().GetWritingDirection(),
physical_fragment);
containing_grid_area.offset += LogicalOffset(
AlignmentOffset(containing_grid_area.size.inline_size,
fragment.InlineSize(), grid_item.margins.inline_start,
grid_item.margins.inline_end,
grid_item.inline_axis_alignment),
fragment.InlineSize(), margins.inline_start,
margins.inline_end, grid_item.inline_axis_alignment),
AlignmentOffset(containing_grid_area.size.block_size,
fragment.BlockSize(), grid_item.margins.block_start,
grid_item.margins.block_end,
grid_item.block_axis_alignment));
fragment.BlockSize(), margins.block_start,
margins.block_end, grid_item.block_axis_alignment));

container_builder_.AddChild(physical_fragment, containing_grid_area.offset);
NGBlockNode(grid_item.node).StoreMargins(ConstraintSpace(), margins);

// Compares GridArea objects in row-major grid order for baseline
// precedence. Returns 'true' if |a| < |b| and 'false' otherwise.
Expand Down
2 changes: 0 additions & 2 deletions third_party/blink/renderer/core/layout/ng/grid/ng_grid_layout_algorithm.h
View file
Expand Up @@ -77,8 +77,6 @@ class CORE_EXPORT NGGridLayoutAlgorithm
const NGBlockNode node;
GridArea resolved_position;

NGBoxStrut margins;

AxisEdge inline_axis_alignment;
AxisEdge block_axis_alignment;

Expand Down
167 changes: 0 additions & 167 deletions third_party/blink/renderer/core/layout/ng/grid/ng_grid_layout_algorithm_test.cc
View file
Expand Up @@ -97,14 +97,6 @@ class NGGridLayoutAlgorithmTest
// is a friend of NGGridLayoutAlgorithm but the individual tests are not.
wtf_size_t GridItemCount() { return grid_items_.item_data.size(); }

Vector<LayoutUnit> GridItemInlineMarginSum(
const NGGridLayoutAlgorithm& algorithm) {
Vector<LayoutUnit> results;
for (const auto& item : grid_items_.item_data)
results.push_back(item.margins.InlineSum());
return results;
}

Vector<GridArea> GridItemGridAreas(const NGGridLayoutAlgorithm& algorithm) {
Vector<GridArea> results;
for (const auto& item : grid_items_.item_data)
Expand Down Expand Up @@ -234,165 +226,6 @@ TEST_F(NGGridLayoutAlgorithmTest, NGGridLayoutAlgorithmBaseSetSizes) {
EXPECT_EQ(BaseRowSizeForChild(algorithm, 4), kIndefiniteSize);
}

// Flaky; see https://crbug.com/1146112 for suggestions on fixing.
TEST_F(NGGridLayoutAlgorithmTest, DISABLED_NGGridLayoutAlgorithmMeasuring) {
if (!RuntimeEnabledFeatures::LayoutNGGridEnabled())
return;

LoadAhem();
SetBodyInnerHTML(R"HTML(
<style>
body {
font: 10px/1 Ahem;
}
#grid1 {
display: grid;
width: 200px;
height: 200px;
grid-template-columns: min-content min-content min-content;
grid-template-rows: 100px 100px 100px;
}
/* Basic fixed width specified, evaluates to 150px (50px width + 50px
margin-left + 50px margin-right). */
#cell1 {
grid-column: 1;
grid-row: 1;
width: 50px;
height: 50px;
margin: 50px;
}
/* 100px content, with margin/border/padding. Evaluates to 146px
(100px width + 15px margin-left + 15px margin-righ + 5px border-left +
5px border-right + 3px padding-left + 3px padding-right). */
#cell2 {
grid-column: 2;
grid-row: 1;
min-width: 50px;
height: 100px;
border: 5px solid black;
margin: 15px;
padding: 3px;
}
/* % resolution, needs another pass for the real computed value. For now,
this is evaluated based on the 200px grid content, so it evaluates
to the (currently incorrect) value of 50% of 200px = 100px. */
#cell3 {
grid-column: 3;
grid-row: 1;
width: 50%;
height: 50%;
}
/* 'auto' sizing, with fixed 100px child, evaluates to 100px. */
#cell4 {
grid-column: 1;
grid-row: 2;
width: auto;
height: auto;
}
/* 'auto' sizing replaced content, evaluates to default replaced width of
300px. */
#cell5 {
grid-column: 2;
grid-row: 2;
width: auto;
height: auto;
}
/* 'auto' sizing replaced content, max-width restricts 300px size to
evaluate to 100px. */
#cell6 {
grid-column: 3;
grid-row: 2;
width: auto;
height: auto;
max-width: 100px;
}
/* 'auto' sizing replaced content, min-width expands to 400px, which
in a total offset size of 410 (400px + 5px margin-left + 5px
margin-right). */
#cell7 {
grid-column: 1;
grid-row: 3;
width: auto;
height: auto;
margin: 5px;
min-width: 400px;
}
/* 'auto' sizing with 100px content, min-width and margin evaluates to
100px + 50px margin-left + 50px margin-right = 200px. */
#cell8 {
grid-column: 2;
grid-row: 3;
width: auto;
height: auto;
margin: 50px;
min-width: 100px;
}
/* 'auto' sizing with text content and vertical writing mode. In horizontal
writing-modes, this would be an expected inline size of 40px (at 10px
per character), but since it's set to a vertical writing mode, the
expected width is 10px (at 10px per character). */
#cell9 {
grid-column: 3;
grid-row: 3;
width: auto;
height: auto;
writing-mode: vertical-lr;
}
#block {
width: 100px;
height: 100px;
}
</style>
<div id="grid1">
<div id="cell1">Cell 1</div>
<div id="cell2"><div id="block"></div></div>
<div id="cell3">Cell 3</div>
<div id="cell4"><div id="block"></div></div>
<svg id="cell5">
<rect width="100%" height="100%" fill="blue" />
</svg>
<svg id="cell6">
<rect width="100%" height="100%" fill="blue" />
</svg>
<svg id="cell7">
<rect width="100%" height="100%" fill="blue" />
</svg>
<div id="cell8"><div id="block"></div></div>
<div id="cell9">Text</div>
</div>
)HTML");

NGBlockNode node(GetLayoutBoxByElementId("grid1"));

NGConstraintSpace space = ConstructBlockLayoutTestConstraintSpace(
{WritingMode::kHorizontalTb, TextDirection::kLtr},
LogicalSize(LayoutUnit(200), LayoutUnit(200)),
/* stretch_inline_size_if_auto */ true,
/* is_new_formatting_context */ true);

NGFragmentGeometry fragment_geometry =
CalculateInitialFragmentGeometry(space, node);

NGGridLayoutAlgorithm algorithm({node, fragment_geometry, space});
EXPECT_EQ(GridItemCount(), 0U);
BuildGridItemsAndTrackCollections(algorithm);
EXPECT_EQ(GridItemCount(), 9U);

Vector<LayoutUnit> actual_inline_margin_sums =
GridItemInlineMarginSum(algorithm);
EXPECT_EQ(GridItemCount(), actual_inline_margin_sums.size());

LayoutUnit expected_inline_margin_sums[] = {
LayoutUnit(100), LayoutUnit(30), LayoutUnit(0),
LayoutUnit(0), LayoutUnit(0), LayoutUnit(0),
LayoutUnit(10), LayoutUnit(100), LayoutUnit(0)};

for (size_t i = 0; i < GridItemCount(); ++i) {
EXPECT_EQ(actual_inline_margin_sums[i], expected_inline_margin_sums[i])
<< " index: " << i;
}
}

TEST_F(NGGridLayoutAlgorithmTest, NGGridLayoutAlgorithmRanges) {
if (!RuntimeEnabledFeatures::LayoutNGGridEnabled())
return;
Expand Down
6 changes: 0 additions & 6 deletions third_party/blink/web_tests/TestExpectations
View file
Expand Up @@ -3626,12 +3626,6 @@ crbug.com/1045599 virtual/layout-ng-grid/external/wpt/css/css-grid/grid-items/gr
crbug.com/1045599 virtual/layout-ng-grid/external/wpt/css/css-grid/grid-items/grid-item-percentage-sizes-003.html [ Failure ]
crbug.com/1045599 virtual/layout-ng-grid/external/wpt/css/css-grid/grid-items/grid-item-overflow-auto-max-height-percentage.html [ Failure ]
crbug.com/1045599 virtual/layout-ng-grid/external/wpt/css/css-grid/grid-items/grid-minimum-size-grid-items-018.html [ Failure ]
crbug.com/1045599 virtual/layout-ng-grid/external/wpt/css/css-grid/grid-items/grid-items-percentage-margins-001.html [ Failure ]
crbug.com/1045599 virtual/layout-ng-grid/external/wpt/css/css-grid/grid-items/grid-items-percentage-margins-002.html [ Failure ]
crbug.com/1045599 virtual/layout-ng-grid/external/wpt/css/css-grid/grid-items/grid-items-percentage-margins-vertical-lr-001.html [ Failure ]
crbug.com/1045599 virtual/layout-ng-grid/external/wpt/css/css-grid/grid-items/grid-items-percentage-margins-vertical-lr-002.html [ Failure ]
crbug.com/1045599 virtual/layout-ng-grid/external/wpt/css/css-grid/grid-items/grid-items-percentage-margins-vertical-rl-001.html [ Failure ]
crbug.com/1045599 virtual/layout-ng-grid/external/wpt/css/css-grid/grid-items/grid-items-percentage-margins-vertical-rl-002.html [ Failure ]
crbug.com/1045599 virtual/layout-ng-grid/external/wpt/css/css-grid/grid-items/grid-items-relative-offsets-002.html [ Failure ]
crbug.com/1045599 virtual/layout-ng-grid/external/wpt/css/css-grid/grid-items/grid-minimum-size-grid-items-021.html [ Failure ]
crbug.com/1045599 virtual/layout-ng-grid/external/wpt/css/css-grid/grid-items/grid-minimum-size-grid-items-022.html [ Failure ]
Expand Down

0 comments on commit cae822c

Please sign in to comment.