Do not collapse margins with padding/border b/w parent and first/last child

Do not collapse top/bottom margins if there is a padding or border between
first/last in-flow child and its parent.
W3C spec: https://www.w3.org/TR/CSS2/box.html#collapsing-margins

BUG=635619

Review-Url: https://codereview.chromium.org/2346473003
Cr-Commit-Position: refs/heads/master@{#418685}

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

@@ -66,8 +66,8 @@ bool NGBlockLayoutAlgorithm::Layout(const NGConstraintSpace* constraint_space,
         NGBoxStrut child_margins = computeMargins(
             *constraint_space_for_children_, *current_child_->Style());
 
-        LayoutUnit margin_block_start =
-            CollapseMargins(child_margins, fragment->MarginStrut());
+        LayoutUnit margin_block_start = CollapseMargins(
+            *constraint_space, child_margins, fragment->MarginStrut());
 
         // TODO(layout-ng): Support auto margins
         builder_->AddChild(fragment,
@@ -107,14 +107,33 @@ bool NGBlockLayoutAlgorithm::Layout(const NGConstraintSpace* constraint_space,
 }
 
 LayoutUnit NGBlockLayoutAlgorithm::CollapseMargins(
+    const NGConstraintSpace& space,
     const NGBoxStrut& margins,
     const NGMarginStrut& children_margin_strut) {
   // Calculate margin strut for the current child.
   NGMarginStrut curr_margin_strut = children_margin_strut;
-  curr_margin_strut.AppendMarginBlockStart(margins.block_start);
-  if (current_child_->Style()->logicalHeight().isAuto()) {
-    // bottom margin of a last in-flow child is only collapsed if
-    // the parent has 'auto' computed height
+
+  // Calculate borders and padding for the current child.
+  NGBoxStrut borders = computeBorders(*current_child_->Style());
+  NGBoxStrut paddings = computePadding(space, *current_child_->Style());
+  LayoutUnit border_and_padding_before =
+      borders.block_start + paddings.block_start;
+  LayoutUnit border_and_padding_after = borders.block_end + paddings.block_end;
+
+  // Collapse BLOCK-START margins if there is no padding or border between
+  // parent (current child) and its first in-flow child.
+  if (border_and_padding_before) {
+    curr_margin_strut.SetMarginBlockStart(margins.block_start);
+  } else {
+    curr_margin_strut.AppendMarginBlockStart(margins.block_start);
+  }
+
+  // Collapse BLOCK-END margins if
+  // 1) there is no padding or border between parent (current child) and its
+  //    first/last in-flow child
+  // 2) parent's logical height is auto.
+  if (current_child_->Style()->logicalHeight().isAuto() &&
+      !border_and_padding_after) {
     curr_margin_strut.AppendMarginBlockEnd(margins.block_end);
   } else {
     curr_margin_strut.SetMarginBlockEnd(margins.block_end);

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

@@ -50,10 +50,12 @@ class CORE_EXPORT NGBlockLayoutAlgorithm : public NGLayoutAlgorithm {
   // fragment's MarginStrut if needed.
   // See https://www.w3.org/TR/CSS2/box.html#collapsing-margins
   //
+  // @param space Constraint space for the block.
   // @param child_margins Margins information for the current child.
   // @param children_margin_strut MarginStrut for children of the current child.
   // @return Margin block start based on collapsed margins result.
-  LayoutUnit CollapseMargins(const NGBoxStrut& child_margins,
+  LayoutUnit CollapseMargins(const NGConstraintSpace& space,
+                             const NGBoxStrut& child_margins,
                              const NGMarginStrut& children_margin_strut);
 
   RefPtr<const ComputedStyle> style_;

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

@@ -231,6 +231,59 @@ TEST_F(NGBlockLayoutAlgorithmTest, CollapsingMarginsCase3) {
             NGMarginStrut({LayoutUnit(0), LayoutUnit(kDiv1MarginBottom)}));
 }
 
+// Verifies that 2 adjoining margins are not collapsed if there is padding or
+// border that separates them.
+//
+// Test case's HTML representation:
+// <div style="margin: 30px 0px; padding: 20px 0px;">    <!-- DIV1 -->
+//   <div style="margin: 200px 0px; height: 50px;"/>     <!-- DIV2 -->
+// </div>
+//
+// Expected:
+// Margins do NOT collapse if there is an interfering padding or border.
+TEST_F(NGBlockLayoutAlgorithmTest, CollapsingMarginsCase4) {
+  const int kHeight = 50;
+  const int kDiv1Margin = 30;
+  const int kDiv1Padding = 20;
+  const int kDiv2Margin = 200;
+
+  // DIV1
+  RefPtr<ComputedStyle> div1_style = ComputedStyle::create();
+  div1_style->setMarginTop(Length(kDiv1Margin, Fixed));
+  div1_style->setMarginBottom(Length(kDiv1Margin, Fixed));
+  div1_style->setPaddingTop(Length(kDiv1Padding, Fixed));
+  div1_style->setPaddingBottom(Length(kDiv1Padding, Fixed));
+  NGBox* div1 = new NGBox(div1_style.get());
+
+  // DIV2
+  RefPtr<ComputedStyle> div2_style = ComputedStyle::create();
+  div2_style->setHeight(Length(kHeight, Fixed));
+  div2_style->setMarginTop(Length(kDiv2Margin, Fixed));
+  div2_style->setMarginBottom(Length(kDiv2Margin, Fixed));
+  NGBox* div2 = new NGBox(div2_style.get());
+
+  div1->SetFirstChild(div2);
+
+  auto* space = new NGConstraintSpace(
+      HorizontalTopBottom, NGLogicalSize(LayoutUnit(100), NGSizeIndefinite));
+  NGPhysicalFragment* frag = RunBlockLayoutAlgorithm(space, div1);
+
+  // Verify that margins do NOT collapse.
+  frag = RunBlockLayoutAlgorithm(space, div1);
+  EXPECT_EQ(frag->MarginStrut(),
+            NGMarginStrut({LayoutUnit(kDiv1Margin), LayoutUnit(kDiv1Margin)}));
+  ASSERT_EQ(frag->Children().size(), 1UL);
+  EXPECT_EQ(frag->Children()[0]->MarginStrut(),
+            NGMarginStrut({LayoutUnit(kDiv2Margin), LayoutUnit(kDiv2Margin)}));
+
+  // Reset padding and verify that margins DO collapse.
+  div1_style->setPaddingTop(Length(0, Fixed));
+  div1_style->setPaddingBottom(Length(0, Fixed));
+  frag = RunBlockLayoutAlgorithm(space, div1);
+  EXPECT_EQ(frag->MarginStrut(),
+            NGMarginStrut({LayoutUnit(kDiv2Margin), LayoutUnit(kDiv2Margin)}));
+}
+
 // Verifies that a box's size includes its borders and padding, and that
 // children are positioned inside the content box.
 //

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

@@ -79,6 +79,14 @@ void NGMarginStrut::AppendMarginBlockEnd(const LayoutUnit& value) {
   }
 }
 
+void NGMarginStrut::SetMarginBlockStart(const LayoutUnit& value) {
+  if (value < 0) {
+    negative_margin_block_start = value;
+  } else {
+    margin_block_start = value;
+  }
+}
+
 void NGMarginStrut::SetMarginBlockEnd(const LayoutUnit& value) {
   if (value < 0) {
     negative_margin_block_end = value;
@@ -88,10 +96,10 @@ void NGMarginStrut::SetMarginBlockEnd(const LayoutUnit& value) {
 }
 
 String NGMarginStrut::ToString() const {
-  return String::format(
-      "Start: (%d %d) End: (%d %d)", negative_margin_block_start.toInt(),
-      margin_block_start.toInt(), negative_margin_block_end.toInt(),
-      margin_block_end.toInt());
+  return String::format("Start: (%d %d) End: (%d %d)",
+                        margin_block_start.toInt(), margin_block_end.toInt(),
+                        negative_margin_block_start.toInt(),
+                        negative_margin_block_end.toInt());
 }
 
 bool NGMarginStrut::operator==(const NGMarginStrut& other) const {

third_party/WebKit/Source/core/layout/ng/ng_units.h

@@ -127,6 +127,7 @@ struct CORE_EXPORT NGMarginStrut {
 
   void AppendMarginBlockStart(const LayoutUnit& value);
   void AppendMarginBlockEnd(const LayoutUnit& value);
+  void SetMarginBlockStart(const LayoutUnit& value);
   void SetMarginBlockEnd(const LayoutUnit& value);
 
   String ToString() const;