Ignore zero-height fragments during margin collapsing

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

Index: third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
diff --git a/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc b/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
index e8e441e9c6df3f30ba2eec8463317d67eb9afdec..2b87dfbe5cdbecb2c63bcd1a88a360537b0f8983 100644
--- a/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
+++ b/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
@@ -70,8 +70,8 @@ bool NGBlockLayoutAlgorithm::Layout(const NGConstraintSpace* constraint_space,
             constraint_space_for_children_->WritingMode(),
             constraint_space_for_children_->Direction());
 
-        LayoutUnit margin_block_start = CollapseMargins(
-            *constraint_space, child_margins, fragment->MarginStrut());
+        LayoutUnit margin_block_start =
+            CollapseMargins(*constraint_space, child_margins, *fragment);
 
         // TODO(layout-ng): Support auto margins
         builder_->AddChild(fragment,
@@ -113,20 +113,23 @@ 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;
+    const NGFragment& children_fragment) {

[ikilpatrick, 2016/09/19 17:04:19]

child_fragment?

[Gleb Lanbin, 2016/09/19 17:39:52]

Done.

+  // Zero-height boxes are ignored.

[ikilpatrick, 2016/09/19 17:04:19]

... are ignored and do not participate in margin collapsing?

[Gleb Lanbin, 2016/09/19 17:39:52]

Done.

+  bool is_zero_height_box = !children_fragment.BlockSize() && margins.IsEmpty();

[cbiesinger, 2016/09/19 17:05:30]

We may eventually need to check for "does not establish a new block formatting
context" here

[Gleb Lanbin, 2016/09/19 17:39:52]

Done.

+  if (is_zero_height_box)
+    return LayoutUnit(0);

[cbiesinger, 2016/09/19 17:05:30]

We usually just use LayoutUnit() for zero, it's a bit more efficient

[Gleb Lanbin, 2016/09/19 17:39:52]

Done.

+
+  // Create the current child's margin strut from its children's margin strut.
+  NGMarginStrut curr_margin_strut = children_fragment.MarginStrut();
 
   // 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;
+  NGBoxStrut border_and_padding =
+      computeBorders(*current_child_->Style()) +
+      computePadding(space, *current_child_->Style());
 
   // 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) {
+  if (border_and_padding.block_start) {
     curr_margin_strut.SetMarginBlockStart(margins.block_start);
   } else {
     curr_margin_strut.AppendMarginBlockStart(margins.block_start);
@@ -137,28 +140,18 @@ LayoutUnit NGBlockLayoutAlgorithm::CollapseMargins(
   //    first/last in-flow child
   // 2) parent's logical height is auto.
   if (current_child_->Style()->logicalHeight().isAuto() &&
-      !border_and_padding_after) {
+      !border_and_padding.block_end) {
     curr_margin_strut.AppendMarginBlockEnd(margins.block_end);
   } else {
     curr_margin_strut.SetMarginBlockEnd(margins.block_end);
   }
 
-  // Set the margin strut for the resultant fragment if this is the first or
-  // last child fragment.
-  if (current_child_ == first_child_)
-    builder_->SetMarginStrutBlockStart(curr_margin_strut);
-  if (!current_child_->NextSibling())
-    builder_->SetMarginStrutBlockEnd(curr_margin_strut);
-
-  // Compute the margin block start for adjoining blocks.
-  LayoutUnit margin_block_start;
-  if (current_child_ != first_child_)

[cbiesinger, 2016/09/19 17:05:30]

Why do you no longer need this check?

[Gleb Lanbin, 2016/09/19 17:39:52]

I mistakenly thought that we only need to calculate the margin block start
between adjoining blocks. While working on the "writing-mode fix" I noticed a
problem with TopOffset, fixed it in this patch, added more test expectations to
verify TopOffset as well.

-    margin_block_start = ComputeCollapsedMarginBlockStart(
-        prev_child_margin_strut_, curr_margin_strut);
-
+  // Update the parent fragment's margin strut and calculate the margin
+  // block start position.
+  builder_->UpdateMarginStrut(curr_margin_strut);
+  LayoutUnit margin_block_start = ComputeCollapsedMarginBlockStart(
+      prev_child_margin_strut_, curr_margin_strut);
   prev_child_margin_strut_ = curr_margin_strut;
-  // TODO(layout-ng): support other Margin Collapsing use cases,
-  // i.e. support 0 height elements etc.
   return margin_block_start;
 }