Bug 168771 - [css-align] Implement the place-content shorthand
Summary: [css-align] Implement the place-content shorthand
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Javier Fernandez
URL:
Keywords: WebExposed
Depends on:
Blocks: 168845
  Show dependency treegraph
 
Reported: 2017-02-23 04:56 PST by Javier Fernandez
Modified: 2017-03-01 12:06 PST (History)
9 users (show)

See Also:


Attachments
Patch (23.81 KB, patch)
2017-02-23 05:05 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (23.75 KB, patch)
2017-02-23 05:28 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (973.22 KB, application/zip)
2017-02-23 06:51 PST, Build Bot
no flags Details
Patch (23.82 KB, patch)
2017-02-23 08:25 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (1.08 MB, application/zip)
2017-02-23 09:42 PST, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-elcapitan (1.69 MB, application/zip)
2017-02-23 10:43 PST, Build Bot
no flags Details
Patch (23.84 KB, patch)
2017-03-01 03:49 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Javier Fernandez 2017-02-23 04:56:27 PST
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
Comment 1 Javier Fernandez 2017-02-23 05:05:37 PST
Created attachment 302504 [details]
Patch
Comment 2 Javier Fernandez 2017-02-23 05:28:18 PST
Created attachment 302506 [details]
Patch
Comment 3 Build Bot 2017-02-23 06:51:27 PST
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
Comment 4 Build Bot 2017-02-23 06:51:32 PST
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
Comment 5 Manuel Rego Casasnovas 2017-02-23 07:14:02 PST
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.
Comment 6 Javier Fernandez 2017-02-23 07:39:27 PST
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.
Comment 7 Javier Fernandez 2017-02-23 08:25:28 PST
Created attachment 302514 [details]
Patch

Applied suggested changes.
Comment 8 Build Bot 2017-02-23 09:42:13 PST
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
Comment 9 Build Bot 2017-02-23 09:42:17 PST
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
Comment 10 Build Bot 2017-02-23 10:43:05 PST
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
Comment 11 Build Bot 2017-02-23 10:43:10 PST
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
Comment 12 Manuel Rego Casasnovas 2017-02-24 08:03:31 PST
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?
Comment 13 Javier Fernandez 2017-03-01 03:49:53 PST
Created attachment 303059 [details]
Patch
Comment 14 WebKit Commit Bot 2017-03-01 10:49:44 PST
Comment on attachment 303059 [details]
Patch

Clearing flags on attachment: 303059

Committed r213230: <http://trac.webkit.org/changeset/213230>
Comment 15 WebKit Commit Bot 2017-03-01 10:49:52 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Simon Fraser (smfr) 2017-03-01 11:23:59 PST
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.
Comment 17 Javier Fernandez 2017-03-01 12:02:20 PST
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 ?
Comment 18 Simon Fraser (smfr) 2017-03-01 12:06:44 PST
(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.