Skip to content

GitLab

T
tangled
Commit 14ae5724 authored by Javier Fernandez's avatar Javier Fernandez Committed by Commit Bot
Browse files
Options

[css-grid] Avoid persistent items list to perform the pre-layout 

We defined two LayoutBox elements list to identify grid items that need
to perform a pre-layout: orthogonal and baseline items. We were
creating these lists as part of the PlaceItemsOnGrid logic.

This design has 2 main problems that lead to bug 855844, and likely
many other:
  * The PlaceItemsOnGrid function is not executed completely if the
grid is not dirty.
  * There might be items in the list that should not be there due to
style changes; also, some should have been added for the same reason.

The simplest and safest approach is to avoid such lists and identify
the items needing pre-layout during the first steps of the grid's
layout logic.

Since style changes affecting either the writing-mode or the Self
Alignment properties of a grid item will force a re-layout of the grid
it belongs to, we can be sure that all the grid items will perform the
pre-layout, if they needed to.

Bug: 855844
Change-Id: I396524b0fc6a4816bcb7962997f129a325c04934
Reviewed-on: https://chromium-review.googlesource.com/1122623
Commit-Queue: Javier Fernandez <jfernandez@igalia.com>
Reviewed-by: default avatarManuel Rego Casasnovas <rego@igalia.com>
Cr-Commit-Position: refs/heads/master@{#572162}
parent 07c50437
Hide whitespace changes
Inline Side-by-side
Showing with 66 additions and 57 deletions
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-self-baseline-followed-by-item-style-change-should-not-crash.html 0 → 100644
View file @ 14ae5724
<!doctype html>
<script src="../../resources/testharness.js"></script>
<script src="../../resources/testharnessreport.js"></script>
<div style="display: grid;">
<div id="item1"></div>
</div>
<div style="display: grid;">
<div id="item2" style="align-self: baseline"></div>
</div>
<div style="display: grid;">
<div id="item3" style="writing-mode: vertical-lr;"></div>
</div>
<script>
test(() => {
document.body.offsetLeft;
var item = document.getElementById("item1");
item.style.alignSelf = "baseline";
assert_equals(item.style.alignSelf, "baseline");
}, "No crash or assertion failure when changing align-self from 'auto' to 'baseline'.");
test(() => {
document.body.offsetLeft;
var item = document.getElementById("item2");
item.style.alignSelf = "start";
assert_equals(item.style.alignSelf, "start");
}, "No crash or assertion failure when changing align-self from 'baseline' to 'start'.");
test(() => {
document.body.offsetLeft;
var item = document.getElementById("item3");
item.style.writingMode = "horizontal-tb";
assert_equals(item.style.writingMode, "horizontal-tb");
}, "No crash or assertion failure when item's writing-mode changes from orthogonal to parallel.");
</script>
third_party/blink/renderer/core/layout/grid.cc
View file @ 14ae5724
......@@ -159,8 +159,6 @@ void Grid::SetNeedsItemsPlacement(bool needs_items_placement) {
auto_repeat_rows_ = 0;
auto_repeat_empty_columns_ = nullptr;
auto_repeat_empty_rows_ = nullptr;
baseline_grid_items_.resize(0);
orthogonal_grid_items_.resize(0);
}
Grid::GridIterator::GridIterator(GridTrackSizingDirection direction,
......
third_party/blink/renderer/core/layout/grid.h
View file @ 14ae5724
......@@ -46,22 +46,6 @@ class Grid {
// Note that out of flow children are not grid items.
bool HasGridItems() const { return !grid_item_area_.IsEmpty(); }
void AddBaselineAlignedItem(LayoutBox& item) {
baseline_grid_items_.push_back(&item);
}
void AddOrthogonalItem(LayoutBox& item) {
orthogonal_grid_items_.push_back(&item);
}
bool HasAnyOrthogonalGridItem() const {
return !orthogonal_grid_items_.IsEmpty();
}
const Vector<LayoutBox*>& OrthogonalGridItems() const {
return orthogonal_grid_items_;
}
const Vector<LayoutBox*>& BaselineGridItems() const {
return baseline_grid_items_;
}
GridArea GridItemArea(const LayoutBox&) const;
void SetGridItemArea(const LayoutBox&, GridArea);
......@@ -149,9 +133,6 @@ class Grid {
std::unique_ptr<OrderedTrackIndexSet> auto_repeat_empty_columns_{nullptr};
std::unique_ptr<OrderedTrackIndexSet> auto_repeat_empty_rows_{nullptr};
Vector<LayoutBox*> orthogonal_grid_items_;
Vector<LayoutBox*> baseline_grid_items_;
};
class VectorGrid final : public Grid {
......
third_party/blink/renderer/core/layout/layout_grid.cc
View file @ 14ae5724
......@@ -244,15 +244,12 @@ void LayoutGrid::RepeatTracksSizingIfNeeded(
// In orthogonal flow cases column track's size is determined by using the
// computed row track's size, which it was estimated during the first cycle of
// the sizing algorithm.
bool has_any_orthogonal =
track_sizing_algorithm_.GetGrid().HasAnyOrthogonalGridItem();
// TODO (lajava): these are just some of the cases which may require
// a new cycle of the sizing algorithm; there may be more. In addition, not
// all the cases with orthogonal flows require this extra cycle; we need a
// more specific condition to detect whether child's min-content contribution
// has changed or not.
if (!has_any_orthogonal)
if (!has_any_orthogonal_item_)
return;
// TODO (lajava): Whenever the min-content contribution of a grid item changes
......@@ -292,9 +289,13 @@ void LayoutGrid::UpdateBlockLayout(bool relayout_children) {
// we need to clear any override height set previously, so it doesn't
// interfere in current layout execution.
// Grid never uses the override width, that's why we don't need to clear it.
has_any_orthogonal_item_ = false;
for (auto* child = FirstInFlowChildBox(); child;
child = child->NextInFlowSiblingBox())
child = child->NextInFlowSiblingBox()) {
child->ClearOverrideLogicalHeight();
if (GridLayoutUtils::IsOrthogonalChild(*this, *child))
has_any_orthogonal_item_ = true;
}
UpdateLogicalWidth();
......@@ -773,7 +774,6 @@ void LayoutGrid::PlaceItemsOnGrid(
#if DCHECK_IS_ON()
DCHECK(!grid.HasAnyGridItemPaintOrder());
#endif
DCHECK(!grid.HasAnyOrthogonalGridItem());
size_t child_index = 0;
for (LayoutBox* child = grid.GetOrderIterator().First(); child;
child = grid.GetOrderIterator().Next()) {
......@@ -788,10 +788,6 @@ void LayoutGrid::PlaceItemsOnGrid(
if (!child->HasOverrideContainingBlockContentLogicalHeight())
child->SetOverrideContainingBlockContentLogicalHeight(LayoutUnit(-1));
if (GridLayoutUtils::IsOrthogonalChild(*this, *child))
grid.AddOrthogonalItem(*child);
if (IsBaselineAlignmentForChild(*child))
grid.AddBaselineAlignedItem(*child);
grid.SetGridItemPaintOrder(*child, child_index++);
GridArea area = grid.GridItemArea(*child);
......@@ -865,34 +861,35 @@ void LayoutGrid::PerformGridItemsPreLayout(
DCHECK(!algorithm.GetGrid().NeedsItemsPlacement());
if (!GetDocument().View()->IsInPerformLayout())
return;
// Blink does a pre-layout of all the orthogonal boxes in the layout
// tree (see how LocalFrameView::PerformLayout calls its
// LayoutOrthogonalWritingModeRoots function). However, grid items
// don't participate in this process (see the function
// PrepareOrthogonalWritingModeRootForLayout) because it's useless
// and even wrong if they don't have their corresponding Grid Area.
// TODO(jfernandez): Consider rafactoring this code with
// LocalFrameView::LayoutOrthogonalWritingModeRoots
for (auto* item : algorithm.GetGrid().OrthogonalGridItems()) {
DCHECK(GridLayoutUtils::IsOrthogonalChild(*this, *item));
if (PrepareOrthogonalWritingModeRootForLayout(*item)) {
UpdateGridAreaLogicalSize(
*item, algorithm.EstimatedGridAreaBreadthForChild(*item));
item->LayoutIfNeeded();
for (auto* child = FirstInFlowChildBox(); child;
child = child->NextInFlowSiblingBox()) {
// Blink does a pre-layout of all the orthogonal boxes in the layout
// tree (see how LocalFrameView::PerformLayout calls its
// LayoutOrthogonalWritingModeRoots function). However, grid items
// don't participate in this process (see the function
// PrepareOrthogonalWritingModeRootForLayout) because it's useless
// and even wrong if they don't have their corresponding Grid Area.
// TODO(jfernandez): Consider rafactoring this code with
// LocalFrameView::LayoutOrthogonalWritingModeRoots
if (GridLayoutUtils::IsOrthogonalChild(*this, *child)) {
if (PrepareOrthogonalWritingModeRootForLayout(*child)) {
UpdateGridAreaLogicalSize(
*child, algorithm.EstimatedGridAreaBreadthForChild(*child));
child->LayoutIfNeeded();
continue;
}
}
}
// We need to layout the item to know whether it must synthesize its
// baseline or not, which may imply a cyclic sizing dependency.
// TODO (jfernandez): Can we avoid it ?
for (auto* item : algorithm.GetGrid().BaselineGridItems()) {
DCHECK(IsBaselineAlignmentForChild(*item));
if (GridLayoutUtils::IsOrthogonalChild(*this, *item))
continue;
if (item->HasRelativeLogicalWidth() || item->HasRelativeLogicalHeight()) {
UpdateGridAreaLogicalSize(
*item, algorithm.EstimatedGridAreaBreadthForChild(*item));
// We need to layout the item to know whether it must synthesize its
// baseline or not, which may imply a cyclic sizing dependency.
// TODO (jfernandez): Can we avoid it ?
if (IsBaselineAlignmentForChild(*child)) {
if (child->HasRelativeLogicalWidth() ||
child->HasRelativeLogicalHeight()) {
UpdateGridAreaLogicalSize(
*child, algorithm.EstimatedGridAreaBreadthForChild(*child));
}
child->LayoutIfNeeded();
}
item->LayoutIfNeeded();
}
}
......
third_party/blink/renderer/core/layout/layout_grid.h
View file @ 14ae5724
......@@ -333,6 +333,7 @@ class LayoutGrid final : public LayoutBlock {
LayoutUnit min_content_height_{-1};
LayoutUnit max_content_height_{-1};
bool has_any_orthogonal_item_{false};
base::Optional<bool> has_definite_logical_height_;
};
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment