Improve how the column balancer handles top margins on floats.

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

 // Copyright 2015 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/ColumnBalancer.h"
 
 #include "core/layout/LayoutMultiColumnFlowThread.h"
 #include "core/layout/LayoutMultiColumnSet.h"
 #include "core/layout/api/LineLayoutBlockFlow.h"
 
@@ skipping 34 matching lines @@
       break;
     examineLine(*line);
   }
 }
 
 void ColumnBalancer::traverseChildren(const LayoutObject& object) {
   // The break-after value from the previous in-flow block-level object to be
   // 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()) {
       // Keep traversing inside inlines. There may be floats there.
       if (child->isLayoutInline())
         traverseChildren(*child);
       continue;
     }
 
     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.
       continue;
     }
     if (childBox.isOutOfFlowPositioned() || childBox.isColumnSpanAll())
       continue;
 
     // 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;
   }
 }
 
 InitialColumnHeightFinder::InitialColumnHeightFinder(
     const LayoutMultiColumnSet& columnSet,
     LayoutUnit logicalTopInFlowThread,
     LayoutUnit logicalBottomInFlowThread)
     : ColumnBalancer(columnSet,
@@ skipping 12 matching lines @@
 
 LayoutUnit InitialColumnHeightFinder::initialMinimalBalancedHeight() const {
   unsigned index = contentRunIndexWithTallestColumns();
   LayoutUnit startOffset = index > 0 ? m_contentRuns[index - 1].breakOffset()
                                      : logicalTopInFlowThread();
   return m_contentRuns[index].columnLogicalHeight(startOffset);
 }
 
 void InitialColumnHeightFinder::examineBoxAfterEntering(
     const LayoutBox& box,
+    LayoutUnit childLogicalHeight,
     EBreak previousBreakAfterValue) {
   if (isLogicalTopWithinBounds(flowThreadOffset() - box.paginationStrut())) {
     if (box.needsForcedBreakBefore(previousBreakAfterValue)) {
       addContentRun(flowThreadOffset());
     } else {
       if (isFirstAfterBreak(flowThreadOffset())) {
         // This box is first after a soft break.
         recordStrutBeforeOffset(flowThreadOffset(), box.paginationStrut());
       }
     }
   }
 
   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
   // piece of content.
   if (!box.isLayoutBlockFlow())
     return;
   LayoutMultiColumnFlowThread* innerFlowThread =
       toLayoutBlockFlow(box).multiColumnFlowThread();
   if (!innerFlowThread || innerFlowThread->isLayoutPagedFlowThread())
     return;
   LayoutUnit offsetInInnerFlowThread =
       flowThreadOffset() -
       innerFlowThread->blockOffsetInEnclosingFragmentationContext();
   LayoutUnit innerUnbreakableHeight =
       innerFlowThread->tallestUnbreakableLogicalHeight(offsetInInnerFlowThread);
   m_tallestUnbreakableLogicalHeight =
       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,
     const RootInlineBox& lastLine) {
   // We may require a certain minimum number of lines per page in order to
   // satisfy orphans and widows, and that may affect the minimum page height.
   unsigned minimumLineCount =
       std::max<unsigned>(style.orphans(), style.widows());
   const RootInlineBox* firstLine = &lastLine;
   for (unsigned i = 1; i < minimumLineCount && firstLine->prevRootBox(); i++)
@@ skipping 104 matching lines @@
                      logicalTopInFlowThread,
                      logicalBottomInFlowThread),
       m_minimumSpaceShortage(LayoutUnit::max()),
       m_pendingStrut(LayoutUnit::min()),
       m_forcedBreaksCount(0) {
   traverse();
 }
 
 void MinimumSpaceShortageFinder::examineBoxAfterEntering(
     const LayoutBox& box,
+    LayoutUnit childLogicalHeight,
     EBreak previousBreakAfterValue) {
   LayoutBox::PaginationBreakability breakability =
       box.getPaginationBreakability();
 
   // Look for breaks before the child box.
   if (isLogicalTopWithinBounds(flowThreadOffset() - box.paginationStrut())) {
     if (box.needsForcedBreakBefore(previousBreakAfterValue)) {
       m_forcedBreaksCount++;
     } else {
       if (isFirstAfterBreak(flowThreadOffset())) {
         // This box is first after a soft break.
         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
           // (e.g. a line or a block-displayed image) inside this block. That
           // ought to be a good candidate for minimum space shortage; a much
           // better one than reporting space shortage for the entire block
           // (which we'll also do (further down), in case we couldn't find
           // anything more suitable).
           m_pendingStrut = strut;
         }
       }
     }
   }
 
   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()) ||
         group.columnLogicalTopForOffset(flowThreadOffset()) !=
             group.columnLogicalTopForOffset(bottomInFlowThread)) {
       // If the child crosses a column boundary, record space shortage, in case
       // nothing inside it has already done so. The column balancer needs to
       // know by how much it has to stretch the columns to make more content
       // fit. If no breaks are reported (but do occur), the balancer will have
       // no clue. Only measure the space after the last column boundary, in case
@@ skipping 19 matching lines @@
     // multicol container. We need to let it walk through all fragmentainer
     // groups in one go, or we'd miss the column boundaries between each
     // fragmentainer group. We need to record space shortage there too.
     MinimumSpaceShortageFinder innerFinder(
         *columnSet, columnSet->logicalTopInFlowThread(),
         columnSet->logicalBottomInFlowThread());
     recordSpaceShortage(innerFinder.minimumSpaceShortage());
   }
 }
 
-void MinimumSpaceShortageFinder::examineBoxBeforeLeaving(const LayoutBox& box) {
+void MinimumSpaceShortageFinder::examineBoxBeforeLeaving(
+    const LayoutBox& box,
+    LayoutUnit childLogicalHeight) {
   if (m_pendingStrut == LayoutUnit::min() ||
       box.getPaginationBreakability() != LayoutBox::ForbidBreaks)
     return;
 
   // The previous break was before a breakable block. Here's the first piece of
   // unbreakable content after / inside that block. We want to record the
   // distance from the top of the column to the bottom of this box as space
   // shortage.
   LayoutUnit logicalOffsetFromCurrentColumn =
       offsetFromColumnLogicalTop(flowThreadOffset());
-  recordSpaceShortage(logicalOffsetFromCurrentColumn + box.logicalHeight() -
+  recordSpaceShortage(logicalOffsetFromCurrentColumn + childLogicalHeight -
                       m_pendingStrut);
   m_pendingStrut = LayoutUnit::min();
 }
 
 void MinimumSpaceShortageFinder::examineLine(const RootInlineBox& line) {
   LayoutUnit lineTop = line.lineTopWithLeading();
   LayoutUnit lineTopInFlowThread = flowThreadOffset() + lineTop;
   LayoutUnit lineHeight = line.lineBottomWithLeading() - lineTop;
   if (m_pendingStrut != LayoutUnit::min()) {
     // The previous break was before a breakable block. Here's the first line
@@ skipping 23 matching lines @@
   if (group.columnLogicalTopForOffset(lineTopInFlowThread) !=
       group.columnLogicalTopForOffset(lineBottomWithOverflow)) {
     LayoutUnit shortage =
         lineBottomWithOverflow -
         group.columnLogicalTopForOffset(lineBottomWithOverflow);
     recordSpaceShortage(shortage);
   }
 }
 
 }  // namespace blink