[New Multicolumn] balancer confused by content with top margins.

[New Multicolumn] balancer confused by content with top margins.

We need to set zero page logical height in LayoutState when column
height is unknown (when the columns haven't yet been
balanced). There's code that assumes that non-zero page height means
that page height is known. Lying about this makes the pagination code
believe that every top margin is adjacent to a column break, which
makes it eat and ignore all top margins.

This should be cleaned up, but it's easier to wait until the old
multicol code has been removed.

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

[abucur, 2014-03-25 10:02:35]

https://codereview.chromium.org/209863003/diff/1/Source/core/rendering/Layout...
File Source/core/rendering/LayoutState.cpp (right):

https://codereview.chromium.org/209863003/diff/1/Source/core/rendering/Layout...
Source/core/rendering/LayoutState.cpp:122: m_isPaginated = m_pageLogicalHeight
|| m_columnInfo || renderer.flowThreadContainingBlock();
I think there's a chance you are breaking unsplittable content here such as
images / scrollable containers. m_isPaginated will be true for all the renderers
inside a flow thread because the inherited page height doesn't control this flag
any more. A couple of lines above, the page logical height is set to 0 for
unsplittable content but m_isPaginated will be true because
flowThreadContainingBlock() will be non-null.

Maybe you can group this isPaginated/isNotPaginated logic into a helper function
that should handle all the situations.

[mstensho (USE GERRIT), 2014-03-26 09:56:24]

https://codereview.chromium.org/209863003/diff/1/Source/core/rendering/Layout...
File Source/core/rendering/LayoutState.cpp (right):

https://codereview.chromium.org/209863003/diff/1/Source/core/rendering/Layout...
Source/core/rendering/LayoutState.cpp:122: m_isPaginated = m_pageLogicalHeight
|| m_columnInfo || renderer.flowThreadContainingBlock();
On 2014/03/25 10:02:36, abucur wrote:
> I think there's a chance you are breaking unsplittable content here such as
> images / scrollable containers. m_isPaginated will be true for all the
renderers
> inside a flow thread because the inherited page height doesn't control this
flag
> any more. A couple of lines above, the page logical height is set to 0 for
> unsplittable content but m_isPaginated will be true because
> flowThreadContainingBlock() will be non-null.
> 
> Maybe you can group this isPaginated/isNotPaginated logic into a helper
function
> that should handle all the situations.

Done.

[ojan, 2014-03-27 23:31:31]

lgtm

https://codereview.chromium.org/209863003/diff/30001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlockFlow.cpp (right):

https://codereview.chromium.org/209863003/diff/30001/Source/core/rendering/Re...
Source/core/rendering/RenderBlockFlow.cpp:233: // This is a hack to always make
sure we have a page logical height, if said height is
This should be a FIXME.

[mstensho (USE GERRIT), 2014-03-28 09:15:09]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2014-03-28 09:15:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mstensho@opera.com/209863003/90001

[commit-bot: I haz the power, 2014-03-28 10:24:29]

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

[commit-bot: I haz the power, 2014-03-28 10:24:30]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[mstensho (USE GERRIT), 2014-03-28 10:36:47]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2014-03-28 10:36:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mstensho@opera.com/209863003/90001

[commit-bot: I haz the power, 2014-03-28 11:40:49]

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

[commit-bot: I haz the power, 2014-03-28 11:40:49]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[mstensho (USE GERRIT), 2014-03-28 11:55:52]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2014-03-28 11:55:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mstensho@opera.com/209863003/90001

[commit-bot: I haz the power, 2014-03-28 13:09:45]

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

[commit-bot: I haz the power, 2014-03-28 13:09:46]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[mstensho (USE GERRIT), 2014-03-28 14:21:29]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2014-03-28 14:21:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mstensho@opera.com/209863003/100001

[mstensho (USE GERRIT), 2014-03-28 14:34:27]

This patch was originally written on top of my column-span:all branch, where
column sets are created during box creation, rather than during layout. I had
forgotten that sets are still created during layout on master, so I had to make
a minor change here after lgtm, to make debug builds happy (and to make it work
properly). Hope that's ok.

[commit-bot: I haz the power, 2014-03-28 15:27:21]

Change committed as 170335

[ojan, 2014-03-28 19:00:21]

lgtm