[LayoutNG] Fix incorrectly positioned empty blocks inside of new BFC

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 5eabe822376d798483b9851d9c5151e6d1f3b678..0026fe415d82e80ef54f14d031afbf43d8c9daf5 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
@@ -418,12 +418,18 @@ RefPtr<NGPhysicalFragment> NGBlockLayoutAlgorithm::Layout() {
   // Margins collapsing:
   //   Do not collapse margins between parent and its child if there is
   //   border/padding between them.
-  if (border_and_padding_.block_start ||
-      ConstraintSpace().IsNewFormattingContext()) {
+  if (border_and_padding_.block_start) {
     curr_bfc_offset_.block_offset += curr_margin_strut_.Sum();
-    builder_->SetBfcOffset(curr_bfc_offset_);
+    UpdateFragmentBfcOffset(curr_bfc_offset_, builder_.get());
     curr_margin_strut_ = NGMarginStrut();
   }

[ikilpatrick, 2017/02/16 17:27:37]

.nit nl below this?

[Gleb Lanbin, 2017/02/16 20:08:27]

Done.

+  // Block that establishes new BFC knows its BFC offset == {}.

[ikilpatrick, 2017/02/16 17:27:37]

Maybe some additional comments:

Block that establishes a new BFC ....
If a new formatting context hits the if branch above then the BFC offset is
still {} as the margin strut from the constraint space must also be empty.

I still wonder if there is a clearer way to structure this.... :/

[Gleb Lanbin, 2017/02/16 20:08:27]

Done.

+  if (ConstraintSpace().IsNewFormattingContext()) {
+    UpdateFragmentBfcOffset(curr_bfc_offset_, builder_.get());
+    DCHECK_EQ(builder_->BfcOffset().value(), NGLogicalOffset());
+    DCHECK_EQ(curr_margin_strut_, NGMarginStrut());
+  }
+
   curr_bfc_offset_.block_offset += content_size_;
 
   while (current_child_) {
@@ -522,8 +528,6 @@ void NGBlockLayoutAlgorithm::FinishCurrentChildLayout(
   NGBoxFragment fragment(ConstraintSpace().WritingMode(),
                          physical_fragment.get());
 
-  if (!physical_fragment->UnpositionedFloats().isEmpty())
-    DCHECK(!builder_->BfcOffset()) << "Parent BFC offset shouldn't be set here";
   // Pull out unpositioned floats to the current fragment. This may needed if
   // for example the child fragment could not position its floats because it's
   // empty and therefore couldn't determine its position in space.
@@ -560,6 +564,9 @@ void NGBlockLayoutAlgorithm::FinishCurrentChildLayout(
     // Fragment that knows its offset can be used to set parent's BFC position.
     curr_bfc_offset_.block_offset = fragment.BfcOffset().value().block_offset;
     bfc_offset = curr_bfc_offset_;
+  } else if (builder_->BfcOffset()) {

[ikilpatrick, 2017/02/16 17:27:37]

can you add a comment for when this will get hit, this is for the case where a
child is a new FC right?

(and maybe a DCHECK wrt. the comment as well).

[Gleb Lanbin, 2017/02/16 20:08:27]

Done.

+    bfc_offset = curr_bfc_offset_;
+    bfc_offset.value().block_offset += curr_margin_strut_.Sum();
   }
   if (bfc_offset) {
     UpdateFragmentBfcOffset(curr_bfc_offset_, builder_.get());
@@ -577,7 +584,15 @@ void NGBlockLayoutAlgorithm::FinishCurrentChildLayout(
   curr_margin_strut_ = fragment.EndMarginStrut();
   curr_margin_strut_.Append(curr_child_margins_.block_end);
 
-  content_size_ = fragment.BlockSize() + logical_offset.block_offset;
+  // Only modify content_size if BlockSize is not empty. It's needed to prevent
+  // the situation when logical_offset is included in content_size for empty
+  // blocks. Example:
+  //   <div style="overflow:hidden">
+  //     <div style="margin-top: 8px"></div>
+  //     <div style="margin-top: 10px"></div>
+  //   </div>
+  if (fragment.BlockSize())
+    content_size_ = fragment.BlockSize() + logical_offset.block_offset;
   max_inline_size_ =
       std::max(max_inline_size_, fragment.InlineSize() +
                                      curr_child_margins_.InlineSum() +
@@ -795,22 +810,22 @@ NGBlockLayoutAlgorithm::CreateConstraintSpaceForCurrentChild() {
                       builder_->BfcOffset().value(), &content_size_);
   }
 
-  // Append the current margin strut with child's block start margin.
-  // Non empty border/padding use cases are handled inside of the child's
-  // layout.
-  curr_margin_strut_.Append(curr_child_margins_.block_start);
-  space_builder_->SetMarginStrut(curr_margin_strut_);
-
   // Set estimated BFC offset to the next child's constraint space.
   curr_bfc_offset_ = builder_->BfcOffset() ? builder_->BfcOffset().value()
                                            : ConstraintSpace().BfcOffset();
   curr_bfc_offset_.block_offset += content_size_;
   curr_bfc_offset_.inline_offset += border_and_padding_.inline_start;
 
-  // Floats margins are not included in child's CS because they are used to
-  // calculate floating exclusions.
+  // Floats margins are not included in child's CS because
+  // 1) Floats do not participate in margins collapsing
+  // 2) Floats margins are used separately to calculate floating exclusions.
   if (!CurrentChildStyle().isFloating()) {
     curr_bfc_offset_.inline_offset += curr_child_margins_.inline_start;
+    // Append the current margin strut with child's block start margin.
+    // Non empty border/padding use cases are handled inside of the child's

[ikilpatrick, 2017/02/16 17:27:38]

.nit Non-empty?

[Gleb Lanbin, 2017/02/16 20:08:27]

Done.

+    // layout.
+    curr_margin_strut_.Append(curr_child_margins_.block_start);
+    space_builder_->SetMarginStrut(curr_margin_strut_);
   }
 
   space_builder_->SetBfcOffset(curr_bfc_offset_);