In layout code, I'm observing this: data:text/html,<div style="display:grid; grid-template-areas:'' "> gridTemplateAreas->mNColumns = 1 data:text/html,<div style="display:grid; grid-template-areas:'x' "> gridTemplateAreas->mNColumns = 2 data:text/html,<div style="display:grid; grid-template-areas:'x x' "> gridTemplateAreas->mNColumns = 3 I was expecting 0, 1, 2 etc

This fixes it for me.

grid-template-areas:'' (resulting in mNColumns==0) seems a bit odd. The spec doesn't seem to mention it: http://dev.w3.org/csswg/css-grid/#grid-template-areas-property Should it be rejected?

The relevant code is in ParseGridTemplateAreasLine: http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSParser.cpp?rev=bfd16de7bf25#7555 Indeed, the name and the value are inconsistent. The value could be changed by adding `- 1` as appropriate in the code linked above, or the name could be changed to something like `mNbColumnLines`. I suggest waiting to have some grid item placement code to decide which is more convenient to have. Note that line numbers start from 1 (not 0) in grid-column-start and related properties, as does (at the moment) GridNamedArea.mColumnStart. But maybe having this all be zero-based will turn out to be more convenient for implementation (in which case the `- 1` could be in ParseGridLine).

Comment on attachment 8413867 [details] [diff] [review] fix Review of attachment 8413867 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/nsCSSParser.cpp @@ +7540,5 @@ > currentArea->mRowEnd = row + 1; > } > } > } > + if (currentArea && currentArea->mColumnEnd != column + 1) { I’m not sure you want this, but running the tests should confirm. IIRC, the right incantation is: mach mochitest-plain layout/style/test

> I’m not sure you want this Nevermind that, a bit more context shows that the patch is correct. Still, see Comment 3 about whether this is the change you want to make.

(In reply to Simon Sapin (:SimonSapin) from comment #3) > I suggest waiting to have some grid item placement code to decide which is > more convenient to have. Yes, I now have code that does explicit grid placement, and I'm reflowing child frames into those areas (see the test/screenshot in bug 616605). > Note that line numbers start from 1 (not 0) ... I found it convenient to use 1-based numbers in layout too, using zero to represent "auto". So, I think it's the right patch, and it matches NRows(). (and it passed mochitest-plain locally)

Comment on attachment 8413867 [details] [diff] [review] fix Review of attachment 8413867 [details] [diff] [review]: ----------------------------------------------------------------- r=me with a "Column numbers start at 1" comment next to "uint32_t column = 0;" which seems to imply they start at 0. I don’t have commit access to push this, though.

With a code comment added: https://hg.mozilla.org/integration/mozilla-inbound/rev/50a85e215d44