[LayoutNG] Don't remove lines from the fragment builder.

Reverting the fragment builder to an earlier state simply isn't
supported. We used to remove line boxes from the fragment builder after
having laid out too many lines. But this doesn't necessarily revert the
builder to the exact state that it had prior to adding those lines.

Case in point is when we discover and add out-of-flow positioned
descendants among the lines that later on get deleted (see the test). We
don't delete the out-of-flow descendants, so we'd end up with two
identical fragments for the out-of-flow positioned box, one in the
fragmentainer that it should occur, and also one in the preceding
fragmentainer, where we laid out too many lines and then deleted them.

We sometimes need to lay out more lines than we can fit in a
fragmentainer, to figure out where to break in order to honor the widows
requirement. The new approach is to do a full relayout of the container
and stop at the correct line.

This CL introduces a new type NGEarlyBreak, which is currently a
glorified int, but it will soon be extended to also support 'avoid'
values for the break-before and break-after properties.

Bug: 829028
Change-Id: I2d1bc5d51dc43360cbff17401d39d65613dea4c2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1819249Reviewed-by: Koji Ishii <kojii@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#699280}

third_party/blink/renderer/core/layout/BUILD.gn

@@ -444,6 +444,7 @@ blink_core_sources("layout") {
     "ng/ng_constraint_space_builder.h",
     "ng/ng_container_fragment_builder.cc",
     "ng/ng_container_fragment_builder.h",
+    "ng/ng_early_break.h",
     "ng/ng_fieldset_layout_algorithm.cc",
     "ng/ng_fieldset_layout_algorithm.h",
     "ng/ng_flex_layout_algorithm.cc",

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

@@ -19,6 +19,7 @@
 #include "third_party/blink/renderer/core/layout/ng/ng_box_fragment_builder.h"
 #include "third_party/blink/renderer/core/layout/ng/ng_constraint_space.h"
 #include "third_party/blink/renderer/core/layout/ng/ng_constraint_space_builder.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"
 #include "third_party/blink/renderer/core/layout/ng/ng_fragmentation_utils.h"
@@ -137,6 +138,11 @@ LogicalOffset LogicalFromBfcOffsets(const NGBfcOffset& child_bfc_offset,
           child_bfc_offset.block_offset - parent_bfc_offset.block_offset};
 }
 
+inline bool IsEarlyBreakpoint(const NGEarlyBreak& breakpoint,
+                              const NGBoxFragmentBuilder& builder) {
+  return breakpoint.LineNumber() == builder.LineCount();
+}
+
 }  // namespace
 
 NGBlockLayoutAlgorithm::NGBlockLayoutAlgorithm(
@@ -326,12 +332,22 @@ LogicalOffset NGBlockLayoutAlgorithm::CalculateLogicalOffset(
 }
 
 scoped_refptr<const NGLayoutResult> NGBlockLayoutAlgorithm::Layout() {
+  scoped_refptr<const NGLayoutResult> result;
   // Inline children require an inline child layout context to be
   // passed between siblings. We want to stack-allocate that one, but
   // only on demand, as it's quite big.
   if (Node().ChildrenInline())
-    return LayoutWithInlineChildLayoutContext();
-  return Layout(nullptr);
+    result = LayoutWithInlineChildLayoutContext();
+  else
+    result = Layout(nullptr);
+  if (UNLIKELY(result->Status() == NGLayoutResult::kNeedsEarlierBreak)) {
+    // If we found a good break somewhere inside this block, re-layout and break
+    // at that location.
+    DCHECK(!early_break_);
+    DCHECK(result->GetEarlyBreak());
+    return RelayoutAndBreakEarlier(*result->GetEarlyBreak());
+  }
+  return result;
 }
 
 NOINLINE scoped_refptr<const NGLayoutResult>
@@ -357,6 +373,23 @@ NGBlockLayoutAlgorithm::LayoutWithItemsBuilder(
   return result;
 }
 
+NOINLINE scoped_refptr<const NGLayoutResult>
+NGBlockLayoutAlgorithm::RelayoutAndBreakEarlier(
+    const NGEarlyBreak& breakpoint) {
+  NGLayoutAlgorithmParams params(Node(),
+                                 container_builder_.InitialFragmentGeometry(),
+                                 ConstraintSpace(), BreakToken());
+  NGBlockLayoutAlgorithm algorithm_with_break(params);
+  algorithm_with_break.early_break_ = &breakpoint;
+  NGBoxFragmentBuilder& new_builder = algorithm_with_break.container_builder_;
+  new_builder.SetBoxType(container_builder_.BoxType());
+  // We're not going to run out of space in the next layout pass, since we're
+  // breaking earlier, so no space shortage will be detected. Repeat what we
+  // found in this pass.
+  new_builder.PropagateSpaceShortage(container_builder_.MinimalSpaceShortage());
+  return algorithm_with_break.Layout();
+}
+
 inline scoped_refptr<const NGLayoutResult> NGBlockLayoutAlgorithm::Layout(
     NGInlineChildLayoutContext* inline_child_layout_context) {
   const LogicalSize border_box_size = container_builder_.InitialBorderBoxSize();
@@ -526,6 +559,10 @@ inline scoped_refptr<const NGLayoutResult> NGBlockLayoutAlgorithm::Layout(
       }
       break;
     } else {
+      if (UNLIKELY(early_break_ &&
+                   FindEarlyBreakpoint(child, &previous_inflow_position)))
+        break;
+
       bool abort;
       if (child.CreatesNewFormattingContext()) {
         abort = !HandleNewFormattingContext(child, child_break_token,
@@ -726,8 +763,10 @@ scoped_refptr<const NGLayoutResult> NGBlockLayoutAlgorithm::FinishLayout(
   // This may occur with a zero block size fragment. We need to know the BFC
   // block offset to determine where the fragmentation line is relative to us.
   if (container_builder_.BfcBlockOffset() &&
-      ConstraintSpace().HasBlockFragmentation())
-    FinalizeForFragmentation();
+      ConstraintSpace().HasBlockFragmentation()) {
+    if (!FinalizeForFragmentation())
+      return container_builder_.Abort(NGLayoutResult::kNeedsEarlierBreak);
+  }
 
   NGOutOfFlowLayoutPart(
       Node(), ConstraintSpace(),
@@ -755,6 +794,19 @@ scoped_refptr<const NGLayoutResult> NGBlockLayoutAlgorithm::FinishLayout(
   return container_builder_.ToBoxFragment();
 }
 
+bool NGBlockLayoutAlgorithm::FindEarlyBreakpoint(
+    NGLayoutInputNode child,
+    NGPreviousInflowPosition* previous_inflow_position) {
+  DCHECK(early_break_);
+  if (!IsEarlyBreakpoint(*early_break_, container_builder_))
+    return false;
+
+  container_builder_.AddBreakBeforeChild(child, /* is_forced_break */ false);
+  previous_inflow_position->logical_block_offset =
+      FragmentainerSpaceAvailable();
+  return true;
+}
+
 const NGInlineBreakToken* NGBlockLayoutAlgorithm::TryReuseFragmentsFromCache(
     NGInlineNode inline_node,
     NGPreviousInflowPosition* previous_inflow_position,
@@ -1879,7 +1931,7 @@ bool NGBlockLayoutAlgorithm::IsFragmentainerOutOfSpace(
   return block_offset >= FragmentainerSpaceAvailable();
 }
 
-void NGBlockLayoutAlgorithm::FinalizeForFragmentation() {
+bool NGBlockLayoutAlgorithm::FinalizeForFragmentation() {
   if (first_overflowing_line_ && !fit_all_lines_) {
     // A line box overflowed the fragmentainer, but we continued layout anyway,
     // in order to determine where to break in order to honor the widows
@@ -1913,26 +1965,36 @@ void NGBlockLayoutAlgorithm::FinalizeForFragmentation() {
     // detect that there's no room for a fragment for this node, and drop the
     // fragment on the floor. Therefore it doesn't matter how we set up the
     // container builder, so just return.
-    return;
+    return true;
   }
 
-  if (container_builder_.DidBreak() && first_overflowing_line_) {
-    int line_number;
-    if (fit_all_lines_) {
-      line_number = first_overflowing_line_;
-    } else {
-      // We managed to finish layout of all the lines for the node, which means
-      // that we won't have enough widows, unless we break earlier than where we
-      // overflowed.
-      int line_count = container_builder_.LineCount();
-      line_number = std::max(line_count - Style().Widows(),
-                             std::min(line_count, int(Style().Orphans())));
+  if (Node().ChildrenInline()) {
+    if (container_builder_.DidBreak() || first_overflowing_line_) {
+      if (first_overflowing_line_ &&
+          first_overflowing_line_ < container_builder_.LineCount()) {
+        int line_number;
+        if (fit_all_lines_) {
+          line_number = first_overflowing_line_;
+        } else {
+          // We managed to finish layout of all the lines for the node, which
+          // means that we won't have enough widows, unless we break earlier
+          // than where we overflowed.
+          int line_count = container_builder_.LineCount();
+          line_number = std::max(line_count - Style().Widows(),
+                                 std::min(line_count, int(Style().Orphans())));
+        }
+        // We need to layout again, and stop at the right line number.
+        NGEarlyBreak breakpoint(line_number);
+        container_builder_.SetEarlyBreak(breakpoint);
+        return false;
+      }
     }
-    container_builder_.AddBreakBeforeLine(line_number);
   }
 
   FinishFragmentation(&container_builder_, block_size, intrinsic_block_size_,
                       consumed_block_size, space_left);
+
+  return true;
 }
 
 bool NGBlockLayoutAlgorithm::BreakBeforeChild(

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

@@ -20,6 +20,7 @@
 namespace blink {
 
 class NGConstraintSpace;
+class NGEarlyBreak;
 class NGFragment;
 class NGLayoutResult;
 class NGPhysicalLineBoxFragment;
@@ -66,11 +67,22 @@ class CORE_EXPORT NGBlockLayoutAlgorithm
   NOINLINE scoped_refptr<const NGLayoutResult> LayoutWithItemsBuilder(
       NGInlineChildLayoutContext* context);
 
+  // Lay out again, this time with a predefined good breakpoint that we
+  // discovered in the first pass. This happens when we run out of space in a
+  // fragmentainer at an less-than-ideal location, due to breaking restrictions,
+  // such as orphans or widows.
+  NOINLINE scoped_refptr<const NGLayoutResult> RelayoutAndBreakEarlier(
+      const NGEarlyBreak&);
+
   inline scoped_refptr<const NGLayoutResult> Layout(
       NGInlineChildLayoutContext* inline_child_layout_context);
 
   scoped_refptr<const NGLayoutResult> FinishLayout(NGPreviousInflowPosition*);
 
+  // If the child is the one we have already determined to break before, do so
+  // and return true. Otherwise, return false.
+  bool FindEarlyBreakpoint(NGLayoutInputNode child, NGPreviousInflowPosition*);
+
   // Return the BFC block offset of this block.
   LayoutUnit BfcBlockOffset() const {
     // If we have resolved our BFC block offset, use that.
@@ -230,10 +242,13 @@ class CORE_EXPORT NGBlockLayoutAlgorithm
                                  LayoutUnit block_offset,
                                  bool is_pushed_by_floats) const;
 
-  // Final adjustments before fragment creation. We need to prevent the
-  // fragment from crossing fragmentainer boundaries, and rather create a break
-  // token if we're out of space.
-  void FinalizeForFragmentation();
+  // Final adjustments before fragment creation. We need to prevent the fragment
+  // from crossing fragmentainer boundaries, and rather create a break token if
+  // we're out of space. As part of finalizing we may also discover that we need
+  // to abort layout, because we've run out of space at a less-than-ideal
+  // location. In this case, false will be returned. Otherwise, true will be
+  // returned.
+  bool FinalizeForFragmentation();
 
   void PropagateBaselinesFromChildren();
   bool AddBaseline(const NGBaselineRequest&,
@@ -364,6 +379,9 @@ class CORE_EXPORT NGBlockLayoutAlgorithm
   bool has_processed_first_child_ = false;
 
   NGExclusionSpace exclusion_space_;
+
+  // When set, this will specify where to break before or inside.
+  const NGEarlyBreak* early_break_ = nullptr;
 };
 
 }  // namespace blink

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

@@ -100,33 +100,6 @@ void NGBoxFragmentBuilder::AddBreakBeforeChild(NGLayoutInputNode child,
   child_break_tokens_.push_back(token);
 }
 
-void NGBoxFragmentBuilder::AddBreakBeforeLine(int line_number) {
-  DCHECK(has_block_fragmentation_);
-  DCHECK_GT(line_number, 0);
-  DCHECK_LE(unsigned(line_number), inline_break_tokens_.size());
-  SetDidBreak();
-  int lines_to_remove = inline_break_tokens_.size() - line_number;
-  if (lines_to_remove > 0) {
-    // Remove widows that should be pushed to the next fragment. We'll also
-    // remove all other child fragments than line boxes (typically floats) that
-    // come after the first line that's moved, as those also have to be re-laid
-    // out in the next fragment.
-    inline_break_tokens_.resize(line_number);
-    DCHECK_GT(children_.size(), 0UL);
-    for (int i = children_.size() - 1; i >= 0; i--) {
-      DCHECK_NE(i, 0);
-      if (!children_[i].fragment->IsLineBox())
-        continue;
-      if (!--lines_to_remove) {
-        // This is the first line that is going to the next fragment. Remove it,
-        // and everything after it.
-        children_.resize(i);
-        break;
-      }
-    }
-  }
-}
-
 void NGBoxFragmentBuilder::AddResult(const NGLayoutResult& child_layout_result,
                                      const LogicalOffset offset,
                                      const LayoutInline* inline_container) {

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

@@ -63,6 +63,11 @@ class CORE_EXPORT NGBoxFragmentBuilder final
     is_initial_block_size_indefinite_ = size_.block_size == kIndefiniteSize;
   }
 
+  const NGFragmentGeometry& InitialFragmentGeometry() const {
+    DCHECK(initial_fragment_geometry_);
+    return *initial_fragment_geometry_;
+  }
+
   void SetIntrinsicBlockSize(LayoutUnit intrinsic_block_size) {
     intrinsic_block_size_ = intrinsic_block_size;
   }
@@ -89,9 +94,6 @@ class CORE_EXPORT NGBoxFragmentBuilder final
   // add a break token for the child, but no fragment.
   void AddBreakBeforeChild(NGLayoutInputNode child, bool is_forced_break);
 
-  // Prepare for a break token before the specified line.
-  void AddBreakBeforeLine(int line_number);
-
   // Add a layout result. This involves appending the fragment and its relative
   // offset to the builder, but also keeping track of out-of-flow positioned
   // descendants, propagating fragmentainer breaks, and more.
@@ -128,6 +130,7 @@ class CORE_EXPORT NGBoxFragmentBuilder final
     if (minimal_space_shortage_ > space_shortage)
       minimal_space_shortage_ = space_shortage;
   }
+  LayoutUnit MinimalSpaceShortage() const { return minimal_space_shortage_; }
 
   void SetInitialBreakBefore(EBreakBetween break_before) {
     initial_break_before_ = break_before;
@@ -164,6 +167,9 @@ class CORE_EXPORT NGBoxFragmentBuilder final
   void SetColumnSpanner(NGBlockNode spanner) { column_spanner_ = spanner; }
   bool FoundColumnSpanner() const { return !!column_spanner_; }
 
+  void SetEarlyBreak(NGEarlyBreak breakpoint) { early_break_ = breakpoint; }
+  bool HasEarlyBreak() const { return early_break_.has_value(); }
+
   // Offsets are not supposed to be set during fragment construction, so we
   // do not provide a setter here.
 

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

@@ -1876,6 +1876,57 @@ TEST_F(NGColumnLayoutAlgorithmTest, UnsatisfiableOrphansAndWidows) {
   EXPECT_EQ(expectation, dump);
 }
 
+TEST_F(NGColumnLayoutAlgorithmTest, WidowsAndAbspos) {
+  SetBodyInnerHTML(R"HTML(
+    <style>
+      #parent {
+        columns: 3;
+        column-fill: auto;
+        column-gap: 10px;
+        width: 320px;
+        height: 70px;
+        line-height: 20px;
+        orphans: 1;
+        widows: 3;
+      }
+    </style>
+    <div id="container">
+      <div id="parent">
+        <div style="position:relative;">
+          <br>
+          <br>
+          <br>
+          <br>
+          <div style="position:absolute; width:33px; height:33px;"></div>
+          <br>
+        </div>
+      </div>
+    </div>
+  )HTML");
+
+  String dump = DumpFragmentTree(GetElementById("container"));
+  String expectation = R"DUMP(.:: LayoutNG Physical Fragment Tree ::.
+  offset:unplaced size:1000x70
+    offset:0,0 size:320x70
+      offset:0,0 size:100x70
+        offset:0,0 size:100x70
+          offset:0,0 size:0x20
+            offset:0,9 size:0x1
+          offset:0,20 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:0,40 size:33x33
+)DUMP";
+  EXPECT_EQ(expectation, dump);
+}
+
 // TODO(crbug.com/915929): Fix inline-level float fragmentation.
 TEST_F(NGColumnLayoutAlgorithmTest, DISABLED_FloatInBlockMovedByOrphans) {
   SetBodyInnerHTML(R"HTML(

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

@@ -13,6 +13,7 @@
 #include "third_party/blink/renderer/core/layout/ng/geometry/ng_margin_strut.h"
 #include "third_party/blink/renderer/core/layout/ng/inline/ng_physical_text_fragment.h"
 #include "third_party/blink/renderer/core/layout/ng/list/ng_unpositioned_list_marker.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_builder.h"
 #include "third_party/blink/renderer/core/layout/ng/ng_link.h"
@@ -214,6 +215,8 @@ class CORE_EXPORT NGContainerFragmentBuilder : public NGFragmentBuilder {
   NGBreakTokenVector child_break_tokens_;
   NGBreakTokenVector inline_break_tokens_;
 
+  base::Optional<NGEarlyBreak> early_break_;
+
   NGAdjoiningObjectTypes adjoining_object_types_ = kAdjoiningNone;
   bool has_adjoining_object_descendants_ = false;
 

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

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef THIRD_PARTY_BLINK_RENDERER_CORE_LAYOUT_NG_NG_EARLY_BREAK_H_
+#define THIRD_PARTY_BLINK_RENDERER_CORE_LAYOUT_NG_NG_EARLY_BREAK_H_
+
+namespace blink {
+
+// Possible early unforced breakpoint. This represents a possible (and good)
+// location to break. In cases where we run out of space at an unideal location,
+// we may want to go back and break here instead.
+class NGEarlyBreak {
+ public:
+  explicit NGEarlyBreak(int line_number) : line_number_(line_number) {}
+
+  int LineNumber() const { return line_number_; }
+
+ private:
+  int line_number_;
+};
+
+}  // namespace blink
+
+#endif  // THIRD_PARTY_BLINK_RENDERER_CORE_LAYOUT_NG_NG_EARLY_BREAK_H_

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

@@ -155,6 +155,11 @@ NGLayoutResult::NGLayoutResult(
     space_.ExclusionSpace().MoveDerivedGeometry(builder->exclusion_space_);
   }
 
+  // If we found an early breakpoint inside that we need to break at, we're
+  // going to re-layout now, and break at the early breakpoint.
+  if (builder->early_break_ && !physical_fragment_)
+    EnsureRareData()->early_break = *builder->early_break_;
+
   if (HasRareData()) {
     rare_data_->bfc_line_offset = builder->bfc_line_offset_;
     rare_data_->bfc_block_offset = builder->bfc_block_offset_;

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

@@ -13,6 +13,7 @@
 #include "third_party/blink/renderer/core/layout/ng/geometry/ng_margin_strut.h"
 #include "third_party/blink/renderer/core/layout/ng/list/ng_unpositioned_list_marker.h"
 #include "third_party/blink/renderer/core/layout/ng/ng_block_node.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"
 #include "third_party/blink/renderer/core/layout/ng/ng_link.h"
@@ -38,6 +39,7 @@ class CORE_EXPORT NGLayoutResult : public RefCounted<NGLayoutResult> {
   enum NGLayoutResultStatus {
     kSuccess = 0,
     kBfcBlockOffsetResolved = 1,
+    kNeedsEarlierBreak = 2,
     // When adding new values, make sure the bit size of |Bitfields::status| is
     // large enough to store.
   };
@@ -75,6 +77,12 @@ class CORE_EXPORT NGLayoutResult : public RefCounted<NGLayoutResult> {
     return HasRareData() ? rare_data_->column_spanner : NGBlockNode(nullptr);
   }
 
+  const NGEarlyBreak* GetEarlyBreak() const {
+    if (!HasRareData())
+      return nullptr;
+    return &rare_data_->early_break.value();
+  }
+
   const NGExclusionSpace& ExclusionSpace() const {
     if (bitfields_.has_rare_data_exclusion_space) {
       DCHECK(HasRareData());
@@ -294,6 +302,7 @@ class CORE_EXPORT NGLayoutResult : public RefCounted<NGLayoutResult> {
     LayoutUnit bfc_line_offset;
     base::Optional<LayoutUnit> bfc_block_offset;
 
+    base::Optional<NGEarlyBreak> early_break;
     LogicalOffset oof_positioned_offset;
     NGMarginStrut end_margin_strut;
     NGUnpositionedListMarker unpositioned_list_marker;
@@ -362,7 +371,7 @@ class CORE_EXPORT NGLayoutResult : public RefCounted<NGLayoutResult> {
     unsigned initial_break_before : 4;  // EBreakBetween
     unsigned final_break_after : 4;     // EBreakBetween
 
-    unsigned status : 1;  // NGLayoutResultStatus
+    unsigned status : 2;  // NGLayoutResultStatus
   };
 
   // The constraint space which generated this layout result, may not be valid