Summary: | CSS3 Multicolumn: column-span should accept value 'none' (instead of '1') | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||
Component: | CSS | Assignee: | Chris Dumez <cdumez> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | cmarcelo, darin, eoconnor, eric.carlson, eric, feature-media-reviews, haraken, hyatt, jchaffraix, kenneth, kling, koivisto, macpherson, menard, ojan, peter, simon.fraser, syoichi, webkit.review.bot | ||||||||||||
Priority: | P2 | Keywords: | WebExposed | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
URL: | http://www.w3.org/TR/css3-multicol/#column-span | ||||||||||||||
Attachments: |
|
Description
Chris Dumez
2012-11-07 06:17:14 PST
Created attachment 172790 [details]
Patch
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 Created attachment 172819 [details]
Patch
Forgot to update the following test case:
fast/multicol/span/span-as-immediate-child-property-removal.html
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 Created attachment 172863 [details]
Patch
Second attempt to make mac-ews happy.
Any feedback on this patch please? Comment on attachment 172863 [details]
Patch
The implementation looks good but I'm not familiar with CSS stuff. CSS experts should look.
Hopefully, people are back from Thanksgiving holiday now :) Could someone please review the patch? Is there anyone else I need to CC? 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.
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 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) Created attachment 176339 [details]
Patch
Take Dave Hyatt's feedback into consideration.
Comment on attachment 176339 [details]
Patch
r=me
Comment on attachment 176339 [details] Patch Clearing flags on attachment: 176339 Committed r136053: <http://trac.webkit.org/changeset/136053> All reviewed patches have been landed. Closing bug. |