Bug 161954 – [css-grid] The 'grid' shorthand has a new syntax.

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

[Javier Fernandez]

The CSS Grid Layout spec defines a new syntax [1] for the 'grid' shorthand. See the related CSS WG discussion [2] to know the rationale of such change.

We should implement the required changes in the CSS Parsing logic to adapt our implementation to the new syntax.

[1] https://drafts.csswg.org/css-grid/#grid-shorthand
[2] https://lists.w3.org/Archives/Public/www-style/2016Apr/0249.html

[Javier Fernandez]

Created attachment 288794 [details]
Patch

[Darin Adler]

Comment on attachment 288794 [details]
Patch

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

Any compatibility issues with changing this syntax?

> Source/WebCore/css/parser/CSSParser.cpp:5780
> +RefPtr<CSSValue> CSSParser::parseImplicitAutoFlow(CSSParserValueList& inputList, AutoFlowType direction)

Why is this a member function? I don’t see anything this is doing that requires access to CSSParser. Can this be a static member function a free function instead?

[Javier Fernandez]

(In reply to comment #2)
> Comment on attachment 288794 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=288794&action=review
> 
> Any compatibility issues with changing this syntax?

Well, the new syntax makes invalid several cases using the old syntax, indeed. However, the CSS Grid Layout feature is not shipped in release versions of any browser. All of them provide the feature behind experimental flags, so I don't think this could cause any issue.

> 
> > Source/WebCore/css/parser/CSSParser.cpp:5780
> > +RefPtr<CSSValue> CSSParser::parseImplicitAutoFlow(CSSParserValueList& inputList, AutoFlowType direction)
> 
> Why is this a member function? I don’t see anything this is doing that
> requires access to CSSParser. Can this be a static member function a free
> function instead?

Yes, it can be defined as static member. I'll do that change in the patch for landing.

[Javier Fernandez]

Created attachment 289369 [details]
Patch

[Javier Fernandez]

I applied several changes to the already reviewed patch. so I guess I should ask for a new review. Basically, I did 2 changes: 

1- solve a bug that would let parser interpret "dense 10px / auto-flow" as "10px / auto-flow", but it should be invalid.

2- some formating changes to avoid assignments as part of the if statements and removed the artificial enumeration for the auto-flow direction-

[Javier Fernandez]

Created attachment 289375 [details]
Patch

[Darin Adler]

Comment on attachment 289375 [details]
Patch

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

> Source/WebCore/css/parser/CSSParser.cpp:5828
> +    CSSParserValue* value = m_valueList->current();

I would have used auto here.

[Javier Fernandez]

(In reply to comment #7)
> Comment on attachment 289375 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=289375&action=review
> 
> > Source/WebCore/css/parser/CSSParser.cpp:5828
> > +    CSSParserValue* value = m_valueList->current();
> 
> I would have used auto here.

I'll do in the patch for landing. Thanks for the quick review, BTW.

[Javier Fernandez]

Created attachment 289385 [details]
Patch

[Javier Fernandez]

Created attachment 289386 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 289386 [details]
Patch

Clearing flags on attachment: 289386

Committed r206161: <http://trac.webkit.org/changeset/206161>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.