Add support for the column-fill property.

Add support for the column-fill property.

Note: This is only supported in the (new) region based multicol implementation.

With column-fill support, a lot of multicol tests need an update.
The old implementation behaved as if column-fill were 'auto', but the
initial value is 'balance', so now we need to be explicit about that.
For auto-height tests it doesn't really matter - such multicols are always
balanced anyway.

Already reviewed in WebKit by David Hyatt.
See https://bugs.webkit.org/show_bug.cgi?id=117693

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

[eseidel, 2013-10-02 17:36:44]

The change looks fine.  But we aren't "webkit" anymore, so it's unfare to them
to hijack their CSS prefix.  We're also not using CSS prefixes in Blink if we
can help it.  We'd rather put everything behind runtime flags and only expose it
to the wild web once things are ready.

Can we runtime-guard all of -webkit-column* and remove the -webkit- prefix?

https://codereview.chromium.org/25687002/diff/1/Source/core/css/CSSProperties.in
File Source/core/css/CSSProperties.in (right):

https://codereview.chromium.org/25687002/diff/1/Source/core/css/CSSProperties...
Source/core/css/CSSProperties.in:171: -webkit-column-fill type_name=ColumnFill
We'd rather not introduce more -webkit- prefixes.  Can we remove all the
-webkit- prefixes from columns and put them behind a flag?

[eseidel, 2013-10-02 17:40:08]

Most of the column features aren't used much at all:
http://www.chromestatus.com/metrics/css/popularity

Seems we should add a "CSS columns" runtime flag, and move more and more of the
implementation behind that?

Is this the spec in question?
http://www.w3.org/TR/css3-multicol/

Seems like with the spec at CR we should be thinking about polishing up a subset
to ship:
http://www.chromium.org/blink#new-features

[eseidel, 2013-10-02 17:42:48]

Based on the dashboard entry, sounds like vendors are supportive:
http://www.chromestatus.com/features/6526151266664448

I strongly recommend that we move as quickly as possible away from developing
this under -webkit- prefixes to the runtime flag model. Whether that just means
shipping or adding a runtime flag and only adding new properties there is really
your call.  I dont' know how ship-ready our current implementation is.

[mstensho (USE GERRIT), 2013-10-03 13:10:09]

I'm not sure if I'm doing the right thing by discussing this in the review. We
could switch to mail or IRC if you like. Anyway, here goes:

On 2013/10/02 17:36:44, eseidel wrote:
> The change looks fine.  But we aren't "webkit" anymore, so it's unfare to them
> to hijack their CSS prefix.  We're also not using CSS prefixes in Blink if we
> can help it.  We'd rather put everything behind runtime flags and only expose
it
> to the wild web once things are ready.

This property is also being added to WebKit, so I don't know how rude it really
is to introduce this particular -webkit- prefixed property. But yeah, it's not
ideal. On the other hand, I feel that making column-fill unprefixed and keeping
all the other multicol properties prefixed would be kind of strange too.

Adding support for both prefixed and unprefixed properties to the parser (and
hide some of it behind a runtime flag) doesn't seem like a tempting option
either. If we could support property aliases based on runtime flags, it could
have worked fine-ish, but that doesn't appear to be possible. Double-storing
properties sounds like a lot of trouble and confusion. Pure aliases seems like
the only viable option.

>
https://codereview.chromium.org/25687002/diff/1/Source/core/css/CSSProperties.in
> File Source/core/css/CSSProperties.in (right):
> 
>
https://codereview.chromium.org/25687002/diff/1/Source/core/css/CSSProperties...
> Source/core/css/CSSProperties.in:171: -webkit-column-fill type_name=ColumnFill
> We'd rather not introduce more -webkit- prefixes.  Can we remove all the
> -webkit- prefixes from columns and put them behind a flag?

I'm pretty sure that will affect a bunch of sites, including wikipedia. We need
to keep multicol enabled for all. It's been part of WebKit (and the web) for
years.

On 2013/10/02 17:40:08, eseidel wrote:
> Most of the column features aren't used much at all:
> http://www.chromestatus.com/metrics/css/popularity
> 
> Seems we should add a "CSS columns" runtime flag, and move more and more of
the
> implementation behind that?

There's already this Settings::regionBasedColumnsEnabled() flag that was added
years ago. This is what enables the new implementation (which uses the same
properties as the old/current implementation). It could be turned into a normal
RuntimeEnabledFeatures.in flag, but I'm not sure if it's worth all the changes.
I'm thinking it's better to finish, ship and remove the current flag.

> Is this the spec in question?
> http://www.w3.org/TR/css3-multicol/

Yes.

On 2013/10/02 17:42:48, eseidel wrote:
> Based on the dashboard entry, sounds like vendors are supportive:
> http://www.chromestatus.com/features/6526151266664448

Yes, these days pretty much everyone supports multicol.

> I strongly recommend that we move as quickly as possible away from developing
> this under -webkit- prefixes to the runtime flag model. Whether that just
means
> shipping or adding a runtime flag and only adding new properties there is
really
> your call.  I dont' know how ship-ready our current implementation is.

The current/old implementation is neither perfect nor complete, but perhaps
still really good enough to become unprefixed and enabled by default? With one
exception [1], the properties supported take the correct values, according to
the spec. The rendering engine has issues, such as not being able to handle
nested multicol containers properly (and support column-fill), but I'm not sure
if that's a showstopper.

[1] -webkit-column-span takes the additional value of '1', which isn't a valid
value according to the current spec (it used to be, though). We should deprecate
it and issue a warning when used, and remove support for it completely after
some years.

Could we add unprefixed properties, and make the current prefixed properties
aliases? With no runtime flags involved?


Then there's the matter of the two different multicol implementations in Blink,
and how confusing it is.

I think we should do this:

1. Make the new (region based) multicol implementation complete enough to
replace the current implementation. In details, that basically means adding
support for column-span and forced breaks, support multicol on table cells, and
also provide support for paged overflow (that's currently on top of the multicol
implementation, but with support for flow threads, that's probably no longer
necessary, or even a good idea).

2. Enable the new multicol implementation if experimental web platform features
are enabled.

3. Ship it and kill the old implementation.

If we could just unprefix multicol NOW (and keep the prefixed properties as
aliases), that would be nice. Then step 1 could also involve removing prefixes
from all layout tests.


I feel that I'm stuck in this mess, and would appreciate some guidance. And if
you'd like to continue the discussion elsewhere, that would be fine, since this
isn't really only about column-fill anymore. :)

[Julien - ping for review, 2013-10-03 21:16:26]

On 2013/10/03 13:10:09, Morten Stenshorne wrote:
> I'm not sure if I'm doing the right thing by discussing this in the review. We
> could switch to mail or IRC if you like. Anyway, here goes:
> 
> On 2013/10/02 17:36:44, eseidel wrote:
> > The change looks fine.  But we aren't "webkit" anymore, so it's unfare to
them
> > to hijack their CSS prefix.  We're also not using CSS prefixes in Blink if
we
> > can help it.  We'd rather put everything behind runtime flags and only
expose
> it
> > to the wild web once things are ready.
> 
> This property is also being added to WebKit, so I don't know how rude it
really
> is to introduce this particular -webkit- prefixed property. But yeah, it's not
> ideal. On the other hand, I feel that making column-fill unprefixed and
keeping
> all the other multicol properties prefixed would be kind of strange too.
> 
> Adding support for both prefixed and unprefixed properties to the parser (and
> hide some of it behind a runtime flag) doesn't seem like a tempting option
> either. If we could support property aliases based on runtime flags, it could
> have worked fine-ish, but that doesn't appear to be possible. Double-storing
> properties sounds like a lot of trouble and confusion. Pure aliases seems like
> the only viable option.

WebKit and Blink have different philosophies so don't expect patches to cross
the boundaries untouched. As far as prefixing goes, I am strongly against adding
any more prefixed properties. I also don't buy into the argument that everything
else is prefixed so we should add more prefix. That's just hurting the whole
platform as well as cutting corners.

For unprefixing, our current mindset (which was discussed in a BlinkOn
presentation) is to alias the prefixed / unprefixed properties so they share the
same backing. Double storing doesn't buy us much (except in terms of complexity)
as people usually use one or the other.

>
https://codereview.chromium.org/25687002/diff/1/Source/core/css/CSSProperties.in
> > File Source/core/css/CSSProperties.in (right):
> > 
> >
>
https://codereview.chromium.org/25687002/diff/1/Source/core/css/CSSProperties...
> > Source/core/css/CSSProperties.in:171: -webkit-column-fill
type_name=ColumnFill
> > We'd rather not introduce more -webkit- prefixes.  Can we remove all the
> > -webkit- prefixes from columns and put them behind a flag?
> 
> I'm pretty sure that will affect a bunch of sites, including wikipedia. We
need
> to keep multicol enabled for all. It's been part of WebKit (and the web) for
> years.

It seems that the usage is enough (some properties around 2-3%) to require an
unprefixing period.

> On 2013/10/02 17:40:08, eseidel wrote:
> > Most of the column features aren't used much at all:
> > http://www.chromestatus.com/metrics/css/popularity
> > 
> > Seems we should add a "CSS columns" runtime flag, and move more and more of
> the
> > implementation behind that?
> 
> There's already this Settings::regionBasedColumnsEnabled() flag that was added
> years ago. 

The effort started last year with Dave Hyatt trying to rewrite multi-column on
top of regions. It's also fairly orthogonal to any change made. It would be nice
to finish it and make it the default but it shouldn't block add new properties
or bug fixing the old code.

> This is what enables the new implementation (which uses the same
> properties as the old/current implementation). It could be turned into a
normal
> RuntimeEnabledFeatures.in flag, but I'm not sure if it's worth all the
changes.
> I'm thinking it's better to finish, ship and remove the current flag.
> 
> > Is this the spec in question?
> > http://www.w3.org/TR/css3-multicol/
> 
> Yes.
> 
> On 2013/10/02 17:42:48, eseidel wrote:
> > Based on the dashboard entry, sounds like vendors are supportive:
> > http://www.chromestatus.com/features/6526151266664448
> 
> Yes, these days pretty much everyone supports multicol.
> 
> > I strongly recommend that we move as quickly as possible away from
developing
> > this under -webkit- prefixes to the runtime flag model. Whether that just
> means
> > shipping or adding a runtime flag and only adding new properties there is
> really
> > your call.  I dont' know how ship-ready our current implementation is.
> 
> The current/old implementation is neither perfect nor complete, but perhaps
> still really good enough to become unprefixed and enabled by default? With one
> exception [1], the properties supported take the correct values, according to
> the spec. The rendering engine has issues, such as not being able to handle
> nested multicol containers properly (and support column-fill), but I'm not
sure
> if that's a showstopper.
> 
> [1] -webkit-column-span takes the additional value of '1', which isn't a valid
> value according to the current spec (it used to be, though). We should
deprecate
> it and issue a warning when used, and remove support for it completely after
> some years.
> 
> Could we add unprefixed properties, and make the current prefixed properties
> aliases? With no runtime flags involved?

I would consider this a prerequisite to land this patch without a prefix. I have
no clue about the quality of the implementation and how much it matches the
up-to-date specification though and that would need to be evaluated and fixed
first.

We should also guard the new properties with a runtime flag so as to have the
option to toggle them on / off (like we would for any new feature).

[mstensho (USE GERRIT), 2013-10-04 07:40:00]

Allright - prefix gone, property behind flag. :)

[eseidel, 2013-10-04 14:00:55]

lgtm

[eseidel, 2013-10-04 14:03:38]

I think we're missing a line to disable this property from getComputedStyle. 

RuntimeCSSEnabled is the file you want:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Otherwise we're going to try and dump this property from getComputedStyle even
in release builds.  (adding the line there will also disable parsing of the file
in a slightly differnet way than you already did).

[mstensho (USE GERRIT), 2013-10-04 14:10:39]

On 2013/10/04 14:03:38, eseidel wrote:
> I think we're missing a line to disable this property from getComputedStyle. 
> 
> RuntimeCSSEnabled is the file you want:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Otherwise we're going to try and dump this property from getComputedStyle even
> in release builds.  (adding the line there will also disable parsing of the
file
> in a slightly differnet way than you already did).

I actually thought about that, but I took a look around and saw that e.g. grid
layout (which I believe isn't enabled in stable either?) didn't bother about
such checks. So I thought we didn't care.

Anyway, I'll fix it then.

[mstensho (USE GERRIT), 2013-10-07 15:39:52]

Done.

I'm having a hard time getting the try bots to work (some CRLF end-of-line
issue), but that shouldn't block reviewing, I guess. Patch Set 10 ("Put
column-fill support behind a runtime flag") is all that's interesting since last
time.

[eseidel, 2013-10-07 16:00:22]

lgtm

https://codereview.chromium.org/25687002/diff/43001/Source/core/css/CSSProper...
File Source/core/css/CSSProperties.in (right):

https://codereview.chromium.org/25687002/diff/43001/Source/core/css/CSSProper...
Source/core/css/CSSProperties.in:171: column-fill type_name=ColumnFill
I'm slightly surprised the typename isn't inferred.  We'll do a cleanup of these
at some point.

[commit-bot: I haz the power, 2013-10-07 16:00:39]

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

[commit-bot: I haz the power, 2013-10-07 16:37:00]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_layout.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-10-09 06:44:13]

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

[commit-bot: I haz the power, 2013-10-09 12:08:37]

Change committed as 159227