[LayoutNG] Support column-span:all margins.

Collapse block margins between sibling spanners and add inline margins,
including auto margins.

We now position spanners correctly.

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

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

@@ -7,6 +7,7 @@
 #include <algorithm>
 #include "third_party/blink/renderer/core/layout/geometry/logical_size.h"
 #include "third_party/blink/renderer/core/layout/ng/geometry/ng_fragment_geometry.h"
+#include "third_party/blink/renderer/core/layout/ng/geometry/ng_margin_strut.h"
 #include "third_party/blink/renderer/core/layout/ng/inline/ng_baseline.h"
 #include "third_party/blink/renderer/core/layout/ng/ng_block_layout_algorithm.h"
 #include "third_party/blink/renderer/core/layout/ng/ng_box_fragment.h"
@@ -212,6 +213,8 @@ base::Optional<MinMaxSize> NGColumnLayoutAlgorithm::ComputeMinMaxSize(
 }
 
 void NGColumnLayoutAlgorithm::LayoutChildren() {
+  NGMarginStrut margin_strut;
+
   // First extract incoming child break tokens.
   scoped_refptr<const NGBlockBreakToken> spanner_break_token;
   scoped_refptr<const NGBlockBreakToken> next_column_token;
@@ -257,7 +260,7 @@ void NGColumnLayoutAlgorithm::LayoutChildren() {
     // resume now.
     spanner_break_token =
         LayoutSpanner(To<NGBlockNode>(spanner_break_token->InputNode()),
-                      spanner_break_token.get());
+                      spanner_break_token.get(), &margin_strut);
 
     if (spanner_break_token) {
       // We broke at the spanner again!
@@ -277,7 +280,7 @@ void NGColumnLayoutAlgorithm::LayoutChildren() {
   // child is a spanner.
   do {
     scoped_refptr<const NGLayoutResult> result =
-        LayoutRow(next_column_token.get());
+        LayoutRow(next_column_token.get(), &margin_strut);
     next_column_token =
         To<NGBlockBreakToken>(result->PhysicalFragment().BreakToken());
 
@@ -289,7 +292,7 @@ void NGColumnLayoutAlgorithm::LayoutChildren() {
       break;
 
     // We found a spanner. Lay it out, and then resume column layout.
-    spanner_break_token = LayoutSpanner(spanner_node, nullptr);
+    spanner_break_token = LayoutSpanner(spanner_node, nullptr, &margin_strut);
 
     if (spanner_break_token) {
       // We broke before or inside the spanner. This may happen if we're nested
@@ -312,12 +315,14 @@ void NGColumnLayoutAlgorithm::LayoutChildren() {
     // context. In that case we must make sure to skip the contents when
     // resuming.
     container_builder_.SetHasSeenAllChildren();
+
+    intrinsic_block_size_ += margin_strut.Sum();
   }
 }
 
 scoped_refptr<const NGLayoutResult> NGColumnLayoutAlgorithm::LayoutRow(
-    const NGBlockBreakToken* next_column_token) {
-  LayoutUnit column_block_offset = intrinsic_block_size_;
+    const NGBlockBreakToken* next_column_token,
+    NGMarginStrut* margin_strut) {
   LogicalSize column_size(column_inline_size_, content_box_size_.block_size);
 
   // If block-size is non-auto, subtract the space for content we've consumed in
@@ -347,6 +352,10 @@ scoped_refptr<const NGLayoutResult> NGColumnLayoutAlgorithm::LayoutRow(
         CalculateBalancedColumnBlockSize(column_size, next_column_token);
   }
 
+  // Column rows have no representation in the DOM and have no margins, but
+  // there may be a trailing margin from a preceding spanner.
+  LayoutUnit column_block_offset = intrinsic_block_size_ + margin_strut->Sum();
+
   bool needs_more_fragments_in_outer = false;
   if (is_constrained_by_outer_fragmentation_context_) {
     LayoutUnit available_outer_space =
@@ -502,20 +511,31 @@ scoped_refptr<const NGLayoutResult> NGColumnLayoutAlgorithm::LayoutRow(
     break;
   } while (true);
 
+  bool keep_margin = false;
+
   // If there was no content inside to process, we don't want the resulting
   // empty column fragment.
   if (new_columns.size() == 1u) {
     const NGPhysicalBoxFragment& column =
         *To<NGPhysicalBoxFragment>(new_columns[0].fragment.get());
-    // TODO(mstensho): Keeping the empty fragment, just so that out-of-flow
-    // descendants get propagated correctly isn't right. Find some other way of
-    // propagating them.
-    if (column.Children().size() == 0 &&
-        !column.HasOutOfFlowPositionedDescendants())
-      return result;
+
+    if (column.Children().size() == 0) {
+      // No content. Keep the trailing margin from any previous column spanner.
+      keep_margin = true;
+
+      // TODO(mstensho): It's wrong to keep the empty fragment, just so that
+      // out-of-flow descendants get propagated correctly. Find some other way
+      // of propagating them.
+      if (!column.HasOutOfFlowPositionedDescendants())
+        return result;
+    }
   }
 
-  intrinsic_block_size_ += column_size.block_size;
+  intrinsic_block_size_ = column_block_offset + column_size.block_size;
+
+  // We added a row. Reset the trailing margin from any previous column spanner.
+  if (!keep_margin)
+    *margin_strut = NGMarginStrut();
 
   // Commit all column fragments to the fragment builder.
   for (auto column : new_columns) {
@@ -528,12 +548,21 @@ scoped_refptr<const NGLayoutResult> NGColumnLayoutAlgorithm::LayoutRow(
 
 scoped_refptr<const NGBlockBreakToken> NGColumnLayoutAlgorithm::LayoutSpanner(
     NGBlockNode spanner_node,
-    const NGBlockBreakToken* break_token) {
-  // TODO(mstensho): Margins (including margin collapsing), inline alignment,
-  // and outer fragmentainer breaks between spanners and rows (the spanner may
-  // be unbreakable inside, and we may be in a nested fragmentation context and
-  // out of space).
-  LayoutUnit block_offset = intrinsic_block_size_;
+    const NGBlockBreakToken* break_token,
+    NGMarginStrut* margin_strut) {
+  const ComputedStyle& spanner_style = spanner_node.Style();
+  NGBoxStrut margins = ComputeMarginsFor(
+      spanner_style, content_box_size_.inline_size,
+      ConstraintSpace().GetWritingMode(), ConstraintSpace().Direction());
+
+  // Collapse the block-start margin of this spanner with the block-end margin
+  // of an immediately preceding spanner, if any.
+  margin_strut->Append(margins.block_start, /* is_quirky */ false);
+
+  // TODO(mstensho): outer fragmentainer breaks between spanners and rows (the
+  // spanner may be unbreakable inside, and we may be in a nested fragmentation
+  // context and out of space).
+  LayoutUnit block_offset = intrinsic_block_size_ + margin_strut->Sum();
   auto spanner_space = CreateConstraintSpaceForSpanner(block_offset);
   // TODO(mstensho): Passing a break-before token shouldn't be a problem, but it
   // would cause problems for the NGPaintFragment code. Just pass nullptr. Won't
@@ -541,10 +570,20 @@ scoped_refptr<const NGBlockBreakToken> NGColumnLayoutAlgorithm::LayoutSpanner(
   if (break_token && break_token->IsBreakBefore())
     break_token = nullptr;
   auto result = spanner_node.Layout(spanner_space, break_token);
-  LogicalOffset offset(border_scrollbar_padding_.inline_start,
-                       intrinsic_block_size_);
+  NGFragment fragment(ConstraintSpace().GetWritingMode(),
+                      result->PhysicalFragment());
+
+  ResolveInlineMargins(spanner_style, Style(), content_box_size_.inline_size,
+                       fragment.InlineSize(), &margins);
+
+  LogicalOffset offset(
+      border_scrollbar_padding_.inline_start + margins.inline_start,
+      block_offset);
   container_builder_.AddResult(*result, offset);
 
+  *margin_strut = NGMarginStrut();
+  margin_strut->Append(margins.block_end, /* is_quirky */ false);
+
   // TODO(mstensho): The correct thing would be to weigh any break inside
   // against the appeal of breaking before the spanner, like we do in
   // BreakBeforeChildIfNeeded() for the block layout algorithm. Just setting the
@@ -554,7 +593,6 @@ scoped_refptr<const NGBlockBreakToken> NGColumnLayoutAlgorithm::LayoutSpanner(
   if (ConstraintSpace().HasBlockFragmentation())
     container_builder_.SetBreakAppeal(kBreakAppealPerfect);
 
-  NGFragment fragment(Style().GetWritingMode(), result->PhysicalFragment());
   intrinsic_block_size_ = offset.block_offset + fragment.BlockSize();
 
   return To<NGBlockBreakToken>(result->PhysicalFragment().BreakToken());

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

@@ -14,6 +14,7 @@ class NGBlockNode;
 class NGBlockBreakToken;
 class NGConstraintSpace;
 struct LogicalSize;
+struct NGMarginStrut;
 
 class CORE_EXPORT NGColumnLayoutAlgorithm
     : public NGLayoutAlgorithm<NGBlockNode,
@@ -34,14 +35,16 @@ class CORE_EXPORT NGColumnLayoutAlgorithm
   // Lay out one row of columns. The layout result returned is for the last
   // column that was laid out. The rows themselves don't create fragments.
   scoped_refptr<const NGLayoutResult> LayoutRow(
-      const NGBlockBreakToken* next_column_token);
+      const NGBlockBreakToken* next_column_token,
+      NGMarginStrut*);
 
   // Lay out a column spanner. Will return a break token if we break before or
   // inside the spanner. If no break token is returned, it means that we can
   // proceed to the next row of columns.
   scoped_refptr<const NGBlockBreakToken> LayoutSpanner(
       NGBlockNode spanner_node,
-      const NGBlockBreakToken* break_token);
+      const NGBlockBreakToken* break_token,
+      NGMarginStrut*);
 
   LayoutUnit CalculateBalancedColumnBlockSize(
       const LogicalSize& column_size,

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

@@ -4374,6 +4374,77 @@ TEST_F(NGColumnLayoutAlgorithmTest, SpannerInBlockWithSiblings) {
   EXPECT_EQ(expectation, dump);
 }
 
+TEST_F(NGColumnLayoutAlgorithmTest, SpannerMargins) {
+  SetBodyInnerHTML(R"HTML(
+    <style>
+      #parent {
+        columns: 3;
+        column-gap: 10px;
+        width: 320px;
+      }
+      .content { break-inside:avoid; height:20px; }
+    </style>
+    <div id="container">
+      <div id="parent">
+        <div style="column-span:all; margin:10px; width:33px; height:10px;"></div>
+        <div class="content"></div>
+        <div style="column-span:all; margin:10px auto; width:44px; height:10px;"></div>
+        <div style="column-span:all; margin:20px; width:55px;"></div>
+        <div style="column-span:all; margin:10px; width:66px; height:10px;"></div>
+      </div>
+    </div>
+  )HTML");
+
+  String dump = DumpFragmentTree(GetElementById("container"));
+  String expectation = R"DUMP(.:: LayoutNG Physical Fragment Tree ::.
+  offset:unplaced size:1000x130
+    offset:0,0 size:320x130
+      offset:10,10 size:33x10
+      offset:0,30 size:100x20
+        offset:0,0 size:100x20
+      offset:138,60 size:44x10
+      offset:20,90 size:55x0
+      offset:10,110 size:66x10
+)DUMP";
+  EXPECT_EQ(expectation, dump);
+}
+
+TEST_F(NGColumnLayoutAlgorithmTest, SpannerMarginsRtl) {
+  SetBodyInnerHTML(R"HTML(
+    <style>
+      #parent {
+        columns: 3;
+        column-gap: 10px;
+        width: 320px;
+        direction: rtl;
+      }
+      .content { break-inside:avoid; height:20px; }
+    </style>
+    <div id="container">
+      <div id="parent">
+        <div style="column-span:all; margin:10px; width:33px; height:10px;"></div>
+        <div class="content"></div>
+        <div style="column-span:all; margin:10px auto; width:44px; height:10px;"></div>
+        <div style="column-span:all; margin:20px; width:55px;"></div>
+        <div style="column-span:all; margin:10px; width:66px; height:10px;"></div>
+      </div>
+    </div>
+  )HTML");
+
+  String dump = DumpFragmentTree(GetElementById("container"));
+  String expectation = R"DUMP(.:: LayoutNG Physical Fragment Tree ::.
+  offset:unplaced size:1000x130
+    offset:0,0 size:320x130
+      offset:277,10 size:33x10
+      offset:220,30 size:100x20
+        offset:0,0 size:100x20
+      offset:138,60 size:44x10
+      offset:245,90 size:55x0
+      offset:244,110 size:66x10
+)DUMP";
+  EXPECT_EQ(expectation, dump);
+}
+
 TEST_F(NGColumnLayoutAlgorithmTest, InvalidSpanners) {
   // Spanners cannot exist inside new formatting context roots. They will just
   // be treated as normal column content then.

third_party/blink/web_tests/TestExpectations

@@ -1078,7 +1078,6 @@ crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/composited-with-ov
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/doubly-nested-with-top-padding-crossing-row-boundaries.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/dynamic/abspos-becomes-spanner.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/dynamic/abspos-multicol-with-spanner-becomes-spanner.html [ Failure ]
-crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/dynamic/block-becomes-spanner.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/dynamic/change-second-row-height.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/dynamic/change-spanner-display.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/dynamic/change-spanner-parent-display.html [ Failure ]
@@ -1212,7 +1211,6 @@ crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/scrolling-overflow
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/shadow-breaking.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/single-line.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/span/abspos-containing-block-outside-spanner.html [ Crash Failure ]
-crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/span/adjacent-spanners-with-margin.html [ Failure ]
 crbug.com/829028 virtual/layout_ng_experimental/fast/multicol/span/after-float.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/span/after-row-with-uneven-height-nested-multicol.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/span/as-inner-multicol.html [ Failure ]
@@ -1226,8 +1224,6 @@ crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/span/block-with-to
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/span/block-with-top-border-and-margin-around-spanner-extra-space.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/span/change-multicol-writing-mode.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/span/change-spanner-margins.html [ Failure ]
-crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/span/empty-block-between-spanners-with-margins.html [ Failure ]
-crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/span/empty-spanner-between-spanners-with-margins.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/span/fill-after-spanner-exact-fit.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/span/fill-after-spanner-extra-height.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/span/float.html [ Failure ]
@@ -1237,7 +1233,6 @@ crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/span/in-nested-mul
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/span/inside-block-with-fixed-height.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/span/invalid-spanner-in-abspos.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/span/invalid-spanner-in-transform.html [ Failure ]
-crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/span/margin-on-multicol.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/span/multicol-with-padding.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/span/nested-multicol.html [ Failure ]
 crbug.com/591099 virtual/layout_ng_experimental/fast/multicol/span/offset-properties-empty-content.html [ Failure ]