Bug 103597 - CSS3 Multicolumn: cannot set "auto <length>" to columns property.
Summary: CSS3 Multicolumn: cannot set "auto <length>" to columns property.
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL: http://jsfiddle.net/syoichi/ERREh/
Keywords: WebExposed
Depends on:
Blocks: 79073 46590
  Show dependency treegraph
 
Reported: 2012-11-28 21:08 PST by Syoichi Tsuyuhara
Modified: 2014-02-05 11:17 PST (History)
11 users (show)

See Also:


Attachments
Patch (10.42 KB, patch)
2013-02-15 06:53 PST, Morten Stenshorne
no flags Details | Formatted Diff | Diff
Patch (11.98 KB, patch)
2013-02-19 05:14 PST, Morten Stenshorne
no flags Details | Formatted Diff | Diff
Patch (16.95 KB, patch)
2013-03-12 15:09 PDT, Morten Stenshorne
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Syoichi Tsuyuhara 2012-11-28 21:08:44 PST
According to CSS Multi-column Layout Module...
>Name: columns
>Value: <‘column-width’> || <‘column-count’>

>Example XV
>Here are some valid declarations using the ‘columns’ property:
>columns: 12em;      /* column-width: 12em; column-count: auto */
>columns: auto 12em; /* column-width: 12em; column-count: auto */
http://www.w3.org/TR/css3-multicol/#columns

On Windows 7 Home Premium SP1 64bit, Firefox 17.0 and Opera 12.11 correspond to this.
But WebKit r131444 and Chromium 25.0.1338.0 (169999) cannot set "auto 12em" to columns property.
http://jsfiddle.net/syoichi/ERREh/
Comment 1 Morten Stenshorne 2013-02-15 06:53:29 PST
Created attachment 188558 [details]
Patch
Comment 2 Benjamin Poulain 2013-02-18 23:48:03 PST
Comment on attachment 188558 [details]
Patch

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

r- because of the test.

I am not familiar with multicolumn so someone will have to look at this.

> Source/WebCore/css/CSSParser.cpp:3346
> +            /* 'auto' may be a valid value for more than one longhand, and at this point
> +               we don't know which longhand(s) it's meant for. We need to look at the
> +               other values first. Just ignore 'auto', since it means the same as setting
> +               properties to the initial value, which we will take care of after having
> +               processed the value list. */

We don't use this style of comments in WebKit.

> Source/WebCore/css/CSSParser.cpp:3359
> +            // if we didn't find at least one match, this is an
> +            // invalid shorthand and we have to ignore it

WebKit comments needs to be proper sentences. In this case, it misses the uppercase for the first character en the ending period.
When editing existing code, do not hesitate to fix the style.

> LayoutTests/fast/multicol/columns-shorthand-parsing-2.html:49
> +      val[count] = document.getElementById('elm1').style.WebkitColumnCount;
> +      pass &= val[count++] == 3;
> +      val[count] = document.getElementById('elm2').style.WebkitColumnWidth;
> +      pass &= val[count++] == "10em";
> +      val[count] = document.getElementById('elm4').style.WebkitColumnWidth;
> +      pass &= val[count++] != "7em";
> +      val[count] = document.getElementById('elm4').style.WebkitColumnCount;
> +      pass &= val[count++] != 7;
> +      val[count] = document.getElementById('elm5').style.WebkitColumnWidth;
> +      pass &= val[count++] == "7em";
> +      val[count] = document.getElementById('elm5').style.WebkitColumnCount;
> +      pass &= val[count++] == 7;
> +      val[count] = document.getElementById('elm6').style.WebkitColumnWidth;
> +      pass &= val[count++] == "7em";
> +      val[count] = document.getElementById('elm6').style.WebkitColumnCount;
> +      pass &= val[count++] == 7;
> +      val[count] = document.getElementById('elm7').style.WebkitColumnCount;
> +      pass &= val[count++] == 3;
> +      val[count] = document.getElementById('elm8').style.WebkitColumnWidth;
> +      pass &= val[count++] == "10em";
> +      var text;
> +      if (pass)
> +        text = "PASS";
> +      else {
> +        text = "FAIL.<br>";
> +        for (var i = 0; i < val.length; i++)
> +          text += val[i] + "<br>";
> +      }

The layout tests have some scripts to make tests a cleaner than this.
See LayoutTests/fast/js/resources/js-test-pre.js && js-test-post.js	

For the conditions, you can you shouldBe.
The test also need a description, and possibly an explanation.
Comment 3 Morten Stenshorne 2013-02-19 05:14:42 PST
Created attachment 189064 [details]
Patch
Comment 4 Dave Hyatt 2013-02-20 19:07:05 PST
Comment on attachment 189064 [details]
Patch

r-

I'm not sure about this. If auto is explicitly specified, that needs to be remembered so that clients (such as the Web Inspector) will see that the value was explicitly set. I don't think "initial" is the expected result here.
Comment 5 Morten Stenshorne 2013-02-21 00:27:05 PST
Yeah, I was surprised about "initial" too. Even without my change, a longhand that's omitted becomes "initial", e.g. style="-webkit-columns:20em;" gives you a style.WebkitColumnCount of "initial". No other engine does this, but it could be that WebKit is the only engine to support "initial", of course, and CSSParser::parseShorthand() does indeed seem very deliberate about setting it to "initial" if a longhand is omitted (unless you have propertiesForInitialization).

So I will have to make them "auto", if "auto" is specified. How would you prefer that I do that? By modifying CSSParser::parseShorthand() even further, or by making a separate method for "columns" shorthand parsing? Or could I actually modify webkitColumnsShorthand() to set propertiesForInitialization ("auto", "auto")?
Comment 6 Benjamin Poulain 2013-02-21 15:40:22 PST
Comment on attachment 189064 [details]
Patch

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

> LayoutTests/fast/multicol/columns-shorthand-parsing-2.html:23
> +      shouldBe("document.getElementById('elm1').style.WebkitColumnWidth", "'initial'");

Thanks for updating the test.

One more comment :)
When the output of an expression is a string, you should use: shouldBeEqualToString.
E.g.:
    shouldBeEqualToString("document.getElementById('elm1').style.WebkitColumnWidth", 'initial')
Comment 7 Morten Stenshorne 2013-02-22 00:59:16 PST
Thanks, I'll do that. But I'll wait until I get some feedback on how to avoid the "auto" -> "initial" conversions (comment #5).
Comment 8 Morten Stenshorne 2013-03-12 15:09:26 PDT
Created attachment 192819 [details]
Patch
Comment 9 Morten Stenshorne 2013-03-12 15:14:30 PDT
OK, I think I've got the "auto" vs. "initial" thing right (use "auto" if "auto" is in the value list; otherwise use "initial", like any other property).

I also addressed the review issue regarding using shouldBeEqualToString().
Comment 10 Morten Stenshorne 2013-03-22 01:18:33 PDT
Any further comments from the reviewers?
Comment 11 Morten Stenshorne 2013-06-14 06:14:03 PDT
Can we get reviewing started again here?
Comment 12 Morten Stenshorne 2013-07-27 09:03:30 PDT
Fixed in Blink: http://code.google.com/p/chromium/issues/detail?id=235415

Unassigning myself.
Comment 13 Andreas Kling 2014-02-05 11:17:01 PST
Comment on attachment 192819 [details]
Patch

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.