Skip to content

Commit

Permalink
[FlexNG] Don't clamp flex row-gaps when fragmenting
Browse files Browse the repository at this point in the history
We don't clamp flex item margins when an item breaks before to more
accurately handle alignment. Thus, we should similarly do the same
for row gaps. Currently, we will clamp row gaps (or at least part
of row gaps - there were other bugs uncovered while investigating),
so this CL updates the logic to correctly preserve the row gap,
matching Firefox's behavior.

To accomplish this, we needed to adjust item offsets based on various
cases, but we only want to make this adjustment the first time the
row breaks before. Thus, the break before row tracking in
NGFlexBreakTokenData was updated to an enum of three values to keep
track of whether the break token represents a break before a row,
as well as whether or not it is the first time we have broken
before that row.

Additionally, when we perform the logic to determine if a row should
break before, we needed to update the call site to pass in the adjusted
row offset to ensure we are handling breaks correctly.

Bug: 1413089
Change-Id: Ieb9cc4470cd96146cddf4f330aefdd06af1a1626
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4335769
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Commit-Queue: Alison Maher <almaher@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1117559}
  • Loading branch information
alisonmaher authored and Chromium LUCI CQ committed Mar 15, 2023
1 parent e6e9d25 commit 5bb636e
Show file tree
Hide file tree
Showing 17 changed files with 572 additions and 37 deletions.
Expand Up @@ -11,18 +11,32 @@
namespace blink {

struct NGFlexBreakTokenData final : NGBlockBreakTokenData {
// NGFlexBreakBeforeRow is used to maintain the state of break before rows
// during flex fragmentation. kNotBreakBeforeRow implies that we are either
// fragmenting a column-based flex container, or the current break token does
// not represent a break before a row. If kAtStartOfBreakBeforeRow is set,
// then the current break token represents a break before a row, and it is the
// first time we broke before the given row. If kPastStartOfBreakBeforeRow is
// set, then the current break token similarly represents a break before a
// row, but it is not the first time we've broken before the given row.
enum NGFlexBreakBeforeRow {
kNotBreakBeforeRow,
kAtStartOfBreakBeforeRow,
kPastStartOfBreakBeforeRow
};

NGFlexBreakTokenData(const NGBlockBreakTokenData* break_token_data,
const HeapVector<NGFlexLine>& flex_lines,
const Vector<EBreakBetween>& row_break_between,
const HeapVector<Member<LayoutBox>>& oof_children,
LayoutUnit intrinsic_block_size,
bool broke_before_row)
NGFlexBreakBeforeRow break_before_row)
: NGBlockBreakTokenData(kFlexBreakTokenData, break_token_data),
flex_lines(flex_lines),
row_break_between(row_break_between),
oof_children(oof_children),
intrinsic_block_size(intrinsic_block_size),
broke_before_row(broke_before_row) {}
break_before_row(break_before_row) {}

void Trace(Visitor* visitor) const override {
visitor->Trace(flex_lines);
Expand All @@ -34,10 +48,17 @@ struct NGFlexBreakTokenData final : NGBlockBreakTokenData {
Vector<EBreakBetween> row_break_between;
HeapVector<Member<LayoutBox>> oof_children;
LayoutUnit intrinsic_block_size;
// |broke_before_row| is only used in the case of row flex containers. If this
// is true, that means that the next row to be processed had broken before,
// as represented by a break before its first child.
bool broke_before_row = false;
// `break_before_row` is only used in the case of row flex containers. If this
// is set to anything other than kNotBreakBeforeRow, that means that the next
// row to be processed has broken before, as represented by a break before its
// first child.
//
// We do not clamp row gaps, so we can have more than one break before a row.
// There are certain adjustments we only want to make the first time a row
// breaks before. Thus, we will also track if the current break before is the
// first, or if we are past the first break before row (as distinguished by
// the kAtStartOfBreakBeforeRow and kPastStartOfBreakBeforeRow values).
NGFlexBreakBeforeRow break_before_row = kNotBreakBeforeRow;
};

template <>
Expand Down
Expand Up @@ -13,7 +13,6 @@
#include "third_party/blink/renderer/core/layout/layout_button.h"
#include "third_party/blink/renderer/core/layout/layout_flexible_box.h"
#include "third_party/blink/renderer/core/layout/ng/flex/layout_ng_flexible_box.h"
#include "third_party/blink/renderer/core/layout/ng/flex/ng_flex_break_token_data.h"
#include "third_party/blink/renderer/core/layout/ng/flex/ng_flex_child_iterator.h"
#include "third_party/blink/renderer/core/layout/ng/flex/ng_flex_data.h"
#include "third_party/blink/renderer/core/layout/ng/flex/ng_flex_item_iterator.h"
Expand Down Expand Up @@ -1097,7 +1096,8 @@ const NGLayoutResult* NGFlexLayoutAlgorithm::LayoutInternal() {
Vector<EBreakBetween> row_break_between_outputs;
HeapVector<NGFlexLine> flex_line_outputs;
HeapVector<Member<LayoutBox>> oof_children;
bool broke_before_row = false;
NGFlexBreakTokenData::NGFlexBreakBeforeRow break_before_row =
NGFlexBreakTokenData::kNotBreakBeforeRow;
ClearCollectionScope<HeapVector<NGFlexLine>> scope(&flex_line_outputs);

bool use_empty_line_block_size;
Expand All @@ -1107,7 +1107,7 @@ const NGLayoutResult* NGFlexLayoutAlgorithm::LayoutInternal() {
total_intrinsic_block_size_ = flex_data->intrinsic_block_size;
flex_line_outputs = flex_data->flex_lines;
row_break_between_outputs = flex_data->row_break_between;
broke_before_row = flex_data->broke_before_row;
break_before_row = flex_data->break_before_row;
oof_children = flex_data->oof_children;

use_empty_line_block_size =
Expand Down Expand Up @@ -1148,7 +1148,7 @@ const NGLayoutResult* NGFlexLayoutAlgorithm::LayoutInternal() {

NGLayoutResult::EStatus status =
GiveItemsFinalPositionAndSizeForFragmentation(
&flex_line_outputs, &row_break_between_outputs, &broke_before_row);
&flex_line_outputs, &row_break_between_outputs, &break_before_row);
if (status != NGLayoutResult::kSuccess)
return container_builder_.Abort(status);

Expand Down Expand Up @@ -1211,7 +1211,7 @@ const NGLayoutResult* NGFlexLayoutAlgorithm::LayoutInternal() {
MakeGarbageCollected<NGFlexBreakTokenData>(
container_builder_.GetBreakTokenData(), flex_line_outputs,
row_break_between_outputs, oof_children,
total_intrinsic_block_size_, broke_before_row));
total_intrinsic_block_size_, break_before_row));
}

#if DCHECK_IS_ON()
Expand Down Expand Up @@ -1546,11 +1546,11 @@ NGLayoutResult::EStatus
NGFlexLayoutAlgorithm::GiveItemsFinalPositionAndSizeForFragmentation(
HeapVector<NGFlexLine>* flex_line_outputs,
Vector<EBreakBetween>* row_break_between_outputs,
bool* broke_before_row) {
NGFlexBreakTokenData::NGFlexBreakBeforeRow* break_before_row) {
DCHECK(InvolvedInBlockFragmentation(container_builder_));
DCHECK(flex_line_outputs);
DCHECK(row_break_between_outputs);
DCHECK(broke_before_row);
DCHECK(break_before_row);

NGFlexItemIterator item_iterator(*flex_line_outputs, BreakToken(),
is_column_);
Expand All @@ -1572,9 +1572,11 @@ NGFlexLayoutAlgorithm::GiveItemsFinalPositionAndSizeForFragmentation(
previously_consumed_block_size = BreakToken()->ConsumedBlockSize();

BaselineAccumulator baseline_accumulator(Style());
for (auto entry = item_iterator.NextItem(*broke_before_row);
bool broke_before_row =
*break_before_row != NGFlexBreakTokenData::kNotBreakBeforeRow;
for (auto entry = item_iterator.NextItem(broke_before_row);
NGFlexItem* flex_item = entry.flex_item;
entry = item_iterator.NextItem(*broke_before_row)) {
entry = item_iterator.NextItem(broke_before_row)) {
wtf_size_t flex_item_idx = entry.flex_item_idx;
wtf_size_t flex_line_idx = entry.flex_line_idx;
NGFlexLine& line_output = (*flex_line_outputs)[flex_line_idx];
Expand Down Expand Up @@ -1626,11 +1628,56 @@ NGFlexLayoutAlgorithm::GiveItemsFinalPositionAndSizeForFragmentation(
// the item or row expanded by. This allows for things like margins
// and alignment offsets to not get sliced by a forced break.
line_output.item_offset_adjustment += previously_consumed_block_size;
} else if (!is_column_ && flex_item_idx == 0 && *broke_before_row) {
LayoutUnit total_row_block_offset =
row_block_offset + line_output.item_offset_adjustment;
line_output.item_offset_adjustment +=
previously_consumed_block_size - total_row_block_offset;
} else if (!is_column_ && flex_item_idx == 0 && broke_before_row) {
// If this is the first time we are handling a break before a row,
// adjust the offset of items in the row to accommodate the break. The
// following cases need to be considered:
//
// 1. If we are not the first line in the container, and the previous
// sibling row overflowed the fragmentainer in the block axis, flex
// items in the current row should be adjusted upward in the block
// direction to account for the overflowed content.
//
// 2. Otherwise, the current row gap should be decreased by the amount
// of extra space in the previous fragmentainer remaining after the
// block-end of the previous row. The reason being that we should not
// clamp row gaps between breaks, similarly to how flex item margins are
// handled during fragmentation.
//
// 3. If the entire row gap was accounted for in the previous
// fragmentainer, the block-offsets of the flex items in the current row
// will need to be adjusted downward in the block direction to
// accommodate the extra space consumed by the container.
if (*break_before_row ==
NGFlexBreakTokenData::kAtStartOfBreakBeforeRow) {
// Calculate the amount of space remaining in the previous
// fragmentainer after the block-end of the previous flex row, if any.
LayoutUnit previous_row_end =
is_first_line
? LayoutUnit()
: (*flex_line_outputs)[flex_line_idx - 1].LineCrossEnd();
LayoutUnit fragmentainer_space_remaining =
(previously_consumed_block_size - previous_row_end)
.ClampNegativeToZero();

// If there was any remaining space after the previous flex line,
// determine how much of the row gap was consumed in the previous
// fragmentainer, if any.
LayoutUnit consumed_row_gap;
if (fragmentainer_space_remaining) {
LayoutUnit total_row_block_offset =
row_block_offset + line_output.item_offset_adjustment;
LayoutUnit row_gap = total_row_block_offset - previous_row_end;
DCHECK_GE(row_gap, LayoutUnit());
consumed_row_gap = std::min(row_gap, fragmentainer_space_remaining);
}

// Adjust the item offsets to account for any overflow or consumed row
// gap in the previous fragmentainer.
LayoutUnit row_adjustment = previously_consumed_block_size -
previous_row_end - consumed_row_gap;
line_output.item_offset_adjustment += row_adjustment;
}
} else {
LayoutUnit total_item_block_offset =
offset.block_offset + line_output.item_offset_adjustment;
Expand Down Expand Up @@ -1666,8 +1713,9 @@ NGFlexLayoutAlgorithm::GiveItemsFinalPositionAndSizeForFragmentation(
container_builder_.AddBreakBeforeChild(flex_item->ng_input_node,
kBreakAppealPerfect,
/* is_forced_break */ false);
if (early_break_->Type() == NGEarlyBreak::kLine)
*broke_before_row = true;
if (early_break_->Type() == NGEarlyBreak::kLine) {
*break_before_row = NGFlexBreakTokenData::kAtStartOfBreakBeforeRow;
}
ConsumeRemainingFragmentainerSpace(previously_consumed_block_size,
&line_output);
// For column flex containers, continue to the next column. For rows,
Expand Down Expand Up @@ -1743,29 +1791,36 @@ NGFlexLayoutAlgorithm::GiveItemsFinalPositionAndSizeForFragmentation(
if (!is_column_) {
has_container_separation =
offset.block_offset > row_block_offset &&
(!item_break_token || (*broke_before_row && flex_item_idx == 0 &&
(!item_break_token || (broke_before_row && flex_item_idx == 0 &&
item_break_token->IsBreakBefore()));
// Don't attempt to break before a row if the fist item is resuming
// layout. In which case, the row should be resuming layout, as well.
if (flex_item_idx == 0 &&
(!item_break_token ||
(item_break_token->IsBreakBefore() && *broke_before_row))) {
(item_break_token->IsBreakBefore() && broke_before_row))) {
// Rows have no layout result, so if the row breaks before, we
// will break before the first item in the row instead.
bool row_container_separation = has_processed_first_line_;
bool is_first_for_row = !item_break_token || *broke_before_row;
bool is_first_for_row = !item_break_token || broke_before_row;
NGBreakStatus row_break_status = BreakBeforeRowIfNeeded(
line_output, (*row_break_between_outputs)[flex_line_idx],
flex_line_idx, flex_item->ng_input_node, row_container_separation,
line_output, row_block_offset,
(*row_break_between_outputs)[flex_line_idx], flex_line_idx,
flex_item->ng_input_node, row_container_separation,
is_first_for_row);
if (row_break_status == NGBreakStatus::kBrokeBefore) {
ConsumeRemainingFragmentainerSpace(previously_consumed_block_size,
&line_output);
*broke_before_row = true;
if (broke_before_row) {
*break_before_row =
NGFlexBreakTokenData::kPastStartOfBreakBeforeRow;
} else {
*break_before_row =
NGFlexBreakTokenData::kAtStartOfBreakBeforeRow;
}
DCHECK_EQ(status, NGLayoutResult::kSuccess);
break;
}
*broke_before_row = false;
*break_before_row = NGFlexBreakTokenData::kNotBreakBeforeRow;
if (row_break_status == NGBreakStatus::kNeedsEarlierBreak) {
status = NGLayoutResult::kNeedsEarlierBreak;
break;
Expand Down Expand Up @@ -1955,7 +2010,7 @@ NGFlexLayoutAlgorithm::GiveItemsFinalPositionAndSizeForFragmentation(
}

if (!container_builder_.HasInflowChildBreakInside() &&
!item_iterator.NextItem(*broke_before_row).flex_item) {
!item_iterator.NextItem(broke_before_row).flex_item) {
container_builder_.SetHasSeenAllChildren();
}

Expand Down Expand Up @@ -2538,6 +2593,7 @@ void NGFlexLayoutAlgorithm::ConsumeRemainingFragmentainerSpace(

NGBreakStatus NGFlexLayoutAlgorithm::BreakBeforeRowIfNeeded(
const NGFlexLine& row,
LayoutUnit row_block_offset,
EBreakBetween row_break_between,
wtf_size_t row_index,
NGLayoutInputNode child,
Expand All @@ -2547,9 +2603,7 @@ NGBreakStatus NGFlexLayoutAlgorithm::BreakBeforeRowIfNeeded(
DCHECK(InvolvedInBlockFragmentation(container_builder_));

LayoutUnit fragmentainer_block_offset =
ConstraintSpace().FragmentainerOffset() + row.cross_axis_offset;
if (BreakToken())
fragmentainer_block_offset -= BreakToken()->ConsumedBlockSize();
ConstraintSpace().FragmentainerOffset() + row_block_offset;

if (has_container_separation) {
if (IsForcedBreakValue(ConstraintSpace(), row_break_between)) {
Expand Down
Expand Up @@ -9,6 +9,7 @@
#include "third_party/blink/renderer/core/layout/ng/ng_layout_algorithm.h"

#include "third_party/blink/renderer/core/layout/flexible_box_algorithm.h"
#include "third_party/blink/renderer/core/layout/ng/flex/ng_flex_break_token_data.h"
#include "third_party/blink/renderer/core/layout/ng/ng_box_fragment_builder.h"

namespace blink {
Expand Down Expand Up @@ -96,7 +97,7 @@ class CORE_EXPORT NGFlexLayoutAlgorithm
NGLayoutResult::EStatus GiveItemsFinalPositionAndSizeForFragmentation(
HeapVector<NGFlexLine>* flex_line_outputs,
Vector<EBreakBetween>* row_break_between_outputs,
bool* broke_before_row);
NGFlexBreakTokenData::NGFlexBreakBeforeRow* break_before_row);
NGLayoutResult::EStatus PropagateFlexItemInfo(FlexItem* flex_item,
wtf_size_t flex_line_idx,
LogicalOffset offset,
Expand Down Expand Up @@ -137,10 +138,12 @@ class CORE_EXPORT NGFlexLayoutAlgorithm
// a layout result, so when breaking before a row, we will insert a
// fragmentainer break before the first child in a row. |child| should be
// those associated with the first child in the row. |row|,
// |row_break_between|, |row_index|, |has_container_separation| and
// |is_first_for_row| are specific to the row itself. See
// |row_block_offset|, |row_break_between|, |row_index|,
// |has_container_separation| and |is_first_for_row| are specific to the row
// itself. See
// |::blink::BreakBeforeChildIfNeeded()| for more documentation.
NGBreakStatus BreakBeforeRowIfNeeded(const NGFlexLine& row,
LayoutUnit row_block_offset,
EBreakBetween row_break_between,
wtf_size_t row_index,
NGLayoutInputNode child,
Expand Down
1 change: 0 additions & 1 deletion third_party/blink/web_tests/TestExpectations
Expand Up @@ -1481,7 +1481,6 @@ crbug.com/1396218 fragmentation/table-overlapping-rowspan.html [ Failure ]

### Tests failing with LayoutNGFlexFragmentation enabled:
crbug.com/1367912 external/wpt/css/css-break/flexbox/multi-line-row-flex-fragmentation-058.html [ Failure ]
crbug.com/1413089 external/wpt/css/css-break/flexbox/multi-line-row-flex-fragmentation-063-print.html [ Failure ]
crbug.com/1367912 external/wpt/css/css-break/flexbox/single-line-column-flex-fragmentation-043.html [ Failure ]
crbug.com/1225630 fast/multicol/flexbox/doubly-nested-with-zero-width-flexbox-and-forced-break-crash.html [ Skip Timeout ]

Expand Down
@@ -0,0 +1,35 @@
<!DOCTYPE html>
<title>
Multi-line row flex fragmentation: row gaps should not be truncated by
fragmentainer breaks (similar to flex-item margins).
</title>
<style>
.multicol {
columns: 2;
column-fill: auto;
width: 300px;
height: 100px;
margin: 20px;
background: yellow;
}
.flex {
display: flex;
flex-wrap: wrap;
background: gray;
}
.flex > div {
contain: size;
width: 100%;
height: 50px;
background: cyan;
}
</style>
<p>Flex row gaps should <strong>not</strong> be truncated when a row breaks.</p>
<div class="multicol">
<div class="flex">
<div></div>
<div style="margin-top:100px;"></div>
<div style="margin-top:100px;"></div>
<div style="margin-top:100px;"></div>
</div>
</div>
@@ -0,0 +1,37 @@
<!DOCTYPE html>
<title>
Multi-line row flex fragmentation: row gaps should not be truncated by
fragmentainer breaks (similar to flex-item margins).
</title>
<link rel="help" href="https://drafts.csswg.org/css-flexbox-1/#pagination">
<link rel="match" href="multi-line-row-flex-fragmentation-065-ref.html">
<style>
.multicol {
columns: 2;
column-fill: auto;
width: 300px;
height: 100px;
margin: 20px;
background: yellow;
}
.flex {
display: flex;
flex-wrap: wrap;
background: gray;
}
.flex > div {
contain: size;
width: 100%;
height: 50px;
background: cyan;
}
</style>
<p>Flex row gaps should <strong>not</strong> be truncated when a row breaks.</p>
<div class="multicol">
<div class="flex" style="row-gap:100px;">
<div></div>
<div></div>
<div></div>
<div></div>
</div>
</div>

0 comments on commit 5bb636e

Please sign in to comment.