[New Multicolumn] Actual support for layout of column-span:all.

[New Multicolumn] Actual support for layout of column-span:all.

There are still issues with block direction margins specified on
spanners and on content adjacent to them. Will take care of that in a
follow-up CL.

Also no support for dynamically inserting and removing spanners
yet. Another follow-up CL can be expected for that.

Added some layout tests. Removed
fast/multicol/newmulticol/direct-child-column-span-all.html .
Although it could detect crashes, it was useless rendering-wise, since
the test and the ref were identical. fast/multicol/span/sole-spanner.html
should be a satisfactory replacement.

BUG=347325
R=jchaffraix@chromium.org

[mstensho (USE GERRIT), 2014-11-07 16:06:01]

Quite a few tests here, that test various aspects of laying out column-span:all
. If they are too many to review in one bite, I could split them into multiple
CLs (although they really do belong together).

https://codereview.chromium.org/712553003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderFlowThread.h (right):

https://codereview.chromium.org/712553003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderFlowThread.h:126: virtual RenderMultiColumnSet*
columnSetAtBlockOffset(LayoutUnit) const = 0;
Note: made this public to be able to write a unit test for it. While it would be
nice to encapsulate RenderMultiColumnSet, so that e.g. RenderBlockFlow doesn't
get tempted to use it, there were already public getters for
RenderMultiColumnSet here, so it doesn't make much of a difference.

https://codereview.chromium.org/712553003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderMultiColumnFlowThread.cpp (right):

https://codereview.chromium.org/712553003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:465:
MultiColumnSetSearchAdapter adapter(offset);
Note: The code here is a revival of code found originally found in
RenderFlowThread::regionAtBlockOffset(), prior to c99ee20cb96d17 (r177342).

[mstensho (USE GERRIT), 2014-11-17 22:35:02]

Julien, do you have time to review this?

[Julien - ping for review, 2014-11-17 23:47:12]

https://codereview.chromium.org/712553003/diff/40001/LayoutTests/fast/multico...
File
LayoutTests/fast/multicol/span/break-in-columns-before-spanner-expected.html
(right):

https://codereview.chromium.org/712553003/diff/40001/LayoutTests/fast/multico...
LayoutTests/fast/multicol/span/break-in-columns-before-spanner-expected.html:2:
<p>There should be a blue <em>square</em> below.</p>
That's not a totally accurate description of the expected output. The square
should be in the middle column.

https://codereview.chromium.org/712553003/diff/40001/LayoutTests/fast/multico...
File LayoutTests/fast/multicol/span/fill-after-spanner-extra-height.html
(right):

https://codereview.chromium.org/712553003/diff/40001/LayoutTests/fast/multico...
LayoutTests/fast/multicol/span/fill-after-spanner-extra-height.html:3: the the
numbers 1, 2 and 3 on separate lines. Large letter spacing is fine.</p>
"Large letter spacing is fine"? Did you mean to say expected?

https://codereview.chromium.org/712553003/diff/40001/LayoutTests/fast/multico...
File LayoutTests/fast/multicol/span/pseudo-before-after-in-content.html (right):

https://codereview.chromium.org/712553003/diff/40001/LayoutTests/fast/multico...
LayoutTests/fast/multicol/span/pseudo-before-after-in-content.html:8: <div
style="-webkit-columns:4; orphans:1; widows:1; line-height:100px;
background:blue;">
It seems like the orphans and widows properties are not used in this use case as
you're using anonymous blocks to split the paragraphs. I wouldn't be surprised
if this applied to other tests.

https://codereview.chromium.org/712553003/diff/40001/LayoutTests/fast/multico...
File LayoutTests/fast/multicol/span/sole-spanner-inside-div.html (right):

https://codereview.chromium.org/712553003/diff/40001/LayoutTests/fast/multico...
LayoutTests/fast/multicol/span/sole-spanner-inside-div.html:3: <div
style="-webkit-columns:10; -webkit-column-gap:0; -webkit-column-rule:1px solid
red; width:5em; background:green;">
Let's avoid red for non failure (which matches the CSS Test policy).

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

https://codereview.chromium.org/712553003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBlockFlow.cpp:1330: if (layoutState->isPaginated()
&& layoutState->pageLogicalHeight() && logicalTop > beforeCollapseLogicalTop &&
!child->isColumnSpanAll()) {
It seems like we should just make column spanning items create new stacking
contexts (see example 27) so that it disable margin collapsing in some context.
This is done by updating RenderBlockFlow::createsNewFormattingContext().

https://codereview.chromium.org/712553003/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/712553003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:1435: cb = containingBlock();
Why don't we patch containingBlock to return the right answer for
isColumnSpanAll()? Those 2 containing block changes seems very artificial.

https://codereview.chromium.org/712553003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:4133: // FIXME: column spanners are only
unsplittable (or rather: they do not participate in
It's unclear what action needs to be taken to close this FIXME. We should make
it clear in case it goes stale.

https://codereview.chromium.org/712553003/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderMultiColumnFlowThread.cpp (right):

https://codereview.chromium.org/712553003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:174:
m_needsColumnHeightsRecalculation = true;
That seems overly broad as you will force recomputing the column heights in any
cases. Shouldn't we only redo column height measurement if there is a column
spanner or the height is auto? If so, we should probably fix this or at least
put a FIXME here.

https://codereview.chromium.org/712553003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:335: ASSERT(spannerSet);
The 2 ASSERTs above could be replaced by:

ASSERT(isColumnSpanner(renderer));

https://codereview.chromium.org/712553003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:425: if
(RenderMultiColumnSet* lastSet = lastMultiColumnSet()) {
Are we not expecting that m_lastSetWorkedOn to be lastMultiColumnSet() at this
point?

https://codereview.chromium.org/712553003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:457: // to locate the
right set.
This FIXME is actionable but it's not super clear for an outsider why this is
desirable or which use cases it fixes.

https://codereview.chromium.org/712553003/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderMultiColumnFlowThreadTest.cpp (right):

https://codereview.chromium.org/712553003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThreadTest.cpp:244: RenderFlowThread*
flowThread = multicolFlowThread; // FIXME: columnSetAtBlockOffset() override is
private in RenderMultiColumnFlowThread. :(
Usually private override are to avoid calling a virtual function when there is
no need. Here I would argue to make it public as there is little harm done.

https://codereview.chromium.org/712553003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThreadTest.cpp:251: columnSet =
columnSet->nextSiblingMultiColumnSet(); // first spanner
How about we have different variable with an explicit name? This would remove
the need for these comments.

[mstensho (USE GERRIT), 2014-11-18 13:29:27]

Addressed all issues (3 separate patch sets). Please take another look.

https://codereview.chromium.org/712553003/diff/40001/LayoutTests/fast/multico...
File
LayoutTests/fast/multicol/span/break-in-columns-before-spanner-expected.html
(right):

https://codereview.chromium.org/712553003/diff/40001/LayoutTests/fast/multico...
LayoutTests/fast/multicol/span/break-in-columns-before-spanner-expected.html:2:
<p>There should be a blue <em>square</em> below.</p>
On 2014/11/17 23:47:11, Julien Chaffraix - PST wrote:
> That's not a totally accurate description of the expected output. The square
> should be in the middle column.

Done.

But it should be in the second and third column. Tried to improve the pass
condition text.

https://codereview.chromium.org/712553003/diff/40001/LayoutTests/fast/multico...
File LayoutTests/fast/multicol/span/fill-after-spanner-extra-height.html
(right):

https://codereview.chromium.org/712553003/diff/40001/LayoutTests/fast/multico...
LayoutTests/fast/multicol/span/fill-after-spanner-extra-height.html:3: the the
numbers 1, 2 and 3 on separate lines. Large letter spacing is fine.</p>
On 2014/11/17 23:47:11, Julien Chaffraix - PST wrote:
> "Large letter spacing is fine"? Did you mean to say expected?

Yeah, I'm pretty sure we can expect that. :) Done.

https://codereview.chromium.org/712553003/diff/40001/LayoutTests/fast/multico...
File LayoutTests/fast/multicol/span/pseudo-before-after-in-content.html (right):

https://codereview.chromium.org/712553003/diff/40001/LayoutTests/fast/multico...
LayoutTests/fast/multicol/span/pseudo-before-after-in-content.html:8: <div
style="-webkit-columns:4; orphans:1; widows:1; line-height:100px;
background:blue;">
On 2014/11/17 23:47:11, Julien Chaffraix - PST wrote:
> It seems like the orphans and widows properties are not used in this use case
as
> you're using anonymous blocks to split the paragraphs. I wouldn't be surprised
> if this applied to other tests.

orphans and widows are inherited by default, so this is no concern.

https://codereview.chromium.org/712553003/diff/40001/LayoutTests/fast/multico...
File LayoutTests/fast/multicol/span/sole-spanner-inside-div.html (right):

https://codereview.chromium.org/712553003/diff/40001/LayoutTests/fast/multico...
LayoutTests/fast/multicol/span/sole-spanner-inside-div.html:3: <div
style="-webkit-columns:10; -webkit-column-gap:0; -webkit-column-rule:1px solid
red; width:5em; background:green;">
On 2014/11/17 23:47:11, Julien Chaffraix - PST wrote:
> Let's avoid red for non failure (which matches the CSS Test policy).

But no rules should be drawn, so red would mean failure here. I updated the pass
condition instead.

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

https://codereview.chromium.org/712553003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBlockFlow.cpp:1330: if (layoutState->isPaginated()
&& layoutState->pageLogicalHeight() && logicalTop > beforeCollapseLogicalTop &&
!child->isColumnSpanAll()) {
On 2014/11/17 23:47:11, Julien Chaffraix - PST wrote:
> It seems like we should just make column spanning items create new stacking
> contexts (see example 27) so that it disable margin collapsing in some
context.
> This is done by updating RenderBlockFlow::createsNewFormattingContext().

They already do create a new block formatting context (and I did update that
method), but doing that doesn't prevent margins of the BFC block from collapsing
with the surrounding margins. It only prevents margins of the BFC's children
from collapsing with BFC's own margins (and the BFC's surroundings).

What's example 27?

And "stacking context" was a typo, no?

https://codereview.chromium.org/712553003/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/712553003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:1435: cb = containingBlock();
On 2014/11/17 23:47:11, Julien Chaffraix - PST wrote:
> Why don't we patch containingBlock to return the right answer for
> isColumnSpanAll()? Those 2 containing block changes seems very artificial.

Done. A patch for this particular issue is uploaded separately, since it
entailed quite a few changes.

https://codereview.chromium.org/712553003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:4133: // FIXME: column spanners are only
unsplittable (or rather: they do not participate in
On 2014/11/17 23:47:11, Julien Chaffraix - PST wrote:
> It's unclear what action needs to be taken to close this FIXME. We should make
> it clear in case it goes stale.

Done.

I just removed the column spanner condition for now. None of the tests depend on
it. Maybe I need it later. It would be fine to have the test here (as long as we
don't support nested fragmentation), although it's not exactly correct, as the
comment tried to communicate. :) If I need it later, I'll try to come up with a
better comment.

https://codereview.chromium.org/712553003/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderMultiColumnFlowThread.cpp (right):

https://codereview.chromium.org/712553003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:174:
m_needsColumnHeightsRecalculation = true;
On 2014/11/17 23:47:11, Julien Chaffraix - PST wrote:
> That seems overly broad as you will force recomputing the column heights in
any
> cases. Shouldn't we only redo column height measurement if there is a column
> spanner or the height is auto? If so, we should probably fix this or at least
> put a FIXME here.

Done.

You're right. There's no need for this if there's only one set and that set
isn't to be balanced.

https://codereview.chromium.org/712553003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:335: ASSERT(spannerSet);
On 2014/11/17 23:47:11, Julien Chaffraix - PST wrote:
> The 2 ASSERTs above could be replaced by:
> 
> ASSERT(isColumnSpanner(renderer));

Done.

https://codereview.chromium.org/712553003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:425: if
(RenderMultiColumnSet* lastSet = lastMultiColumnSet()) {
On 2014/11/17 23:47:11, Julien Chaffraix - PST wrote:
> Are we not expecting that m_lastSetWorkedOn to be lastMultiColumnSet() at this
> point?

Done.

Adding that assertion caused some tests to crash [1]. In order to fix that, I
needed to steal more code from my (no longer so) huge patch (that I've been
breaking into pieces for review). We need to remove spanner sets when a spanner
is removed, or the spanner map will contain dangling pointers (in the test the
pointer happened to be re-used for another (non-spanner) renderer, which caused
flowThreadDescendantWasInserted() to incorrectly believe that a inserted
renderer was a spanner).

I uploaded a separate patch for this particular issue, instead of lumping it
together with a bunch of trivial fixes for the other issues you raised.

[1] One such failing test:
virtual/regionbasedmulticol/fast/multicol/span/generated-child-split-flow-crash.html

https://codereview.chromium.org/712553003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:457: // to locate the
right set.
On 2014/11/17 23:47:11, Julien Chaffraix - PST wrote:
> This FIXME is actionable but it's not super clear for an outsider why this is
> desirable or which use cases it fixes.

Done.

I'll just remove the FIXME. There are no tests that currently depend on this,
including those that I've planned that are not part of this CL.

It's a bit unclear to me whether there are legitimate reasons for dealing with
this. It will typically upset the balancing machinery if we revisit a column set
that we have already finished.

In RenderBlockFlow::adjustBlockChildForPagination() (which is called when
finishing layout of a child) there's a call to pageLogicalHeightForOffset() that
leads to this method, and the offset will typically be outside the bounds of the
current set if the child block in question contains a column spanner:

<div style="-webkit-columns:2;">
    <div id="child" style="height:100px;">
        <div style="-webkit-column:span:all;"></div>
    </div>
</div>

We try to find the available height at the top of the child block, which isn't
in the same column set as the bottom, and record space shortage to the balancer
based on that. Need to revisit this at some point, if it turns out to matter
enough.

https://codereview.chromium.org/712553003/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderMultiColumnFlowThreadTest.cpp (right):

https://codereview.chromium.org/712553003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThreadTest.cpp:244: RenderFlowThread*
flowThread = multicolFlowThread; // FIXME: columnSetAtBlockOffset() override is
private in RenderMultiColumnFlowThread. :(
On 2014/11/17 23:47:11, Julien Chaffraix - PST wrote:
> Usually private override are to avoid calling a virtual function when there is
> no need. Here I would argue to make it public as there is little harm done.

Done.

Generally not a big fan of private overrides myself, as you suddenly end up
having to do stupid upcasts just to be able to call the method.

Made the method final while I was at it, which, with a sufficiently clever
compiler will make direct RenderFlowThread::colmnSetAtBlockOffset() calls
non-virtual. :)

https://codereview.chromium.org/712553003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThreadTest.cpp:251: columnSet =
columnSet->nextSiblingMultiColumnSet(); // first spanner
On 2014/11/17 23:47:12, Julien Chaffraix - PST wrote:
> How about we have different variable with an explicit name? This would remove
> the need for these comments.

Done.

[Julien - ping for review, 2014-11-19 17:54:34]

https://codereview.chromium.org/712553003/diff/40001/LayoutTests/fast/multico...
File LayoutTests/fast/multicol/span/pseudo-before-after-in-content.html (right):

https://codereview.chromium.org/712553003/diff/40001/LayoutTests/fast/multico...
LayoutTests/fast/multicol/span/pseudo-before-after-in-content.html:8: <div
style="-webkit-columns:4; orphans:1; widows:1; line-height:100px;
background:blue;">
On 2014/11/18 13:29:26, mstensho wrote:
> On 2014/11/17 23:47:11, Julien Chaffraix - PST wrote:
> > It seems like the orphans and widows properties are not used in this use
case
> as
> > you're using anonymous blocks to split the paragraphs. I wouldn't be
surprised
> > if this applied to other tests.
> 
> orphans and widows are inherited by default, so this is no concern.

I don't think I understand what you're saying here. Unneeded properties distract
the readers from what the test is *actually testing*. If we don't need these
properties, they should be removed.

https://codereview.chromium.org/712553003/diff/40001/LayoutTests/fast/multico...
File LayoutTests/fast/multicol/span/sole-spanner-inside-div.html (right):

https://codereview.chromium.org/712553003/diff/40001/LayoutTests/fast/multico...
LayoutTests/fast/multicol/span/sole-spanner-inside-div.html:3: <div
style="-webkit-columns:10; -webkit-column-gap:0; -webkit-column-rule:1px solid
red; width:5em; background:green;">
On 2014/11/18 at 13:29:26, mstensho wrote:
> On 2014/11/17 23:47:11, Julien Chaffraix - PST wrote:
> > Let's avoid red for non failure (which matches the CSS Test policy).
> 
> But no rules should be drawn, so red would mean failure here. I updated the
pass condition instead.

Is this really what the test checks or just piggy-backing on it to do more
testing?

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

https://codereview.chromium.org/712553003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBlockFlow.cpp:1330: if (layoutState->isPaginated()
&& layoutState->pageLogicalHeight() && logicalTop > beforeCollapseLogicalTop &&
!child->isColumnSpanAll()) {
On 2014/11/18 13:29:26, mstensho wrote:
> On 2014/11/17 23:47:11, Julien Chaffraix - PST wrote:
> > It seems like we should just make column spanning items create new stacking
> > contexts (see example 27) so that it disable margin collapsing in some
> context.
> > This is done by updating RenderBlockFlow::createsNewFormattingContext().
> 
> They already do create a new block formatting context (and I did update that
> method), but doing that doesn't prevent margins of the BFC block from
collapsing
> with the surrounding margins. It only prevents margins of the BFC's children
> from collapsing with BFC's own margins (and the BFC's surroundings).

I missed that you actually changed the function in this patch. What I meant to
write (but missed the intent of the code) is that we should not get into the
collapsing margin logic if we shouldn't collapse them. I don't have a good
suggestion unfortunately as I don't know the collapsing margin code very well so
I could be missing something.

> What's example 27?

Sorry, I meant example 27 of the multi-column specification (scroll down a bit
from http://dev.w3.org/csswg/css-multicol/#column-span)

> And "stacking context" was a typo, no?

Yeah, I meant block formatting context.

https://codereview.chromium.org/712553003/diff/120001/Source/core/rendering/R...
File Source/core/rendering/RenderFlowThread.h (right):

https://codereview.chromium.org/712553003/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderFlowThread.h:75: virtual void
leaveColumnSpannerAfterLayout(RenderBox*, LayoutUnit logicalBottom) { }
A better antonym of enter is "exit". At least that's what is written everywhere
in shops ;)

https://codereview.chromium.org/712553003/diff/120001/Source/core/rendering/R...
File Source/core/rendering/RenderMultiColumnFlowThread.cpp (right):

https://codereview.chromium.org/712553003/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:154: if
(m_lastSetWorkedOn) {
This is the only real use of |m_lastSetWorkedOn|, the other ones could be
replaced by a hash lookup or some parameter.

The fact that we need to lie in this function during layout is a definite bad
smell. Why do we call this function mid-layout as we *know* we can't answer this
truthfully? Can't we avoid doing that altogether?

https://codereview.chromium.org/712553003/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:404: if (nextSet) {
This should be a single line:

if (RenderMultiColumnSet* nextSet =
m_lastSetWorkedOn->nextSiblingMultiColumnSet()) {

https://codereview.chromium.org/712553003/diff/120001/Source/core/rendering/R...
File Source/core/rendering/RenderMultiColumnSet.cpp (right):

https://codereview.chromium.org/712553003/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnSet.cpp:80: rect.setX(logicalTop);
It seems like the flowThreadPortionRect() is in logical coordinate as it is
inferred from the visual overflow. Could you explain the need for this code?

https://codereview.chromium.org/712553003/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnSet.cpp:102: // If we're followed by a
spanner, we need to balance.
This comment (and the rest of the logic) shows that the name is not well done.
It seems like it should just be called needsBalancing() or something similar.

It's also super weird to have the only difference between this function and
requiresBalancing() is a type check. Can't we just squash them together?

https://codereview.chromium.org/712553003/diff/120001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/712553003/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:644: curr = curr->container();
Is this really correct? The main difference between containingBlock() and
container() is relative positioned inlines, which you shouldn't impact
multi-column.

https://codereview.chromium.org/712553003/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:845: o =
toRenderMultiColumnFlowThread(flowThreadContainingBlock())->multiColumnBlockFlow();
If you update containingBlock(), you should also update container().

[mstensho (USE GERRIT), 2014-11-21 15:39:24]

Sorry about the slow response. Got interrupted by a crasher that I had to fix.

I just uploaded two patch sets, one with misc. minor review issues addressed,
and one about breaking containingBlock() into pieces.

I haven't addressed your request regarding patching container() for
column-span:all yet. I'd like your feedback on what I've done so far first.

https://codereview.chromium.org/712553003/diff/40001/LayoutTests/fast/multico...
File LayoutTests/fast/multicol/span/pseudo-before-after-in-content.html (right):

https://codereview.chromium.org/712553003/diff/40001/LayoutTests/fast/multico...
LayoutTests/fast/multicol/span/pseudo-before-after-in-content.html:8: <div
style="-webkit-columns:4; orphans:1; widows:1; line-height:100px;
background:blue;">
On 2014/11/19 17:54:33, Julien Chaffraix - PST wrote:
> On 2014/11/18 13:29:26, mstensho wrote:
> > On 2014/11/17 23:47:11, Julien Chaffraix - PST wrote:
> > > It seems like the orphans and widows properties are not used in this use
> case
> > as
> > > you're using anonymous blocks to split the paragraphs. I wouldn't be
> surprised
> > > if this applied to other tests.
> > 
> > orphans and widows are inherited by default, so this is no concern.
> 
> I don't think I understand what you're saying here. Unneeded properties
distract
> the readers from what the test is *actually testing*. If we don't need these
> properties, they should be removed.

No, I misunderstood YOU. :)

We have discussed this one at some point earlier. If Blink had implemented
orphans and widows according to the spec (initial value should be 2, according
to the spec, but Blink doesn't do that), it's typically desirable to set them to
1 to allow breaking between lines anywhere (unless you want to test the
orphans/widows implementation).

But you're right: in this test they are unneeded, no matter how correctly they
are implemented. I'll remove them.

Done.

https://codereview.chromium.org/712553003/diff/40001/LayoutTests/fast/multico...
File LayoutTests/fast/multicol/span/sole-spanner-inside-div.html (right):

https://codereview.chromium.org/712553003/diff/40001/LayoutTests/fast/multico...
LayoutTests/fast/multicol/span/sole-spanner-inside-div.html:3: <div
style="-webkit-columns:10; -webkit-column-gap:0; -webkit-column-rule:1px solid
red; width:5em; background:green;">
On 2014/11/19 17:54:34, Julien Chaffraix - PST wrote:
> On 2014/11/18 at 13:29:26, mstensho wrote:
> > On 2014/11/17 23:47:11, Julien Chaffraix - PST wrote:
> > > Let's avoid red for non failure (which matches the CSS Test policy).
> > 
> > But no rules should be drawn, so red would mean failure here. I updated the
> pass condition instead.
> 
> Is this really what the test checks or just piggy-backing on it to do more
> testing?

I think it belongs here. If column-span:all fails, you end up with two lines as
regular column content, and the column balancer should make the multicol
container tall enough to contain one line, and you'd get two columns and a red
rule between them.

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

https://codereview.chromium.org/712553003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBlockFlow.cpp:1330: if (layoutState->isPaginated()
&& layoutState->pageLogicalHeight() && logicalTop > beforeCollapseLogicalTop &&
!child->isColumnSpanAll()) {
On 2014/11/19 17:54:34, Julien Chaffraix - PST wrote:
> On 2014/11/18 13:29:26, mstensho wrote:
> > On 2014/11/17 23:47:11, Julien Chaffraix - PST wrote:
> > > It seems like we should just make column spanning items create new
stacking
> > > contexts (see example 27) so that it disable margin collapsing in some
> > context.
> > > This is done by updating RenderBlockFlow::createsNewFormattingContext().
> > 
> > They already do create a new block formatting context (and I did update that
> > method), but doing that doesn't prevent margins of the BFC block from
> collapsing
> > with the surrounding margins. It only prevents margins of the BFC's children
> > from collapsing with BFC's own margins (and the BFC's surroundings).
> 
> I missed that you actually changed the function in this patch. What I meant to
> write (but missed the intent of the code) is that we should not get into the
> collapsing margin logic if we shouldn't collapse them. I don't have a good
> suggestion unfortunately as I don't know the collapsing margin code very well
so
> I could be missing something.

This code here is about the top margin. Being a BFC here or not doesn't really
matter. A new BFC also lives in a BFC established by some ancestor, and it will
collapse with its previous siblings and parents.

<div style="margin-bottom:100px;">xxx</div><div style="overflow:hidden;
margin-top:100px;"></div>

The margin between the two DIVs here will be 100px, even if the latter is a BFC.

But we digress: This isn't really about regular margin collapsing, but rather
about eating top margins of elements that end up as the first element in a
column. [1]

But hey, I just realized that this change isn't needed by any of the current
tests, so I'll just back it out for now. I will probably have to reintroduce
this later, since I have some more tests (and code) in the oven.

[1] http://dev.w3.org/csswg/css-break/#break-margins

https://codereview.chromium.org/712553003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBlockFlow.cpp:1546: if (layoutState->isPaginated()
&& layoutState->pageLogicalHeight() && logicalTopEstimate > logicalHeight() &&
!child->isColumnSpanAll())
I'll remove this for now. No test covers it. See previous comment.

https://codereview.chromium.org/712553003/diff/120001/Source/core/rendering/R...
File Source/core/rendering/RenderFlowThread.h (right):

https://codereview.chromium.org/712553003/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderFlowThread.h:75: virtual void
leaveColumnSpannerAfterLayout(RenderBox*, LayoutUnit logicalBottom) { }
On 2014/11/19 17:54:34, Julien Chaffraix - PST wrote:
> A better antonym of enter is "exit". At least that's what is written
everywhere
> in shops ;)

I think I enter and leave shops all the time, but maybe that's something you can
only do here in this non-English-speaking country. :)

Anyway: Done.

https://codereview.chromium.org/712553003/diff/120001/Source/core/rendering/R...
File Source/core/rendering/RenderMultiColumnFlowThread.cpp (right):

https://codereview.chromium.org/712553003/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:154: if
(m_lastSetWorkedOn) {
On 2014/11/19 17:54:34, Julien Chaffraix - PST wrote:
> This is the only real use of |m_lastSetWorkedOn|, the other ones could be
> replaced by a hash lookup or some parameter.
> 
> The fact that we need to lie in this function during layout is a definite bad
> smell. Why do we call this function mid-layout as we *know* we can't answer
this
> truthfully? Can't we avoid doing that altogether?

This is called a lot mid-layout (and intentionally so).
RenderBlock::pageRemainingLogicalHeightForOffset(), for example, will take us
here. You want to know how much space is left in a column when laying out some
element. To find out how much space you have left, you need to know the column
height. To find the column height, you need to know which column set you're
currently working on.

So I wouldn't call it lying. But if we for some reason, during layout, use a
flow-tread offset here that's not contained by the current column set, things
will go wrong.

But we've briefly discussed that earlier in this review:
https://codereview.chromium.org/712553003/diff/40001/Source/core/rendering/Re...

https://codereview.chromium.org/712553003/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:404: if (nextSet) {
On 2014/11/19 17:54:34, Julien Chaffraix - PST wrote:
> This should be a single line:
> 
> if (RenderMultiColumnSet* nextSet =
> m_lastSetWorkedOn->nextSiblingMultiColumnSet()) {

Done.

https://codereview.chromium.org/712553003/diff/120001/Source/core/rendering/R...
File Source/core/rendering/RenderMultiColumnSet.cpp (right):

https://codereview.chromium.org/712553003/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnSet.cpp:80: rect.setX(logicalTop);
On 2014/11/19 17:54:34, Julien Chaffraix - PST wrote:
> It seems like the flowThreadPortionRect() is in logical coordinate as it is
> inferred from the visual overflow. Could you explain the need for this code?

flowThreadPortionRect() is in the same coordinate system as
RenderBox::m_frameRect, i.e. rather physical. (but both m_frameRect and the flow
thread portion rect have their block direction coordinate (either x or y)
flipped for reverse block direction mode (vertical-rl and horizontal-bt),
though). Visual overflow is in the same coordinate system, isn't it?

The logicalTop that we have here, on the other hand, is super-logical. It's a
block offset from the start of the flow thread, which starts at 0, and increases
as you add children, regardless of writing-mode.

https://codereview.chromium.org/712553003/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnSet.cpp:102: // If we're followed by a
spanner, we need to balance.
On 2014/11/19 17:54:34, Julien Chaffraix - PST wrote:
> This comment (and the rest of the logic) shows that the name is not well done.
> It seems like it should just be called needsBalancing() or something similar.

Callers of this method should really only want to know if the set height is
auto, and not worry about balancing. I found one call site now that (harmlessly)
assumed that auto height meant balancing. I changed it to calling
requiresBalancing(). That will result in an additional virtual method call, but
I suppose we can afford that for the sake of clarity.

> It's also super weird to have the only difference between this function and
> requiresBalancing() is a type check. Can't we just squash them together?

That's a crucial type check. A spanner shouldn't be balanced, but its height may
still be auto.

This can be considered to be a result of letting RenderMultiColumnSpannerSet
inherit from RenderMultiColumnSet (see FIXME in RenderMultiColumnSpannerSet.h).
I think the two classes would be better off inheriting from some common base
class. That would be RenderRegion in the current code, but I'd rather get rid of
/ clean up / rename that one before cleaning up this.

Asking a spanner if it requires balancing is meaningless, and we'd probably be
better off avoiding calling requiresBalancing() on something that may
potentially be a spanner set.

https://codereview.chromium.org/712553003/diff/120001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/712553003/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:644: curr = curr->container();
On 2014/11/19 17:54:34, Julien Chaffraix - PST wrote:
> Is this really correct? The main difference between containingBlock() and
> container() is relative positioned inlines, which you shouldn't impact
> multi-column.

Now that containingBlock() may end up calling this method (because of spanner
awareness), I cannot call containingBlock() from here, or we'd get an infinite
recursion. :( This is why I went for the ugly stuff in RenderBox.cpp, that your
review comment had me revert.

So, what to do?

A: Introduce a parameter to containingBlock, that can be used to ignore special
treatment for spanners.
B: Split containingBlock() into pieces, so that we can easily jump to containing
blocks for abspos and fixedpos here.
C: duplicate as much as we need from containingBlock() here.

I went for B, because it seemed like the least ugly option (but admittedly the
big winner in terms of the number of changed code lines). I uploaded this as a
separate patch set. I kept using the beautiful variable 'o' for RenderObject*
here and there, mainly to minimize the diff, but maybe we could come up with
something longer?

https://codereview.chromium.org/712553003/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:845: o =
toRenderMultiColumnFlowThread(flowThreadContainingBlock())->multiColumnBlockFlow();
On 2014/11/19 17:54:34, Julien Chaffraix - PST wrote:
> If you update containingBlock(), you should also update container().

If I do that, I also have to deal with changed behavior in
RenderObject::markContainingBlocksForLayout(), which means that I'll have to
change how we get to the spanners during layout, since we won't necessarily
reach the parent RenderBlock of spanners anymore. By all means, this may turn
out to be a good and beautiful thing, but it will be a rather fundamental change
in this CL.

But I feel that I've already changed the code too much since last time you gave
feedback, so I won't do this until you've had a look at what I've done so far.
You might ask me to go back to doing a couple of ugly things with
RenderBox::containingBlockLogical{Width,Height}ForContent() instead. :)

Let me know if you still want me to patch container().

[Julien - ping for review, 2014-11-26 21:09:01]

https://codereview.chromium.org/712553003/diff/120001/Source/core/rendering/R...
File Source/core/rendering/RenderFlowThread.h (right):

https://codereview.chromium.org/712553003/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderFlowThread.h:75: virtual void
leaveColumnSpannerAfterLayout(RenderBox*, LayoutUnit logicalBottom) { }
On 2014/11/21 15:39:24, mstensho wrote:
> On 2014/11/19 17:54:34, Julien Chaffraix - PST wrote:
> > A better antonym of enter is "exit". At least that's what is written
> everywhere
> > in shops ;)
> 
> I think I enter and leave shops all the time, but maybe that's something you
can
> only do here in this non-English-speaking country. :)
> 
> Anyway: Done.

Official leave sign:
http://squamcreativeservices.files.wordpress.com/2012/08/squam-creative-teele...
;-)

https://codereview.chromium.org/712553003/diff/120001/Source/core/rendering/R...
File Source/core/rendering/RenderMultiColumnSet.cpp (right):

https://codereview.chromium.org/712553003/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnSet.cpp:80: rect.setX(logicalTop);
On 2014/11/21 15:39:24, mstensho wrote:
> On 2014/11/19 17:54:34, Julien Chaffraix - PST wrote:
> > It seems like the flowThreadPortionRect() is in logical coordinate as it is
> > inferred from the visual overflow. Could you explain the need for this code?
> 
> flowThreadPortionRect() is in the same coordinate system as
> RenderBox::m_frameRect, i.e. rather physical. (but both m_frameRect and the
flow
> thread portion rect have their block direction coordinate (either x or y)
> flipped for reverse block direction mode (vertical-rl and horizontal-bt),
> though). Visual overflow is in the same coordinate system, isn't it?

Visual overflow is in the same coordinate system indeed (which is what most of
the layout code manipulate).

> The logicalTop that we have here, on the other hand, is super-logical. It's a
> block offset from the start of the flow thread, which starts at 0, and
increases
> as you add children, regardless of writing-mode.

OK but in this case, shouldn't we also check for isFlippingWritingMode() as this
is the primary determinant of whether the need to flip?

https://codereview.chromium.org/712553003/diff/160001/LayoutTests/fast/multic...
File LayoutTests/fast/multicol/newmulticol/direct-child-column-span-all.html
(left):

https://codereview.chromium.org/712553003/diff/160001/LayoutTests/fast/multic...
LayoutTests/fast/multicol/newmulticol/direct-child-column-span-all.html:14:
<div>Test added as part of <a
href="https://code.google.com/p/chromium/issues/detail?id=242129">242129 -
ASSERTION FAILED: !node || node->isElementNode(), UNKNOWN in
WebCore::RenderBlock::clone</a></div>
What's wrong with this test?

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
File Source/core/rendering/RenderBlockFlow.cpp (right):

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderBlockFlow.cpp:552: : m_spanner(0) { }
Nit: I prefer to stick with a statement per line, which moves the curly braces
to their own lines.

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
File Source/core/rendering/RenderMultiColumnFlowThread.cpp (right):

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:197:
hasSpannerOrAutoHeightColumnSet = columnSet->isRenderMultiColumnSpannerSet() ||
columnSet->heightIsAuto();
We usually avoid a branch but using some boolean magic:

hasSpannerOrAutoHeightColumnSet |= columnSet->isRenderMultiColumnSpannerSet() ||
columnSet->heightIsAuto();

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:201:
m_needsColumnHeightsRecalculation = hasSpannerOrAutoHeightColumnSet;
Note that you don't really need hasSpannerOrAutoHeightColumnSet if you just
reset the member boolean before the for-loop above :)

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:378: // after the spanner
(or the spanner itself) don't bleed into the column preceding the
> so that contents that's supposed .... don't bleed into....

It seems like it's missing an 'any' before contents (or just use content
singular but we need to update the verb). I am not a native speakers so I could
be completely wrong.

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:446: nextSet->destroy();
It would be worth checking that nextSet is not in m_spannerMap. The best way
should be a RELEASE_ASSERT(isValidColumnSpanAll()) check to
RenderObject::willBeDestroyed (with a comment that we should have removed it
from the flow-thread map before destroying it).

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:447:
previousSet->setNeedsLayout();
What ensures that the exiting content under |nextSet| gets moved into the
previousSet?

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
File Source/core/rendering/RenderMultiColumnFlowThreadTest.cpp (right):

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThreadTest.cpp:263: }
It seems like we should have a vertical writing mode test for that too as it is
a logical function.

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
File Source/core/rendering/RenderMultiColumnSet.cpp (right):

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnSet.cpp:91:
setFlowThreadPortionRect(rect);
I assume we have testing for that?

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
File Source/core/rendering/RenderMultiColumnSpannerSet.cpp (right):

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnSpannerSet.cpp:39: return
m_rendererInFlowThread->logicalHeight();
Should we ASSERT that m_rendererInFlowThread doesn't need layout?

More conceptually, it doesn't seem correct to return the logical height of the
content for the page logical height. It's not a paginated value at all.

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:882: }
A lot of this containing-block related change is orthogonal to adding
column-span: all support. Please split it into a preparatory change (or a
post-landing refactoring but it's more tricky as it assumes column-span: all
sticks) to reduce the size of this change.

I am happy to review those preparatory changes as it reduces the amount of time
I have to spend on this mega-patch :(

[mstensho (USE GERRIT), 2014-11-27 21:20:30]

https://codereview.chromium.org/712553003/diff/160001/LayoutTests/fast/multic...
File LayoutTests/fast/multicol/newmulticol/direct-child-column-span-all.html
(left):

https://codereview.chromium.org/712553003/diff/160001/LayoutTests/fast/multic...
LayoutTests/fast/multicol/newmulticol/direct-child-column-span-all.html:14:
<div>Test added as part of <a
href="https://code.google.com/p/chromium/issues/detail?id=242129">242129 -
ASSERTION FAILED: !node || node->isElementNode(), UNKNOWN in
WebCore::RenderBlock::clone</a></div>
On 2014/11/26 21:09:00, Julien Chaffraix - PST wrote:
> What's wrong with this test?

The test and the ref are 100% identical. That doesn't really matter if you want
to catch crashers, of course, but I thought it was silly. See the commit
comment. The new test fast/multicol/span/sole-spanner.html is the replacement.

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
File Source/core/rendering/RenderBlockFlow.cpp (right):

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderBlockFlow.cpp:552: : m_spanner(0) { }
On 2014/11/26 21:09:00, Julien Chaffraix - PST wrote:
> Nit: I prefer to stick with a statement per line, which moves the curly braces
> to their own lines.

Done.

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
File Source/core/rendering/RenderMultiColumnFlowThread.cpp (right):

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:197:
hasSpannerOrAutoHeightColumnSet = columnSet->isRenderMultiColumnSpannerSet() ||
columnSet->heightIsAuto();
On 2014/11/26 21:09:01, Julien Chaffraix - PST wrote:
> We usually avoid a branch but using some boolean magic:
> 
> hasSpannerOrAutoHeightColumnSet |= columnSet->isRenderMultiColumnSpannerSet()
||
> columnSet->heightIsAuto();

|= is a bit-wise operator (there's no logical ||= operator), so then you would
have to evaluate "columnSet->isRenderMultiColumnSpannerSet() ||
columnSet->heightIsAuto()" even when hasSpannerOrAutoHeightColumnSet is set.

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:201:
m_needsColumnHeightsRecalculation = hasSpannerOrAutoHeightColumnSet;
On 2014/11/26 21:09:00, Julien Chaffraix - PST wrote:
> Note that you don't really need hasSpannerOrAutoHeightColumnSet if you just
> reset the member boolean before the for-loop above :)

Doh! :) Done.

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:378: // after the spanner
(or the spanner itself) don't bleed into the column preceding the
On 2014/11/26 21:09:01, Julien Chaffraix - PST wrote:
> > so that contents that's supposed .... don't bleed into....
> 
> It seems like it's missing an 'any' before contents (or just use content
> singular but we need to update the verb). I am not a native speakers so I
could
> be completely wrong.

Your country of residence trumps mine, I guess. Besides, I agree. :)

My verb usage seems inconsistent as well. I hate that "contents" word.

Done.

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:446: nextSet->destroy();
On 2014/11/26 21:09:01, Julien Chaffraix - PST wrote:
> It would be worth checking that nextSet is not in m_spannerMap. The best way
> should be a RELEASE_ASSERT(isValidColumnSpanAll()) check to
> RenderObject::willBeDestroyed (with a comment that we should have removed it
> from the flow-thread map before destroying it).

Done.

Better do it in willBeRemovedFromTree(), though. In willBeDestroyed(), there's
no way to locate the flow thread, since you've been taken out from the tree.
Also, the correct expression is !isColumnSpanAll().

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:447:
previousSet->setNeedsLayout();
On 2014/11/26 21:09:01, Julien Chaffraix - PST wrote:
> What ensures that the exiting content under |nextSet| gets moved into the
> previousSet?

A column set doesn't own / contain / reference any of the renderers inside the
flow thread. It just has a flow thread portion rectangle, which gets updated to
enclose everything it should during layout.

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
File Source/core/rendering/RenderMultiColumnFlowThreadTest.cpp (right):

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThreadTest.cpp:263: }
On 2014/11/26 21:09:01, Julien Chaffraix - PST wrote:
> It seems like we should have a vertical writing mode test for that too as it
is
> a logical function.

Done.

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
File Source/core/rendering/RenderMultiColumnSet.cpp (right):

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnSet.cpp:91:
setFlowThreadPortionRect(rect);
On 2014/11/26 21:09:01, Julien Chaffraix - PST wrote:
> I assume we have testing for that?

I just wrote unit tests for it. :)

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
File Source/core/rendering/RenderMultiColumnSpannerSet.cpp (right):

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnSpannerSet.cpp:39: return
m_rendererInFlowThread->logicalHeight();
On 2014/11/26 21:09:01, Julien Chaffraix - PST wrote:
> Should we ASSERT that m_rendererInFlowThread doesn't need layout?

Good idea. Done. But then we obviously have to avoid calling this method before
and during layout. I had to add some guards against column spanners here and
there now, including the ones I removed in patch set 8.

> More conceptually, it doesn't seem correct to return the logical height of the
> content for the page logical height. It's not a paginated value at all.

Indeed. This is another unfortunate consequence of RenderMultiColumnSpannerSet
inheriting from RenderMultiColumnSet, and that spanners are part of the flow
thread at all.

This is the weakness of my current proposed implementation of column-span:all;
laying it out inside of the flow thread. My first proposal [1] teleported the
spanner from its DOM position in the tree to becoming a sibling of the column
sets, so that it wouldn't be part of the flow thread at all. Then we wouldn't
have this problem (but we'd have some questionable teleportation magic instead).

[1] https://codereview.chromium.org/296413007/

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:882: }
On 2014/11/26 21:09:01, Julien Chaffraix - PST wrote:
> A lot of this containing-block related change is orthogonal to adding
> column-span: all support. Please split it into a preparatory change (or a
> post-landing refactoring but it's more tricky as it assumes column-span: all
> sticks) to reduce the size of this change.
> 
> I am happy to review those preparatory changes as it reduces the amount of
time
> I have to spend on this mega-patch :(

Yeah, as long as you think this is a good change on its own, I can land it
separately, and then rebase this CL on top of it.

[Julien - ping for review, 2014-12-03 23:02:22]

I think the next iteration (with the containing block changes out) will look
good.

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
File Source/core/rendering/RenderMultiColumnSpannerSet.cpp (right):

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnSpannerSet.cpp:39: return
m_rendererInFlowThread->logicalHeight();
On 2014/11/27 21:20:29, mstensho wrote:
> On 2014/11/26 21:09:01, Julien Chaffraix - PST wrote:
> > Should we ASSERT that m_rendererInFlowThread doesn't need layout?
> 
> Good idea. Done. But then we obviously have to avoid calling this method
before
> and during layout. I had to add some guards against column spanners here and
> there now, including the ones I removed in patch set 8.
> 
> > More conceptually, it doesn't seem correct to return the logical height of
the
> > content for the page logical height. It's not a paginated value at all.
> 
> Indeed. This is another unfortunate consequence of RenderMultiColumnSpannerSet
> inheriting from RenderMultiColumnSet, and that spanners are part of the flow
> thread at all.
> 
> This is the weakness of my current proposed implementation of column-span:all;
> laying it out inside of the flow thread. My first proposal [1] teleported the
> spanner from its DOM position in the tree to becoming a sibling of the column
> sets, so that it wouldn't be part of the flow thread at all. Then we wouldn't
> have this problem (but we'd have some questionable teleportation magic
instead).
> 
> [1] https://codereview.chromium.org/296413007/

Both patches seem as complicated at the end of the day. I still like this a bit
more as it produces a tree that's closer to the DOM.

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:882: }
On 2014/11/27 21:20:29, mstensho wrote:
> On 2014/11/26 21:09:01, Julien Chaffraix - PST wrote:
> > A lot of this containing-block related change is orthogonal to adding
> > column-span: all support. Please split it into a preparatory change (or a
> > post-landing refactoring but it's more tricky as it assumes column-span: all
> > sticks) to reduce the size of this change.
> > 
> > I am happy to review those preparatory changes as it reduces the amount of
> time
> > I have to spend on this mega-patch :(
> 
> Yeah, as long as you think this is a good change on its own, I can land it
> separately, and then rebase this CL on top of it.

Anything that shrinks this patch should be split out and landed separately.

https://codereview.chromium.org/712553003/diff/180001/Source/core/rendering/R...
File Source/core/rendering/RenderBlockFlow.cpp (right):

https://codereview.chromium.org/712553003/diff/180001/Source/core/rendering/R...
Source/core/rendering/RenderBlockFlow.cpp:1336: if (layoutState->isPaginated()
&& layoutState->pageLogicalHeight() && logicalTop > beforeCollapseLogicalTop &&
!child->isColumnSpanAll()) {
This check feels magical. Why do column-span: all element not have collapsing
margins with the page edge?

https://codereview.chromium.org/712553003/diff/180001/Source/core/rendering/R...
Source/core/rendering/RenderBlockFlow.cpp:1552: if (layoutState->isPaginated()
&& layoutState->pageLogicalHeight() && logicalTopEstimate > logicalHeight() &&
!child->isColumnSpanAll())
Same comment :(

https://codereview.chromium.org/712553003/diff/180001/Source/core/rendering/R...
File Source/core/rendering/RenderMultiColumnFlowThreadTest.cpp (right):

https://codereview.chromium.org/712553003/diff/180001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThreadTest.cpp:290:
TEST_F(MultiColumnRenderingTest, columnSetAtBlockOffset_verticalLr)
The name is unconsistent, you should either useCamelCaseVariables or
use_underscored_variables but not both.

https://codereview.chromium.org/712553003/diff/180001/Source/core/rendering/R...
File Source/core/rendering/RenderMultiColumnSpannerSet.cpp (right):

https://codereview.chromium.org/712553003/diff/180001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnSpannerSet.cpp:40: return
m_rendererInFlowThread->logicalHeight();
Isn't there an issue here if we don't have enough room for the content and thus
should fall back to the non-spanning case?

[mstensho (USE GERRIT), 2014-12-10 13:48:19]

As you have pointed out, we cheat regarding the containing block, and the
proposed solution here also demands that we take extreme caution (strange
isColumnSpanAll() checks all over the place) when laying out spanners, so that
we're not affected by or affect the fragmentation code.

So I want to kill this review and restart with a modified solution, if you don't
mind (too much). It's necessary in order to address
https://codereview.chromium.org/712553003/diff/120001/Source/core/rendering/R...

First, the restructuring of what's already on master (containing block changes,
and renaming RenderColumnSpannerSet to RenderMultiColumnSpannerPlaceholder):
https://codereview.chromium.org/789433006/

Then support for actual column-span:all layout:
https://codereview.chromium.org/792803002/

With this solution, we still need hooks (and more of them than I've made so far)
in central classes like RenderBlock, but they will be similar to existing hooks
for absolutely positioned descendants, which I hope makes a lot more sense.

https://codereview.chromium.org/712553003/diff/120001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/712553003/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:845: o =
toRenderMultiColumnFlowThread(flowThreadContainingBlock())->multiColumnBlockFlow();
On 2014/11/21 15:39:24, mstensho wrote:
> On 2014/11/19 17:54:34, Julien Chaffraix - PST wrote:
> > If you update containingBlock(), you should also update container().
> 
> If I do that, I also have to deal with changed behavior in
> RenderObject::markContainingBlocksForLayout(), which means that I'll have to
> change how we get to the spanners during layout, since we won't necessarily
> reach the parent RenderBlock of spanners anymore. By all means, this may turn
> out to be a good and beautiful thing, but it will be a rather fundamental
change
> in this CL.
> 
> But I feel that I've already changed the code too much since last time you
gave
> feedback, so I won't do this until you've had a look at what I've done so far.
> You might ask me to go back to doing a couple of ugly things with
> RenderBox::containingBlockLogical{Width,Height}ForContent() instead. :)
> 
> Let me know if you still want me to patch container().

To follow up here: I tried this (make container() tell the truth, i.e. return
the multicol container as containing block for spanners). This has vast
consequences for the code, as expected, and kind of forces us to truly take
spanners out from the flow thread (pretty much similar to what we do for
absolutely positioned elements in general). I really liked the result, but I've
created new CLs for it (see top of comment). If you like them, we can drop this
CL.

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
File Source/core/rendering/RenderMultiColumnSpannerSet.cpp (right):

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnSpannerSet.cpp:39: return
m_rendererInFlowThread->logicalHeight();
On 2014/12/03 23:02:22, Julien Chaffraix - PST wrote:
> On 2014/11/27 21:20:29, mstensho wrote:
> > On 2014/11/26 21:09:01, Julien Chaffraix - PST wrote:
> > > Should we ASSERT that m_rendererInFlowThread doesn't need layout?
> > 
> > Good idea. Done. But then we obviously have to avoid calling this method
> before
> > and during layout. I had to add some guards against column spanners here and
> > there now, including the ones I removed in patch set 8.
> > 
> > > More conceptually, it doesn't seem correct to return the logical height of
> the
> > > content for the page logical height. It's not a paginated value at all.
> > 
> > Indeed. This is another unfortunate consequence of
RenderMultiColumnSpannerSet
> > inheriting from RenderMultiColumnSet, and that spanners are part of the flow
> > thread at all.
> > 
> > This is the weakness of my current proposed implementation of
column-span:all;
> > laying it out inside of the flow thread. My first proposal [1] teleported
the
> > spanner from its DOM position in the tree to becoming a sibling of the
column
> > sets, so that it wouldn't be part of the flow thread at all. Then we
wouldn't
> > have this problem (but we'd have some questionable teleportation magic
> instead).
> > 
> > [1] https://codereview.chromium.org/296413007/
> 
> Both patches seem as complicated at the end of the day. I still like this a
bit
> more as it produces a tree that's closer to the DOM.

I've found a third way, which I dislike even less. As mentioned in another
comment, if we fix spanner->container() to return the multicol container, we
need to change quite a few things, and for the better, at that. The main benefit
is that we don't have to lay out spanners inside the flow thread and be careful
to walk around out any kind of fragmentation code that could break the spanner.
Margin handling will be easier (when we get to that) too.

See top of comment for new CLs.

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/712553003/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:882: }
On 2014/12/03 23:02:22, Julien Chaffraix - PST wrote:
> On 2014/11/27 21:20:29, mstensho wrote:
> > On 2014/11/26 21:09:01, Julien Chaffraix - PST wrote:
> > > A lot of this containing-block related change is orthogonal to adding
> > > column-span: all support. Please split it into a preparatory change (or a
> > > post-landing refactoring but it's more tricky as it assumes column-span:
all
> > > sticks) to reduce the size of this change.
> > > 
> > > I am happy to review those preparatory changes as it reduces the amount of
> > time
> > > I have to spend on this mega-patch :(
> > 
> > Yeah, as long as you think this is a good change on its own, I can land it
> > separately, and then rebase this CL on top of it.
> 
> Anything that shrinks this patch should be split out and landed separately.

I found a way around making this big change. The current problem is that while
you work on locating your containing block, you have to walk your containing
block chain to locate your flow thread, just to figure out if you're a spanner.
This information is currently found in the spanner hash map in
RenderMultiColumnFlowThread. If the column-span:all renderer instead had a
pointer to its RenderMultiColumnSpannerSet placeholder, we could avoid involving
the flow thread here.

Since we have established that the real containing block of a spanner is the
multicol container, the spanner really isn't part of the flow thread, so calling
spanner->flowThreadContainingBlock() should by right skip the nearest ancestor
flow thread, since it's not really part of it.

https://codereview.chromium.org/712553003/diff/180001/Source/core/rendering/R...
File Source/core/rendering/RenderBlockFlow.cpp (right):

https://codereview.chromium.org/712553003/diff/180001/Source/core/rendering/R...
Source/core/rendering/RenderBlockFlow.cpp:1336: if (layoutState->isPaginated()
&& layoutState->pageLogicalHeight() && logicalTop > beforeCollapseLogicalTop &&
!child->isColumnSpanAll()) {
On 2014/12/03 23:02:22, Julien Chaffraix - PST wrote:
> This check feels magical. Why do column-span: all element not have collapsing
> margins with the page edge?

Calling nextPageLogicalTop() (see a couple of code lines below) is bogus and
nonsensical for spanners. Furthermore, that would take you to
RenderMultiColumnSpannerSet::pageLogicalHeight(), which now has an assertion. It
would never fail in this case, though, since this we're now inside
collapseMargins(), which are called after layout anyway.

I added this when I realized that we needed a similar check in
adjustForUnsplittableChild() (further up in this file). One test would fail
without that check.

https://codereview.chromium.org/712553003/diff/180001/Source/core/rendering/R...
Source/core/rendering/RenderBlockFlow.cpp:1552: if (layoutState->isPaginated()
&& layoutState->pageLogicalHeight() && logicalTopEstimate > logicalHeight() &&
!child->isColumnSpanAll())
On 2014/12/03 23:02:22, Julien Chaffraix - PST wrote:
> Same comment :(

Same reply as above, :) only that here we could potentially end up in an
assertion failure. We're now in estimateLogicalTopPosition(), which is called
before layout, so the spanner hasn't been laid out yet, so nextPageLogicalTop()
would take you to a RenderMultiColumnSpannerSet::pageLogicalHeight() whose
assertion would fail.

None of the existing tests trigger this. You'd need a top margin on the spanner,
and we don't support margins on spanners yet (I was saving that for another CL).
Currently, any top margin specified on a spanner would make layout look
miserable, so I don't know if it's worthwhile to add a test for it just yet. Or,
I could of course add a crash test.

https://codereview.chromium.org/712553003/diff/180001/Source/core/rendering/R...
File Source/core/rendering/RenderMultiColumnFlowThreadTest.cpp (right):

https://codereview.chromium.org/712553003/diff/180001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThreadTest.cpp:290:
TEST_F(MultiColumnRenderingTest, columnSetAtBlockOffset_verticalLr)
On 2014/12/03 23:02:22, Julien Chaffraix - PST wrote:
> The name is unconsistent, you should either useCamelCaseVariables or
> use_underscored_variables but not both.

Done.

https://codereview.chromium.org/712553003/diff/180001/Source/core/rendering/R...
File Source/core/rendering/RenderMultiColumnSpannerSet.cpp (right):

https://codereview.chromium.org/712553003/diff/180001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnSpannerSet.cpp:40: return
m_rendererInFlowThread->logicalHeight();
On 2014/12/03 23:02:22, Julien Chaffraix - PST wrote:
> Isn't there an issue here if we don't have enough room for the content and
thus
> should fall back to the non-spanning case?

Talking about "When space is limited, it may be impossible to find room for the
spanning element. In these cases, user agents may treat the element as if none
had been specified on this property."?

http://dev.w3.org/csswg/css-multicol/#column-span

Yeah, we won't implement that. In the current implementation that would mean
that we would have to modify the render tree structure dynamically, based on
actual layout, which is just evil (and also verboten in Blink). I've been
fighting with Håkon to get that removed from the spec. :)

[mstensho (USE GERRIT), 2014-12-15 08:45:27]

https://codereview.chromium.org/789433006/ has landed. Closing this. Support for
actual spanner layout is here now: https://codereview.chromium.org/792803002/