Multi-column properties in a flexbox hide its content

Multi-column properties in a flexbox hide its content

In a flexbox if you set any of the multi-column properties (column-*)
the content of the flexbox is hidden.

According to the spec http://www.w3.org/TR/css3-flexbox/#propdef-flex
muti-column properties should be ignored:

"all of the column-* properties in the Multicol module have no
effect on a flex container."

This patch fixes this bug by adding a check in RenderBlock::requiresColumns
that columns are not required for Flexible box.

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

[harpreet.sk, 2014-08-21 16:44:57]

ptal...

[mstensho (USE GERRIT), 2014-08-21 20:11:51]

multi-column misspelled as "muti-column" in the file names.

Also: would it be an idea to make a test that detects that (and fails if)
multicol gets enabled? While the bug currently is in such a way that everything
just disappears, this may not be the case in the future, and perhaps the test
should be a bit more robust/evil. A reftest, perhaps?

[cbiesinger, 2014-08-21 20:50:47]

On 2014/08/21 20:11:51, mstensho wrote:
> multi-column misspelled as "muti-column" in the file names.
> 
> Also: would it be an idea to make a test that detects that (and fails if)
> multicol gets enabled? While the bug currently is in such a way that
everything
> just disappears, this may not be the case in the future, and perhaps the test
> should be a bit more robust/evil. A reftest, perhaps?

Yeah, why not make it a reftest which would be identical except not having the
column-count property?

[harpreet.sk, 2014-08-22 11:31:22]

On 2014/08/21 20:11:51, mstensho wrote:
> multi-column misspelled as "muti-column" in the file names.
> 
> Also: would it be an idea to make a test that detects that (and fails if)
> multicol gets enabled? While the bug currently is in such a way that
everything
> just disappears, this may not be the case in the future, and perhaps the test
> should be a bit more robust/evil. A reftest, perhaps?


@mstensho: thanks for the review. I have corrected the file names. Also
converted the added test into a reftest. PTAL.

[mstensho (USE GERRIT), 2014-08-22 11:52:34]

Sorry, I forgot to remark that a reftest that doesn't really make any
assumptions about how things might fail,
"flexbox-hidden-by-multi-column-property" isn't an optimal name. 

I suggest "flexbox-with-multi-column-property".

https://codereview.chromium.org/488183003/diff/40001/LayoutTests/css3/flexbox...
File LayoutTests/css3/flexbox/flexbox-hidden-by-multi-column-property.html
(right):

https://codereview.chromium.org/488183003/diff/40001/LayoutTests/css3/flexbox...
LayoutTests/css3/flexbox/flexbox-hidden-by-multi-column-property.html:14:
<div>This text should be displayed</div>
Should have something here that's guaranteed to be split into columns. Currently
it may all end up as one line, so that it will look the same in a multicol
container as in a regular container.

Something like this, maybe:
First line<br>
Second line<br>
Third line<br>

[harpreet.sk, 2014-08-22 12:43:44]

Modified layout test. PTAL

https://codereview.chromium.org/488183003/diff/40001/LayoutTests/css3/flexbox...
File LayoutTests/css3/flexbox/flexbox-hidden-by-multi-column-property.html
(right):

https://codereview.chromium.org/488183003/diff/40001/LayoutTests/css3/flexbox...
LayoutTests/css3/flexbox/flexbox-hidden-by-multi-column-property.html:14:
<div>This text should be displayed</div>
On 2014/08/22 11:52:34, mstensho wrote:
> Should have something here that's guaranteed to be split into columns.
Currently
> it may all end up as one line, so that it will look the same in a multicol
> container as in a regular container.
> 
> Something like this, maybe:
> First line<br>
> Second line<br>
> Third line<br>

Acknowledged.

[mstensho (USE GERRIT), 2014-08-22 13:02:17]

non-owner lgtm

[harpreet.sk, 2014-08-22 13:18:37]

Owner's please give lgtm

[rune, 2014-08-22 13:25:09]

lgtm with nits.

The description lines look kinda short? (better than too long, of course).

https://codereview.chromium.org/488183003/diff/60001/LayoutTests/css3/flexbox...
File LayoutTests/css3/flexbox/flexbox-with-multi-column-property-expected.html
(right):

https://codereview.chromium.org/488183003/diff/60001/LayoutTests/css3/flexbox...
LayoutTests/css3/flexbox/flexbox-with-multi-column-property-expected.html:2:
<html>
Leave html/body elements out

https://codereview.chromium.org/488183003/diff/60001/LayoutTests/css3/flexbox...
File LayoutTests/css3/flexbox/flexbox-with-multi-column-property.html (right):

https://codereview.chromium.org/488183003/diff/60001/LayoutTests/css3/flexbox...
LayoutTests/css3/flexbox/flexbox-with-multi-column-property.html:2: <html>
Leave html/body elements out

[harpreet.sk, 2014-08-22 14:31:04]

nits incorporated and description lines made little long.

https://codereview.chromium.org/488183003/diff/60001/LayoutTests/css3/flexbox...
File LayoutTests/css3/flexbox/flexbox-with-multi-column-property-expected.html
(right):

https://codereview.chromium.org/488183003/diff/60001/LayoutTests/css3/flexbox...
LayoutTests/css3/flexbox/flexbox-with-multi-column-property-expected.html:2:
<html>
On 2014/08/22 13:25:09, rune wrote:
> Leave html/body elements out

Done.

https://codereview.chromium.org/488183003/diff/60001/LayoutTests/css3/flexbox...
File LayoutTests/css3/flexbox/flexbox-with-multi-column-property.html (right):

https://codereview.chromium.org/488183003/diff/60001/LayoutTests/css3/flexbox...
LayoutTests/css3/flexbox/flexbox-with-multi-column-property.html:2: <html>
On 2014/08/22 13:25:09, rune wrote:
> Leave html/body elements out

Done.

[harpreet.sk, 2014-08-22 14:31:34]

The CQ bit was checked by harpreet.sk@samsung.com

[commit-bot: I haz the power, 2014-08-22 14:32:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/harpreet.sk@samsung.com/488183003/80001

[commit-bot: I haz the power, 2014-08-22 15:10:44]

Committed patchset #5 (80001) as 180776