Bug 110777 – [CSS Grid Layout] Extend our grammar to support 2 positions for grid-{row|column}

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

[Julien Chaffraix]

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>

[Julien Chaffraix]

> 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}.

[Julien Chaffraix]

Created attachment 190105 [details]
Proposed change 1.

[Tony Chang]

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.

[Build Bot]

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

[Julien Chaffraix]

(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).

[Build Bot]

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

[Julien Chaffraix]

Created attachment 190162 [details]
Proposed change 2. Fixed the EWS.

[Tony Chang]

(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.

[Ojan Vafai]

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.

[Julien Chaffraix]

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.

[Tab Atkins]

(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.

[Ojan Vafai]

(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.

[Julien Chaffraix]

Created attachment 191638 [details]
New approach (v3): Made grid-{row|column} a shorthand and added parsing for 2 values.

[WebKit Review Bot]

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

[Build Bot]

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

[Julien Chaffraix]

Created attachment 191755 [details]
V4: Same as V3 but updated the expected results.

[Tony Chang]

(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.

[Julien Chaffraix]

(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.

[Tony Chang]

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"?

[Julien Chaffraix]

Created attachment 191890 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 191890 [details]
Patch for landing

Clearing flags on attachment: 191890

Committed r145029: <http://trac.webkit.org/changeset/145029>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.