Bug 103597

Created attachment 188558 [details] Patch

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.

Created attachment 189064 [details] Patch

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.

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 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')

Thanks, I'll do that. But I'll wait until I get some feedback on how to avoid the "auto" -> "initial" conversions (comment #5).

Created attachment 192819 [details] Patch

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().

Any further comments from the reviewers?

Can we get reviewing started again here?

Fixed in Blink: http://code.google.com/p/chromium/issues/detail?id=235415 Unassigning myself.

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.

I am unable to reproduce this bug in Safari 15.5 on macOS 12.4 using attached JSFiddle in URL field. The results are as follow: ** Safari 15.5 -webkit-column-width: 12em -webkit-column-count: auto ** Chrome Canary 105 -webkit-column-width: 12em -webkit-column-count: auto I tried the test case in Firefox Nightly 103 but JSFiddle didn't work for some reason and I tried to tweak by removing -webkit- prefixes etc. but it just showed "undefined" for both values. Additionally, the test case attached to linked Blink (Chrome) bug in Comment 12 also show "PASS" now for Safari 15.5 on macOS 12.4. As per Comment 01, these are valid declarations and test case from Chrome bug also passes - can we consider this as "RESOLVED CONFIGURATION CHANGED" or "RESOLVED INVALID"? Thank!

This works now whether we use webkit prefix or not.