[LayoutNG] Fix order in which fragments are added in OOF frag

In NGSimplifiedOOFLayoutAlgorithm, the order in which fragments are
added to the builder is important and apparently more complex than I
initially thought. The way it was implemented, it caused problem when
we were trying to find the matching break token since the order was
different. For more info on the new implementation, see my comments in
ng_simplified_oof_layout_algorithm.cc.

Fixing the order fixed all disabled tests in NGOutOfFlowLayoutPartTest,
but one related to nested fragmentation that is unrelated.

Bug: 1143301, 1117625, 1148875
Change-Id: I0e2c559930da3f0042ada106c334d35521163d72
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2552076
Commit-Queue: Benjamin Beaudry <benjamin.beaudry@microsoft.com>
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Reviewed-by: Alison Maher <almaher@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#832006}

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

@@ -100,8 +100,7 @@ void NGFragmentChildIterator::UpdateSelfFromFragment(
     if (layout_object &&
         layout_object !=
             current_.block_break_token_->InputNode().GetLayoutBox()) {
-      DCHECK(current_.link_.fragment->IsColumnSpanAll() ||
-             current_.block_break_token_->InputNode().IsOutOfFlowPositioned());
+      DCHECK(current_.link_.fragment->IsColumnSpanAll());
       current_.break_token_for_fragmentainer_only_ = true;
     } else {
       current_.break_token_for_fragmentainer_only_ = false;

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

@@ -1108,12 +1108,8 @@ void NGOutOfFlowLayoutPart::AddOOFResultsToFragmentainer(
   // NGSimplifiedOOFLayoutAlgorithm::Layout, we can append new items to it.
   NGSimplifiedOOFLayoutAlgorithm algorithm(params, fragment, is_new_fragment);
 
-  for (const auto& result : results) {
-    // TODO(bebeaudr): Is the offset returned by OutOfFlowPositionedOffset the
-    // one to use?
-    algorithm.AppendOutOfFlowResult(result,
-                                    result->OutOfFlowPositionedOffset());
-  }
+  for (const auto& result : results)
+    algorithm.AppendOutOfFlowResult(result);
 
   if (is_new_fragment) {
     LogicalOffset offset;

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

@@ -569,10 +569,8 @@ TEST_F(NGOutOfFlowLayoutPartTest, ChildBreakAfterAvoid) {
 
 // Tests that a positioned element with a negative top property moves the OOF
 // node to the previous fragmentainer and spans 3 columns.
-// TODO(bebeaudr): Figure out why this is crashing. https://crbug.com/1117625.
-TEST_F(
-    NGOutOfFlowLayoutPartTest,
-    DISABLED_PositionedFragmentationWithNegativeTopPropertyAndNewEmptyColumn) {
+TEST_F(NGOutOfFlowLayoutPartTest,
+       PositionedFragmentationWithNegativeTopPropertyAndNewEmptyColumn) {
   SetBodyInnerHTML(
       R"HTML(
       <style>
@@ -588,7 +586,7 @@ TEST_F(
       </style>
       <div id="container">
         <div id="multicol">
-          <div class="rel" style="height: 60px; width: 32px;"></div>
+          <div style="height: 60px; width: 32px;"></div>
           <div class="rel">
             <div class="abs"></div>
           </div>
@@ -605,8 +603,8 @@ TEST_F(
         offset:0,20 size:5x20
       offset:508,0 size:492x40
         offset:0,0 size:32x20
-        offset:0,20 size:30x0
         offset:0,0 size:5x40
+        offset:0,20 size:30x0
       offset:1016,0 size:492x40
         offset:0,0 size:5x20
 )DUMP";

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

@@ -26,45 +26,100 @@ NGSimplifiedOOFLayoutAlgorithm::NGSimplifiedOOFLayoutAlgorithm(
       params.space.FragmentainerBlockSize());
 
   // Don't apply children to new fragments.
-  if (is_new_fragment)
+  if (is_new_fragment) {
+    children_ = {};
+    iterator_ = children_.end();
     return;
+  }
 
   // We need the previous physical container size to calculate the position of
   // any child fragments.
   previous_physical_container_size_ = fragment.Size();
 
-  // The OOF fragments need to be added after the already existing child
-  // fragments. Add them now so they are added before we append the OOF results.
-  for (const auto& child_link : fragment.Children()) {
-    AddChildFragment(child_link,
-                     *To<NGPhysicalContainerFragment>(child_link.get()));
+  // The OOF fragments need to be added in a particular order that matches the
+  // order of break tokens. Here's a list of rules to follow , in order:
+  // 1. If there are any already appended fragments that are a continuation of
+  //    layout (i.e. they are the Nth fragment of a layout box, where N > 1),
+  //    they should be appended before anything else.
+  // 2. If we're trying to append a fragment that is a continuation of layout
+  //    for an OOF node (from AppendOutOfFlowResult), add it after the fragments
+  //    added in step 1.
+  // 3. Add the remaining children that were not appended during step 1.
+  // 4. Add the OOF fragments that were not a continuation of layout, the ones
+  //    that weren't appended in step 2.
+  children_ = fragment.Children();
+  iterator_ = children_.begin();
+
+  while (iterator_ != children_.end()) {
+    const auto& child_link = *iterator_;
+    const auto* child_fragment = To<NGPhysicalBoxFragment>(child_link.get());
+    if (!child_fragment->IsFirstForNode()) {
+      AddChildFragment(child_link);
+      iterator_++;
+    } else {
+      // We can break here because fragments that are a continuation of layout
+      // are always the first children of a physical fragment.
+      break;
+    }
   }
 }
 
 scoped_refptr<const NGLayoutResult> NGSimplifiedOOFLayoutAlgorithm::Layout() {
+  // There might not be any children to append, whether it's because we are in a
+  // new fragmentainer or because they have all been added in Step 1.
+  if (iterator_ != children_.end()) {
+    // Step 3: Add the remaining children that were not added in step 1.
+    while (iterator_ != children_.end()) {
+      const auto& child_link = *iterator_;
+      const auto* child_fragment = To<NGPhysicalBoxFragment>(child_link.get());
+      DCHECK(child_fragment->IsFirstForNode());
+      AddChildFragment(child_link);
+      iterator_++;
+    }
+
+    // Step 4: Add the OOF fragments that aren't a continuation of layout.
+    for (auto result : remaining_oof_results_) {
+      container_builder_.AddResult(*result,
+                                   result->OutOfFlowPositionedOffset());
+    }
+  }
   return container_builder_.ToBoxFragment();
 }
 
 void NGSimplifiedOOFLayoutAlgorithm::AppendOutOfFlowResult(
-    scoped_refptr<const NGLayoutResult> result,
-    LogicalOffset offset) {
-  // Add the new result to the builder.
-  container_builder_.AddResult(*result, offset);
+    scoped_refptr<const NGLayoutResult> result) {
+  // Add the new result directly to the builder when the fragment of the result
+  // to append is not the first fragment of its corresponding layout box,
+  // meaning that it's positioned directly at the start of the fragmentainer.
+  // This ensures that we keep the fragments and the break tokens in order.
+  //
+  // Also add the result directly to the builder if there are no more children
+  // to append. This can happen when all children have been added in Step 1 or
+  // when we are in a new fragmentainer since a new fragmentainer doesn't have
+  // any child.
+  if (iterator_ == children_.end() ||
+      !To<NGPhysicalBoxFragment>(result->PhysicalFragment()).IsFirstForNode()) {
+    // Step 2: Add the fragments that are a continuation of layout directly to
+    // the builder.
+    container_builder_.AddResult(*result, result->OutOfFlowPositionedOffset());
+    return;
+  }
+  // Since there is no previous break token associated with the first fragment
+  // of a fragmented OOF element, we cannot append this result before any other
+  // children of this fragmentainer. Keep the order by adding it after.
+  remaining_oof_results_.push_back(result);
 }
 
-void NGSimplifiedOOFLayoutAlgorithm::AddChildFragment(
-    const NGLink& old_fragment,
-    const NGPhysicalContainerFragment& new_fragment) {
-  DCHECK_EQ(old_fragment->Size(), new_fragment.Size());
-
+void NGSimplifiedOOFLayoutAlgorithm::AddChildFragment(const NGLink& child) {
+  const auto* fragment = To<NGPhysicalContainerFragment>(child.get());
   // Determine the previous position in the logical coordinate system.
   LogicalOffset child_offset =
       WritingModeConverter(writing_direction_,
                            previous_physical_container_size_)
-          .ToLogical(old_fragment.Offset(), new_fragment.Size());
+          .ToLogical(child.Offset(), fragment->Size());
 
-  // Add the new fragment to the builder.
-  container_builder_.AddChild(new_fragment, child_offset);
+  // Add the fragment to the builder.
+  container_builder_.AddChild(*fragment, child_offset);
 }
 
 }  // namespace blink

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

@@ -13,7 +13,6 @@
 namespace blink {
 
 struct NGLink;
-class NGPhysicalContainerFragment;
 
 // This is more a copy-and-append algorithm than a layout algorithm.
 // This algorithm will only run when we are trying to add OOF-positioned
@@ -35,15 +34,17 @@ class CORE_EXPORT NGSimplifiedOOFLayoutAlgorithm
     return {MinMaxSizes(), /* depends_on_percentage_block_size */ true};
   }
 
-  void AppendOutOfFlowResult(scoped_refptr<const NGLayoutResult> child,
-                             LogicalOffset offset);
+  void AppendOutOfFlowResult(scoped_refptr<const NGLayoutResult> child);
 
  private:
-  void AddChildFragment(const NGLink& old_fragment,
-                        const NGPhysicalContainerFragment& new_fragment);
+  void AddChildFragment(const NGLink& old_fragment);
 
   const WritingDirectionMode writing_direction_;
   PhysicalSize previous_physical_container_size_;
+
+  Vector<scoped_refptr<const NGLayoutResult>> remaining_oof_results_;
+  base::span<const NGLink> children_;
+  base::span<const NGLink>::iterator iterator_;
 };
 
 }  // namespace blink