[LayoutNG] Break before a float if breaking inside isn't ideal.

We currently don't let a break before a float affect the appeal of
breaking inside their container. This is possible and fairly straight-forward
to implement, at least for floats that don't participate in an inline
formatting context.

Also, since we're not hooking the floats up to the break appeal system
at all yet, we won't see class A breakpoints between floats, or between
a float and an in-flow block, which the spec actually requires.

Will get back to this when inline formatting context float fragmentation is
supported. Being as consistent as possible regardless of a float belonging to
an inline formatting context or not seems like a good idea. Let's figure out
what's feasible for inline formatting context floats first.

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

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

@@ -976,6 +976,16 @@ void NGBlockLayoutAlgorithm::HandleFloat(
   DCHECK(!IsResumingLayout(child_break_token) ||
          container_builder_.BfcBlockOffset());
 
+  if (broke_before_float_) {
+    // We have already broken before a float. This means that we cannot place
+    // any more floats now, as a float isn't allowed to start before any
+    // preceding float.
+    DCHECK(!child_break_token);
+    container_builder_.AddBreakBeforeChild(child, base::nullopt,
+                                           /* is_forced_break */ false);
+    return;
+  }
+
   // If we don't have a BFC block-offset yet, the "expected" BFC block-offset
   // is used to optimistically place floats.
   NGBfcOffset origin_bfc_offset = {
@@ -1005,8 +1015,40 @@ void NGBlockLayoutAlgorithm::HandleFloat(
   NGPositionedFloat positioned_float =
       PositionFloat(&unpositioned_float, &exclusion_space_);
 
-  const auto& physical_fragment =
-      positioned_float.layout_result->PhysicalFragment();
+  const NGLayoutResult& layout_result = *positioned_float.layout_result;
+  const auto& physical_fragment = layout_result.PhysicalFragment();
+  if (const NGBreakToken* token = physical_fragment.BreakToken()) {
+    DCHECK(ConstraintSpace().HasBlockFragmentation());
+    if (!child_break_token && token->BreakAppeal() != kBreakAppealPerfect) {
+      LayoutUnit fragmentainer_block_offset =
+          ConstraintSpace().FragmentainerOffsetAtBfc() +
+          positioned_float.bfc_offset.block_offset;
+      if (fragmentainer_block_offset > LayoutUnit()) {
+        // The float broke inside, and not at an ideal breakpoint. Break before
+        // the float instead. Note that we don't check if we're at a valid class
+        // A or C breakpoint (we only check that we're not at the start of the
+        // fragmentainer (in which case breaking typically wouldn't eliminate
+        // the unappealing break inside the float)). While no other browsers do
+        // this either, we should consider doing this in the future. For now,
+        // don't let the float affect the appeal of breaking inside this
+        // container.
+        BreakBeforeChild(ConstraintSpace(), child, layout_result,
+                         fragmentainer_block_offset,
+                         /* appeal */ base::nullopt,
+                         /* is_forced_break */ false, &container_builder_);
+
+        // Then carry on with layout of this container. The float constitutes a
+        // parallel flow, and there may be siblings that could still fit in the
+        // current fragmentainer.
+        broke_before_float_ = true;
+        return;
+      }
+    }
+  }
+
+  // TODO(mstensho): There should be a class A breakpoint between a float and
+  // another float, and also between a float and an in-flow block.
+
   LayoutUnit float_inline_size =
       NGFragment(ConstraintSpace().GetWritingMode(), physical_fragment)
           .InlineSize();

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

@@ -386,6 +386,12 @@ class CORE_EXPORT NGBlockLayoutAlgorithm
   // A or B breakpoint (between block-level siblings or line box siblings).
   bool has_processed_first_child_ = false;
 
+  // Set once we've inserted a break before a float. We need to know this, so
+  // that we don't attempt to lay out any more floats in the current
+  // fragmentainer. Floats aren't allowed have an earlier block-start offset
+  // than earlier floats.
+  bool broke_before_float_ = false;
+
   bool did_break_before_child_ = false;
 
   NGExclusionSpace exclusion_space_;

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

@@ -79,15 +79,17 @@ void GatherInlineContainerFragmentsFromLinebox(
 
 }  // namespace
 
-void NGBoxFragmentBuilder::AddBreakBeforeChild(NGLayoutInputNode child,
-                                               NGBreakAppeal appeal,
-                                               bool is_forced_break) {
-  break_appeal_ = appeal;
+void NGBoxFragmentBuilder::AddBreakBeforeChild(
+    NGLayoutInputNode child,
+    base::Optional<NGBreakAppeal> appeal,
+    bool is_forced_break) {
+  if (appeal)
+    break_appeal_ = *appeal;
   if (is_forced_break) {
     SetHasForcedBreak();
     // A forced break is considered to always have perfect appeal; they should
     // never be weighed against other potential breakpoints.
-    DCHECK_EQ(appeal, kBreakAppealPerfect);
+    DCHECK(!appeal || *appeal == kBreakAppealPerfect);
   }
 
   DCHECK(has_block_fragmentation_);

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

@@ -95,9 +95,12 @@ class CORE_EXPORT NGBoxFragmentBuilder final
 
   // Add a break token for a child that doesn't yet have any fragments, because
   // its first fragment is to be produced in the next fragmentainer. This will
-  // add a break token for the child, but no fragment.
+  // add a break token for the child, but no fragment. Break appeal should
+  // always be provided for regular in-flow children. For other types of
+  // children it may be omitted, if the break shouldn't affect the appeal of
+  // breaking inside this container.
   void AddBreakBeforeChild(NGLayoutInputNode child,
-                           NGBreakAppeal,
+                           base::Optional<NGBreakAppeal> appeal,
                            bool is_forced_break);
 
   // Add a layout result. This involves appending the fragment and its relative

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

@@ -752,6 +752,167 @@ TEST_F(NGColumnLayoutAlgorithmTest, FloatWithMarginBelowFloat) {
   EXPECT_EQ(expectation, dump);
 }
 
+TEST_F(NGColumnLayoutAlgorithmTest, FloatWithLastResortBreak) {
+  // Breaking inside the line is not possible, and breaking between the
+  // block-start content edge and the first child should be avoided.
+  SetBodyInnerHTML(R"HTML(
+    <style>
+      #parent {
+        columns: 3;
+        column-fill: auto;
+        column-gap: 10px;
+        width: 320px;
+        height: 100px;
+        line-height: 20px;
+        orphans: 1;
+        widows: 1;
+      }
+    </style>
+    <div id="container">
+      <div id="parent">
+        <div style="width:99px; height:90px;"></div>
+        <div style="float:left; width:88px;">
+          <br>
+        </div>
+      </div>
+    </div>
+  )HTML");
+
+  String dump = DumpFragmentTree(GetElementById("container"));
+  String expectation = R"DUMP(.:: LayoutNG Physical Fragment Tree ::.
+  offset:unplaced size:1000x100
+    offset:0,0 size:320x100
+      offset:0,0 size:100x100
+        offset:0,0 size:99x90
+      offset:110,0 size:100x20
+        offset:0,0 size:88x20
+          offset:0,0 size:0x20
+            offset:0,9 size:0x1
+)DUMP";
+  EXPECT_EQ(expectation, dump);
+}
+
+TEST_F(NGColumnLayoutAlgorithmTest, FloatWithAvoidBreak) {
+  // We want to avoid breaking inside the float child, and breaking before it
+  // should be avoided (not a valid breakpoint).
+  SetBodyInnerHTML(R"HTML(
+    <style>
+      #parent {
+        columns: 3;
+        column-fill: auto;
+        column-gap: 10px;
+        width: 320px;
+        height: 100px;
+      }
+      .content { break-inside:avoid; height:20px; }
+    </style>
+    <div id="container">
+      <div id="parent">
+        <div style="width:99px; height:90px;"></div>
+        <div style="float:left; width:88px;">
+          <div class="content" style="width:77px;"></div>
+        </div>
+      </div>
+    </div>
+  )HTML");
+
+  String dump = DumpFragmentTree(GetElementById("container"));
+  String expectation = R"DUMP(.:: LayoutNG Physical Fragment Tree ::.
+  offset:unplaced size:1000x100
+    offset:0,0 size:320x100
+      offset:0,0 size:100x100
+        offset:0,0 size:99x90
+      offset:110,0 size:100x20
+        offset:0,0 size:88x20
+          offset:0,0 size:77x20
+)DUMP";
+  EXPECT_EQ(expectation, dump);
+}
+
+TEST_F(NGColumnLayoutAlgorithmTest, FloatWithMarginAndAvoidBreak) {
+  // We want to avoid breaking inside the float child, and breaking before it
+  // should be avoided (not a valid breakpoint). The top margin should be kept
+  // in the next column.
+  SetBodyInnerHTML(R"HTML(
+    <style>
+      #parent {
+        columns: 3;
+        column-fill: auto;
+        column-gap: 10px;
+        width: 320px;
+        height: 100px;
+      }
+      .content { break-inside:avoid; height:20px; }
+    </style>
+    <div id="container">
+      <div id="parent">
+        <div style="width:99px; height:90px;"></div>
+        <div style="float:left; width:88px; margin-top:5px;">
+          <div class="content" style="width:77px;"></div>
+        </div>
+      </div>
+    </div>
+  )HTML");
+
+  String dump = DumpFragmentTree(GetElementById("container"));
+  String expectation = R"DUMP(.:: LayoutNG Physical Fragment Tree ::.
+  offset:unplaced size:1000x100
+    offset:0,0 size:320x100
+      offset:0,0 size:100x100
+        offset:0,0 size:99x90
+      offset:110,0 size:100x25
+        offset:0,5 size:88x20
+          offset:0,0 size:77x20
+)DUMP";
+  EXPECT_EQ(expectation, dump);
+}
+
+TEST_F(NGColumnLayoutAlgorithmTest, UnbreakableFloatBeforeBreakable) {
+  // https://www.w3.org/TR/CSS22/visuren.html#float-position
+  //
+  // "The outer top of a floating box may not be higher than the outer top of
+  // any block or floated box generated by an element earlier in the source
+  // document."
+  //
+  // This means that if we decide to break before one float, we also need to
+  // break before all subsequent floats, even if such floats don't require that
+  // on their own.
+  SetBodyInnerHTML(R"HTML(
+    <style>
+      #parent {
+        columns: 3;
+        column-fill: auto;
+        column-gap: 10px;
+        width: 320px;
+        height: 100px;
+      }
+      .content { break-inside:avoid; height:20px; }
+    </style>
+    <div id="container">
+      <div id="parent">
+        <div style="width:99px; height:90px;"></div>
+        <div style="float:left; width:22px; height:50px;">
+          <div class="content" style="width:11px;"></div>
+        </div>
+        <div style="float:left; width:33px; height:50px;"></div>
+      </div>
+    </div>
+  )HTML");
+
+  String dump = DumpFragmentTree(GetElementById("container"));
+  String expectation = R"DUMP(.:: LayoutNG Physical Fragment Tree ::.
+  offset:unplaced size:1000x100
+    offset:0,0 size:320x100
+      offset:0,0 size:100x100
+        offset:0,0 size:99x90
+      offset:110,0 size:100x50
+        offset:0,0 size:22x50
+          offset:0,0 size:11x20
+        offset:22,0 size:33x50
+)DUMP";
+  EXPECT_EQ(expectation, dump);
+}
+
 TEST_F(NGColumnLayoutAlgorithmTest, BlockWithTopMarginInThreeColumns) {
   SetBodyInnerHTML(R"HTML(
     <style>

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

@@ -267,7 +267,7 @@ void BreakBeforeChild(const NGConstraintSpace& space,
                       NGLayoutInputNode child,
                       const NGLayoutResult& layout_result,
                       LayoutUnit fragmentainer_block_offset,
-                      NGBreakAppeal appeal,
+                      base::Optional<NGBreakAppeal> appeal,
                       bool is_forced_break,
                       NGBoxFragmentBuilder* builder) {
 #if DCHECK_IS_ON()

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

@@ -152,7 +152,7 @@ void BreakBeforeChild(const NGConstraintSpace&,
                       NGLayoutInputNode child,
                       const NGLayoutResult&,
                       LayoutUnit fragmentainer_block_offset,
-                      NGBreakAppeal,
+                      base::Optional<NGBreakAppeal> appeal,
                       bool is_forced_break,
                       NGBoxFragmentBuilder*);
 

third_party/blink/web_tests/TestExpectations

@@ -1046,7 +1046,6 @@ crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/abspos-after-break-a
 crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/abspos-new-width-rebalance.html [ Crash Failure ]
 crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/balance-float-after-forced-break.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/balance-float-in-inline.html [ Failure ]
-crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/balance-floats.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/balance-float-with-margin-top-and-line-after-break-2.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/balance-float-with-margin-top-and-line-after-break-3.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/balance-float-with-margin-top-and-line-after-break.html [ Crash Failure ]
@@ -1111,8 +1110,6 @@ crbug.com/874506 virtual/layout_ng_block_frag/fast/multicol/event-offset-complex
 crbug.com/874506 virtual/layout_ng_block_frag/fast/multicol/event-offset.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/event-offset-in-nested.html [ Failure ]
 crbug.com/874506 virtual/layout_ng_block_frag/fast/multicol/filter-in-second-column.html [ Failure ]
-crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/first-line-in-float-below-next-column-top.html [ Failure ]
-crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/first-line-in-float-with-margin.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/fixedpos-child-becomes-static.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/float-after-break-after.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/float-avoidance.html [ Failure ]
@@ -1129,7 +1126,6 @@ crbug.com/954171 virtual/layout_ng_block_frag/fast/multicol/float-with-margin-mo
 crbug.com/954171 virtual/layout_ng_block_frag/fast/multicol/float-with-margin-moved-by-child-block.html [ Failure Crash ]
 crbug.com/954171 virtual/layout_ng_block_frag/fast/multicol/float-with-margin-moved-by-child-line-and-unbreakable.html [ Failure Crash ]
 crbug.com/954171 virtual/layout_ng_block_frag/fast/multicol/float-with-margin-moved-by-child-line.html [ Failure Crash ]
-crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/float-with-margin-moved-unbreakable.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/forced-break-after-block-with-spanner.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/forced-break-after-empty-block-after-spanner.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/forced-break-after-last-block-before-spanner.html [ Failure ]
@@ -1138,7 +1134,6 @@ crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/forced-break-too-sho
 crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/hit-test-above-or-below.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/hit-test-end-of-column.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/hit-test-end-of-column-with-line-height.html [ Crash Failure ]
-crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/image-inside-nested-blocks-with-border.html [ Failure ]
 crbug.com/829181 virtual/layout_ng_block_frag/fast/multicol/infinitely-tall-content-in-outer-crash.html [ Skip ]
 crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/inline-block-baseline.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/inline-getclientrects.html [ Failure ]
@@ -1271,7 +1266,6 @@ crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/vertical-lr/float-br
 crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/vertical-lr/float-content-break.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/vertical-lr/float-edge.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/vertical-lr/float-paginate.html [ Failure ]
-crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/vertical-lr/image-inside-nested-blocks-with-border.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/vertical-lr/nested-columns.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/vertical-lr/offset-top-and-left-at-boundaries.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/vertical-lr/offset-top-and-left-at-boundaries-nested.html [ Failure ]
@@ -1294,7 +1288,6 @@ crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/vertical-rl/float-br
 crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/vertical-rl/float-content-break.html [ Crash Failure ]
 crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/vertical-rl/float-edge.html [ Crash Failure ]
 crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/vertical-rl/float-paginate.html [ Crash Failure ]
-crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/vertical-rl/image-inside-nested-blocks-with-border.html [ Failure ]
 crbug.com/467477 virtual/layout_ng_block_frag/fast/multicol/vertical-rl/nested-columns.html [ Crash Failure ]
 crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/vertical-rl/offset-top-and-left-at-boundaries.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_block_frag/fast/multicol/vertical-rl/offset-top-and-left-at-boundaries-nested.html [ Failure ]
@@ -1307,14 +1300,11 @@ crbug.com/591099 virtual/layout_ng_block_frag/fragmentation/auto-scrollbar-shrin
 crbug.com/591099 virtual/layout_ng_block_frag/fragmentation/become-unfragmented-with-lines.html [ Crash Pass ]
 crbug.com/591099 virtual/layout_ng_block_frag/fragmentation/become-unfragmented-with-unbreakable-blocks.html [ Crash Pass ]
 crbug.com/591099 virtual/layout_ng_block_frag/fragmentation/block-after-float-first-child.html [ Failure ]
-crbug.com/591099 virtual/layout_ng_block_frag/fragmentation/change-fragmentainer-height-block-float-2.html [ Failure ]
-crbug.com/591099 virtual/layout_ng_block_frag/fragmentation/change-fragmentainer-height-block-float.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_block_frag/fragmentation/change-fragmentainer-height-inline-float.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_block_frag/fragmentation/change-fragmentainer-height-line-float.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_block_frag/fragmentation/content-preceding-first-fragmentainer.html [ Crash ]
 crbug.com/591099 virtual/layout_ng_block_frag/fragmentation/float-after-forced-break.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_block_frag/fragmentation/float-margin-top.html [ Failure ]
-crbug.com/591099 virtual/layout_ng_block_frag/fragmentation/float-pushed-to-next-fragmentainer-by-floats.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_block_frag/fragmentation/forced-break-clearance-unsplittable-content.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_block_frag/fragmentation/overflow-crossing-boundary.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_block_frag/fragmentation/relayout-abspos.html [ Failure ]