Bug 143248 – [CSS MultiColumn] Parse "columns: auto <length>" shorthand property value properly

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Joonghun Park]

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.

[Joonghun Park]

Created attachment 249808 [details]
Patch

[Joonghun Park]

Created attachment 249809 [details]
Patch

[Joonghun Park]

Created attachment 249810 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Joonghun Park]

Created attachment 249815 [details]
Patch

[Darin Adler]

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.

[Joonghun Park]

Created attachment 249884 [details]
Patch

[Joonghun Park]

(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?

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Joonghun Park]

Created attachment 249890 [details]
Patch

[Darin Adler]

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?

[Joonghun Park]

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

[Joonghun Park]

Created attachment 249962 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 249962 [details]
Patch

cq=me. It looks GTK ews is broken.

[Gyuyoung Kim]

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

[Joonghun Park]

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

[WebKit Commit Bot]

Comment on attachment 249962 [details]
Patch

Clearing flags on attachment: 249962

Committed r182270: <http://trac.webkit.org/changeset/182270>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.