[LayoutNG] Detect break before a float with margin-block-start.

If the block-start edge of a float was above the fragmentainer boundary,
and its margin-block-start would take us past the boundary, we'd fail to
detect the need for a break. The bug was that MovePastBreakpoint()
expects a border edge block-start offset, but we were passing the margin
edge from the float code.

We also need to introduce an additional check before deferring to
MovePastBreakpoint(), because in our implementation, float margins are
unbreakable and also don't collapse with the fragmentainer boundaries
(which MovePastBreakpoint() assumes that we can).

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

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

@@ -281,21 +281,41 @@ NGPositionedFloat PositionFloat(NGUnpositionedFloat* unpositioned_float,
       break;
     } while (true);
 
-    LayoutUnit fragmentainer_block_offset =
+    LayoutUnit fragmentainer_margin_edge_block_offset =
         parent_space.FragmentainerOffsetAtBfc() +
         opportunity.rect.start_offset.block_offset;
 
     // Note that we don't check if we're at a valid class A, B or C breakpoint
-    // (we only check that we're not at the start of the fragmentainer). While
-    // no other browsers do this either, we should consider doing this in the
-    // future. But for now, don't let the float affect the appeal of breaking
-    // inside this container.
-    if (!MovePastBreakpoint(parent_space, node, *layout_result,
-                            fragmentainer_block_offset, kBreakAppealPerfect,
-                            /* builder */ nullptr)) {
-      need_break_before = true;
-    } else if (layout_result->PhysicalFragment().BreakToken()) {
-      fragment_margins.block_end = LayoutUnit();
+    // (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. But for now, don't let the float affect the
+    // appeal of breaking inside this container.
+    //
+    // If we're past the fragmentainer start, we can consider breaking before
+    // this float. Otherwise we cannot, or there'd be no content
+    // progression. The common fragmentation machinery assumes that margins can
+    // collapse with fragmentainer boundaries, but this isn't the case for
+    // floats. We don't allow float margins to collapse with anything, nor be
+    // split into multiple fragmentainers. Hence this additional check. Note
+    // that we might want to reconsider this behavior, since browsers disagree
+    // (what we do now is relatively similar to legacy Blink, though). Should we
+    // split a margin in cases where it helps prevent fragmentainer overflow?
+    // Should we always split them if they occur at fragmentainer boundaries? Or
+    // even allow them to collapse with the fragmentainer boundary? Exact
+    // behavior is currently unspecified.
+    if (fragmentainer_margin_edge_block_offset > LayoutUnit()) {
+      LayoutUnit fragmentainer_block_offset =
+          fragmentainer_margin_edge_block_offset + fragment_margins.block_start;
+      if (!MovePastBreakpoint(parent_space, node, *layout_result,
+                              fragmentainer_block_offset, kBreakAppealPerfect,
+                              /* builder */ nullptr)) {
+        need_break_before = true;
+      } else if (layout_result->PhysicalFragment().BreakToken()) {
+        // We need to resume in the next fragmentainer, which means that
+        // there'll be no block-end margin here.
+        fragment_margins.block_end = LayoutUnit();
+      }
     }
   }
 

third_party/blink/web_tests/TestExpectations

@@ -1099,7 +1099,6 @@ crbug.com/591099 virtual/layout_ng_block_frag/fragmentation/become-unfragmented-
 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/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/1066625 virtual/layout_ng_block_frag/fragmentation/outline-crossing-columns.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 ]