Ignore relpos offsets when balancing columns.

Calculate the block-size during layout that is to be used as input to the initial column balancing attempt. Do this before relative offsets are applied. Doing it after layout, like we used to, is tricky, since it's hard to un-apply relative offsets correctly at that point, both with regards to percentages, which would require a constraint space, and whatever special rules we have (grid layout) for relative positioning. This allows us to share code between column balancing and actual fragmentation. No longer need for BlockAxisLayoutOverflow() -- use BlockSizeForFragmentation() instead. This also fixes the unit test PaintPropertyTreeBuilderTest.ColumnSpannerUnderRelativePositioned when LayoutNGBlockFragmentation is enabled. Bug: 829028 Change-Id: I82c599fd01c1244f56a95f512fe6de33bb05e9fb Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3270688 Commit-Queue: Morten Stenshorne <mstensho@chromium.org> Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org> Cr-Commit-Position: refs/heads/main@{#940671}
chromium · Nov 11, 2021 · d4daa97 · d4daa97
1 parent 87fce26
commit d4daa97
Show file tree

Hide file tree

Showing 8 changed files with 109 additions and 72 deletions.
diff --git a/third_party/blink/renderer/core/layout/ng/ng_box_fragment_builder.cc b/third_party/blink/renderer/core/layout/ng/ng_box_fragment_builder.cc
@@ -87,19 +87,24 @@ void NGBoxFragmentBuilder::AddResult(
     absl::optional<LogicalOffset> relative_offset,
     const NGInlineContainer<LogicalOffset>* inline_container) {
   const auto& fragment = child_layout_result.PhysicalFragment();
-  const NGLayoutResult* child_box_layout_result = nullptr;
-  if (fragment.IsBox()) {
-    child_box_layout_result = &child_layout_result;
-  } else if (items_builder_) {
+
+  // We'll normally propagate info from child_layout_result here, but if that's
+  // a line box with a block inside, we'll use the result for that block
+  // instead. The fact that we create a line box at all in such cases is just an
+  // implementation detail -- anything of interest is stored on the child block
+  // fragment.
+  const NGLayoutResult* result_for_propagation = &child_layout_result;
+
+  if (!fragment.IsBox() && items_builder_) {
     if (const NGPhysicalLineBoxFragment* line =
             DynamicTo<NGPhysicalLineBoxFragment>(&fragment)) {
       if (UNLIKELY(line->IsBlockInInline() && has_block_fragmentation_)) {
         // If this line box contains a block-in-inline, propagate break data
         // from the block-in-inline.
         const NGLogicalLineItems& line_items =
             items_builder_->LogicalLineItems(*line);
-        child_box_layout_result = line_items.BlockInInlineLayoutResult();
-        DCHECK(child_box_layout_result);
+        result_for_propagation = line_items.BlockInInlineLayoutResult();
+        DCHECK(result_for_propagation);
       }
 
       items_builder_->AddLine(*line, offset);
@@ -120,8 +125,8 @@ void NGBoxFragmentBuilder::AddResult(
            child_layout_result.IsSelfCollapsing(), relative_offset,
            inline_container, adjustment_for_oof_propagation);
 
-  if (UNLIKELY(has_block_fragmentation_ && child_box_layout_result))
-    PropagateBreakInfo(*child_box_layout_result);
+  if (UNLIKELY(has_block_fragmentation_))
+    PropagateBreakInfo(*result_for_propagation, offset);
 }
 
 void NGBoxFragmentBuilder::AddChild(
@@ -347,19 +352,33 @@ void NGBoxFragmentBuilder::MoveChildrenInBlockDirection(LayoutUnit delta) {
 }
 
 void NGBoxFragmentBuilder::PropagateBreakInfo(
-    const NGLayoutResult& child_layout_result) {
+    const NGLayoutResult& child_layout_result,
+    LogicalOffset offset) {
   DCHECK(has_block_fragmentation_);
-  const auto& child_fragment =
-      To<NGPhysicalBoxFragment>(child_layout_result.PhysicalFragment());
-  if (const auto* token = child_fragment.BreakToken()) {
+
+  // Include the bounds of this child (in the block direction).
+  LayoutUnit block_end_in_container =
+      offset.block_offset -
+      child_layout_result.AnnotationBlockOffsetAdjustment() +
+      BlockSizeForFragmentation(child_layout_result, writing_direction_);
+
+  block_size_for_fragmentation_ =
+      std::max(block_size_for_fragmentation_, block_end_in_container);
+
+  const auto* child_box_fragment =
+      DynamicTo<NGPhysicalBoxFragment>(&child_layout_result.PhysicalFragment());
+  if (!child_box_fragment)
+    return;
+
+  if (const auto* token = child_box_fragment->BreakToken()) {
     // Figure out if this child break is in the same flow as this parent. If
     // it's an out-of-flow positioned box, it's not. If it's in a parallel flow,
     // it's also not.
     if (!token->IsBlockType() ||
         !To<NGBlockBreakToken>(token)->IsAtBlockEnd()) {
-      if (child_fragment.IsFloating())
+      if (child_box_fragment->IsFloating())
         has_float_break_inside_ = true;
-      else if (!child_fragment.IsOutOfFlowPositioned())
+      else if (!child_box_fragment->IsOutOfFlowPositioned())
         has_inflow_child_break_inside_ = true;
     }
 
@@ -412,11 +431,25 @@ scoped_refptr<const NGLayoutResult> NGBoxFragmentBuilder::ToBoxFragment(
                node_.IsBlockInInline()))
     SetIsBlockInInline();
 
-  if (UNLIKELY(has_block_fragmentation_ && !break_token_ && node_)) {
-    if (last_inline_break_token_)
-      child_break_tokens_.push_back(std::move(last_inline_break_token_));
-    if (DidBreakSelf() || HasChildBreakInside())
-      break_token_ = NGBlockBreakToken::Create(this);
+  if (UNLIKELY(has_block_fragmentation_ && node_)) {
+    if (!break_token_) {
+      if (last_inline_break_token_)
+        child_break_tokens_.push_back(std::move(last_inline_break_token_));
+      if (DidBreakSelf() || HasChildBreakInside())
+        break_token_ = NGBlockBreakToken::Create(this);
+    }
+
+    OverflowClipAxes block_axis =
+        GetWritingDirection().IsHorizontal() ? kOverflowClipY : kOverflowClipX;
+    if (To<NGBlockNode>(node_).GetOverflowClipAxes() & block_axis) {
+      // Block-axis overflow is clipped, so ignore child overflow and just use
+      // the border-box size of the fragment itself.
+      block_size_for_fragmentation_ = FragmentBlockSize();
+    } else {
+      // Include the border-box size of the fragment itself.
+      block_size_for_fragmentation_ =
+          std::max(block_size_for_fragmentation_, FragmentBlockSize());
+    }
   }
 
   if (!has_floating_descendants_for_paint_ && items_builder_) {

diff --git a/third_party/blink/renderer/core/layout/ng/ng_box_fragment_builder.h b/third_party/blink/renderer/core/layout/ng/ng_box_fragment_builder.h
@@ -595,7 +595,7 @@ class CORE_EXPORT NGBoxFragmentBuilder final
   // Propagate fragmentation details. This includes checking whether we have
   // fragmented in this flow, break appeal, column spanner detection, and column
   // balancing hints.
-  void PropagateBreakInfo(const NGLayoutResult&);
+  void PropagateBreakInfo(const NGLayoutResult&, LogicalOffset);
 
   void SetHasForcedBreak() {
     has_forced_break_ = true;
@@ -636,6 +636,7 @@ class CORE_EXPORT NGBoxFragmentBuilder final
 
   LayoutUnit minimal_space_shortage_ = LayoutUnit::Max();
   LayoutUnit tallest_unbreakable_block_size_ = LayoutUnit::Min();
+  LayoutUnit block_size_for_fragmentation_;
 
   // The break-before value on the initial child we cannot honor. There's no
   // valid class A break point before a first child, only *between* siblings.

diff --git a/third_party/blink/renderer/core/layout/ng/ng_column_layout_algorithm.cc b/third_party/blink/renderer/core/layout/ng/ng_column_layout_algorithm.cc
@@ -23,36 +23,6 @@ namespace blink {
 
 namespace {
 
-LayoutUnit CalculateColumnContentBlockSize(
-    const NGPhysicalFragment& fragment,
-    WritingDirectionMode writing_direction) {
-  WritingModeConverter converter(writing_direction, fragment.Size());
-  // Note that what we're doing here is almost the same as what we do when
-  // calculating overflow, with at least one important difference: If the
-  // inline-size of a fragment is 0, the overflow rectangle becomes empty, even
-  // if the fragment's block-size is non-zero. This is correct for overflow
-  // handling, but it would be wrong for column balancing.
-  LayoutUnit total_size;
-  for (const auto& child : fragment.Children()) {
-    LayoutUnit size = converter.ToLogical(child->Size()).block_size;
-    LayoutUnit offset =
-        converter.ToLogical(child.offset, child->Size()).block_offset;
-    // TODO(mstensho): Need to detect whether we're actually clipping in the
-    // block direction. The combination of overflow-x:clip and
-    // overflow-y:visible should enter children here.
-    if (child->IsContainer() && !child->HasNonVisibleOverflow()) {
-      LayoutUnit children_size =
-          CalculateColumnContentBlockSize(*child, writing_direction);
-      if (size < children_size)
-        size = children_size;
-    }
-    LayoutUnit block_end = offset + size;
-    if (total_size < block_end)
-      total_size = block_end;
-  }
-  return total_size;
-}
-
 // An itinerary of multicol container parts to walk separately for layout. A
 // part is either a chunk of regular column content, or a column spanner.
 class MulticolPartWalker {
@@ -1171,8 +1141,8 @@ LayoutUnit NGColumnLayoutAlgorithm::CalculateBalancedColumnBlockSize(
     // content that's doomed to end up in overflowing columns (because of too
     // many forced breaks).
     if (forced_break_count < used_column_count_) {
-      LayoutUnit column_block_size = CalculateColumnContentBlockSize(
-          fragment, space.GetWritingDirection());
+      LayoutUnit column_block_size = BlockSizeForFragmentation(
+          *result, ConstraintSpace().GetWritingDirection());
 
       // Encompass the block-size of the (single-strip column) fragment, to
       // account for any trailing margins. We let them affect the column

diff --git a/third_party/blink/renderer/core/layout/ng/ng_fragmentation_utils.cc b/third_party/blink/renderer/core/layout/ng/ng_fragmentation_utils.cc
@@ -57,25 +57,6 @@ inline int FragmentainerBreakPrecedence(EBreakBetween break_value) {
   }
 }
 
-// Return layout overflow block-size that's not clipped (or simply the
-// block-size if it *is* clipped).
-LayoutUnit BlockAxisLayoutOverflow(const NGLayoutResult& result,
-                                   WritingDirectionMode writing_direction) {
-  const NGPhysicalFragment& fragment = result.PhysicalFragment();
-  LayoutUnit block_size = NGFragment(writing_direction, fragment).BlockSize();
-  if (!result.PhysicalFragment().IsLineBox())
-    return block_size;
-
-  // Ruby annotations do not take up space in the line box, so we need this to
-  // make sure that we don't let them cross the fragmentation line without
-  // noticing.
-  LayoutUnit annotation_overflow = result.AnnotationOverflow();
-  if (annotation_overflow > LayoutUnit())
-    block_size += annotation_overflow;
-  block_size += result.AnnotationBlockOffsetAdjustment();
-  return block_size;
-}
-
 // Return true if the container is being resumed after a fragmentainer break,
 // and the child is at the first fragment of a node, and we are allowed to break
 // before it. Normally, this isn't allowed, as that would take us nowhere,
@@ -791,7 +772,7 @@ bool MovePastBreakpoint(const NGConstraintSpace& space,
          appeal_inside >= builder->EarlyBreak().BreakAppeal()))
       return true;
   } else if (refuse_break_before ||
-             BlockAxisLayoutOverflow(
+             BlockSizeForFragmentation(
                  layout_result, space.GetWritingDirection()) <= space_left) {
     // The child either fits, or we are not allowed to break. So we can move
     // past this breakpoint.
@@ -1076,4 +1057,29 @@ PhysicalOffset OffsetInStitchedFragments(
   return converter.ToPhysical(offset_in_stitched_box, fragment.Size());
 }
 
+LayoutUnit BlockSizeForFragmentation(
+    const NGLayoutResult& result,
+    WritingDirectionMode container_writing_direction) {
+  LayoutUnit block_size = result.BlockSizeForFragmentation();
+  if (block_size == kIndefiniteSize) {
+    // Just use the border-box size of the fragment if block-size for
+    // fragmentation hasn't been calculated. This happens for line boxes and any
+    // other kind of monolithic content.
+    WritingMode writing_mode = container_writing_direction.GetWritingMode();
+    LogicalSize logical_size =
+        result.PhysicalFragment().Size().ConvertToLogical(writing_mode);
+    block_size = logical_size.block_size;
+  }
+
+  // Ruby annotations do not take up space in the line box, so we need this to
+  // make sure that we don't let them cross the fragmentation line without
+  // noticing.
+  block_size += result.AnnotationBlockOffsetAdjustment();
+  LayoutUnit annotation_overflow = result.AnnotationOverflow();
+  if (annotation_overflow > LayoutUnit())
+    block_size += annotation_overflow;
+
+  return block_size;
+}
+
 }  // namespace blink
diff --git a/third_party/blink/renderer/core/layout/ng/ng_fragmentation_utils.h b/third_party/blink/renderer/core/layout/ng/ng_fragmentation_utils.h
@@ -381,6 +381,14 @@ PhysicalOffset OffsetInStitchedFragments(
     const NGPhysicalBoxFragment&,
     PhysicalSize* out_stitched_fragments_size = nullptr);
 
+// Return the block-size that this fragment will take up inside a fragmentation
+// context. This will include overflow from descendants (if it is visible and
+// supposed to affect block fragmentation), and also out-of-flow positioned
+// descendants (in the initial balancing pass), but not relative offsets.
+LayoutUnit BlockSizeForFragmentation(
+    const NGLayoutResult&,
+    WritingDirectionMode container_writing_direction);
+
 }  // namespace blink
 
 #endif  // THIRD_PARTY_BLINK_RENDERER_CORE_LAYOUT_NG_NG_FRAGMENTATION_UTILS_H_
diff --git a/third_party/blink/renderer/core/layout/ng/ng_layout_result.cc b/third_party/blink/renderer/core/layout/ng/ng_layout_result.cc
@@ -84,6 +84,9 @@ NGLayoutResult::NGLayoutResult(
       rare_data->minimal_space_shortage = builder->minimal_space_shortage_;
     }
 
+    rare_data->block_size_for_fragmentation =
+        builder->block_size_for_fragmentation_;
+
     bitfields_.break_appeal = builder->break_appeal_;
     bitfields_.initial_break_before = static_cast<unsigned>(
         builder->initial_break_before_.value_or(EBreakBetween::kAuto));

diff --git a/third_party/blink/renderer/core/layout/ng/ng_layout_result.h b/third_party/blink/renderer/core/layout/ng/ng_layout_result.h
@@ -238,6 +238,19 @@ class CORE_EXPORT NGLayoutResult : public RefCounted<NGLayoutResult> {
     return rare_data_->tallest_unbreakable_block_size;
   }
 
+  // Return the block-size that this fragment will take up inside a
+  // fragmentation context. This will include overflow from descendants (if it
+  // is visible and supposed to affect block fragmentation), and also
+  // out-of-flow positioned descendants (in the initial balancing pass), but not
+  // relative offsets. kIndefiniteSize will be returned if block fragmentation
+  // wasn't performed on the node (e.g. monolithic content such as line boxes,
+  // or if the node isn't inside a fragmentation context at all).
+  LayoutUnit BlockSizeForFragmentation() const {
+    if (!HasRareData())
+      return kIndefiniteSize;
+    return rare_data_->block_size_for_fragmentation;
+  }
+
   // Return the (lowest) appeal among any unforced breaks inside the resulting
   // fragment (or kBreakAppealPerfect if there are no such breaks).
   //
@@ -516,6 +529,7 @@ class CORE_EXPORT NGLayoutResult : public RefCounted<NGLayoutResult> {
       // increased by this amount.
       LayoutUnit minimal_space_shortage = kIndefiniteSize;
     };
+    LayoutUnit block_size_for_fragmentation = kIndefiniteSize;
     NGExclusionSpace exclusion_space;
     scoped_refptr<SerializedScriptValue> custom_layout_data;
 

diff --git a/third_party/blink/web_tests/TestExpectations b/third_party/blink/web_tests/TestExpectations
@@ -1384,6 +1384,7 @@ virtual/layout_ng_block_frag/external/wpt/css/css-multicol/multicol-fill-balance
 virtual/layout_ng_block_frag/external/wpt/css/css-multicol/multicol-fill-balance-012.html [ Pass ]
 virtual/layout_ng_block_frag/external/wpt/css/css-multicol/multicol-fill-balance-013.html [ Pass ]
 virtual/layout_ng_block_frag/external/wpt/css/css-multicol/multicol-fill-balance-014.html [ Pass ]
+virtual/layout_ng_block_frag/external/wpt/css/css-multicol/multicol-fill-balance-017.html [ Pass ]
 virtual/layout_ng_block_frag/external/wpt/css/css-multicol/multicol-list-item-004.html [ Pass ]
 virtual/layout_ng_block_frag/external/wpt/css/css-multicol/multicol-list-item-006.html [ Pass ]
 virtual/layout_ng_block_frag/external/wpt/css/css-multicol/multicol-nested-007.html [ Pass ]
@@ -4099,6 +4100,7 @@ crbug.com/829028 external/wpt/css/css-multicol/multicol-fill-balance-011.html [
 crbug.com/829028 external/wpt/css/css-multicol/multicol-fill-balance-012.html [ Failure ]
 crbug.com/829028 external/wpt/css/css-multicol/multicol-fill-balance-013.html [ Failure ]
 crbug.com/829028 external/wpt/css/css-multicol/multicol-fill-balance-014.html [ Failure ]
+crbug.com/829028 external/wpt/css/css-multicol/multicol-fill-balance-017.html [ Failure ]
 crbug.com/829028 [ Mac ] external/wpt/css/css-multicol/multicol-list-item-004.html [ Failure ]
 crbug.com/829028 [ Mac ] external/wpt/css/css-multicol/multicol-list-item-005.html [ Failure ]
 crbug.com/829028 external/wpt/css/css-multicol/multicol-nested-007.html [ Failure ]