CSS multicolumn 'columns' shorthand property does not work properly when setting "auto <column-width>" to it. According to http://www.w3.org/TR/2011/CR-css3-multicol-20110412/#columns, setting 'columns' property to "auto(column-count) <column-width>" should work properly.
Created attachment 249808 [details] Patch
Created attachment 249809 [details] Patch
Created attachment 249810 [details] Patch
Comment on attachment 249810 [details] Patch Attachment 249810 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6613561828704256 New failing tests: fast/multicol/columns-shorthand-parsing-2.html
Created attachment 249812 [details] Archive of layout-test-results from ews100 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 249810 [details] Patch Attachment 249810 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5443719574585344 New failing tests: fast/multicol/columns-shorthand-parsing-2.html
Created attachment 249813 [details] Archive of layout-test-results from ews105 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 249815 [details] Patch
Comment on attachment 249815 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249815&action=review Where is the test case that covers “implicit” vs. “explicit”? > Source/WebCore/css/CSSParser.cpp:3641 > + return false; // Too many vlaues for this shorthand. Invalid declaration. Typo "vlaues" > Source/WebCore/css/CSSParser.cpp:3665 > + // Time to assign the previously skipped 'auto' values to a property. > + // If both properties are unassigned at this point (i.e. 'columns:auto'), > + // it doesn't matter that much which one we set. > + // The one we don't set here will get an implicit 'auto' value further down. If the implicit vs. explicit distinction matters, I don’t understand why it doesn’t matter in this case and an implicit auto is sufficient. To put it another way, if the implicit vs. explicit distinction did not matter, then we wouldn’t need hasPendingExplicitAuto at all! > Source/WebCore/css/CSSParser.cpp:3680 > + addProperty(CSSPropertyColumnWidth, cssValuePool().createIdentifierValue(CSSValueAuto), important, true /* implicit */); I think this code should just pass !hasPendingExplicitAuto instead of passing "true" for implicit. Then we could get rid of the entire if statement above. > Source/WebCore/css/CSSParser.cpp:3685 > + addProperty(CSSPropertyColumnCount, cssValuePool().createIdentifierValue(CSSValueAuto), important, true /* implicit */); Ditto. > Source/WebCore/css/CSSParser.h:196 > + PassRefPtr<CSSValue> parseColumnWidth(); > + PassRefPtr<CSSValue> parseColumnCount(); For new functions, return type should just be RefPtr. There is no reason to use PassRefPtr for return values any more.
Created attachment 249884 [details] Patch
(In reply to comment #9) > Comment on attachment 249815 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=249815&action=review > > Where is the test case that covers “implicit” vs. “explicit”? > > > Source/WebCore/css/CSSParser.cpp:3641 > > + return false; // Too many vlaues for this shorthand. Invalid declaration. > > Typo "vlaues" > > > Source/WebCore/css/CSSParser.cpp:3665 > > + // Time to assign the previously skipped 'auto' values to a property. > > + // If both properties are unassigned at this point (i.e. 'columns:auto'), > > + // it doesn't matter that much which one we set. > > + // The one we don't set here will get an implicit 'auto' value further down. > > If the implicit vs. explicit distinction matters, I don’t understand why it > doesn’t matter in this case and an implicit auto is sufficient. To put it > another way, if the implicit vs. explicit distinction did not matter, then > we wouldn’t need hasPendingExplicitAuto at all! > > > Source/WebCore/css/CSSParser.cpp:3680 > > + addProperty(CSSPropertyColumnWidth, cssValuePool().createIdentifierValue(CSSValueAuto), important, true /* implicit */); > > I think this code should just pass !hasPendingExplicitAuto instead of > passing "true" for implicit. Then we could get rid of the entire if > statement above. > > > Source/WebCore/css/CSSParser.cpp:3685 > > + addProperty(CSSPropertyColumnCount, cssValuePool().createIdentifierValue(CSSValueAuto), important, true /* implicit */); > > Ditto. > > > Source/WebCore/css/CSSParser.h:196 > > + PassRefPtr<CSSValue> parseColumnWidth(); > > + PassRefPtr<CSSValue> parseColumnCount(); > > For new functions, return type should just be RefPtr. There is no reason to > use PassRefPtr for return values any more. I applied your valuable comments in a new patch, so would you please review this patch again?
Comment on attachment 249884 [details] Patch Attachment 249884 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4624464763420672 New failing tests: fast/css/getPropertyValue-columns.html
Created attachment 249887 [details] Archive of layout-test-results from ews100 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 249884 [details] Patch Attachment 249884 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4541021065052160 New failing tests: fast/css/getPropertyValue-columns.html
Created attachment 249889 [details] Archive of layout-test-results from ews105 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 249890 [details] Patch
Comment on attachment 249890 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249890&action=review > Source/WebCore/css/CSSParser.cpp:3618 > +RefPtr<CSSValue> CSSParser::parseColumnWidth() > +{ > + ValueWithCalculation valueWithCalculation(*m_valueList->current()); > + CSSValueID id = valueWithCalculation.value().id; > + // Always parse this property in strict mode, since it would be ambiguous otherwise when used in the 'columns' shorthand property. > + if (id == CSSValueAuto || (validateUnit(valueWithCalculation, FLength | FNonNeg, CSSStrictMode) && parsedDouble(valueWithCalculation))) { > + RefPtr<CSSValue> parsedValue = parseValidPrimitive(id, valueWithCalculation); > + m_valueList->next(); > + return parsedValue; > + } > + > + return nullptr; > +} I think these functions would read better with early return (check for error and return nullptr) rather than nesting the normal code path inside an if. > Source/WebCore/css/CSSParser.cpp:3661 > + ASSERT(columnCount || coulmnWidth); This will fail to compile because of the typo: coulmnWidth. > LayoutTests/fast/css/getPropertyValue-columns.html:38 > + shouldBeEqualToString('columnsValue("columns4")', 'auto auto'); Is this serialization of "auto" as "auto auto" correct behavior, or a bug?
(In reply to comment #17) > Comment on attachment 249890 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=249890&action=review > > > Source/WebCore/css/CSSParser.cpp:3618 > > +RefPtr<CSSValue> CSSParser::parseColumnWidth() > > +{ > > + ValueWithCalculation valueWithCalculation(*m_valueList->current()); > > + CSSValueID id = valueWithCalculation.value().id; > > + // Always parse this property in strict mode, since it would be ambiguous otherwise when used in the 'columns' shorthand property. > > + if (id == CSSValueAuto || (validateUnit(valueWithCalculation, FLength | FNonNeg, CSSStrictMode) && parsedDouble(valueWithCalculation))) { > > + RefPtr<CSSValue> parsedValue = parseValidPrimitive(id, valueWithCalculation); > > + m_valueList->next(); > > + return parsedValue; > > + } > > + > > + return nullptr; > > +} > > I think these functions would read better with early return (check for error > and return nullptr) rather than nesting the normal code path inside an if. > Yes, I'd apply it. > > Source/WebCore/css/CSSParser.cpp:3661 > > + ASSERT(columnCount || coulmnWidth); > > This will fail to compile because of the typo: coulmnWidth. > After applying your feedback, I found this assert statment is not safisfied when we are in "columns: auto" case. So I removed it. > > LayoutTests/fast/css/getPropertyValue-columns.html:38 > > + shouldBeEqualToString('columnsValue("columns4")', 'auto auto'); > > Is this serialization of "auto" as "auto auto" correct behavior, or a bug? Hmm, it seems that spec doesn't say anything about it. In my opinion, 'auto auto' looks more consistent, but I choose to maintain current state.
Created attachment 249962 [details] Patch
Comment on attachment 249962 [details] Patch cq=me. It looks GTK ews is broken.
Comment on attachment 249890 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249890&action=review >> Source/WebCore/css/CSSParser.cpp:3661 >> + ASSERT(columnCount || coulmnWidth); > > This will fail to compile because of the typo: coulmnWidth. I missed this one. Why do you remove this ASSERT ? Don't you need to add below ? ASSERT(columnCount || columnWidth);
(In reply to comment #21) > Comment on attachment 249890 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=249890&action=review > > >> Source/WebCore/css/CSSParser.cpp:3661 > >> + ASSERT(columnCount || coulmnWidth); > > > > This will fail to compile because of the typo: coulmnWidth. > > I missed this one. Why do you remove this ASSERT ? > > Don't you need to add below ? > > ASSERT(columnCount || columnWidth); As you can see in https://bugs.webkit.org/attachment.cgi?id=249815&action=review, this patch originally included 'if (hasPendingExplicitAuto) clause' in CSSParser.cpp:3661. But Darin pointed out that this is not necessary and can be more simplified, and told me how to do that. So I removed the assert statement applying darin's feedback. :)
Comment on attachment 249962 [details] Patch Clearing flags on attachment: 249962 Committed r182270: <http://trac.webkit.org/changeset/182270>
All reviewed patches have been landed. Closing bug.