Bug 168771 – [css-align] Implement the place-content shorthand

• Component: CSS
• Status: RESOLVED FIXED

[Javier Fernandez]

Spec: 

https://drafts.csswg.org/css-align/#propdef-place-content

Check the last comments on this issue before implementing them:
https://github.com/w3c/csswg-drafts/issues/595#issuecomment-262709407

[Javier Fernandez]

Created attachment 302504 [details]
Patch

[Javier Fernandez]

Created attachment 302506 [details]
Patch

[Build Bot]

Comment on attachment 302506 [details]
Patch

Attachment 302506 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3178618

New failing tests:
imported/w3c/web-platform-tests/media-source/mediasource-config-change-mp4-a-bitrate.html

[Build Bot]

Created attachment 302508 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Manuel Rego Casasnovas]

Comment on attachment 302506 [details]
Patch

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

Nice work on this, the patch LGTM but I've some comments on the test.

> Source/WebCore/ChangeLog:15
> +       Test: css3/parse-place-content.html

Nit: Bad indentation on this line.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:5265
> +    if (identMatches<CSSValueStart, CSSValueEnd, CSSValueCenter, CSSValueFlexStart, CSSValueFlexEnd, CSSValueLeft, CSSValueRight>(id))
> +        return CSSContentDistributionValue::create(CSSValueInvalid, range.consumeIncludingWhitespace().id(), CSSValueInvalid);

Couldn't this be merged with the first "if"?

> LayoutTests/css3/parse-place-content-expected.txt:21
> +FAIL Test setting values through JS. assert_equals: placeContent specified value is not what it should.. expected "center" but got "center center"

Do we want a FAIL here?

> LayoutTests/css3/parse-place-content.html:61
> +    <p>Test to verify that the new alignment values are parsed as invalid if Grid Layout is disabled and in any case they do not cause a crash because assertions in flexbox layout code.</p>

Is this accurate? Where are you disabling Grid Layout?

> LayoutTests/css3/resources/alignment-parsing-utils-th.js:5
> +    assert_equals(eval('element.style.' + property), value, property + ' specified value is not what it should..');

Nit: Extra dot at the end.

Why do you need the eval() call?

> LayoutTests/css3/resources/alignment-parsing-utils-th.js:6
> +    assert_equals(eval("window.getComputedStyle(" + elementID + ", '').getPropertyValue('" + propertyID + "')"), computedValue, property + " is not what is should.");

Nit: Typo "is".

> LayoutTests/css3/resources/alignment-parsing-utils-th.js:9
> +function checkBadValues(element, property, propertyID, value)

You're not calling this function.

> LayoutTests/css3/resources/alignment-parsing-utils-th.js:17
> +function checkInitialValues(element, property, propertyID, value, initial)

Ditto.

> LayoutTests/css3/resources/alignment-parsing-utils-th.js:25
> +function checkInheritValues(property, propertyID, value)

Ditto.

> LayoutTests/css3/resources/alignment-parsing-utils-th.js:38
> +function checkLegacyValues(property, propertyID, value)

Ditto.

> LayoutTests/css3/resources/alignment-parsing-utils-th.js:50
> +function checkSupportedValues(elementID, property)

Ditto.

[Javier Fernandez]

Comment on attachment 302506 [details]
Patch

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

>> Source/WebCore/css/parser/CSSPropertyParser.cpp:5265
>> +        return CSSContentDistributionValue::create(CSSValueInvalid, range.consumeIncludingWhitespace().id(), CSSValueInvalid);
> 
> Couldn't this be merged with the first "if"?

The first one checks: normal | <baseline-position> 
The second one checks: <content-position>
The third one checks: <item-position>

I'd rather keep these 3 groups separated, even if some of them imply the same return value. 
I think this structure of the code matches better the spec syntax.

>> LayoutTests/css3/parse-place-content-expected.txt:21
>> +FAIL Test setting values through JS. assert_equals: placeContent specified value is not what it should.. expected "center" but got "center center"
> 
> Do we want a FAIL here?

Yeah, this is because we still don't know how to serialize the shorthand value and WK seems to do different than Blink.
I'll add a comment to explain why we deliberately fail, but I'd prefer to keep the test as it is, for now.

>> LayoutTests/css3/resources/alignment-parsing-utils-th.js:9
>> +function checkBadValues(element, property, propertyID, value)
> 
> You're not calling this function.

This script file is just an adaptation of the one we already have for the alignment related tests, but in this case, using the testharness.th functions. Even if some of these functions are not used by my test, I think it's useful to have the whole file already adapted for future tests or for adapting the ones we already have.

[Javier Fernandez]

Created attachment 302514 [details]
Patch

Applied suggested changes.

[Build Bot]

Comment on attachment 302514 [details]
Patch

Attachment 302514 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3179232

New failing tests:
editing/spelling/spellcheck-async.html

[Build Bot]

Created attachment 302521 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 302514 [details]
Patch

Attachment 302514 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3179459

New failing tests:
media/modern-media-controls/volume-down-support/volume-down-support.html
fast/dom/timer-throttling-hidden-page-non-nested.html

[Build Bot]

Created attachment 302532 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Manuel Rego Casasnovas]

Comment on attachment 302514 [details]
Patch

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

> LayoutTests/css3/parse-place-content.html:217
> +    // We will get FAIL for placeContent specified values tests because we still don't know how to serialize this shortand.

Nit: Please add "FIXME" here.

> LayoutTests/css3/resources/alignment-parsing-utils-th.js:9
> +function checkBadValues(element, property, propertyID, value)

I'm still not convinced to add so  many methods that we don't use.

Are you sure we're going to use them in the future?

[Javier Fernandez]

Created attachment 303059 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 303059 [details]
Patch

Clearing flags on attachment: 303059

Committed r213230: <http://trac.webkit.org/changeset/213230>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

Comment on attachment 303059 [details]
Patch

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

> Source/WebCore/css/CSSProperties.json:3094
> +        "place-content": {
> +            "codegen-properties": {
> +                "longhands": [
> +                    "align-content",
> +                    "justify-content"
> +                ]
> +            }
> +        },

You should have added a "specification" section here.

[Javier Fernandez]

Comment on attachment 303059 [details]
Patch

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

>> Source/WebCore/css/CSSProperties.json:3094
>> +        },
> 
> You should have added a "specification" section here.

I'm not sure what that "specification" section would be; is there any property in the CSSProperties.json file I can use as example ?

[Simon Fraser (smfr)]

(In reply to comment #17)
> Comment on attachment 303059 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=303059&action=review
> 
> >> Source/WebCore/css/CSSProperties.json:3094
> >> +        },
> > 
> > You should have added a "specification" section here.
> 
> I'm not sure what that "specification" section would be; is there any
> property in the CSSProperties.json file I can use as example ?

Update to TOT. You'll see lots of them.