[LayoutNG] Honor unbreakable content when column-balancing.

This ensures that we have room for the tallest piece of monolithic
content, and also the tallest container with break-inside:avoid.

There are more things we could pay attention to here, but at least for
now we don't. We could take break avoidance between siblings into
account, and also orphans and widows. Orphans and widows are honored
when balancing in the legacy engine, but it's somewhat buggy; see
crbug.com/1013151 - it may lead to taller columns than necessary.

The net increase in passing tests isn't impressive, but this is largely
due to lack of support for fragment painting. The "regressing"
span-all-dynamic tests are laid out more correctly now than before, but
doing things dynamically vs. doing it right away matters for the
fragment painting code.

This CL also introduces a mild regression. By honoring unbreakable
content, we should also make sure that underflow is ignored (i.e.
content that comes before the start of a fragmentainer, caused by
negative margins). Otherwise we risk overstretching. This is what
fast/multicol/balance-line-underflow-1.html is about. I'll submit a
separate CL for that, since this one is big enough on its own.

Bug: 829028
Change-Id: Iee389525c34961a7748bd5597f22c75026919671
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1854224
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#705018}

third_party/blink/renderer/core/layout/ng/ng_block_layout_algorithm.cc

@@ -441,6 +441,9 @@ inline scoped_refptr<const NGLayoutResult> NGBlockLayoutAlgorithm::Layout(
     // business, but we store its appeal, so that we don't look for breakpoints
     // with lower appeal than that.
     container_builder_.SetBreakAppeal(ConstraintSpace().EarlyBreakAppeal());
+
+    if (ConstraintSpace().IsInitialColumnBalancingPass())
+      container_builder_.SetIsInitialColumnBalancingPass();
   }
   container_builder_.SetBfcLineOffset(
       ConstraintSpace().BfcOffset().line_offset);
@@ -2091,12 +2094,26 @@ NGBlockLayoutAlgorithm::BreakBeforeChildIfNeeded(
     appeal_before = kBreakAppealLastResort;
   }
 
-  // We only care about soft breaks if we have a fragmentainer block-size.
-  // During column balancing this may be unknown.
-  if (!ConstraintSpace().HasKnownFragmentainerBlockSize())
+  const auto& physical_fragment = layout_result.PhysicalFragment();
+  NGFragment fragment(ConstraintSpace().GetWritingMode(), physical_fragment);
+
+  if (!ConstraintSpace().HasKnownFragmentainerBlockSize()) {
+    if (ConstraintSpace().IsInitialColumnBalancingPass()) {
+      if (child.IsMonolithic() ||
+          (child.IsBlock() &&
+           IsAvoidBreakValue(ConstraintSpace(), child.Style().BreakInside()))) {
+        // If this is the initial column balancing pass, attempt to make the
+        // column block-size at least as large as the tallest piece of
+        // monolithic content and/or block with break-inside:avoid.
+        container_builder_.PropagateTallestUnbreakableBlockSize(
+            fragment.BlockSize());
+      }
+    }
+    // We only care about soft breaks if we have a fragmentainer block-size.
+    // During column balancing this may be unknown.
     return kContinueWithoutBreaking;
+  }
 
-  const auto& physical_fragment = layout_result.PhysicalFragment();
   if (IsA<NGBlockBreakToken>(physical_fragment.BreakToken())) {
     // The block child broke inside. We now need to decide whether to keep that
     // break, or if it would be better to break before it.
@@ -2116,8 +2133,6 @@ NGBlockLayoutAlgorithm::BreakBeforeChildIfNeeded(
     if (child.IsMonolithic()) {
       // If the monolithic piece of content (e.g. a line, or block-level
       // replaced content) doesn't fit, we need a break.
-      NGFragment fragment(ConstraintSpace().GetWritingMode(),
-                          physical_fragment);
       want_break = fragment.BlockSize() > space_left;
     } else {
       // If the block-offset is past the fragmentainer boundary (or exactly at

third_party/blink/renderer/core/layout/ng/ng_box_fragment_builder.cc

@@ -189,10 +189,14 @@ void NGBoxFragmentBuilder::PropagateBreak(
     const auto* token = child_layout_result.PhysicalFragment().BreakToken();
     did_break_ = token && !token->IsFinished();
   }
-  if (child_layout_result.HasForcedBreak())
+  if (child_layout_result.HasForcedBreak()) {
     SetHasForcedBreak();
-  else
+  } else if (IsInitialColumnBalancingPass()) {
+    PropagateTallestUnbreakableBlockSize(
+        child_layout_result.TallestUnbreakableBlockSize());
+  } else {
     PropagateSpaceShortage(child_layout_result.MinimalSpaceShortage());
+  }
 }
 
 scoped_refptr<const NGLayoutResult> NGBoxFragmentBuilder::ToBoxFragment(

third_party/blink/renderer/core/layout/ng/ng_box_fragment_builder.h

@@ -124,11 +124,41 @@ class CORE_EXPORT NGBoxFragmentBuilder final
   // used by the column balancer to stretch columns.
   void PropagateSpaceShortage(LayoutUnit space_shortage) {
     DCHECK_GT(space_shortage, LayoutUnit());
+
+    // Space shortage should only be reported when we already have a tentative
+    // fragmentainer block-size. It's meaningless to talk about space shortage
+    // in the initial column balancing pass, because then we have no
+    // fragmentainer block-size at all, so who's to tell what's too short or
+    // not?
+    DCHECK(!IsInitialColumnBalancingPass());
+
     if (minimal_space_shortage_ > space_shortage)
       minimal_space_shortage_ = space_shortage;
   }
   LayoutUnit MinimalSpaceShortage() const { return minimal_space_shortage_; }
 
+  void PropagateTallestUnbreakableBlockSize(LayoutUnit unbreakable_block_size) {
+    // We should only calculate the block-size of the tallest piece of
+    // unbreakable content during the initial column balancing pass, when we
+    // haven't set a tentative fragmentainer block-size yet.
+    DCHECK(IsInitialColumnBalancingPass());
+
+    tallest_unbreakable_block_size_ =
+        std::max(tallest_unbreakable_block_size_, unbreakable_block_size);
+  }
+
+  void SetIsInitialColumnBalancingPass() {
+    // Note that we have no dedicated flag for being in the initial column
+    // balancing pass here. We'll just bump tallest_unbreakable_block_size_ to
+    // 0, so that NGLayoutResult knows that we need to store unbreakable
+    // block-size.
+    DCHECK_EQ(tallest_unbreakable_block_size_, LayoutUnit::Min());
+    tallest_unbreakable_block_size_ = LayoutUnit();
+  }
+  bool IsInitialColumnBalancingPass() const {
+    return tallest_unbreakable_block_size_ >= LayoutUnit();
+  }
+
   void SetInitialBreakBefore(EBreakBetween break_before) {
     initial_break_before_ = break_before;
   }
@@ -280,6 +310,7 @@ class CORE_EXPORT NGBoxFragmentBuilder final
   LayoutUnit consumed_block_size_;
 
   LayoutUnit minimal_space_shortage_ = LayoutUnit::Max();
+  LayoutUnit tallest_unbreakable_block_size_ = LayoutUnit::Min();
 
   // 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.

third_party/blink/renderer/core/layout/ng/ng_column_layout_algorithm.cc

@@ -629,6 +629,7 @@ LayoutUnit NGColumnLayoutAlgorithm::CalculateBalancedColumnBlockSize(
   // First split into content runs at explicit (forced) breaks.
   ContentRuns content_runs;
   scoped_refptr<const NGBlockBreakToken> break_token = child_break_token;
+  LayoutUnit tallest_unbreakable_block_size;
   do {
     NGBlockLayoutAlgorithm balancing_algorithm(
         {Node(), fragment_geometry, space, break_token.get()});
@@ -639,6 +640,9 @@ LayoutUnit NGColumnLayoutAlgorithm::CalculateBalancedColumnBlockSize(
         fragment, IsHorizontalWritingMode(space.GetWritingMode()));
     content_runs.emplace_back(column_block_size);
 
+    tallest_unbreakable_block_size = std::max(
+        tallest_unbreakable_block_size, result->TallestUnbreakableBlockSize());
+
     // Stop when we reach a spanner. That's where this row of columns will end.
     if (result->ColumnSpanner())
       break;
@@ -665,7 +669,9 @@ LayoutUnit NGColumnLayoutAlgorithm::CalculateBalancedColumnBlockSize(
   // lay out into columns to figure out if they are tall enough or not (and
   // stretch and retry if not). Also honor {,min-,max-}{height,width} properties
   // before returning.
-  LayoutUnit block_size = content_runs.TallestColumnBlockSize();
+  LayoutUnit block_size = std::max(content_runs.TallestColumnBlockSize(),
+                                   tallest_unbreakable_block_size);
+
   return ConstrainColumnBlockSize(block_size);
 }
 

third_party/blink/renderer/core/layout/ng/ng_column_layout_algorithm_test.cc

@@ -2798,6 +2798,34 @@ TEST_F(NGColumnLayoutAlgorithmTest, ColumnBalancingEmptyBlock) {
   EXPECT_EQ(expectation, dump);
 }
 
+TEST_F(NGColumnLayoutAlgorithmTest, ColumnBalancingSingleLine) {
+  SetBodyInnerHTML(R"HTML(
+    <style>
+      #parent {
+        columns: 3;
+        column-gap: 10px;
+        width: 320px;
+        line-height: 20px;
+      }
+    </style>
+    <div id="container">
+      <div id="parent">
+        <br>
+      </div>
+    </div>
+  )HTML");
+
+  String dump = DumpFragmentTree(GetElementById("container"));
+  String expectation = R"DUMP(.:: LayoutNG Physical Fragment Tree ::.
+  offset:unplaced size:1000x20
+    offset:0,0 size:320x20
+      offset:0,0 size:100x20
+        offset:0,0 size:0x20
+          offset:0,9 size:0x1
+)DUMP";
+  EXPECT_EQ(expectation, dump);
+}
+
 TEST_F(NGColumnLayoutAlgorithmTest, ColumnBalancingOverflow) {
   SetBodyInnerHTML(R"HTML(
     <style>
@@ -3165,6 +3193,56 @@ TEST_F(NGColumnLayoutAlgorithmTest, ColumnBalancingLinesAvoidBreakInside) {
   EXPECT_EQ(expectation, dump);
 }
 
+TEST_F(NGColumnLayoutAlgorithmTest, ColumnBalancingLinesAvoidBreakInside2) {
+  // We have 5 lines and 3 columns. If we make the columns tall enough to hold 2
+  // lines each, it should all fit. But then there's a block with 3 lines and
+  // break-inside:avoid...
+  SetBodyInnerHTML(R"HTML(
+    <style>
+      #parent {
+        columns: 3;
+        column-gap: 10px;
+        width: 320px;
+        line-height: 20px;
+        orphans: 1;
+        widows: 1;
+      }
+    </style>
+    <div id="container">
+      <div id="parent">
+        <br>
+        <div style="break-inside:avoid;">
+           <br><br><br>
+        </div>
+        <br>
+      </div>
+    </div>
+  )HTML");
+
+  String dump = DumpFragmentTree(GetElementById("container"));
+  String expectation = R"DUMP(.:: LayoutNG Physical Fragment Tree ::.
+  offset:unplaced size:1000x60
+    offset:0,0 size:320x60
+      offset:0,0 size:100x60
+        offset:0,0 size:100x20
+          offset:0,0 size:0x20
+            offset:0,9 size:0x1
+      offset:110,0 size:100x60
+        offset:0,0 size:100x60
+          offset:0,0 size:0x20
+            offset:0,9 size:0x1
+          offset:0,20 size:0x20
+            offset:0,9 size:0x1
+          offset:0,40 size:0x20
+            offset:0,9 size:0x1
+      offset:220,0 size:100x20
+        offset:0,0 size:100x20
+          offset:0,0 size:0x20
+            offset:0,9 size:0x1
+)DUMP";
+  EXPECT_EQ(expectation, dump);
+}
+
 TEST_F(NGColumnLayoutAlgorithmTest, ClassCBreakPointBeforeBfc) {
   SetBodyInnerHTML(R"HTML(
     <style>

third_party/blink/renderer/core/layout/ng/ng_layout_result.cc

@@ -48,8 +48,20 @@ NGLayoutResult::NGLayoutResult(
   bitfields_.subtree_modified_margin_strut =
       builder->subtree_modified_margin_strut_;
   intrinsic_block_size_ = builder->intrinsic_block_size_;
-  if (builder->minimal_space_shortage_ != LayoutUnit::Max())
+  if (builder->minimal_space_shortage_ != LayoutUnit::Max()) {
+#if DCHECK_IS_ON()
+    DCHECK(!HasRareData() || !rare_data_->has_tallest_unbreakable_block_size);
+#endif
     EnsureRareData()->minimal_space_shortage = builder->minimal_space_shortage_;
+  }
+  if (builder->tallest_unbreakable_block_size_ >= LayoutUnit()) {
+    auto* rare_data = EnsureRareData();
+    rare_data->tallest_unbreakable_block_size =
+        builder->tallest_unbreakable_block_size_;
+#if DCHECK_IS_ON()
+    rare_data->has_tallest_unbreakable_block_size = true;
+#endif
+  }
   if (builder->custom_layout_data_) {
     EnsureRareData()->custom_layout_data =
         std::move(builder->custom_layout_data_);

third_party/blink/renderer/core/layout/ng/ng_layout_result.h

@@ -141,8 +141,23 @@ class CORE_EXPORT NGLayoutResult : public RefCounted<NGLayoutResult> {
   }
 
   LayoutUnit MinimalSpaceShortage() const {
-    return HasRareData() ? rare_data_->minimal_space_shortage
-                         : LayoutUnit::Max();
+    if (!HasRareData())
+      return LayoutUnit::Max();
+#if DCHECK_IS_ON()
+    // This field shares storage with another field.
+    DCHECK(!rare_data_->has_tallest_unbreakable_block_size);
+#endif
+    return rare_data_->minimal_space_shortage;
+  }
+
+  LayoutUnit TallestUnbreakableBlockSize() const {
+    if (!HasRareData())
+      return LayoutUnit();
+#if DCHECK_IS_ON()
+    // This field shares storage with another field.
+    DCHECK(rare_data_->has_tallest_unbreakable_block_size);
+#endif
+    return rare_data_->tallest_unbreakable_block_size;
   }
 
   SerializedScriptValue* CustomLayoutData() const {
@@ -310,9 +325,24 @@ class CORE_EXPORT NGLayoutResult : public RefCounted<NGLayoutResult> {
     NGMarginStrut end_margin_strut;
     NGUnpositionedListMarker unpositioned_list_marker;
     NGBlockNode column_spanner = nullptr;
-    LayoutUnit minimal_space_shortage = LayoutUnit::Max();
+    union {
+      // Only set in the initial column balancing layout pass, when we have no
+      // clue what the column block-size is going to be.
+      LayoutUnit tallest_unbreakable_block_size;
+
+      // Only set in subsequent column balancing passes, when we have set a
+      // tentative column block-size. At every column boundary we'll record
+      // space shortage, and store the smallest one here. If the columns
+      // couldn't fit all the content, and we're allowed to stretch columns
+      // further, we'll perform another pass with the column block-size
+      // increased by this amount.
+      LayoutUnit minimal_space_shortage = LayoutUnit::Max();
+    };
     NGExclusionSpace exclusion_space;
     scoped_refptr<SerializedScriptValue> custom_layout_data;
+#if DCHECK_IS_ON()
+    bool has_tallest_unbreakable_block_size = false;
+#endif
   };
 
   bool HasRareData() const { return bitfields_.has_rare_data; }

third_party/blink/web_tests/TestExpectations

@@ -1010,8 +1010,11 @@ crbug.com/829028 virtual/layout_ng_experimental/external/wpt/css/css-multicol/mu
 crbug.com/829028 virtual/layout_ng_experimental/external/wpt/css/css-multicol/multicol-span-all-children-height-007.html [ Failure ]
 crbug.com/829028 virtual/layout_ng_experimental/external/wpt/css/css-multicol/multicol-span-all-children-height-008.html [ Failure ]
 crbug.com/829028 virtual/layout_ng_experimental/external/wpt/css/css-multicol/multicol-span-all-dynamic-add-004.html [ Failure ]
-crbug.com/829028 virtual/layout_ng_experimental/external/wpt/css/css-multicol/multicol-span-all-dynamic-add-007.html [ Failure ]
+crbug.com/988015 virtual/layout_ng_experimental/external/wpt/css/css-multicol/multicol-span-all-dynamic-add-002.html [ Failure ]
+crbug.com/988015 virtual/layout_ng_experimental/external/wpt/css/css-multicol/multicol-span-all-dynamic-add-003.html [ Failure ]
+crbug.com/994172 virtual/layout_ng_experimental/external/wpt/css/css-multicol/multicol-span-all-dynamic-add-007.html [ Failure Crash ]
 crbug.com/924142 virtual/layout_ng_experimental/external/wpt/css/css-multicol/multicol-span-all-dynamic-add-010.html [ Crash Pass ]
+crbug.com/988015 virtual/layout_ng_experimental/external/wpt/css/css-multicol/multicol-span-all-dynamic-add-013.html [ Failure ]
 crbug.com/874051 virtual/layout_ng_experimental/external/wpt/css/css-multicol/multicol-span-all-fieldset-001.html [ Failure ]
 crbug.com/874051 virtual/layout_ng_experimental/external/wpt/css/css-multicol/multicol-span-all-fieldset-002.html [ Failure ]
 crbug.com/874051 virtual/layout_ng_experimental/external/wpt/css/css-multicol/multicol-span-all-fieldset-003.html [ Failure ]
@@ -1048,7 +1051,7 @@ crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/balance-float-with
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/balance-float-with-margin-top-and-line-after-break.html [ Crash Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/balance-float-with-margin-top-and-line-before-break.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/balance-line-overflow.html [ Failure ]
-crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/balance-line-underflow-1.html [ Crash ]
+crbug.com/829028 virtual/layout_ng_experimental/fast/multicol/balance-line-underflow-1.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/balance-line-underflow-2.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/basic-rtl.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/break-before-first-line-in-first-child.html [ Failure ]
@@ -1106,8 +1109,6 @@ crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/dynamic/remove-and
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/dynamic/remove-and-insert-block-between-spanners.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/dynamic/remove-block-between-spanners.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/dynamic/remove-column-content-next-to-abspos-between-spanners.html [ Failure ]
-crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/dynamic/remove-spanner-after-content.html [ Failure ]
-crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/dynamic/remove-spanner-before-content.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/dynamic/remove-spanner-in-content.html [ Crash Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/dynamic/spanner-after-content-becomes-regular-block.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/dynamic/spanner-ancestor-becomes-spanner.html [ Failure ]
@@ -1183,9 +1184,6 @@ crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/nested-with-forced
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/nested-with-padding.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/nested-with-single-empty-block.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/nested-with-single-tall-line.html [ Failure ]
-crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/newmulticol/avoid-column-break-inside.html [ Failure ]
-crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/newmulticol/balance2.html [ Failure ]
-crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/newmulticol/balance3.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/newmulticol/break-before.html [ Failure ]
 crbug.com/874506 virtual/layout_ng_experimental/fast/multicol/newmulticol/breaks-2-columns-3.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/newmulticol/breaks-2-columns-3-no-balancing.html [ Failure ]
@@ -1238,7 +1236,6 @@ crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/span/empty-spanner
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/span/fill-after-spanner-exact-fit.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/span/fill-after-spanner-extra-height.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/span/float.html [ Failure ]
-crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/span/height-decrease.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/span/height-increase.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/span/in-nested-multicol-with-hard-breaks.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/span/in-nested-multicol-with-soft-breaks-inside.html [ Failure ]