Bug 161954 - [css-grid] The 'grid' shorthand has a new syntax.
Summary: [css-grid] The 'grid' shorthand has a new syntax.
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:
Depends on:
Blocks: 60731
  Show dependency treegraph
 
Reported: 2016-09-14 02:23 PDT by Javier Fernandez
Modified: 2016-09-20 11:44 PDT (History)
7 users (show)

See Also:


Attachments
Patch (66.85 KB, patch)
2016-09-14 02:40 PDT, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (65.05 KB, patch)
2016-09-20 10:05 PDT, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (65.03 KB, patch)
2016-09-20 10:28 PDT, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (65.02 KB, patch)
2016-09-20 11:04 PDT, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (65.00 KB, patch)
2016-09-20 11:08 PDT, 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 2016-09-14 02:23:41 PDT
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
Comment 1 Javier Fernandez 2016-09-14 02:40:38 PDT
Created attachment 288794 [details]
Patch
Comment 2 Darin Adler 2016-09-14 12:33:06 PDT
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?
Comment 3 Javier Fernandez 2016-09-14 13:25:51 PDT
(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.
Comment 4 Javier Fernandez 2016-09-20 10:05:03 PDT
Created attachment 289369 [details]
Patch
Comment 5 Javier Fernandez 2016-09-20 10:09:06 PDT
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-
Comment 6 Javier Fernandez 2016-09-20 10:28:26 PDT
Created attachment 289375 [details]
Patch
Comment 7 Darin Adler 2016-09-20 10:37:39 PDT
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.
Comment 8 Javier Fernandez 2016-09-20 11:02:28 PDT
(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.
Comment 9 Javier Fernandez 2016-09-20 11:04:49 PDT
Created attachment 289385 [details]
Patch
Comment 10 Javier Fernandez 2016-09-20 11:08:54 PDT
Created attachment 289386 [details]
Patch
Comment 11 WebKit Commit Bot 2016-09-20 11:43:57 PDT
Comment on attachment 289386 [details]
Patch

Clearing flags on attachment: 289386

Committed r206161: <http://trac.webkit.org/changeset/206161>
Comment 12 WebKit Commit Bot 2016-09-20 11:44:03 PDT
All reviewed patches have been landed.  Closing bug.