Bug 161954

Summary: [css-grid] The 'grid' shorthand has a new syntax.
Product: WebKit Reporter: Javier Fernandez <jfernandez>
Component: CSSAssignee: Javier Fernandez <jfernandez>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, darin, hyatt, jfernandez, rego, svillar
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 60731    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Javier Fernandez
Reported 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
Attachments
Patch (66.85 KB, patch)
2016-09-14 02:40 PDT, Javier Fernandez 		no flags
DetailsFormatted DiffDiff
Patch (65.05 KB, patch)
2016-09-20 10:05 PDT, Javier Fernandez 		no flags
DetailsFormatted DiffDiff
Patch (65.03 KB, patch)
2016-09-20 10:28 PDT, Javier Fernandez 		no flags
DetailsFormatted DiffDiff
Patch (65.02 KB, patch)
2016-09-20 11:04 PDT, Javier Fernandez 		no flags
DetailsFormatted DiffDiff
Patch (65.00 KB, patch)
2016-09-20 11:08 PDT, Javier Fernandez 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Javier Fernandez
Comment 1 2016-09-14 02:40:38 PDT
Created attachment 288794 [details] Patch
Darin Adler
Comment 2 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?
Javier Fernandez
Comment 3 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.
Javier Fernandez
Comment 4 2016-09-20 10:05:03 PDT
Created attachment 289369 [details] Patch
Javier Fernandez
Comment 5 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-
Javier Fernandez
Comment 6 2016-09-20 10:28:26 PDT
Created attachment 289375 [details] Patch
Darin Adler
Comment 7 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.
Javier Fernandez
Comment 8 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.
Javier Fernandez
Comment 9 2016-09-20 11:04:49 PDT
Created attachment 289385 [details] Patch
Javier Fernandez
Comment 10 2016-09-20 11:08:54 PDT
Created attachment 289386 [details] Patch
WebKit Commit Bot
Comment 11 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>
WebKit Commit Bot
Comment 12 2016-09-20 11:44:03 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.