Commit d123c14a authored by Morten Stenshorne's avatar Morten Stenshorne Committed by Commit Bot

Reland: Improve used column count allowance calculation.

Better detection of "legitimate" reasons to use many columns. Let the
allowance be based on the amount of "content". The more content (lines
or boxes) inside a multicol container, the more columns will be allowed.
We used to base it on the column height, but it turned out that it
wasn't good enough. Also, there are legitimate reasons to use more than
500 columns in some cases, so increase the limit to 2000. With the
current implementation, this may get very slow, because of quadratic
performance complexity, presumably somewhere in paint code.

One multicol regressed in LayoutNG. Not sure why. It shouldn't have
passed in the first place.

This CL was previously landed as
https://chromium-review.googlesource.com/c/chromium/src/+/1057629 and
then reverted because of test flakiness. This new CL has reduced the
number of columns in those tests, so that the tests don't occasionally
time out on slow bots.

Bug: 808189,845155
Change-Id: I55d560faba73128518a689ae38f628ee89e58568
Reviewed-on: https://chromium-review.googlesource.com/1068929
Commit-Queue: Emil A Eklund <eae@chromium.org>
Reviewed-by: default avatarChris Harrelson <chrishtr@chromium.org>
Reviewed-by: default avatarEmil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560820}
parent 39e95ef8
...@@ -720,6 +720,7 @@ crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/client-rects-cross ...@@ -720,6 +720,7 @@ crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/client-rects-cross
crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/client-rects-crossing-boundaries.html [ Failure ] crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/client-rects-crossing-boundaries.html [ Failure ]
crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/client-rects-rtl.html [ Failure ] crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/client-rects-rtl.html [ Failure ]
crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/client-rects.html [ Failure ] crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/client-rects.html [ Failure ]
crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/column-clamping.html [ Failure ]
crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/column-count-with-rules.html [ Failure ] crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/column-count-with-rules.html [ Failure ]
crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/column-rules.html [ Failure ] crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/column-rules.html [ Failure ]
crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/columns-shorthand-parsing.html [ Failure ] crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/columns-shorthand-parsing.html [ Failure ]
......
<!DOCTYPE html> <!DOCTYPE html>
<p>The word "PASS" should be seen below.</p> <p>The word "PASS" should be seen below.</p>
<div id="scrollable" style="overflow:hidden; columns:1; column-gap:0; width:100px; height:100px;"> <div id="scrollable" style="overflow:hidden; columns:1; column-gap:0; width:100px; height:100px;">
<div style="height:9000px;"></div> <div style="height:900px;"></div>
PASS PASS
</div> </div>
<script> <script>
......
...@@ -1461,4 +1461,45 @@ void LayoutMultiColumnFlowThread::RestoreMultiColumnLayoutState( ...@@ -1461,4 +1461,45 @@ void LayoutMultiColumnFlowThread::RestoreMultiColumnLayoutState(
last_set_worked_on_ = state.ColumnSet(); last_set_worked_on_ = state.ColumnSet();
} }
unsigned LayoutMultiColumnFlowThread::CalculateActualColumnCountAllowance()
const {
// To avoid performance problems, limit the maximum number of columns. Try to
// identify legitimate reasons for creating many columns, and allow many
// columns in such cases. The amount of "content" will determine the
// allowance.
unsigned allowance = 0;
// This isn't a particularly clever algorithm. For example, we don't account
// for parallel flows (absolute positioning, floats, visible overflow, table
// cells, flex items). We just generously add everything together.
for (const LayoutObject* descendant = this; descendant;) {
bool examine_children = false;
if (descendant->IsBox() && !descendant->IsInline() &&
!ToLayoutBox(descendant)->IsWritingModeRoot()) {
// Give one point to any kind of block level content.
allowance++;
if (descendant->IsLayoutBlockFlow() && descendant->ChildrenInline()) {
// It's a block-level block container in the same writing mode, and it
// has inline children. Count the lines and add it to the allowance.
allowance += ToLayoutBlockFlow(descendant)->LineCount();
} else {
// We could examine other types of layout modes (tables, flexbox, etc.)
// as well, but then again, that might be overkill. Just enter and see
// what we find.
examine_children = true;
}
}
if (allowance >= ColumnCountClampMax())
return ColumnCountClampMax();
descendant = examine_children
? descendant->NextInPreOrder(this)
: descendant->NextInPreOrderAfterChildren(this);
}
DCHECK_LE(allowance, ColumnCountClampMax());
return std::max(allowance, ColumnCountClampMin());
}
} // namespace blink } // namespace blink
...@@ -190,6 +190,18 @@ class CORE_EXPORT LayoutMultiColumnFlowThread : public LayoutFlowThread, ...@@ -190,6 +190,18 @@ class CORE_EXPORT LayoutMultiColumnFlowThread : public LayoutFlowThread,
unsigned ColumnCount() const { return column_count_; } unsigned ColumnCount() const { return column_count_; }
// Calculate and return the actual column count allowance. This method should
// only be called when we ended up with an actual column count larger than
// ColumnCountClampMin() in some fragmentainer group.
unsigned CalculateActualColumnCountAllowance() const;
// Any actual column count value lower than this will be allowed
// unconditionally.
static unsigned ColumnCountClampMin() { return 10; }
// The maximum actual column count we're going to allow.
static unsigned ColumnCountClampMax() { return 2000; }
// Total height available to columns and spanners. This is the multicol // Total height available to columns and spanners. This is the multicol
// container's content box logical height, or 0 if auto. // container's content box logical height, or 0 if auto.
LayoutUnit ColumnHeightAvailable() const { return column_height_available_; } LayoutUnit ColumnHeightAvailable() const { return column_height_available_; }
......
...@@ -62,6 +62,7 @@ LayoutUnit MultiColumnFragmentainerGroup::LogicalHeightInFlowThreadAt( ...@@ -62,6 +62,7 @@ LayoutUnit MultiColumnFragmentainerGroup::LogicalHeightInFlowThreadAt(
} }
void MultiColumnFragmentainerGroup::ResetColumnHeight() { void MultiColumnFragmentainerGroup::ResetColumnHeight() {
actual_column_count_allowance_ = 0;
max_logical_height_ = CalculateMaxColumnHeight(); max_logical_height_ = CalculateMaxColumnHeight();
LayoutMultiColumnFlowThread* flow_thread = LayoutMultiColumnFlowThread* flow_thread =
...@@ -137,6 +138,24 @@ bool MultiColumnFragmentainerGroup::RecalculateColumnHeight( ...@@ -137,6 +138,24 @@ bool MultiColumnFragmentainerGroup::RecalculateColumnHeight(
// height. // height.
is_logical_height_known_ = true; is_logical_height_known_ = true;
unsigned column_count = UnclampedActualColumnCount();
if (column_count > LayoutMultiColumnFlowThread::ColumnCountClampMax() ||
(column_count > LayoutMultiColumnFlowThread::ColumnCountClampMin() &&
column_count > column_set_.UsedColumnCount())) {
// That's a lot of columns! We have either exceeded the maximum value, or we
// have overflowing columns, and the proposed count is within clamping
// range. Calculate allowance to make sure we have a legitimate reason for
// it, or else clamp it. We have quadratic performance complexity for
// painting columns.
if (!actual_column_count_allowance_) {
const auto* flow_thread = column_set_.MultiColumnFlowThread();
unsigned allowance = flow_thread->CalculateActualColumnCountAllowance();
DCHECK_GE(allowance, LayoutMultiColumnFlowThread::ColumnCountClampMin());
DCHECK_LE(allowance, LayoutMultiColumnFlowThread::ColumnCountClampMax());
actual_column_count_allowance_ = allowance;
}
}
if (logical_height_ == old_column_height) if (logical_height_ == old_column_height)
return false; // No change. We're done. return false; // No change. We're done.
...@@ -309,36 +328,9 @@ LayoutRect MultiColumnFragmentainerGroup::CalculateOverflow() const { ...@@ -309,36 +328,9 @@ LayoutRect MultiColumnFragmentainerGroup::CalculateOverflow() const {
} }
unsigned MultiColumnFragmentainerGroup::ActualColumnCount() const { unsigned MultiColumnFragmentainerGroup::ActualColumnCount() const {
// We must always return a value of 1 or greater. Column count = 0 is a unsigned count = UnclampedActualColumnCount();
// meaningless situation, and will confuse and cause problems in other parts if (actual_column_count_allowance_)
// of the code. count = std::min(count, actual_column_count_allowance_);
if (!IsLogicalHeightKnown())
return 1;
// Our flow thread portion determines our column count. We have as many
// columns as needed to fit all the content.
LayoutUnit flow_thread_portion_height = LogicalHeightInFlowThread();
if (!flow_thread_portion_height)
return 1;
LayoutUnit column_height = ColumnLogicalHeight();
unsigned count = (flow_thread_portion_height / column_height).Floor();
// flowThreadPortionHeight may be saturated, so detect the remainder manually.
if (count * column_height < flow_thread_portion_height)
count++;
static const unsigned kColumnCountClampMin = 10;
static const unsigned kColumnCountClampMax = 500;
if (count > kColumnCountClampMin) {
// To avoid performance problems, limit the maximum number of columns. Try
// to identify legitimate reasons for creating many columns, and allow many
// columns in such cases. We currently use a function of column height to
// determine this (limit the column count to the column height in pixels,
// but never clamp to less than 10 columns).
unsigned max_count = column_height.ToInt();
count = std::min(count, std::max(std::min(max_count, kColumnCountClampMax),
kColumnCountClampMin));
}
DCHECK_GE(count, 1u); DCHECK_GE(count, 1u);
return count; return count;
} }
...@@ -648,6 +640,28 @@ void MultiColumnFragmentainerGroup::ColumnIntervalForVisualRect( ...@@ -648,6 +640,28 @@ void MultiColumnFragmentainerGroup::ColumnIntervalForVisualRect(
DCHECK_LE(first_column, last_column); DCHECK_LE(first_column, last_column);
} }
unsigned MultiColumnFragmentainerGroup::UnclampedActualColumnCount() const {
// We must always return a value of 1 or greater. Column count = 0 is a
// meaningless situation, and will confuse and cause problems in other parts
// of the code.
if (!IsLogicalHeightKnown())
return 1;
// Our flow thread portion determines our column count. We have as many
// columns as needed to fit all the content.
LayoutUnit flow_thread_portion_height = LogicalHeightInFlowThread();
if (!flow_thread_portion_height)
return 1;
LayoutUnit column_height = ColumnLogicalHeight();
unsigned count = (flow_thread_portion_height / column_height).Floor();
// flowThreadPortionHeight may be saturated, so detect the remainder manually.
if (count * column_height < flow_thread_portion_height)
count++;
DCHECK_GE(count, 1u);
return count;
}
MultiColumnFragmentainerGroupList::MultiColumnFragmentainerGroupList( MultiColumnFragmentainerGroupList::MultiColumnFragmentainerGroupList(
LayoutMultiColumnSet& column_set) LayoutMultiColumnSet& column_set)
: column_set_(column_set) { : column_set_(column_set) {
......
...@@ -193,6 +193,8 @@ class CORE_EXPORT MultiColumnFragmentainerGroup { ...@@ -193,6 +193,8 @@ class CORE_EXPORT MultiColumnFragmentainerGroup {
// to a column, even if said point is not inside any of the columns. // to a column, even if said point is not inside any of the columns.
unsigned ColumnIndexAtVisualPoint(const LayoutPoint& visual_point) const; unsigned ColumnIndexAtVisualPoint(const LayoutPoint& visual_point) const;
unsigned UnclampedActualColumnCount() const;
const LayoutMultiColumnSet& column_set_; const LayoutMultiColumnSet& column_set_;
LayoutUnit logical_top_; LayoutUnit logical_top_;
...@@ -207,6 +209,8 @@ class CORE_EXPORT MultiColumnFragmentainerGroup { ...@@ -207,6 +209,8 @@ class CORE_EXPORT MultiColumnFragmentainerGroup {
// Maximum logical height allowed. // Maximum logical height allowed.
LayoutUnit max_logical_height_; LayoutUnit max_logical_height_;
unsigned actual_column_count_allowance_ = 0;
bool is_logical_height_known_ = false; bool is_logical_height_known_ = false;
}; };
......
...@@ -100,9 +100,9 @@ TEST_F(MultiColumnFragmentainerGroupTest, ...@@ -100,9 +100,9 @@ TEST_F(MultiColumnFragmentainerGroupTest,
// The following test tests that we restrict actual column count, to not run // The following test tests that we restrict actual column count, to not run
// into unnecessary performance problems. The code that applies this limitation // into unnecessary performance problems. The code that applies this limitation
// is in MultiColumnFragmentainerGroup::ActualColumnCount(). // is in MultiColumnFragmentainerGroup::ActualColumnCount().
TEST_F(MultiColumnFragmentainerGroupTest, ShortColumnTallContent) { TEST_F(MultiColumnFragmentainerGroupTest, TallEmptyContent) {
SetBodyInnerHTML(R"HTML( SetBodyInnerHTML(R"HTML(
<div id="multicol" style="columns:3; column-gap:1px; width:101px; height:1px;"> <div id="multicol" style="columns:3; column-gap:1px; width:101px; line-height:50px; height:200px;">
<div style="height:1000000px;"></div> <div style="height:1000000px;"></div>
</div> </div>
)HTML"); )HTML");
...@@ -115,23 +115,28 @@ TEST_F(MultiColumnFragmentainerGroupTest, ShortColumnTallContent) { ...@@ -115,23 +115,28 @@ TEST_F(MultiColumnFragmentainerGroupTest, ShortColumnTallContent) {
const auto& fragmentainer_group = const auto& fragmentainer_group =
ToLayoutMultiColumnSet(column_set)->FirstFragmentainerGroup(); ToLayoutMultiColumnSet(column_set)->FirstFragmentainerGroup();
EXPECT_EQ(fragmentainer_group.ActualColumnCount(), 10U); EXPECT_EQ(fragmentainer_group.ActualColumnCount(), 10U);
EXPECT_EQ(fragmentainer_group.GroupLogicalHeight(), LayoutUnit(1)); EXPECT_EQ(fragmentainer_group.GroupLogicalHeight(), LayoutUnit(200));
auto overflow = ToLayoutBox(multicol)->LayoutOverflowRect(); auto overflow = ToLayoutBox(multicol)->LayoutOverflowRect();
EXPECT_EQ(ToLayoutBox(multicol)->LogicalWidth(), LayoutUnit(101)); EXPECT_EQ(ToLayoutBox(multicol)->LogicalWidth(), LayoutUnit(101));
EXPECT_EQ(ToLayoutBox(multicol)->LogicalHeight(), LayoutUnit(1)); EXPECT_EQ(ToLayoutBox(multicol)->LogicalHeight(), LayoutUnit(200));
EXPECT_EQ(overflow.Width(), LayoutUnit(339)); EXPECT_EQ(overflow.Width(), LayoutUnit(339));
EXPECT_EQ(overflow.Height(), LayoutUnit(999991)); EXPECT_EQ(overflow.Height(), LayoutUnit(998200));
} }
// The following test tests that we restrict actual column count, to not run // The following test tests that we DON'T restrict actual column count, when
// into unnecessary performance problems. The code that applies this limitation // there's a legitimate reason to use many columns. The code that checks the
// is in MultiColumnFragmentainerGroup::ActualColumnCount(). // allowance and potentially applies this limitation is in
TEST_F(MultiColumnFragmentainerGroupTest, MediumTallColumnTallContent) { // MultiColumnFragmentainerGroup::ActualColumnCount().
SetBodyInnerHTML(R"HTML( TEST_F(MultiColumnFragmentainerGroupTest, LotsOfContent) {
<div id="multicol" style="columns:3; column-gap:1px; width:101px; height:100px;"> StringBuilder builder;
<div style="height:1000000px;"></div> builder.Append(
</div> "<div id='multicol' style='columns:3; column-gap:1px; width:101px; "
)HTML"); "line-height:50px; orphans:1; widows:1; height:60px;'>");
for (int i = 0; i < 100; i++)
builder.Append("line<br>");
builder.Append("</div>");
String html;
SetBodyInnerHTML(builder.ToString());
const auto* multicol = GetLayoutObjectByElementId("multicol"); const auto* multicol = GetLayoutObjectByElementId("multicol");
ASSERT_TRUE(multicol); ASSERT_TRUE(multicol);
ASSERT_TRUE(multicol->IsLayoutBlockFlow()); ASSERT_TRUE(multicol->IsLayoutBlockFlow());
...@@ -141,23 +146,28 @@ TEST_F(MultiColumnFragmentainerGroupTest, MediumTallColumnTallContent) { ...@@ -141,23 +146,28 @@ TEST_F(MultiColumnFragmentainerGroupTest, MediumTallColumnTallContent) {
const auto& fragmentainer_group = const auto& fragmentainer_group =
ToLayoutMultiColumnSet(column_set)->FirstFragmentainerGroup(); ToLayoutMultiColumnSet(column_set)->FirstFragmentainerGroup();
EXPECT_EQ(fragmentainer_group.ActualColumnCount(), 100U); EXPECT_EQ(fragmentainer_group.ActualColumnCount(), 100U);
EXPECT_EQ(fragmentainer_group.GroupLogicalHeight(), LayoutUnit(100)); EXPECT_EQ(fragmentainer_group.GroupLogicalHeight(), LayoutUnit(60));
auto overflow = ToLayoutBox(multicol)->LayoutOverflowRect(); auto overflow = ToLayoutBox(multicol)->LayoutOverflowRect();
EXPECT_EQ(ToLayoutBox(multicol)->LogicalWidth(), LayoutUnit(101)); EXPECT_EQ(ToLayoutBox(multicol)->LogicalWidth(), LayoutUnit(101));
EXPECT_EQ(ToLayoutBox(multicol)->LogicalHeight(), LayoutUnit(100)); EXPECT_EQ(ToLayoutBox(multicol)->LogicalHeight(), LayoutUnit(60));
EXPECT_EQ(overflow.Width(), LayoutUnit(3399)); EXPECT_EQ(overflow.Width(), LayoutUnit(3399));
EXPECT_EQ(overflow.Height(), LayoutUnit(990100)); EXPECT_EQ(overflow.Height(), LayoutUnit(60));
} }
// The following test tests that we restrict actual column count, to not run // The following test tests that we DON'T restrict actual column count, when
// into unnecessary performance problems. The code that applies this limitation // there's a legitimate reason to use many columns. The code that checks the
// is in MultiColumnFragmentainerGroup::ActualColumnCount(). // allowance and potentially applies this limitation is in
TEST_F(MultiColumnFragmentainerGroupTest, TallColumnTallContent) { // MultiColumnFragmentainerGroup::ActualColumnCount().
SetBodyInnerHTML(R"HTML( TEST_F(MultiColumnFragmentainerGroupTest, LotsOfNestedBlocksWithText) {
<div id="multicol" style="columns:3; column-gap:1px; width:101px; height:1000px;"> StringBuilder builder;
<div style="height:1000000px;"></div> builder.Append(
</div> "<div id='multicol' style='columns:3; column-gap:1px; width:101px; "
)HTML"); "line-height:50px; height:200px;'>");
for (int i = 0; i < 1000; i++)
builder.Append("<div><div><div>line</div></div></div>");
builder.Append("</div>");
String html;
SetBodyInnerHTML(builder.ToString());
const auto* multicol = GetLayoutObjectByElementId("multicol"); const auto* multicol = GetLayoutObjectByElementId("multicol");
ASSERT_TRUE(multicol); ASSERT_TRUE(multicol);
ASSERT_TRUE(multicol->IsLayoutBlockFlow()); ASSERT_TRUE(multicol->IsLayoutBlockFlow());
...@@ -166,13 +176,44 @@ TEST_F(MultiColumnFragmentainerGroupTest, TallColumnTallContent) { ...@@ -166,13 +176,44 @@ TEST_F(MultiColumnFragmentainerGroupTest, TallColumnTallContent) {
ASSERT_TRUE(column_set->IsLayoutMultiColumnSet()); ASSERT_TRUE(column_set->IsLayoutMultiColumnSet());
const auto& fragmentainer_group = const auto& fragmentainer_group =
ToLayoutMultiColumnSet(column_set)->FirstFragmentainerGroup(); ToLayoutMultiColumnSet(column_set)->FirstFragmentainerGroup();
EXPECT_EQ(fragmentainer_group.ActualColumnCount(), 500U); EXPECT_EQ(fragmentainer_group.ActualColumnCount(), 250U);
EXPECT_EQ(fragmentainer_group.GroupLogicalHeight(), LayoutUnit(1000)); EXPECT_EQ(fragmentainer_group.GroupLogicalHeight(), LayoutUnit(200));
auto overflow = ToLayoutBox(multicol)->LayoutOverflowRect(); auto overflow = ToLayoutBox(multicol)->LayoutOverflowRect();
EXPECT_EQ(ToLayoutBox(multicol)->LogicalWidth(), LayoutUnit(101)); EXPECT_EQ(ToLayoutBox(multicol)->LogicalWidth(), LayoutUnit(101));
EXPECT_EQ(ToLayoutBox(multicol)->LogicalHeight(), LayoutUnit(1000)); EXPECT_EQ(ToLayoutBox(multicol)->LogicalHeight(), LayoutUnit(200));
EXPECT_EQ(overflow.Width(), LayoutUnit(16999)); EXPECT_EQ(overflow.Width(), LayoutUnit(8499));
EXPECT_EQ(overflow.Height(), LayoutUnit(501000)); EXPECT_EQ(overflow.Height(), LayoutUnit(200));
}
// The following test tests that we DON'T restrict actual column count, when
// there's a legitimate reason to use many columns. The code that checks the
// allowance and potentially applies this limitation is in
// MultiColumnFragmentainerGroup::ActualColumnCount().
TEST_F(MultiColumnFragmentainerGroupTest, NestedBlocksWithLotsOfContent) {
StringBuilder builder;
builder.Append(
"<div id='multicol' style='columns:3; column-gap:1px; width:101px; "
"line-height:50px; orphans:1; widows:1; height:60px;'><div><div><div>");
for (int i = 0; i < 100; i++)
builder.Append("line<br>");
builder.Append("</div></div></div></div>");
String html;
SetBodyInnerHTML(builder.ToString());
const auto* multicol = GetLayoutObjectByElementId("multicol");
ASSERT_TRUE(multicol);
ASSERT_TRUE(multicol->IsLayoutBlockFlow());
const auto* column_set = multicol->SlowLastChild();
ASSERT_TRUE(column_set);
ASSERT_TRUE(column_set->IsLayoutMultiColumnSet());
const auto& fragmentainer_group =
ToLayoutMultiColumnSet(column_set)->FirstFragmentainerGroup();
EXPECT_EQ(fragmentainer_group.ActualColumnCount(), 100U);
EXPECT_EQ(fragmentainer_group.GroupLogicalHeight(), LayoutUnit(60));
auto overflow = ToLayoutBox(multicol)->LayoutOverflowRect();
EXPECT_EQ(ToLayoutBox(multicol)->LogicalWidth(), LayoutUnit(101));
EXPECT_EQ(ToLayoutBox(multicol)->LogicalHeight(), LayoutUnit(60));
EXPECT_EQ(overflow.Width(), LayoutUnit(3399));
EXPECT_EQ(overflow.Height(), LayoutUnit(60));
} }
} // anonymous namespace } // anonymous namespace
......
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