Prevent overflowing content from affecting multicol layout.

Prevent overflowing content from affecting multicol layout.

Don't let visual overflow (caused by outlines, text-shadow, text overflowing
its line box, and other things) affect layout. Note that this CL does exchange
one problem for another, but it should be better to have stable and predictable
(and correct) layout, than hacking line positions and column heights to make
the overflow fit in the same column as the line box.

This fixes a problem with links in multicol lists on wikipedia jumping back and
forth between columns.

What we're really lacking (after this CL, more than ever) is to be able to
paint column overflow correctly in the block direction. Fixing that would be
the right thing to do long-term, but it's going to require substantial amounts
of work (and probably a great deal of multicol awareness in the painting and
hit-testing code).

Removed the fast/multicol/split-in-top-margin.html test, since the behavior it
tested is what we remove in this CL.

calculateMinimumPageHeight() could be simplified because of this change,
including removal of (now) redundant parameters.

BUG=439820
R=dsinclair@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198683

[dsinclair, 2015-07-07 19:47:50]

dsinclair@chromium.org changed reviewers:
+ jchaffraix+rendering@chromium.org, wangxianzhu@google.com

[dsinclair, 2015-07-07 19:47:51]

+a few more folks.

[mstensho (USE GERRIT), 2015-07-08 18:34:25]

mstensho@opera.com changed reviewers:
+ leviw@chromium.org

[mstensho (USE GERRIT), 2015-07-08 18:34:26]

Would be nice with some feedback on this one asap-ish, since I think we want it
in M45.

[leviw_travelin_and_unemployed, 2015-07-08 21:06:49]

Multicolumn is hard :(

Before I hit the button, are there real-world sites we know the new overflow
issue is going to effect? Was there a useful (e.g. referencing an actual web
page) bug that precipitated the code you're removing?

[mstensho (USE GERRIT), 2015-07-09 06:52:40]

On 2015/07/08 21:06:49, leviw wrote:
> Multicolumn is hard :(

It's not meant for the soul.

> Before I hit the button, are there real-world sites we know the new overflow
> issue is going to effect?

I don't know of any, but I'm totally sure such sites exist. But I believe that
any such cosmetic bugs are better that what we had at wikipedia, where some
links were basically unclickable.

> Was there a useful (e.g. referencing an actual web
> page) bug that precipitated the code you're removing?

Nope. https://bugs.webkit.org/show_bug.cgi?id=38402 ("paginate columns at layout
time rather than at paint time"). This is a giant patch from 2010 that rewrote
the multicol implementation from doing everything during painting (where I guess
column content was sliced brutally into columns without caring much about what
happened at column boundaries) into being a bit conscious about it during
layout. This is the very patch that introduces the pagination strut concept, as
far as I can tell.

[leviw_travelin_and_unemployed, 2015-07-09 20:38:03]

On 2015/07/09 at 06:52:40, mstensho wrote:
> On 2015/07/08 21:06:49, leviw wrote:
> > Multicolumn is hard :(
> 
> It's not meant for the soul.
> 
> > Before I hit the button, are there real-world sites we know the new overflow
> > issue is going to effect?
> 
> I don't know of any, but I'm totally sure such sites exist. But I believe that
any such cosmetic bugs are better that what we had at wikipedia, where some
links were basically unclickable.
> 
> > Was there a useful (e.g. referencing an actual web
> > page) bug that precipitated the code you're removing?
> 
> Nope. https://bugs.webkit.org/show_bug.cgi?id=38402 ("paginate columns at
layout time rather than at paint time"). This is a giant patch from 2010 that
rewrote the multicol implementation from doing everything during painting (where
I guess column content was sliced brutally into columns without caring much
about what happened at column boundaries) into being a bit conscious about it
during layout. This is the very patch that introduces the pagination strut
concept, as far as I can tell.

^^^ This is precisely what I was expecting, but I appreciate you looking it up.

LGTM.

[leviw_travelin_and_unemployed, 2015-07-09 20:38:09]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2015-07-09 20:38:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1224973002/20001

[commit-bot: I haz the power, 2015-07-09 21:55:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-09 21:55:06]

Failed to apply patch for Source/core/layout/LayoutBlockFlow.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/layout/LayoutBlockFlow.cpp
  Hunk #1 FAILED at 727.
  Hunk #2 succeeded at 786 (offset 1 line).
  1 out of 2 hunks FAILED -- saving rejects to file
Source/core/layout/LayoutBlockFlow.cpp.rej

Patch:       Source/core/layout/LayoutBlockFlow.cpp
Index: Source/core/layout/LayoutBlockFlow.cpp
diff --git a/Source/core/layout/LayoutBlockFlow.cpp
b/Source/core/layout/LayoutBlockFlow.cpp
index
c25ca934ec4b6d6834d4fae89959c523da19e4db..ca94a8af422990e99d00200e065505ff6f4aade7
100644
--- a/Source/core/layout/LayoutBlockFlow.cpp
+++ b/Source/core/layout/LayoutBlockFlow.cpp
@@ -727,52 +727,33 @@ LayoutUnit
LayoutBlockFlow::adjustBlockChildForPagination(LayoutUnit logicalTopA
     return result;
 }
 
-static inline LayoutUnit calculateMinimumPageHeight(const ComputedStyle& style,
RootInlineBox* lastLine, LayoutUnit lineTop, LayoutUnit lineBottom)
+static inline LayoutUnit calculateMinimumPageHeight(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 lineCount = std::max<unsigned>(style.hasAutoOrphans() ? 1 :
style.orphans(), style.widows());
-    if (lineCount > 1) {
-        RootInlineBox* line = lastLine;
-        for (unsigned i = 1; i < lineCount && line->prevRootBox(); i++)
-            line = line->prevRootBox();
-
-        // FIXME: Paginating using line overflow isn't all fine. See FIXME in
-        // adjustLinePositionForPagination() for more details.
-        LayoutRect overflow = line->logicalVisualOverflowRect(line->lineTop(),
line->lineBottom());
-        lineTop = std::min(line->lineTopWithLeading(), overflow.y());
-    }
-    return lineBottom - lineTop;
+    const RootInlineBox* firstLine = &lastLine;
+    for (unsigned i = 1; i < lineCount && firstLine->prevRootBox(); i++)
+        firstLine = firstLine->prevRootBox();
+    return lastLine.lineBottomWithLeading() - firstLine->lineTopWithLeading();
 }
 
 void LayoutBlockFlow::adjustLinePositionForPagination(RootInlineBox& lineBox,
LayoutUnit& delta, LayoutFlowThread* flowThread)
 {
-    // FIXME: For now we paginate using line overflow. This ensures that lines
don't overlap at all when we
-    // put a strut between them for pagination purposes. However, this really
isn't the desired layout, since
-    // the line on the top of the next page will appear too far down relative
to the same kind of line at the top
-    // of the first column.
-    //
-    // The layout we would like to see is one where the lineTopWithLeading is
at the top of the column, and any line overflow
-    // simply spills out above the top of the column. This effect would match
what happens at the top of the first column.
-    // We can't achieve this layout, however, until we stop columns from
clipping to the column bounds (thus allowing
-    // for overflow to occur), and then cache visible overflow for each column
rect.
-    //
-    // Furthermore, the paint we have to do when a column has overflow has to
be special. We need to exclude
+    // TODO(mstensho): Pay attention to line overflow. It should be painted in
the same column as
+    // the rest of the line, possibly overflowing the column. We currently only
allow overflow above
+    // the first column. We clip at all other column boundaries, and that's how
it has to be for
+    // now. The paint we have to do when a column has overflow has to be
special. We need to exclude
     // content that paints in a previous column (and content that paints in the
following column).
     //
-    // For now we'll at least honor the lineTopWithLeading when paginating if
it is above the logical top overflow. This will
-    // at least make positive leading work in typical cases.
-    //
     // FIXME: Another problem with simply moving lines is that the available
line width may change (because of floats).
     // Technically if the location we move the line to has a different line
width than our old position, then we need to dirty the
     // line and all following lines.
-    LayoutRect logicalVisualOverflow =
lineBox.logicalVisualOverflowRect(lineBox.lineTop(), lineBox.lineBottom());
-    LayoutUnit logicalOffset = std::min(lineBox.lineTopWithLeading(),
logicalVisualOverflow.y());
-    LayoutUnit logicalBottom = std::max(lineBox.lineBottomWithLeading(),
logicalVisualOverflow.maxY());
-    LayoutUnit lineHeight = logicalBottom - logicalOffset;
-    updateMinimumPageHeight(logicalOffset,
calculateMinimumPageHeight(styleRef(), &lineBox, logicalOffset, logicalBottom));
+    LayoutUnit logicalOffset = lineBox.lineTopWithLeading();
+    LayoutUnit lineHeight = lineBox.lineBottomWithLeading() - logicalOffset;
+    updateMinimumPageHeight(logicalOffset,
calculateMinimumPageHeight(styleRef(), lineBox));
     logicalOffset += delta;
-    lineBox.setPaginationStrut(0);
+    lineBox.setPaginationStrut(LayoutUnit());
     lineBox.setIsFirstAfterPageBreak(false);
     LayoutUnit pageLogicalHeight = pageLogicalHeightForOffset(logicalOffset);
     if (!pageLogicalHeight)
@@ -785,10 +766,6 @@ void
LayoutBlockFlow::adjustLinePositionForPagination(RootInlineBox& lineBox, La
             clearShouldBreakAtLineToAvoidWidow();
             setDidBreakAtLineToAvoidWidow();
         }
-        if (lineHeight > pageLogicalHeight) {
-            // Split the top margin in order to avoid splitting the visible
part of the line.
-            remainingLogicalHeight -= std::min(lineHeight - pageLogicalHeight,
std::max<LayoutUnit>(0, logicalVisualOverflow.y() -
lineBox.lineTopWithLeading()));
-        }
         LayoutUnit totalLogicalHeight = lineHeight + std::max<LayoutUnit>(0,
logicalOffset);
         LayoutUnit pageLogicalHeightAtNewOffset =
pageLogicalHeightForOffset(logicalOffset + remainingLogicalHeight);
         setPageBreak(logicalOffset, lineHeight - remainingLogicalHeight);

[commit-bot: I haz the power, 2015-07-09 21:55:53]

The CQ bit was unchecked by commit-bot@chromium.org

[mstensho (USE GERRIT), 2015-07-10 12:39:48]

The CQ bit was checked by mstensho@opera.com

[mstensho (USE GERRIT), 2015-07-10 12:39:48]

The patchset sent to the CQ was uploaded after l-g-t-m from leviw@chromium.org
Link to the patchset: https://codereview.chromium.org/1224973002/#ps40001
(title: "rebase master")

[commit-bot: I haz the power, 2015-07-10 12:39:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1224973002/40001

[commit-bot: I haz the power, 2015-07-10 12:44:25]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198683