Last Comment Bug 446569 - Implement CSS3 columns shorthand
: Implement CSS3 columns shorthand
Status: RESOLVED FIXED
[css3-multicol]
: css3, dev-doc-complete
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- enhancement with 6 votes (vote)
: mozilla9
Assigned To: Michael Ventnor
:
Mentors:
http://www.w3.org/TR/css3-multicol/#l...
: 621252 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-07-22 02:37 PDT by Michael Ventnor
Modified: 2011-10-18 03:19 PDT (History)
22 users (show)
roc: blocking1.9.2-
roc: wanted1.9.2-
roc: wanted1.9.1+
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (12.88 KB, patch)
2008-07-22 02:37 PDT, Michael Ventnor
no flags Details | Diff | Splinter Review
Patch (8.77 KB, patch)
2011-06-22 02:08 PDT, Michael Ventnor
no flags Details | Diff | Splinter Review
Patch prefixed (7.50 KB, patch)
2011-06-23 07:28 PDT, Michael Ventnor
dbaron: review-
Details | Diff | Splinter Review
Patch 2 (10.95 KB, patch)
2011-08-22 05:22 PDT, Michael Ventnor
dbaron: review+
Details | Diff | Splinter Review

Description Michael Ventnor 2008-07-22 02:37:09 PDT
Created attachment 330742 [details] [diff] [review]
Patch

The W3 has a spec where column-count and column-width can be specified with a "columns" shorthand. I think its the only major part of the CSS3 columns spec we still don't support.

Searched and couldn't find a bug on this already.
Comment 1 Michael Ventnor 2008-07-22 02:53:23 PDT
Comment on attachment 330742 [details] [diff] [review]
Patch

Actually, I'll need a different parsing approach, this won't work very well.
Comment 2 philippe (part-time) 2010-12-23 21:26:52 PST
*** Bug 621252 has been marked as a duplicate of this bug. ***
Comment 3 Oli Studholme 2010-12-23 23:23:06 PST
CSS3 multi-column layout spec: http://www.w3.org/TR/css3-multicol/#columns

Additional enhancement requests:
* column-span: https://bugzilla.mozilla.org/show_bug.cgi?id=616436
* column breaks (break-before, break-after, break-inside): https://bugzilla.mozilla.org/show_bug.cgi?id=549114

Original multi-column bug (fixed): https://bugzilla.mozilla.org/show_bug.cgi?id=multicol

@Philippe thanks, I missed this when searching (“columns” was too general, “multicolumn” didn’t show it). Hopefully these keywords will help someone
Comment 4 Peter Gasston 2011-05-05 03:57:06 PDT
Opera, WebKit and IE10 Platform Preview all support the multi-column module now (either completely or near-completely); Opera and IE10 have dropped prefixes so they must consider the spec sufficiently stable.

Opera: http://www.opera.com/docs/specs/presto28/css/multicolumnlayout/
Comment 5 Michael Ventnor 2011-06-22 02:08:05 PDT
We have a more sensible length parser, so this is feasible now. Also, columns have reached CR, so I implemented this unprefixed. We should unprefix the other column properties in another bug.
Comment 6 Michael Ventnor 2011-06-22 02:08:48 PDT
Created attachment 540995 [details] [diff] [review]
Patch

Blimey, this code has changed a lot since I last looked at it...
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-22 04:55:31 PDT
Before we implement this unprefixed I think we need to do a sanity check of our columns implementation against the spec. It's been a long time since we updated our implementation and the spec has changed quite a bit since then.
Comment 8 :Ms2ger 2011-06-22 04:57:51 PDT
I'm not convinced it's a good idea to implement columns unprefixed unless we unprefix the other properties at the same time. (I don't know how much work is left before we can do that.)

(Bah, mid-airs)
Comment 9 Michael Ventnor 2011-06-22 05:03:25 PDT
Comment on attachment 540995 [details] [diff] [review]
Patch

Alright, but I didn't notice any differences while I was reading the spec.
Comment 10 :Ms2ger 2011-06-23 00:19:00 PDT
Michael, please leave the dev-doc-needed keyword; the people who are so kind to update documentation only look at bugs once they're fixed, and I'm sure you agree that this bug will need docs when it's fixed.
Comment 11 Michael Ventnor 2011-06-23 06:09:07 PDT
(In reply to comment #10)
> Michael, please leave the dev-doc-needed keyword; the people who are so kind
> to update documentation only look at bugs once they're fixed, and I'm sure
> you agree that this bug will need docs when it's fixed.

I see, I thought they looked at all bugs and the keyword was only necessary for fixed bugs.
Comment 12 Michael Ventnor 2011-06-23 07:28:35 PDT
Created attachment 541374 [details] [diff] [review]
Patch prefixed

I've decided to implement it prefixed anyway, so that when we do unprefix CSS3 columns then the work for this is already done anyway.
Comment 13 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2011-08-16 16:32:37 PDT
Comment on attachment 541374 [details] [diff] [review]
Patch prefixed

># HG changeset patch
># Parent c931c8b1d8f623693c4d630777a63447cd7de417

In the future, please include the From: line and a commit message in patches posted for review.

>diff --git a/layout/style/nsCSSParser.cpp b/layout/style/nsCSSParser.cpp

>+PRBool
>+CSSParserImpl::ParseColumns()
>+{
>+  const PRInt32 numProps = 2;
>+  static const nsCSSProperty columnIDs[] = {
>+    eCSSProperty__moz_column_count,
>+    eCSSProperty__moz_column_width
>+  };

Probably better to declare numProps second, and make it:

  const PRInt32 numProps = NS_ARRAY_LENGTH(columnIDs);


r=dbaron with that.  Sorry for taking so long to get to this.
Comment 15 Marco Bonardo [::mak] 2011-08-17 07:48:00 PDT
backed out due to a M4 permaorange in test_ch_ex_no_infloops.html

INFO TEST-PASS | /tests/layout/style/test/test_ch_ex_no_infloops.html | Setting '-moz-columns' to 'auto 2ch' should not cause infinite loop - auto ; 16px should not equal
###!!! ABORT: no other shorthands: 'false', file /builds/slave/m-in-osx-dbg/build/layout/style/Declaration.cpp, line 773

the other changeset was about d2d changes, so this was looking like the most obvious culprit.
Comment 16 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2011-08-17 09:17:35 PDT
Ah, yeah, you need changes to Declaration::GetValue too :-)
Comment 17 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2011-08-17 09:34:57 PDT
I also find it interesting that that's the only test exercising Declaration::GetValue on shorthand properties (which it does because nsDOMCSSDeclaration::RemoveProperty calls nsDOMCSSDeclaration::GetPropertyValue which calls Declaration::GetValue).
Comment 18 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2011-08-17 09:35:57 PDT
(Or maybe other tests do too and you just didn't run layout/style mochitests yourself, and since it's an abort we didn't get the errors from tinderbox?  I was thinking it was the only one since I added that test just a few days ago.)
Comment 19 Michael Ventnor 2011-08-18 03:52:29 PDT
Is there anything else apart from that which needs to be done?

I added this to the switch statement of Declaration::GetValue:

    case eCSSProperty__moz_columns: {
      // Two values, column-count and column-width, separated by a space.
      const nsCSSProperty* subprops =
        nsCSSProps::SubpropertyEntryFor(aProperty);
      AppendValueToString(subprops[0], aValue);
      aValue.Append(PRUnichar(' '));
      AppendValueToString(subprops[1], aValue);
      break;
    }

which fixes the test in question, but now I get other failures. In test_value_cloning.html and test_value_storage.html, if it sets "-moz-columns: auto 2" then the computed value returns the empty string.
Comment 20 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2011-08-18 08:43:17 PDT
What's the exact test failure message that you now get?
Comment 21 Michael Ventnor 2011-08-18 23:10:43 PDT
Still haven't managed to figure this out...

(In reply to David Baron [:dbaron] from comment #20)
> What's the exact test failure message that you now get?

In test_value_storage.html I get three identical errors:
"failed | setting 'auto 2' on '-moz-columns' - didn't expect , but got it"

In test_value_computation.html:
"failed | should not get initial value for '-moz-columns:auto 2' on elementn. - didn't expect auto ; auto, but got it
failed | should not get initial value for '-moz-columns:auto 2' on elementf. - didn't expect auto ; auto, but got it"

In test_value_cloning.html:
"failed | serialization should be nonempty for -moz-columns: auto 2 - didn't expect , but got it"

The problem seems to be specific to setting columns to "auto 2", but I haven't figured out why.
Comment 22 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2011-08-19 14:09:10 PDT
(In reply to Michael Ventnor from comment #21)
> The problem seems to be specific to setting columns to "auto 2", but I
> haven't figured out why.

Basically, I think the problem is that you can't use ParseChoice, because it's only ok to use ParseChoice in cases where there's no overlap between the values accepted by the different properties.  Your patch goes wrong with "auto 2" because the "auto" ends up assigned to -moz-column-count.

CSSParserImpl::ParseListStyle has a hacky workaround for this for the case where two properties accept 'none'.  You'll probably need to do something similar here for 'auto' (though it's also possible we could make the hack less ugly).

(In the future, could you run style system mochitests before requesting review?
Comment 23 Michael Ventnor 2011-08-22 05:22:01 PDT
Created attachment 554830 [details] [diff] [review]
Patch 2

This patch seems to work and pass all tests. I added some invalid values to test the bounds of this hack. The hack took me a while to understand but I think I got it.
Comment 24 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2011-08-22 07:36:04 PDT
Comment on attachment 554830 [details] [diff] [review]
Patch 2

In Declaration.cpp, could you put the new case either (a) between background and font or (b) at the end?

Please also add a test for the invalid value "auto auto auto".

r=dbaron with that
Comment 25 Michael Ventnor 2011-08-22 21:24:13 PDT
Sorry for the big delay.

http://hg.mozilla.org/integration/mozilla-inbound/rev/ecb597abc0bf
Comment 26 Mounir Lamouri (:mounir) 2011-08-23 01:45:09 PDT
http://hg.mozilla.org/mozilla-central/rev/ecb597abc0bf

Marking in-test-suite+ because there is a test but I would have expect reftests for a CSS feature...
Comment 27 Eric Shepherd [:sheppy] 2011-10-17 22:01:38 PDT
Documented here by teoli:

https://developer.mozilla.org/en/CSS/columns

Also listed on Firefox 9 for developers.
Comment 28 Jean-Yves Perrier [:teoli] 2011-10-18 03:19:56 PDT
I've also updated the Using CSS3 Columns document:
https://developer.mozilla.org/en/CSS/Using_CSS_multi-column_layouts

(column-rule-* is still not described there, though they are in the CSS Reference part of the MDN)

Note You need to log in before you can comment on or make changes to this bug.