1240956 - [css-grid] Swap the order of column/row values for 'grid', 'grid-template' and 'grid-gap' properties.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Mats Palmgren (inactive)]

https://lists.w3.org/Archives/Public/www-style/2015Nov/0267.html
"
  - RESOLVED: Make logical coordinates always block, then inline,
              even though physical coordinates in background-
              position are horizontal, vertical.
"

Hmm, I didn't realize it at the time that it affected Grid properties...

But the spec was changed some time ago:
https://drafts.csswg.org/css-grid/#explicit-grid-shorthand
https://drafts.csswg.org/css-grid/#grid-shorthand
https://drafts.csswg.org/css-grid/#propdef-grid-gap

So, row values now comes first, then column values.

Comment 1

[Mats Palmgren (inactive)]

Attachment: fix

Comment 2

[Mats Palmgren (inactive)]

Attachment: test updates

https://treeherder.mozilla.org/#/jobs?repo=try&revision=255e4a1de0f9

Comment 3

[Mats Palmgren (inactive)]

FYI, there were some failures in the run above:
 TEST-UNEXPECTED-FAIL | layout/style/test/test_value_cloning.html | serialization should be nonempty for grid: [foo] 40px / [bar] 'fizz' 100px [buzz]
...

I'll look into that...

Comment 4

[Mats Palmgren (inactive)]

Attachment: test updates

No worries, it was just a few of grid-template's "other_values" in
property_database.js that were now invalid.  Swapping the values
around the '/' fixed it locally.  Here's a new test run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ada37aa1febc

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8710238 [details] [diff] [review]
fix

Review of attachment 8710238 [details] [diff] [review]:
-----------------------------------------------------------------

Have to run to a meeting, but I think this is mostly-good. Haven't gotten to the end of nsCSSParser.cpp yet, but I did notice one bug (I think).  I'll finish this off later today.

::: layout/style/nsCSSParser.cpp
@@ +9122,3 @@
>        AppendValue(eCSSProperty_grid_template_rows, value);
>        value.SetNoneValue();
>        AppendValue(eCSSProperty_grid_template_areas, value);

I think these AppendValue(...) calls are premature here, because we may still be going to fail. (There's an explicit "return false" and also a "return [otherFunction]" after this.

Comment 6

[Mats Palmgren (inactive)]

IIRC, dbaron pointed out on some other bug that all values are discarded if we return false
so that shouldn't be a problem.

Comment 7

[Mats Palmgren (inactive)]

...  in bug 1176792 comment 50.

Comment 8

[Daniel Holbert [:dholbert]]

Thanks, I wasn't aware of that. I won't worry about that then.

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8710238 [details] [diff] [review]
fix

Review of attachment 8710238 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsCSSParser.cpp
@@ +9124,5 @@
>        AppendValue(eCSSProperty_grid_template_areas, value);
> +      if (ExpectSymbol('/', true)) {
> +        return ParseGridTemplateColumnsRows(eCSSProperty_grid_template_columns);
> +      }
> +      if (value.GetListValue()->mNext) {

I think this "value.GetListValue()" call is bogus, and will assert fatally if we actually hit it... (which means we must never be hitting it during testing)

In your patch, when we get here, we've just called value.SetNoneValue() (to set grid-template-areas:none).  So I'd expect this value.GetListValue() call to fail (and assert fatally), since we don't have a list-flavored unit... Maybe I'm missing something, though?

You could work around this by dropping the SetNoneValue call, and instead doing:
  AppendValue(eCSSProperty_grid_template_areas, nsCSSValue(eCSSUnit_None));


I'm guessing this isn't getting exercised during testing (& you didn't trip over this fatal assertion) because bug 1221677 made us skip all this subgrid stuff during our test runs.  If that's the case, please make sure to do a full layout/style mochitest run with that pref enabled (e.g. on Try, by adding the subgrid pref alongside the main grid pref in testing/profiles/prefs_general.js), and you might even want to add a printf or test locally with gdb to be sure you're actually hitting this codepath.

I suspect that might turn up some more test-failures, though maybe not.

@@ +9131,5 @@
> +        // which must be followed by a slash.
> +        return false;
> +      }
> +      // 'subgrid' by itself sets both grid-template-rows/columns.
> +      AppendValue(eCSSProperty_grid_template_columns, value);

Please make sure |value| actually holds whatever value we want it to hold, at this point in the code.

(Right now, I'm pretty sure this suffers from the same problem noted above -- with the current patch, we've clobbered |value| by calling SetNoneValue on it when we get to this line. Previously, the SetNoneValue call came after this line.)

@@ +9139,5 @@
>    }
>  
>    // [ <line-names>? ] here is ambiguous:
>    // it can be either the start of a <track-list>,
>    // or the start of [ <line-names>? <string> <track-size>? <line-names>? ]+

This comment needs updating.  The "start of a <track-list>" possibility that it describes here is mainly referring to the *old* possibility of an optional "<track-list> /" at the beginning. But we're no longer expecting that.  So it's no longer clear why we might be looking for a <track-list> here.

I *think* the <track-list> that we're still maybe-expecting here would be part of a  <'grid-template-rows'> expression. So, please clarify the comment to mention that, e.g. with a parenthetical like the following:
  // [ <line-names>? ] here is ambiguous:
  // it can be either the start of a <track-list> (in a <'grid-template-rows'>
  // expression), [...]

This is important to clarify, because the grammar only explicitly mentions one <track-list> for this shorthand's grammar, and that's *not* (anymore) the <track-list> that we're potentially parsing here. 

ALSO: Please update the documentation above the ParseGridTrackListWithFirstLineNames function-decl, linked here:
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSParser.cpp?rev=a1674bc106ab#952
This documentation also hints at a <track-list> in the grid-template syntax, which is confusing since the <track-list> we actually care about isn't explicitly there in the specced grammar. (As noted above, it's indirect, via expanding the possibilities for <'grid-template-rows'>.)  So, please clarify that documentation to mention <'grid-template-rows'>.

@@ +9154,5 @@
> +    // Parse an optional [ / <track-list> ] as the columns value.
> +    if (ExpectSymbol('/', true)) {
> +      nsCSSValue lineNames;
> +      if (ParseGridLineNames(lineNames) == CSSParseResult::Error ||
> +          !ParseGridTrackListWithFirstLineNames(value, lineNames)) {

This pattern (Parse just some line names, and then immediately parse the rest of a track-list) feels clumsy.  It's necessary elsewhere, in cases where we might do something else with |lineNames|.  But here, we're expecting (and requiring) |lineNames| to be part of a track-list, so the two-step parsing dance seems silly.

This same pattern happens at the end of ParseGridTemplateColumnsRows, too.

I think we should add a short "ParseGridTrackList()" convenience function, which would just be a one-liner that makes this ^^ exact call... Then we can invoke that from here & from ParseGridTemplateColumnsRows, to make these calls a little more readable than this two-step dance.

(That seems like it might make for a good "part 2" minor-refactoring patch here, since this is a refactoring opportunity that only makes sense after this part introduced a second occurrence of this dance.)

@@ +9166,5 @@
>    }
>    UngetToken();
>  
> +  // Finish parsing <'grid-template-rows'> with the |firstLineNames| we have
> +  // then parse a mandatory [ / <'grid-template-columns'> ].

Nit:
s/have then/have, and then/)

Comment 10

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #9)
> I think this "value.GetListValue()" call is bogus, and will assert fatally
> if we actually hit it... (which means we must never be hitting it during
> testing)

Good catch.  We actually do have tests for this but subgrid support was
disabled also for tests unfortunately (and we do hit that assertion when
I enable it).  I filed bug 1242053 to fix some existing issues and to
enable subgrid support for tests.

Comment 11

[Mats Palmgren (inactive)]

Attachment: fix

(In reply to Daniel Holbert [:dholbert] from comment #9)
> You could work around this by dropping the SetNoneValue call, and instead
> doing:
>   AppendValue(eCSSProperty_grid_template_areas, nsCSSValue(eCSSUnit_None));

Fixed.

I also removed this comment:
  // TODO (bug 983175): add parsing for 'subgrid' by itself
since clearly we do handle that case already ("subgrid" by itself
is even included in property_database.js which I think means it
round-trips correctly).

> Please make sure |value| actually holds whatever value we want it to hold,
> at this point in the code.

It's implied by the "value.GetListValue()->mNext" that we expect
a list value and GetListValue() asserts that.

> >    // [ <line-names>? ] here is ambiguous:
> >    // it can be either the start of a <track-list>,
> >    // or the start of [ <line-names>? <string> <track-size>? <line-names>? ]+
> 
> This comment needs updating.

Fixed.

> ALSO: Please update the documentation above the
> ParseGridTrackListWithFirstLineNames function-decl, linked here:
> http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSParser.
> cpp?rev=a1674bc106ab#952

Fixed.

> Nit:
> s/have then/have, and then/)

Fixed.

Comment 12

[Mats Palmgren (inactive)]

Try run on top of bug 1242053 (i.e. with subgrid tests enabled):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7d1bb214196

Comment 13

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-grid] Add a ParseGridTrackList convenience method.

Try run on top of the other patches including bug 1242053:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a82f637eaa74

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8711342 [details] [diff] [review]
part 3 - [css-grid] Add a ParseGridTrackList convenience method.

Review of attachment 8711342 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d4a7ea7b3e0
https://hg.mozilla.org/integration/mozilla-inbound/rev/fffc339c695c
https://hg.mozilla.org/integration/mozilla-inbound/rev/be1d458a17b0

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8d4a7ea7b3e0
https://hg.mozilla.org/mozilla-central/rev/fffc339c695c
https://hg.mozilla.org/mozilla-central/rev/be1d458a17b0

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8d4a7ea7b3e0
https://hg.mozilla.org/mozilla-central/rev/fffc339c695c
https://hg.mozilla.org/mozilla-central/rev/be1d458a17b0

Comment 18

[yisibl]

This performance is better?