Improve breaking before nested multicols and their rows.

1. Avoid breaking inside an inner multicol fragment if there were
suboptimal breaks inside even if the inner multicol container didn't
break in the outer fragmentation context. multicol-nested-018.html,
multicol-nested-022.html and multicol-nested-023.html test this.

2. Properly constrain balanced column block-size to remaining space in
the outer fragmentainer.  We used to let unbreakable content override
the remaining space, but this is wrong. This fixes
forced-break-too-short-column.html and
nested-short-first-row-extra-tall-line.html

Store break appeal in NGLayoutResult, rather than in NGBreakToken. The
reason is that we may have a violating break inside a fragment that
happened inside a nested fragmentation context, even if the fragment
doesn't necessarily break within the outer fragmentation context.

Also create a proper break token inside a nested multicol container if
there is none and we want to break before the first piece of column
content. Calling SetDidBreakSelf() manually, like we used to, confused
FinishFragmentation() into thinking that we were past the end of the
block-end content box of the multicol container. This is tested by
nested-with-padding.html Because of this change, the code in
PreviousInnerFragmentainerIndex() needed an update, to ignore this break
token, since break-before tokens don't have a sequence number.

Remove the has_violating_descendant_break flag, and use the break appeal
in the layout result instead. We had a column-balancing bug where where
we were missing break-inside:avoid violations, because such violations
don't affect the child's stored break appeal (the reason for this is
explained in further detail inside CalculateBreakAppealInside()).
Such violations were already propagated correctly to the builder's break
appeal, so now it just works. This is tested by
multicol-fill-balance-014.html . Also remove
LayoutNGBlockFragmentation-specific baselines for
moz-multicol3-column-balancing-break-inside-avoid-1.html because of
this, as we now render identically to the legacy engine.

This change fixes some existing tests. It also made
outer-column-break-after-inner-spanner-2.html render correctly, but not
according to the expectation, which was written for the legacy engine.
The legacy engine fails to push the block with the BR inside entirely to
the next outer fragmentainer. So replce it with a correct test:
multicol-nested-017.html

Bug: 829028
Change-Id: I13b2d95a0eb0407a82c8c24070a6dff4d2f620e1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3150413
Commit-Queue: Morten Stenshorne <[email protected]>
Reviewed-by: Ian Kilpatrick <[email protected]>
Cr-Commit-Position: refs/heads/main@{#919825}

Author: mstensho

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

@@ -42,7 +42,6 @@ NGBlockBreakToken::NGBlockBreakToken(PassKey key,
           builder.consumed_block_size_legacy_adjustment_),
       sequence_number_(builder.sequence_number_),
       const_num_children_(builder.child_break_tokens_.size()) {
-  break_appeal_ = builder.break_appeal_;
   has_seen_all_children_ = builder.has_seen_all_children_;
   is_caused_by_column_spanner_ = builder.FoundColumnSpanner();
   is_at_block_end_ = builder.is_at_block_end_;

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

@@ -35,12 +35,6 @@ void NGBoxFragmentBuilder::AddBreakBeforeChild(
     DCHECK(!appeal || *appeal == kBreakAppealPerfect);
   } else if (appeal) {
     ClampBreakAppeal(*appeal);
-    // If we're violating any orphans / widows or
-    // break-{after,before,inside}:avoid requests, or if we're inserting a
-    // last-resort break, remember this. If we're balancing columns, we may be
-    // able to stretch the columns to resolve the situation.
-    if (*appeal < kBreakAppealPerfect)
-      has_violating_descendant_break_ = true;
   }
 
   DCHECK(has_block_fragmentation_);
@@ -380,9 +374,6 @@ void NGBoxFragmentBuilder::PropagateBreak(
     PropagateSpaceShortage(child_layout_result.MinimalSpaceShortage());
   }
 
-  if (!is_fragmentation_context_root_)
-    has_violating_descendant_break_ |= child_layout_result.HasViolatingBreak();
-
   // If a spanner was found inside the child, we need to finish up and propagate
   // the spanner to the column layout algorithm, so that it can take care of it.
   if (UNLIKELY(ConstraintSpace()->IsInColumnBfc())) {

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

@@ -368,12 +368,6 @@ class CORE_EXPORT NGBoxFragmentBuilder final
   void SetIsAtBlockEnd() { is_at_block_end_ = true; }
   bool IsAtBlockEnd() const { return is_at_block_end_; }
 
-  // True if we have inserted a break before or inside a descendant at a
-  // less-than-ideal breakpoint, i.e. if the break appeal isn't perfect.
-  bool HasViolatingDescendantBreak() const {
-    return has_violating_descendant_break_;
-  }
-
   // See |NGPhysicalBoxFragment::InflowBounds|.
   void SetInflowBounds(const LogicalRect& inflow_bounds) {
     DCHECK_NE(box_type_, NGPhysicalBoxFragment::NGBoxType::kInlineBox);
@@ -619,7 +613,6 @@ class CORE_EXPORT NGBoxFragmentBuilder final
   bool is_math_fraction_ = false;
   bool is_math_operator_ = false;
   bool is_at_block_end_ = false;
-  bool has_violating_descendant_break_ = false;
   LayoutUnit consumed_block_size_;
   LayoutUnit consumed_block_size_legacy_adjustment_;
   LayoutUnit block_offset_for_additional_columns_;

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

@@ -7,7 +7,6 @@
 
 #include "base/dcheck_is_on.h"
 #include "third_party/blink/renderer/core/core_export.h"
-#include "third_party/blink/renderer/core/layout/ng/ng_break_appeal.h"
 #include "third_party/blink/renderer/core/layout/ng/ng_layout_input_node.h"
 
 namespace blink {
@@ -47,10 +46,6 @@ class CORE_EXPORT NGBreakToken : public GarbageCollected<NGBreakToken> {
         box_, static_cast<NGLayoutInputNode::NGLayoutInputNodeType>(type_));
   }
 
-  NGBreakAppeal BreakAppeal() const {
-    return static_cast<NGBreakAppeal>(break_appeal_);
-  }
-
 #if DCHECK_IS_ON()
   virtual String ToString() const;
   void ShowBreakTokenTree() const;
@@ -69,7 +64,6 @@ class CORE_EXPORT NGBreakToken : public GarbageCollected<NGBreakToken> {
         is_forced_break_(false),
         is_caused_by_column_spanner_(false),
         is_at_block_end_(false),
-        break_appeal_(kBreakAppealPerfect),
         has_seen_all_children_(false) {
     DCHECK_EQ(type, static_cast<NGBreakTokenType>(node.Type()));
   }
@@ -102,11 +96,6 @@ class CORE_EXPORT NGBreakToken : public GarbageCollected<NGBreakToken> {
   // a parallel flow.
   unsigned is_at_block_end_ : 1;
 
-  // If the break is unforced, this is the appeal of the break. Higher is
-  // better. Violating breaking rules decreases appeal. Forced breaks always
-  // have perfect appeal.
-  unsigned break_appeal_ : kNGBreakAppealBitsNeeded;  // NGBreakAppeal
-
   // All children of this container have been "seen" at this point. This means
   // that all children have been fully laid out, or have break tokens. No more
   // children left to discover.

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

@@ -464,9 +464,14 @@ NGBreakStatus NGColumnLayoutAlgorithm::LayoutChildren() {
           // children, but "continue" layout, so that we produce a
           // fragment. Note that we normally don't want to break right after
           // initial border/padding, but will do so as a last resort. It's up to
-          // our containing block to decide what's best. In case there is no
-          // break token inside, we need to manually mark that we broke.
-          container_builder_.SetDidBreakSelf();
+          // our containing block to decide what's best. If there's no incoming
+          // break token, it means that we're at the very start of column
+          // layout, and we need to create a break token before the first
+          // column.
+          if (!child_break_token) {
+            container_builder_.AddBreakBeforeChild(
+                Node(), kBreakAppealLastResort, /* is_forced_break */ false);
+          }
 
           break;
         }
@@ -732,7 +737,7 @@ scoped_refptr<const NGLayoutResult> NGColumnLayoutAlgorithm::LayoutRow(
         break;
       }
 
-      has_violating_break |= result->HasViolatingBreak();
+      has_violating_break |= result->BreakAppeal() != kBreakAppealPerfect;
       column_inline_offset += column_inline_progression_;
 
       if (result->HasForcedBreak())
@@ -762,11 +767,9 @@ scoped_refptr<const NGLayoutResult> NGColumnLayoutAlgorithm::LayoutRow(
         // lower appeal than what we've seen so far. Anything that would cause
         // "too severe" breaking violations will be pushed to the next outer
         // fragmentainer.
-        if (column_break_token) {
-          min_break_appeal =
-              std::min(min_break_appeal.value_or(kBreakAppealPerfect),
-                       column_break_token->BreakAppeal());
-        }
+        min_break_appeal =
+            std::min(min_break_appeal.value_or(kBreakAppealPerfect),
+                     result->BreakAppeal());
 
         // Avoid creating rows that are too short to hold monolithic content.
         // Bail, discarding all columns. Note that this is safe to do even if
@@ -1233,6 +1236,9 @@ LayoutUnit NGColumnLayoutAlgorithm::CalculateBalancedColumnBlockSize(
 LayoutUnit NGColumnLayoutAlgorithm::ConstrainColumnBlockSize(
     LayoutUnit size,
     LayoutUnit row_offset) const {
+  // Avoid becoming shorter than the tallest piece of unbreakable content.
+  size = std::max(size, tallest_unbreakable_block_size_);
+
   if (is_constrained_by_outer_fragmentation_context_) {
     // Don't become too tall to fit in the outer fragmentation context.
     LayoutUnit available_outer_space =
@@ -1241,9 +1247,6 @@ LayoutUnit NGColumnLayoutAlgorithm::ConstrainColumnBlockSize(
     size = std::min(size, available_outer_space);
   }
 
-  // But avoid becoming shorter than the tallest piece of unbreakable content.
-  size = std::max(size, tallest_unbreakable_block_size_);
-
   // The {,min-,max-}block-size properties are specified on the multicol
   // container, but here we're calculating the column block sizes inside the
   // multicol container, which isn't exactly the same. We may shrink the column

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

@@ -207,23 +207,26 @@ NGBreakAppeal CalculateBreakAppealBefore(const NGConstraintSpace& space,
   return break_appeal;
 }
 
-NGBreakAppeal CalculateBreakAppealInside(const NGConstraintSpace& space,
-                                         const NGLayoutResult& layout_result) {
+NGBreakAppeal CalculateBreakAppealInside(
+    const NGConstraintSpace& space,
+    const NGLayoutResult& layout_result,
+    absl::optional<NGBreakAppeal> hypothetical_appeal) {
   if (layout_result.HasForcedBreak())
     return kBreakAppealPerfect;
   const auto& physical_fragment = layout_result.PhysicalFragment();
-  NGBreakAppeal appeal = kBreakAppealLastResort;
-  // If we actually broke, get the appeal from the break token. Otherwise, get
-  // the early break appeal.
-  if (const auto* block_break_token =
-          DynamicTo<NGBlockBreakToken>(physical_fragment.BreakToken())) {
-    appeal = block_break_token->BreakAppeal();
-  } else if (const NGEarlyBreak* early_break = layout_result.GetEarlyBreak()) {
-    appeal = early_break->BreakAppeal();
+  const auto* break_token =
+      DynamicTo<NGBlockBreakToken>(physical_fragment.BreakToken());
+  NGBreakAppeal appeal;
+  bool consider_break_inside_avoidance;
+  if (hypothetical_appeal) {
+    // The hypothetical appeal of breaking inside should only be considered if
+    // we haven't actually broken.
+    DCHECK(!break_token);
+    appeal = *hypothetical_appeal;
+    consider_break_inside_avoidance = true;
   } else {
-    // If we have neither a break token nor an early-break object, we shouldn't
-    // really be here.
-    NOTREACHED();
+    appeal = layout_result.BreakAppeal();
+    consider_break_inside_avoidance = break_token;
   }
 
   // We don't let break-inside:avoid affect the child's stored break appeal, but
@@ -233,7 +236,8 @@ NGBreakAppeal CalculateBreakAppealInside(const NGConstraintSpace& space,
   // rule on the child itself. This prevents us from violating more rules than
   // necessary: if we need to break inside the child (even if it should be
   // avoided), we'll at least break at the most appealing location inside.
-  if (appeal > kBreakAppealViolatingBreakAvoid &&
+  if (consider_break_inside_avoidance &&
+      appeal > kBreakAppealViolatingBreakAvoid &&
       IsAvoidBreakValue(space, physical_fragment.Style().BreakInside()))
     appeal = kBreakAppealViolatingBreakAvoid;
   return appeal;
@@ -781,14 +785,14 @@ bool MovePastBreakpoint(const NGConstraintSpace& space,
     return false;
   }
 
-  if (break_token) {
-    // 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.
-    NGBreakAppeal appeal_inside =
-        CalculateBreakAppealInside(space, layout_result);
-    // Allow breaking inside if it has the same appeal or higher than breaking
-    // before or breaking earlier. Also, if breaking before is impossible, break
-    // inside regardless of appeal.
+  NGBreakAppeal appeal_inside =
+      CalculateBreakAppealInside(space, layout_result);
+  if (break_token || appeal_inside < kBreakAppealPerfect) {
+    // The block child broke inside, either in this fragmentation context, or in
+    // an inner one. We now need to decide whether to keep that break, or if it
+    // would be better to break before it. Allow breaking inside if it has the
+    // same appeal or higher than breaking before or breaking earlier. Also, if
+    // breaking before is impossible, break inside regardless of appeal.
     if (refuse_break_before)
       return true;
     if (appeal_inside >= appeal_before &&
@@ -828,7 +832,8 @@ void UpdateEarlyBreakAtBlockChild(const NGConstraintSpace& space,
   // See if there's a good breakpoint inside the child.
   NGBreakAppeal appeal_inside = kBreakAppealLastResort;
   if (const NGEarlyBreak* breakpoint = layout_result.GetEarlyBreak()) {
-    appeal_inside = CalculateBreakAppealInside(space, layout_result);
+    appeal_inside = CalculateBreakAppealInside(space, layout_result,
+                                               breakpoint->BreakAppeal());
     if (!builder->HasEarlyBreak() ||
         builder->EarlyBreak().BreakAppeal() <= breakpoint->BreakAppeal()) {
       // Found a good breakpoint inside the child. Add the child to the early
@@ -1030,7 +1035,13 @@ wtf_size_t PreviousInnerFragmentainerIndex(
         // Not a fragmentainer (probably a spanner)
         continue;
       }
-      idx = To<NGBlockBreakToken>(child_token.Get())->SequenceNumber() + 1;
+      const auto& block_child_token = To<NGBlockBreakToken>(*child_token);
+      // There may be a break before the first column, if we had to break
+      // between the block-start border/padding of the multicol container and
+      // its contents due to space shortage.
+      if (block_child_token.IsBreakBefore())
+        continue;
+      idx = block_child_token.SequenceNumber() + 1;
       break;
     }
   }

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

@@ -74,9 +74,14 @@ NGBreakAppeal CalculateBreakAppealBefore(const NGConstraintSpace&,
                                          const NGBoxFragmentBuilder&,
                                          bool has_container_separation);
 
-// Calculate the appeal of breaking inside this child.
-NGBreakAppeal CalculateBreakAppealInside(const NGConstraintSpace& space,
-                                         const NGLayoutResult&);
+// Calculate the appeal of breaking inside this child. The appeal is based on
+// the one stored in the layout result, unless hypothetical_appeal is specified.
+// hypothetical_appeal is used to assess the appeal at breakpoints where we
+// didn't break, but still need to consider (see NGEarlyBreak).
+NGBreakAppeal CalculateBreakAppealInside(
+    const NGConstraintSpace& space,
+    const NGLayoutResult&,
+    absl::optional<NGBreakAppeal> hypothetical_appeal = absl::nullopt);
 
 // To ensure content progression, we need fragmentainers to hold something
 // larger than 0. The spec says that fragmentainers have to accept at least 1px

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

@@ -84,11 +84,7 @@ NGLayoutResult::NGLayoutResult(
       rare_data->minimal_space_shortage = builder->minimal_space_shortage_;
     }
 
-    bitfields_.has_violating_break =
-        builder->HasViolatingDescendantBreak() ||
-        (builder->DidBreakSelf() &&
-         builder->break_appeal_ < kBreakAppealPerfect);
-
+    bitfields_.break_appeal = builder->break_appeal_;
     bitfields_.initial_break_before = static_cast<unsigned>(
         builder->initial_break_before_.value_or(EBreakBetween::kAuto));
     bitfields_.final_break_after =

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

@@ -15,6 +15,7 @@
 #include "third_party/blink/renderer/core/layout/ng/geometry/ng_margin_strut.h"
 #include "third_party/blink/renderer/core/layout/ng/grid/layout_ng_grid.h"
 #include "third_party/blink/renderer/core/layout/ng/ng_block_node.h"
+#include "third_party/blink/renderer/core/layout/ng/ng_break_appeal.h"
 #include "third_party/blink/renderer/core/layout/ng/ng_early_break.h"
 #include "third_party/blink/renderer/core/layout/ng/ng_floats_utils.h"
 #include "third_party/blink/renderer/core/layout/ng/ng_fragment.h"
@@ -223,11 +224,25 @@ class CORE_EXPORT NGLayoutResult : public RefCounted<NGLayoutResult> {
     return rare_data_->tallest_unbreakable_block_size;
   }
 
-  // Return true if we weren't able to honor all break avoidance hints requested
-  // by break-{after,before,inside}:avoid or orphans / widows, or if we had to
-  // break at an invalid breakpoint (e.g. before/after the first/last
-  // child). This is used for column balancing.
-  bool HasViolatingBreak() const { return bitfields_.has_violating_break; }
+  // Return the (lowest) appeal among any unforced breaks inside the resulting
+  // fragment (or kBreakAppealPerfect if there are no such breaks).
+  //
+  // A higher value is better. Violating breaking rules decreases appeal. Forced
+  // breaks always have perfect appeal.
+  //
+  // If a node breaks, the resulting fragment usually carries an outgoing break
+  // token, but this isn't necessarily the case if the break happened inside an
+  // inner fragmentation context. The block-size of an inner multicol is
+  // constrained by the available block-size in the outer fragmentation
+  // context. This may cause suboptimal column breaks inside. The entire inner
+  // multicol container may fit in the outer fragmentation context, but we may
+  // also need to consider the inner column breaks (in an inner fragmentation
+  // context). If there are any suboptimal breaks, we may want to push the
+  // entire multicol container to the next outer fragmentainer, if it's likely
+  // that we'll avoid suboptimal column breaks inside that way.
+  NGBreakAppeal BreakAppeal() const {
+    return static_cast<NGBreakAppeal>(bitfields_.break_appeal);
+  }
 
   SerializedScriptValue* CustomLayoutData() const {
     return HasRareData() ? rare_data_->custom_layout_data.get() : nullptr;
@@ -528,7 +543,7 @@ class CORE_EXPORT NGLayoutResult : public RefCounted<NGLayoutResult> {
           can_use_out_of_flow_positioned_first_tier_cache(false),
           is_bfc_block_offset_nullopt(false),
           has_forced_break(false),
-          has_violating_break(false),
+          break_appeal(kBreakAppealPerfect),
           is_empty_spanner_parent(false),
           is_self_collapsing(is_self_collapsing),
           is_pushed_by_floats(is_pushed_by_floats),
@@ -548,7 +563,7 @@ class CORE_EXPORT NGLayoutResult : public RefCounted<NGLayoutResult> {
     unsigned is_bfc_block_offset_nullopt : 1;
 
     unsigned has_forced_break : 1;
-    unsigned has_violating_break : 1;
+    unsigned break_appeal : kNGBreakAppealBitsNeeded;
     unsigned is_empty_spanner_parent : 1;
 
     unsigned is_self_collapsing : 1;