Bug 72531 – Add limited parsing support for grid-columns and grid-rows

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

[Julien Chaffraix]

This is about implementing a small part of the spec's grammar for <track-list> (section 6.5).

My goal is to implement the following subset as a first step:

<track-list> := <length> | <percentage> | 'none' | 'auto'

We will then increase our supported set to better match the spec.

[Julien Chaffraix]

Created attachment 115446 [details]
WIP: early review appreciated on naming and such, doesn't build on all platforms.

[Ojan Vafai]

Comment on attachment 115446 [details]
WIP: early review appreciated on naming and such, doesn't build on all platforms.

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

> Source/WebCore/ChangeLog:16
> +        Helper function to convert our Rendertyle information to a proper CSSValue.

typo: Rendertyle

> Source/WebCore/css/CSSParser.cpp:3506
> +bool CSSParser::parseTrackList(int propId, bool important)

parseGridTrackList maybe?

> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:118
> +    DataRef<StyleGridElementData> m_gridLayout;

As discussed in person, call this StyleGridData. Maybe just call this m_grid? That matches better the names of the other member variables here.

[Julien Chaffraix]

Comment on attachment 115446 [details]
WIP: early review appreciated on naming and such, doesn't build on all platforms.

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

>> Source/WebCore/ChangeLog:16
>> +        Helper function to convert our Rendertyle information to a proper CSSValue.
> 
> typo: Rendertyle

Good catch!

>> Source/WebCore/css/CSSParser.cpp:3506
>> +bool CSSParser::parseTrackList(int propId, bool important)
> 
> parseGridTrackList maybe?

It is coherent with valueForGridTrackList. Will change.

>> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:118
>> +    DataRef<StyleGridElementData> m_gridLayout;
> 
> As discussed in person, call this StyleGridData. Maybe just call this m_grid? That matches better the names of the other member variables here.

Will be renamed. I would have leaned towards m_gridData as it would make more sense at a call site (we are not manipulating the grid but the grid data). However consistency is more important.

[Julien Chaffraix]

Created attachment 115467 [details]
Proposed patch 1: Fully building this time, after the renamings.

[Julien Chaffraix]

Tab, we had a discussion yesterday about what getComputedStyle should return. There seems to be no real convention (our code is messy and looks like a per-case choice). What's your opinion on our results? Should we come back to the spec and have it amended? Thanks!

[Tab Atkins]

(In reply to comment #5)
> Tab, we had a discussion yesterday about what getComputedStyle should return. There seems to be no real convention (our code is messy and looks like a per-case choice). What's your opinion on our results? Should we come back to the spec and have it amended? Thanks!

According to Alex Mogilevsky, one of the implementors on IE, it returns the percentage in getComputedStyle.  In other words, it just returns the computed value.

It looks like this is what we're doing, but I'm not entirely sure.  If not, we should match them.  I've asked them to put these details into the spec.

[Tony Chang]

Comment on attachment 115467 [details]
Proposed patch 1: Fully building this time, after the renamings.

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

> LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set.html:28
> +    -webkit-grid-columns: 10em;
> +    -webkit-grid-rows: 15em;

These might resolve to different pixel amounts depending on the platform, but we can rebaseline if that's the case.

[Julien Chaffraix]

Comment on attachment 115467 [details]
Proposed patch 1: Fully building this time, after the renamings.

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

>> LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set.html:28
>> +    -webkit-grid-rows: 15em;
> 
> These might resolve to different pixel amounts depending on the platform, but we can rebaseline if that's the case.

Good point, maybe we should use the Ahem font with a fixed size then to mitigate this risk?

[Tony Chang]

(In reply to comment #8)
> (From update of attachment 115467 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115467&action=review
> 
> >> LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set.html:28
> >> +    -webkit-grid-rows: 15em;
> > 
> > These might resolve to different pixel amounts depending on the platform, but we can rebaseline if that's the case.
> 
> Good point, maybe we should use the Ahem font with a fixed size then to mitigate this risk?

That sounds like a good idea to me.

[Julien Chaffraix]

Created attachment 116809 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 116809 [details]
Patch for landing

Clearing flags on attachment: 116809

Committed r101288: <http://trac.webkit.org/changeset/101288>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.