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

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

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "core/layout/ng/ng_block_layout_algorithm.h"
 
 #include "core/layout/ng/ng_absolute_utils.h"
 #include "core/layout/ng/ng_block_break_token.h"
 #include "core/layout/ng/ng_box_fragment.h"
 #include "core/layout/ng/ng_column_mapper.h"
@@ skipping 400 matching lines @@
     content_size_ = border_and_padding_.block_start;
     current_child_ = first_child_;
   }
 
   curr_margin_strut_ = ConstraintSpace().MarginStrut();
   curr_bfc_offset_ = ConstraintSpace().BfcOffset();
 
   // 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_) {
     if (current_child_->Type() == NGLayoutInputNode::kLegacyBlock) {
       NGBlockNode* current_block_child = toNGBlockNode(current_child_);
       EPosition position = current_block_child->Style().position();
       if (position == EPosition::kAbsolute || position == EPosition::kFixed) {
         builder_->AddOutOfFlowChildCandidate(current_block_child,
                                              GetChildSpaceOffset());
         current_child_ = current_block_child->NextSibling();
@@ skipping 78 matching lines @@
   line_builder.CopyFragmentDataToLayoutBlockFlow();
   FinishCurrentChildLayout(child_fragment.get());
   current_child_ = nullptr;
 }
 
 void NGBlockLayoutAlgorithm::FinishCurrentChildLayout(
     RefPtr<NGPhysicalBoxFragment> physical_fragment) {
   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.
   builder_->MutableUnpositionedFloats().appendVector(
       physical_fragment->UnpositionedFloats());
 
   if (current_child_->Type() == NGLayoutInputNode::kLegacyBlock &&
       CurrentChildStyle().isFloating()) {
     NGFloatingObject* floating_object =
         new NGFloatingObject(physical_fragment.get(), space_for_current_child_,
@@ skipping 16 matching lines @@
   // Determine the fragment's position in the parent space either by using
   // content_size_ or known fragment's BFC offset.
   WTF::Optional<NGLogicalOffset> bfc_offset;
   if (CurrentChildConstraintSpace().IsNewFormattingContext()) {
     curr_bfc_offset_.block_offset += curr_margin_strut_.Sum();
     bfc_offset = curr_bfc_offset_;
   } else if (fragment.BfcOffset()) {
     // 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());
     PositionPendingFloats(curr_bfc_offset_.block_offset, ConstraintSpace(),
                           builder_.get());
   }
   NGLogicalOffset logical_offset = CalculateLogicalOffset(bfc_offset);
 
   if (fragmentainer_mapper_)
     fragmentainer_mapper_->ToVisualOffset(logical_offset);
   else
     logical_offset.block_offset -= PreviousBreakOffset();
 
   // Update margin strut.
   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() +
                                      border_and_padding_.InlineSum());
 
   builder_->AddChild(std::move(physical_fragment), logical_offset);
 }
 
 bool NGBlockLayoutAlgorithm::ProceedToNextUnfinishedSibling(
     NGPhysicalFragment* child_fragment) {
@@ skipping 197 matching lines @@
     if (!content_size_) {
       curr_margin_strut_ = NGMarginStrut();
       curr_child_margins_.block_start = LayoutUnit();
     }
     PositionPendingFloats(curr_bfc_offset_.block_offset, ConstraintSpace(),
                           builder_.get());
     AdjustToClearance(constraint_space_->Exclusions(), current_child_style,
                       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_);
 
   return space_builder_->ToConstraintSpace(
       FromPlatformWritingMode(current_child_style.getWritingMode()));
 }
 
 }  // namespace blink