[LayoutNG] Handle margins at unforced breaks.

A margin before a block may push the start of the block to the next
fragmentainer. The remaining part of the margin will then be "eaten" by
the fragmentainer boundary, as per spec:

https://www.w3.org/TR/css-break-3/#break-margins

Blocks that start exactly at a fragmentainer boundary no longer create
a bogus empty fragment in the former fragmentainer. This affects the
BlockStartAtColumnBoundary unit test.

Quite a few layout tests behave better now, by going from Crash to
Failure, from Crash to Pass, or from Failure to Pass.

fast/multicol/dynamic/insert-spanner-between-out-of-flow.html regresses.
Not sure why, but it's not so interesting, since that test uses

column-span: all, which we still don't support.
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_layout_ng
Change-Id: I3be1b735e70f05a375ea328a7fc11fd515e77e3a
Reviewed-on: https://chromium-review.googlesource.com/803482
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521242}

third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc

@@ -1106,7 +1106,20 @@ void NGBlockLayoutAlgorithm::FinalizeForFragmentation() {
          "block_size for this fragment smaller than zero.";
 
   LayoutUnit space_left = FragmentainerSpaceAvailable();
-  DCHECK_GE(space_left, LayoutUnit());
+
+  if (space_left <= LayoutUnit()) {
+    // The amount of space available may be zero, or even negative, if the
+    // border-start edge of this block starts exactly at, or even after the
+    // fragmentainer boundary. We're going to need a break before this block,
+    // because no part of it fits in the current fragmentainer. Due to margin
+    // collapsing with children, this situation is something that we cannot
+    // always detect prior to layout. The fragment produced by this algorithm is
+    // going to be thrown away. The parent layout algorithm will eventually
+    // 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;
+  }
 
   if (container_builder_.DidBreak()) {
     // One of our children broke. Even if we fit within the remaining space we
@@ -1152,7 +1165,9 @@ bool NGBlockLayoutAlgorithm::BreakBeforeChild(
 
   // The remaining part of the fragmentainer (the unusable space for child
   // content, due to the break) should still be occupied by this container.
-  intrinsic_block_size_ = FragmentainerSpaceAvailable();
+  // TODO(mstensho): Figure out if we really need to <0 here. It doesn't seem
+  // right to have negative available space.
+  intrinsic_block_size_ = FragmentainerSpaceAvailable().ClampNegativeToZero();
   // Drop the fragment on the floor and retry at the start of the next
   // fragmentainer.
   container_builder_.AddBreakBeforeChild(child);
@@ -1164,12 +1179,9 @@ bool NGBlockLayoutAlgorithm::ShouldBreakBeforeChild(
     NGLayoutInputNode child,
     const NGPhysicalFragment& physical_fragment,
     LayoutUnit block_offset) const {
-  const auto* token = physical_fragment.BreakToken();
-  if (!token || token->IsFinished())
-    return false;
-  // TODO(mstensho): There are other break-inside values to consider here.
-  if (child.Style().BreakInside() != EBreakInside::kAvoid)
+  if (!container_builder_.BfcOffset().has_value())
     return false;
+
   // If we haven't used any space at all in the fragmentainer yet, we cannot
   // break, or there'd be no progress. We'd end up creating an infinite number
   // of fragmentainers without putting any content into them.
@@ -1177,6 +1189,23 @@ bool NGBlockLayoutAlgorithm::ShouldBreakBeforeChild(
   if (space_left >= ConstraintSpace().FragmentainerBlockSize())
     return false;
 
+  // If the block offset is past the fragmentainer boundary (or exactly at the
+  // boundary), no part of the fragment is going to fit in the current
+  // fragmentainer. Fragments may be pushed past the fragmentainer boundary by
+  // margins.
+  // TODO(mstensho): The inline check here shouldn't be necessary, but
+  // it's needed as long as we don't support breaking between line
+  // boxes.
+  if (space_left <= LayoutUnit() && !child.IsInline())
+    return true;
+
+  const auto* token = physical_fragment.BreakToken();
+  if (!token || token->IsFinished())
+    return false;
+  // TODO(mstensho): There are other break-inside values to consider here.
+  if (child.Style().BreakInside() != EBreakInside::kAvoid)
+    return false;
+
   // The child broke, and we're not at the start of a fragmentainer, and we're
   // supposed to avoid breaking inside the child.
   DCHECK(IsFirstFragment(ConstraintSpace(), physical_fragment));

third_party/WebKit/Source/core/layout/ng/ng_column_layout_algorithm_test.cc

@@ -603,7 +603,6 @@ TEST_F(NGColumnLayoutAlgorithmTest, BlockStartAtColumnBoundary) {
     offset:0,0 size:320x100
       offset:0,0 size:100x100
         offset:0,0 size:50x100
-        offset:0,100 size:60x0
       offset:110,0 size:100x100
         offset:0,0 size:60x100
 )DUMP";
@@ -800,6 +799,144 @@ TEST_F(NGColumnLayoutAlgorithmTest, NestedBreakInsideAvoidTall) {
   EXPECT_EQ(expectation, dump);
 }
 
+TEST_F(NGColumnLayoutAlgorithmTest, MarginTopPastEndOfFragmentainer) {
+  // A block whose border box would start past the end of the current
+  // fragmentainer should start exactly at the start of the next fragmentainer,
+  // discarding what's left of the margin.
+  // https://www.w3.org/TR/css-break-3/#break-margins
+  SetBodyInnerHTML(R"HTML(
+    <style>
+      #parent {
+        columns: 3;
+        column-fill: auto;
+        column-gap: 10px;
+        width: 320px;
+        height: 100px;
+      }
+    </style>
+    <div id="container">
+      <div id="parent">
+        <div style="height:90px;"></div>
+        <div style="margin-top:20px; width:20px; height:20px;"></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:100x90
+      offset:110,0 size:100x20
+        offset:0,0 size:20x20
+)DUMP";
+  EXPECT_EQ(expectation, dump);
+}
+
+TEST_F(NGColumnLayoutAlgorithmTest, MarginBottomPastEndOfFragmentainer) {
+  // A block whose border box would start past the end of the current
+  // fragmentainer should start exactly at the start of the next fragmentainer,
+  // discarding what's left of the margin.
+  // https://www.w3.org/TR/css-break-3/#break-margins
+  SetBodyInnerHTML(R"HTML(
+    <style>
+      #parent {
+        columns: 3;
+        column-fill: auto;
+        column-gap: 10px;
+        width: 320px;
+        height: 100px;
+      }
+    </style>
+    <div id="container">
+      <div id="parent">
+        <div style="margin-bottom:20px; height:90px;"></div>
+        <div style="width:20px; height:20px;"></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:100x90
+      offset:110,0 size:100x20
+        offset:0,0 size:20x20
+)DUMP";
+  EXPECT_EQ(expectation, dump);
+}
+
+TEST_F(NGColumnLayoutAlgorithmTest, MarginTopAtEndOfFragmentainer) {
+  // A block whose border box is flush with the end of the fragmentainer
+  // shouldn't produce an empty fragment there - only one fragment in the next
+  // fragmentainer.
+  SetBodyInnerHTML(R"HTML(
+    <style>
+      #parent {
+        columns: 3;
+        column-fill: auto;
+        column-gap: 10px;
+        width: 320px;
+        height: 100px;
+      }
+    </style>
+    <div id="container">
+      <div id="parent">
+        <div style="height:90px;"></div>
+        <div style="margin-top:10px; width:20px; height:20px;"></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:100x90
+      offset:110,0 size:100x20
+        offset:0,0 size:20x20
+)DUMP";
+  EXPECT_EQ(expectation, dump);
+}
+
+TEST_F(NGColumnLayoutAlgorithmTest, MarginBottomAtEndOfFragmentainer) {
+  // A block whose border box is flush with the end of the fragmentainer
+  // shouldn't produce an empty fragment there - only one fragment in the next
+  // fragmentainer.
+  SetBodyInnerHTML(R"HTML(
+    <style>
+      #parent {
+        columns: 3;
+        column-fill: auto;
+        column-gap: 10px;
+        width: 320px;
+        height: 100px;
+      }
+    </style>
+    <div id="container">
+      <div id="parent">
+        <div style="margin-bottom:10px; height:90px;"></div>
+        <div style="width:20px; height:20px;"></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:100x90
+      offset:110,0 size:100x20
+        offset:0,0 size:20x20
+)DUMP";
+  EXPECT_EQ(expectation, dump);
+}
+
 TEST_F(NGColumnLayoutAlgorithmTest, BorderAndPadding) {
   SetBodyInnerHTML(R"HTML(
     <style>