Bug 110777

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

Created attachment 190105 [details] Proposed change 1.

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

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

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

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

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

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

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

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

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

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

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

Created attachment 191890 [details] Patch for landing

Comment on attachment 191890 [details] Patch for landing Clearing flags on attachment: 191890 Committed r145029: <http://trac.webkit.org/changeset/145029>

All reviewed patches have been landed. Closing bug.