Improve how the column balancer handles top margins on floats.

third_party/WebKit/Source/core/layout/ColumnBalancer.cpp

Index: third_party/WebKit/Source/core/layout/ColumnBalancer.cpp
diff --git a/third_party/WebKit/Source/core/layout/ColumnBalancer.cpp b/third_party/WebKit/Source/core/layout/ColumnBalancer.cpp
index 1da394c505ab71d68d6a91a8da5925e656c8db2a..9dab24714dc51d0578e7b36c804556c0f09772d6 100644
--- a/third_party/WebKit/Source/core/layout/ColumnBalancer.cpp
+++ b/third_party/WebKit/Source/core/layout/ColumnBalancer.cpp
@@ -52,9 +52,6 @@ void ColumnBalancer::traverseChildren(const LayoutObject& object) {
   // joined with the break-before value of the next in-flow block-level sibling.
   EBreak previousBreakAfterValue = BreakAuto;
 
-  const LayoutFlowThread* flowThread = columnSet().flowThread();
-  bool isHorizontalWritingMode = flowThread->isHorizontalWritingMode();
-
   for (const LayoutObject* child = object.slowFirstChild(); child;
        child = child->nextSibling()) {
     if (!child->isBox()) {
@@ -65,20 +62,33 @@ void ColumnBalancer::traverseChildren(const LayoutObject& object) {
     }
 
     const LayoutBox& childBox = toLayoutBox(*child);
-    LayoutRect overflowRect = childBox.layoutOverflowRect();
-    LayoutUnit childLogicalBottomWithOverflow =
-        childBox.logicalTop() +
-        (isHorizontalWritingMode ? overflowRect.maxY() : overflowRect.maxX());
-    if (m_flowThreadOffset + childLogicalBottomWithOverflow <=
+
+    LayoutUnit borderBoxOffset;
+    LayoutUnit logicalTop = childBox.logicalTop();
+    LayoutUnit logicalHeight = childBox.logicalHeightWithVisibleOverflow();
+    // Floats' margins don't collapse with column boundaries, and we don't want
+    // to break inside them, or separate them from the float's border box. Set
+    // the offset to the margin-before edge (rather than border-before edge),
+    // and include the block direction margins in the child height.
+    if (childBox.isFloating()) {
+      LayoutUnit marginBefore = childBox.marginBefore(object.style());
+      LayoutUnit marginAfter = childBox.marginAfter(object.style());
+      logicalHeight =
+          std::max(logicalHeight, childBox.logicalHeight() + marginAfter);
+      logicalTop -= marginBefore;
+      logicalHeight += marginBefore;
+
+      // As soon as we want to process content inside this child, though, we
+      // need to get to its border-before edge, though.
+      borderBoxOffset = marginBefore;
+    }
+
+    if (m_flowThreadOffset + logicalTop + logicalHeight <=
         logicalTopInFlowThread()) {
       // This child is fully above the flow thread portion we're examining.
       continue;
     }
-    LayoutUnit childLogicalTopWithOverflow =
-        childBox.logicalTop() +
-        (isHorizontalWritingMode ? overflowRect.y() : overflowRect.x());
-    if (m_flowThreadOffset + childLogicalTopWithOverflow >=
-        logicalBottomInFlowThread()) {
+    if (m_flowThreadOffset + logicalTop >= logicalBottomInFlowThread()) {
       // This child is fully below the flow thread portion we're examining. We
       // cannot just stop here, though, thanks to negative margins.
       // So keep looking.
@@ -90,18 +100,22 @@ void ColumnBalancer::traverseChildren(const LayoutObject& object) {
     // Tables are wicked. Both table rows and table cells are relative to their
     // table section.
     LayoutUnit offsetForThisChild =
-        childBox.isTableRow() ? LayoutUnit() : childBox.logicalTop();
+        childBox.isTableRow() ? LayoutUnit() : logicalTop;
+
     m_flowThreadOffset += offsetForThisChild;
 
-    examineBoxAfterEntering(childBox, previousBreakAfterValue);
+    examineBoxAfterEntering(childBox, logicalHeight, previousBreakAfterValue);
     // Unless the child is unsplittable, or if the child establishes an inner
     // multicol container, we descend into its subtree for further examination.
     if (childBox.getPaginationBreakability() != LayoutBox::ForbidBreaks &&
         (!childBox.isLayoutBlockFlow() ||
-         !toLayoutBlockFlow(childBox).multiColumnFlowThread()))
+         !toLayoutBlockFlow(childBox).multiColumnFlowThread())) {
+      m_flowThreadOffset += borderBoxOffset;

[eae, 2016/11/01 21:46:30]

This looks a little fishy, instead of inflating the offset by the
borderBoxOffset why not pass the borderBoxOffset as a param to traverseSubtree?

[mstensho (USE GERRIT), 2016/11/01 22:09:38]

I'm not sure if that would be cleaner, since we already modify
m_flowThreadOffset here (inflate by offsetForThisChild a few lines further up),
and this is the only method where we change m_flowThreadOffset.

Maybe there should be no m_flowThreadOffset at all, and instead add some offset
parameter to most of the methods? That would be for a different CL though.

       traverseSubtree(childBox);
+      m_flowThreadOffset -= borderBoxOffset;
+    }
     previousBreakAfterValue = childBox.breakAfter();
-    examineBoxBeforeLeaving(childBox);
+    examineBoxBeforeLeaving(childBox, logicalHeight);
 
     m_flowThreadOffset -= offsetForThisChild;
   }
@@ -134,6 +148,7 @@ LayoutUnit InitialColumnHeightFinder::initialMinimalBalancedHeight() const {
 
 void InitialColumnHeightFinder::examineBoxAfterEntering(
     const LayoutBox& box,
+    LayoutUnit childLogicalHeight,
     EBreak previousBreakAfterValue) {
   if (isLogicalTopWithinBounds(flowThreadOffset() - box.paginationStrut())) {
     if (box.needsForcedBreakBefore(previousBreakAfterValue)) {
@@ -147,11 +162,8 @@ void InitialColumnHeightFinder::examineBoxAfterEntering(
   }
 
   if (box.getPaginationBreakability() != LayoutBox::AllowAnyBreaks) {
-    LayoutUnit unsplittableLogicalHeight = box.logicalHeight();
-    if (box.isFloating())
-      unsplittableLogicalHeight += box.marginBefore() + box.marginAfter();
     m_tallestUnbreakableLogicalHeight =
-        std::max(m_tallestUnbreakableLogicalHeight, unsplittableLogicalHeight);
+        std::max(m_tallestUnbreakableLogicalHeight, childLogicalHeight);
     return;
   }
   // Need to examine inner multicol containers to find their tallest unbreakable
@@ -171,7 +183,9 @@ void InitialColumnHeightFinder::examineBoxAfterEntering(
       std::max(m_tallestUnbreakableLogicalHeight, innerUnbreakableHeight);
 }
 
-void InitialColumnHeightFinder::examineBoxBeforeLeaving(const LayoutBox& box) {}
+void InitialColumnHeightFinder::examineBoxBeforeLeaving(
+    const LayoutBox& box,
+    LayoutUnit childLogicalHeight) {}
 
 static inline LayoutUnit columnLogicalHeightRequirementForLine(
     const ComputedStyle& style,
@@ -296,6 +310,7 @@ MinimumSpaceShortageFinder::MinimumSpaceShortageFinder(
 
 void MinimumSpaceShortageFinder::examineBoxAfterEntering(
     const LayoutBox& box,
+    LayoutUnit childLogicalHeight,
     EBreak previousBreakAfterValue) {
   LayoutBox::PaginationBreakability breakability =
       box.getPaginationBreakability();
@@ -310,7 +325,7 @@ void MinimumSpaceShortageFinder::examineBoxAfterEntering(
         LayoutUnit strut = box.paginationStrut();
         // Figure out how much more space we would need to prevent it from being
         // pushed to the next column.
-        recordSpaceShortage(box.logicalHeight() - strut);
+        recordSpaceShortage(childLogicalHeight - strut);
         if (breakability != LayoutBox::ForbidBreaks &&
             m_pendingStrut == LayoutUnit::min()) {
           // We now want to look for the first piece of unbreakable content
@@ -327,7 +342,7 @@ void MinimumSpaceShortageFinder::examineBoxAfterEntering(
 
   if (breakability != LayoutBox::ForbidBreaks) {
     // See if this breakable box crosses column boundaries.
-    LayoutUnit bottomInFlowThread = flowThreadOffset() + box.logicalHeight();
+    LayoutUnit bottomInFlowThread = flowThreadOffset() + childLogicalHeight;
     const MultiColumnFragmentainerGroup& group =
         groupAtOffset(flowThreadOffset());
     if (isFirstAfterBreak(flowThreadOffset()) ||
@@ -367,7 +382,9 @@ void MinimumSpaceShortageFinder::examineBoxAfterEntering(
   }
 }
 
-void MinimumSpaceShortageFinder::examineBoxBeforeLeaving(const LayoutBox& box) {
+void MinimumSpaceShortageFinder::examineBoxBeforeLeaving(
+    const LayoutBox& box,
+    LayoutUnit childLogicalHeight) {
   if (m_pendingStrut == LayoutUnit::min() ||
       box.getPaginationBreakability() != LayoutBox::ForbidBreaks)
     return;
@@ -378,7 +395,7 @@ void MinimumSpaceShortageFinder::examineBoxBeforeLeaving(const LayoutBox& box) {
   // shortage.
   LayoutUnit logicalOffsetFromCurrentColumn =
       offsetFromColumnLogicalTop(flowThreadOffset());
-  recordSpaceShortage(logicalOffsetFromCurrentColumn + box.logicalHeight() -
+  recordSpaceShortage(logicalOffsetFromCurrentColumn + childLogicalHeight -
                       m_pendingStrut);
   m_pendingStrut = LayoutUnit::min();
 }