Bug 110777

Summary: [CSS Grid Layout] Extend our grammar to support 2 positions for grid-{row|column}
Product: WebKit Reporter: Julien Chaffraix <jchaffraix>
Component: CSSAssignee: Julien Chaffraix <jchaffraix>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, buildbot, dglazkov, eric, esprehn+autocc, macpherson, menard, ojan.autocc, ojan, rniwa, tabatkins, tony, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 103314, 103334    
Attachments:
Description Flags
Proposed change 1.
none
Proposed change 2. Fixed the EWS.
none
New approach (v3): Made grid-{row|column} a shorthand and added parsing for 2 values.
none
V4: Same as V3 but updated the expected results.
none
Patch for landing none

Description Julien Chaffraix 2013-02-25 11:40:50 PST
The specification now defines grid-{row|column} as a shorthand for resp. grid-{start|end} and grid-{before|after} and thus accepts 2 values: 

grid-{row|column} := <grid-line> [ / <grid-line> ]?
<grid-line> = 
  auto | 
  [ span? && <integer> ] | 
  [ span? && <string> && <integer>? ] | 
  <ident>

This bug will extend our handling of grid-{row|column} to accept the optional second <grid-line>
Comment 1 Julien Chaffraix 2013-02-25 12:13:37 PST
> grid-{row|column} as a shorthand for resp. grid-{start|end} and grid-{before|after}

This is wrong: it is the opposite ie grid-row is a shorthand for grid-{before|after}.
Comment 2 Julien Chaffraix 2013-02-25 12:31:41 PST
Created attachment 190105 [details]
Proposed change 1.
Comment 3 Tony Chang 2013-02-25 15:00:41 PST
Comment on attachment 190105 [details]
Proposed change 1.

Would it make more sense for us to
1) Rename grid-column/grid-row to grid-before/grid-start.
2) Add grid-end/grid-after.
3) Add the shorthand rules for grid-column/grid-row.

There's code for handling shorthands automatically so we shouldn't have extra code for parsing grid-column/grid-row.
Comment 4 Build Bot 2013-02-25 15:21:25 PST
Comment on attachment 190105 [details]
Proposed change 1.

Attachment 190105 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16745551

New failing tests:
svg/css/getComputedStyle-basic.xhtml
fast/css/getComputedStyle/computed-style.html
fast/css/getComputedStyle/computed-style-without-renderer.html
Comment 5 Julien Chaffraix 2013-02-25 15:53:52 PST
(In reply to comment #3)
> (From update of attachment 190105 [details])
> Would it make more sense for us to
> 1) Rename grid-column/grid-row to grid-before/grid-start.
> 2) Add grid-end/grid-after.
> 3) Add the shorthand rules for grid-column/grid-row.
> 
> There's code for handling shorthands automatically so we shouldn't have extra code for parsing grid-column/grid-row.

Ideally that would be the best path BUT the "logical" grid properties are very new (they were added this month) and contains a lot of issues. As a whole, the more syntax we can postpone implementing the better as the spec is still unstable. That's why I took the path of least resistance in this patch and just extended the existing properties. Note that some split are explicitly for code reuse once we decide to implement them.

I don't agree with your assessment that the shorthand code will give us parsing / style resolution for free (look at CSSPropertyFont or CSSPropertyMargin).
Comment 6 Build Bot 2013-02-25 16:19:52 PST
Comment on attachment 190105 [details]
Proposed change 1.

Attachment 190105 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16751504

New failing tests:
svg/css/getComputedStyle-basic.xhtml
fast/css/getComputedStyle/computed-style.html
fast/css/getComputedStyle/computed-style-without-renderer.html
Comment 7 Julien Chaffraix 2013-02-25 17:16:58 PST
Created attachment 190162 [details]
Proposed change 2. Fixed the EWS.
Comment 8 Tony Chang 2013-02-25 18:24:39 PST
(In reply to comment #5)
> Ideally that would be the best path BUT the "logical" grid properties are very new (they were added this month) and contains a lot of issues. As a whole, the more syntax we can postpone implementing the better as the spec is still unstable. That's why I took the path of least resistance in this patch and just extended the existing properties. Note that some split are explicitly for code reuse once we decide to implement them.

Ultimately, we want RenderStyle to reflect the CSS structure.  Temporarily putting them into a single struct in RenderStyle means we're just going to undo it later.  Why not just move in that direction now?  If you want to avoid implementing all the CSS syntax now, I would remove grid-row/grid-column since it's only a shorthand.

> I don't agree with your assessment that the shorthand code will give us parsing / style resolution for free (look at CSSPropertyFont or CSSPropertyMargin).

See css/StylePropertyShorthand.*  We should be able to use parseShorthand() for this because there's no special handling for the longhand properties.
Comment 9 Ojan Vafai 2013-02-25 18:45:58 PST
Not really related to this, but it's strange to me that this shorthand has a slash separate instead of just being whitespaced separated like most other properties. Maybe something to bring up with the working group.
Comment 10 Julien Chaffraix 2013-02-26 10:36:02 PST
Comment on attachment 190162 [details]
Proposed change 2. Fixed the EWS.

I discussed this on IRC with Tony and the bottom line is that if we don't implement the 4 properties (grid-{start|end}, grid-{before|after}) then we *will* have some scaffolding code that would need to get away once we support it. We agreed that this is not a good situation and I will add the new properties, building a new patch for this bug on top of that.
Comment 11 Tab Atkins 2013-02-26 10:40:13 PST
(In reply to comment #9)
> Not really related to this, but it's strange to me that this shorthand has a slash separate instead of just being whitespaced separated like most other properties. Maybe something to bring up with the working group.

It's required, otherwise the properties are ambiguous.  You can slice up "grid-row: 'start' 2;" as "grid-before: 'start'; grid-after: 2;", "grid-before: 'start' 2; grid-after: auto;", or "grid-before: auto; grid-after: 'start' 2;".

I can't quite recall why we decided to use a slash rather than a comma there, though.  I'll ping fantasai to see if she remembers us having a reason for it.
Comment 12 Ojan Vafai 2013-02-26 10:43:29 PST
(In reply to comment #11)
> (In reply to comment #9)
> > Not really related to this, but it's strange to me that this shorthand has a slash separate instead of just being whitespaced separated like most other properties. Maybe something to bring up with the working group.
> 
> It's required, otherwise the properties are ambiguous.  You can slice up "grid-row: 'start' 2;" as "grid-before: 'start'; grid-after: 2;", "grid-before: 'start' 2; grid-after: auto;", or "grid-before: auto; grid-after: 'start' 2;".
> 
> I can't quite recall why we decided to use a slash rather than a comma there, though.  I'll ping fantasai to see if she remembers us having a reason for it.

In that case, slash seems fine.
Comment 13 Julien Chaffraix 2013-03-05 19:17:04 PST
Created attachment 191638 [details]
New approach (v3): Made grid-{row|column} a shorthand and added parsing for 2 values.
Comment 14 WebKit Review Bot 2013-03-05 22:58:29 PST
Comment on attachment 191638 [details]
New approach (v3): Made grid-{row|column} a shorthand and added parsing for 2 values.

Attachment 191638 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17008044

New failing tests:
svg/css/getComputedStyle-basic.xhtml
fast/css/getComputedStyle/computed-style.html
fast/css-grid-layout/grid-item-column-row-get-set.html
fast/css/getComputedStyle/computed-style-without-renderer.html
Comment 15 Build Bot 2013-03-06 01:14:04 PST
Comment on attachment 191638 [details]
New approach (v3): Made grid-{row|column} a shorthand and added parsing for 2 values.

Attachment 191638 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17078026

New failing tests:
fast/css/getComputedStyle/computed-style-without-renderer.html
fast/css/getComputedStyle/computed-style.html
fast/css-grid-layout/grid-item-column-row-get-set.html
svg/css/getComputedStyle-basic.xhtml
Comment 16 Julien Chaffraix 2013-03-06 08:14:43 PST
Created attachment 191755 [details]
V4: Same as V3 but updated the expected results.
Comment 17 Tony Chang 2013-03-06 11:41:29 PST
(In reply to comment #14)
> (From update of attachment 191638 [details])
> Attachment 191638 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://webkit-commit-queue.appspot.com/results/17008044
> 
> New failing tests:
> svg/css/getComputedStyle-basic.xhtml
> fast/css/getComputedStyle/computed-style.html
> fast/css-grid-layout/grid-item-column-row-get-set.html
> fast/css/getComputedStyle/computed-style-without-renderer.html

I think it would be ok to move the grid related styles out of the getComputedStyle tests and into a separate test in fast/css-grid-layout.  Doesn't need to happen in this patch.
Comment 18 Julien Chaffraix 2013-03-06 14:15:01 PST
(In reply to comment #17)
> (In reply to comment #14)
> > (From update of attachment 191638 [details] [details])
> > Attachment 191638 [details] [details] did not pass chromium-ews (chromium-xvfb):
> > Output: http://webkit-commit-queue.appspot.com/results/17008044
> > 
> > New failing tests:
> > svg/css/getComputedStyle-basic.xhtml
> > fast/css/getComputedStyle/computed-style.html
> > fast/css-grid-layout/grid-item-column-row-get-set.html
> > fast/css/getComputedStyle/computed-style-without-renderer.html
> 
> I think it would be ok to move the grid related styles out of the getComputedStyle tests and into a separate test in fast/css-grid-layout.  Doesn't need to happen in this patch.

The failing tests iterate over *all* the properties returned by getComputedStyle. There is no grid magic involved so the only action we could do is blacklist grid properties. Probably not what you are suggesting here.
Comment 19 Tony Chang 2013-03-06 17:28:34 PST
Comment on attachment 191755 [details]
V4: Same as V3 but updated the expected results.

Can you add some tests that have / but no whitespace like "5/5"?
Comment 20 Julien Chaffraix 2013-03-06 19:01:28 PST
Created attachment 191890 [details]
Patch for landing
Comment 21 WebKit Review Bot 2013-03-06 19:54:00 PST
Comment on attachment 191890 [details]
Patch for landing

Clearing flags on attachment: 191890

Committed r145029: <http://trac.webkit.org/changeset/145029>
Comment 22 WebKit Review Bot 2013-03-06 19:54:06 PST
All reviewed patches have been landed.  Closing bug.