Bug 103799 - [CSS Grid Layout] Implement CSS parsing and handling for <track-minmax>
Summary: [CSS Grid Layout] Implement CSS parsing and handling for <track-minmax>
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Julien Chaffraix
URL:
Keywords:
Depends on:
Blocks: 103311
  Show dependency treegraph
 
Reported: 2012-11-30 18:36 PST by Julien Chaffraix
Modified: 2012-12-04 16:26 PST (History)
9 users (show)

See Also:


Attachments
Proposed change. (25.70 KB, patch)
2012-12-03 10:41 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Rebaselined patch with extra testing, fixed a tiny coding style violation (no else after return). (29.97 KB, patch)
2012-12-04 14:36 PST, Julien Chaffraix
tony: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 2012-11-30 18:36:08 PST
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>)
Comment 1 Julien Chaffraix 2012-12-03 10:41:24 PST
Created attachment 177283 [details]
Proposed change.
Comment 2 Tony Chang 2012-12-03 12:17:18 PST
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?.
Comment 3 Julien Chaffraix 2012-12-03 18:10:30 PST
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.
Comment 4 Julien Chaffraix 2012-12-04 14:36:05 PST
Created attachment 177566 [details]
Rebaselined patch with extra testing, fixed a tiny coding style violation (no else after return).
Comment 5 Tony Chang 2012-12-04 14:38:35 PST
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.
Comment 6 Julien Chaffraix 2012-12-04 16:26:52 PST
Committed r136588: <http://trac.webkit.org/changeset/136588>