Bug 101462 - CSS3 Multicolumn: column-span should accept value 'none' (instead of '1')
Summary: CSS3 Multicolumn: column-span should accept value 'none' (instead of '1')
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL: http://www.w3.org/TR/css3-multicol/#c...
Keywords: WebExposed
Depends on:
Blocks:
 
Reported: 2012-11-07 06:17 PST by Chris Dumez
Modified: 2012-11-28 13:31 PST (History)
19 users (show)

See Also:


Attachments
Patch (9.45 KB, patch)
2012-11-07 06:59 PST, Chris Dumez
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch (10.24 KB, patch)
2012-11-07 09:08 PST, Chris Dumez
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch (10.23 KB, patch)
2012-11-07 13:05 PST, Chris Dumez
hyatt: review-
hyatt: commit-queue-
Details | Formatted Diff | Diff
Patch (10.29 KB, patch)
2012-11-27 13:58 PST, Chris Dumez
hyatt: review-
hyatt: commit-queue-
Details | Formatted Diff | Diff
Patch (9.59 KB, patch)
2012-11-27 14:29 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2012-11-07 06:17:14 PST
In http://www.w3.org/TR/2009/CR-css3-multicol-20091217/#column-span:
column-span was accepting values: 1 | all

but in http://www.w3.org/TR/2011/CR-css3-multicol-20110412/#column-span:
possibles values are now: none | all

There was no change in meaning but the value '1' was renamed to 'none'. We should update the WebKit implementation accordingly.
Comment 1 Chris Dumez 2012-11-07 06:59:49 PST
Created attachment 172790 [details]
Patch
Comment 2 Build Bot 2012-11-07 08:53:44 PST
Comment on attachment 172790 [details]
Patch

Attachment 172790 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14745695

New failing tests:
fast/multicol/span/span-as-immediate-child-property-removal.html
Comment 3 Chris Dumez 2012-11-07 09:08:03 PST
Created attachment 172819 [details]
Patch

Forgot to update the following test case:
fast/multicol/span/span-as-immediate-child-property-removal.html
Comment 4 Build Bot 2012-11-07 12:44:50 PST
Comment on attachment 172819 [details]
Patch

Attachment 172819 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14765195

New failing tests:
fast/multicol/span/span-as-immediate-child-property-removal.html
Comment 5 Chris Dumez 2012-11-07 13:05:34 PST
Created attachment 172863 [details]
Patch

Second attempt to make mac-ews happy.
Comment 6 Chris Dumez 2012-11-09 12:26:55 PST
Any feedback on this patch please?
Comment 7 Kentaro Hara 2012-11-15 06:14:17 PST
Comment on attachment 172863 [details]
Patch

The implementation looks good but I'm not familiar with CSS stuff. CSS experts should look.
Comment 8 Chris Dumez 2012-11-27 11:29:35 PST
Hopefully, people are back from Thanksgiving holiday now :) Could someone please review the patch?

Is there anyone else I need to CC?
Comment 9 Dave Hyatt 2012-11-27 13:01:44 PST
Comment on attachment 172863 [details]
Patch

This will cause a compatibility issue. For the -webkit- prefix, we need to continue to support the value of 1. If you want to add, none and make it the default, I think that should be safe, but the explicit value of 1 needs to continue to work. When we implement unprefixed column properties, we can drop support for 1 at that time.
Comment 10 Chris Dumez 2012-11-27 13:58:26 PST
Created attachment 176334 [details]
Patch

Take Dave Hyatt's feedback into consideration. This new patch keeps supporting 1 value for compatibility and maps it to none.
Comment 11 Dave Hyatt 2012-11-27 14:21:40 PST
Comment on attachment 176334 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176334&action=review

Looking good. Just some minor suggestions now.

> LayoutTests/fast/multicol/span/span-as-immediate-child-property-removal.html:135
> -    spanners[i].style.webkitColumnSpan = 1;
> +    spanners[i].style.webkitColumnSpan = 'none';

You don't have to fix up this test now. I would suggest reverting this change so that we keep testing 1 as a possible value here.

> Source/WebCore/css/CSSParser.cpp:2524
> +    case CSSPropertyWebkitColumnSpan: // none | all | 1

Maybe mention that 1 will be removed when we drop the prefix. Something like:

case CSSPropertyWebkitColumnSpan: // none | all | 1 (will be dropped in the unprefixed property)
Comment 12 Chris Dumez 2012-11-27 14:29:01 PST
Created attachment 176339 [details]
Patch

Take Dave Hyatt's feedback into consideration.
Comment 13 Dave Hyatt 2012-11-28 13:04:47 PST
Comment on attachment 176339 [details]
Patch

r=me
Comment 14 WebKit Review Bot 2012-11-28 13:31:03 PST
Comment on attachment 176339 [details]
Patch

Clearing flags on attachment: 176339

Committed r136053: <http://trac.webkit.org/changeset/136053>
Comment 15 WebKit Review Bot 2012-11-28 13:31:09 PST
All reviewed patches have been landed.  Closing bug.