Bug 103799 – [CSS Grid Layout] Implement CSS parsing and handling for <track-minmax>

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Julien Chaffraix]

This is the CSS side of bug 103311, once this landed we can work on implementing the rendering. The idea is to implement parsing, applying and getting back through getComputedStyle the following syntax:

<track-minmax>  => minmax( <track-breadth> , <track-breadth> ) | auto | <track-breadth>

(We already support auto | <track-breadth>)

[Julien Chaffraix]

Created attachment 177283 [details]
Proposed change.

[Tony Chang]

Comment on attachment 177283 [details]
Proposed change.

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

Please add some more test cases before landing.

> Source/WebCore/css/CSSFunctionValue.h:54
> +    explicit CSSFunctionValue(String, PassRefPtr<CSSValueList>);

Nit: No explicit.

> Source/WebCore/css/CSSParser.cpp:4538
> +        // We only accept the following grammar: minmax( <track-breadth> , <track-breadth> )

It's not clear to me what 'we' refers to here. I would probably say, "The spec defines the following grammar:".

> Source/WebCore/rendering/style/GridTrackSize.h:64
> +        m_minTrackBreadth = length;
> +        m_maxTrackBreadth = length;

I see, you set both min and max so you can simplify operator==?  I would make operator== more complex instead.

> LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-expected.txt:18
> +PASS getComputedStyle(gridWithMinMax, '').getPropertyValue('-webkit-grid-columns') is 'minmax(10%, 15px)'
> +PASS getComputedStyle(gridWithMinMax, '').getPropertyValue('-webkit-grid-rows') is 'minmax(20px, 50%)'

Can you add some test cases with 'auto', '-webkit-min-content' and '-webkit-calc' as arguments to minmax?.

[Julien Chaffraix]

Comment on attachment 177283 [details]
Proposed change.

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

>> Source/WebCore/css/CSSFunctionValue.h:54
>> +    explicit CSSFunctionValue(String, PassRefPtr<CSSValueList>);
> 
> Nit: No explicit.

I would prefer to keep it but it's a personal preference.

>> Source/WebCore/css/CSSParser.cpp:4538
>> +        // We only accept the following grammar: minmax( <track-breadth> , <track-breadth> )
> 
> It's not clear to me what 'we' refers to here. I would probably say, "The spec defines the following grammar:".

Sounds very good.

>> Source/WebCore/rendering/style/GridTrackSize.h:64
>> +        m_maxTrackBreadth = length;
> 
> I see, you set both min and max so you can simplify operator==?  I would make operator== more complex instead.

It's not only for operator==. The track sizing algorithm in the specification (section 9.2) doesn't special case the single length case but always checks both min and max track sizing functions.

We could special case it for performance but for now, I am taking the approach of following the algorithm to the letter.

>> LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-expected.txt:18
>> +PASS getComputedStyle(gridWithMinMax, '').getPropertyValue('-webkit-grid-rows') is 'minmax(20px, 50%)'
> 
> Can you add some test cases with 'auto', '-webkit-min-content' and '-webkit-calc' as arguments to minmax?.

More testing is always better! However there are 2 issues here:
* -webkit-min-content is not allowed yet (we don't handle it in StyleResolver and probably also CSSParser).
* -webkit-calc is something we can't get back from getComputedStyle properly. See bug 103761 and the discussion in bug 103335.

[Julien Chaffraix]

Created attachment 177566 [details]
Rebaselined patch with extra testing, fixed a tiny coding style violation (no else after return).

[Tony Chang]

Comment on attachment 177566 [details]
Rebaselined patch with extra testing, fixed a tiny coding style violation (no else after return).

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

> Source/WebCore/rendering/RenderGrid.cpp:147
> +            // FIXME: we stil need to support calc() here (bug 103761)

Nit: Add a period to the end of this sentence and capitalize the first letter.

[Julien Chaffraix]

Committed r136588: <http://trac.webkit.org/changeset/136588>