1266124 - [css-grid] update grid shorthand syntax

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The CSS WG just resolved to accept the proposal in:
https://lists.w3.org/Archives/Public/www-style/2016Apr/0249.html
to change the syntax of the 'grid' shorthand.

Comment 1

[Mats Palmgren (inactive)]

Given the 'grid-template' debacle, I think we should wait a few months before implementing
this to allow the CSSWG change its mind a few times...

Comment 2

[Mats Palmgren (inactive)]

FTR:  https://lists.w3.org/Archives/Public/www-style/2016Apr/0351.html
  - RESOLVED: Accept 'grid' shorthand improvements here:
              https://lists.w3.org/Archives/Public/www-style/2016Apr/0249.html

Comment 3

[Mats Palmgren (inactive)]

I'm working on this...

Comment 4

[Mats Palmgren (inactive)]

Attachment: fix+tests

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8764248 [details] [diff] [review]
fix+tests

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

Sorry, not quite done with this review yet (I just need to look at the Declaration.cpp changes in a bit more detail), but here's what I've got so far:

::: layout/style/Declaration.cpp
@@ +1184,5 @@
>        aValue.AppendLiteral(" / ");
>        AppendValueToString(subprops[3], aValue, aSerialization);
>        break;
>      }
> +      

Nit: this is adding whitespace on a blank line.

@@ +1216,5 @@
>        const nsCSSValue& autoRowsValue =
>          *data->ValueFor(eCSSProperty_grid_auto_rows);
>  
> +      // grid-template-rows/areas:none + non-default grid-auto-flow or
> +      // non-default grid-auto-rows value + default grid-auto-columns.

This code could would be easier to read if this comment clearly tied it back to the version of the "grid" syntax that we're trying to serialize to here.

Maybe you could adjust your comment further up with the "grid:" syntax, to add numeric labels for each option, and then refer to those numeric labels in your sections here? So that would look like:
    // "grid" has 3 different possibilities for syntax:
    // #1 <'grid-template'>
    // #2 <'grid-template-rows'> / [ auto-flow && dense? ] <'grid-auto-columns'>?
    // #3 [ auto-flow && dense? ] <'grid-auto-rows'>? / <'grid-template-columns'>
    case eCSSProperty_grid: {
    [...SNIP...]
    // grid-template-rows/areas:none + non-default grid-auto-flow or
    // non-default grid-auto-rows value + default grid-auto-columns.
    // --> Try to serialize as 'grid' syntax #3.

@@ +1222,5 @@
> +          areasValue.GetUnit() == eCSSUnit_None &&
> +          autoColumnsValue.GetUnit() == eCSSUnit_Auto &&
> +          autoFlowValue.GetUnit() == eCSSUnit_Enumerated &&
> +          (autoFlowValue.GetIntValue() & NS_STYLE_GRID_AUTO_FLOW_ROW)) {
> +        if (autoFlowValue.GetIntValue() != NS_STYLE_GRID_AUTO_FLOW_ROW ||

I don't entirely follow what this code is doing yet, but I wonder if this not-equal comparison really what you want? Note that autoFlowValue is really a bitmask, so checking it for equality against one of the mask values seems suspicious.  It could be storing a value that represents "row,dense". (i.e. NS_STYLE_GRID_AUTO_FLOW_ROW | NS_STYLE_GRID_AUTO_FLOW_DENSE). Are you wanting that sort of combination value to pass this "!= row" check?  (It currently will.)

::: layout/style/nsCSSParser.cpp
@@ +9537,5 @@
> +{
> +  MOZ_ASSERT(aAutoFlowAxis == NS_STYLE_GRID_AUTO_FLOW_ROW ||
> +             aAutoFlowAxis == NS_STYLE_GRID_AUTO_FLOW_COLUMN);
> +  if (!GetToken(true)) {
> +    return CSSParseResult::Error;

Nit: please return NotFound for this early-return (for the immediate "no more tokens at all" case).

As justification, see the documentation of NotFound vs. Error:
http://searchfox.org/mozilla-central/source/layout/style/nsCSSParser.cpp#95
====
  enum class CSSParseResult : int32_t {
    [...]
    // Did not find what we were looking for, but did not consume any token:
    NotFound,
    // Unexpected token or token value, too late for UngetToken() to be enough:
    Error
====
In this clause, we have *not* hit an "Unexpected token or token value".  So Error seems incorrect.  Rather, we exactly match the description of NotFound (did not find what we were looking for; did not consume any token.)

(Ultimately it doesn't *really* matter, because this function's caller should end up failing when it tries to parse something else, and it'll just bail slightly later.  But bottom line, we should honor the meanings of CSSParseResult in this function here, for optimal consistency & separation-of-concerns.  Also: see ParseVariant, ParseColor, & ParseGridTrackBreadth, which all return NotFound for this same first-GetToken-call-fails scenario.)

Comment 6

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> > +          areasValue.GetUnit() == eCSSUnit_None &&
> > +          autoColumnsValue.GetUnit() == eCSSUnit_Auto &&
> > +          autoFlowValue.GetUnit() == eCSSUnit_Enumerated &&
> > +          (autoFlowValue.GetIntValue() & NS_STYLE_GRID_AUTO_FLOW_ROW)) {
> > +        if (autoFlowValue.GetIntValue() != NS_STYLE_GRID_AUTO_FLOW_ROW ||
> 
> I don't entirely follow what this code is doing yet, but I wonder if this
> not-equal comparison really what you want? Note that autoFlowValue is really
> a bitmask, so checking it for equality against one of the mask values seems
> suspicious.  It could be storing a value that represents "row,dense". (i.e.
> NS_STYLE_GRID_AUTO_FLOW_ROW | NS_STYLE_GRID_AUTO_FLOW_DENSE). Are you
> wanting that sort of combination value to pass this "!= row" check?  (It
> currently will.)

Yes, the intention is to check for "non-default grid-auto-flow or
non-default grid-auto-rows".  So, rather than generating
"auto-flow / <columns>" here, we fall through to generate
"none / <columns>" in the grid_template branch.  The latter is
a more canonical "all row values are default" IMO.

The inner 'if' is somewhat arbitrary though.  I can move those
checks to the end of the outer 'if' instead if you prefer.

Comment 7

[Mats Palmgren (inactive)]

>  we fall through to generate "none / <columns>"

... and potentially just "none" if all column values are default too.
We wouldn't want "auto-flow / none" in that case (although it's
equivalent).

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #6)
> So, rather than generating
> "auto-flow / <columns>" here, we fall through to generate
> "none / <columns>" in the grid_template branch.

I see -- thanks! That was not at all clear to me from the code -- could you add a comment to make that more clear?  (In particular: the comment should make it clear that, with default grid-auto-flow/grid-auto-row values, we perhaps *could* serialize as "auto-flow / <grid-template-columns>", but we'll opt for the equivalent & more-concise "none / <grid-template-columns> serialization via a different codepath.)

> The inner 'if' is somewhat arbitrary though.  I can move those
> checks to the end of the outer 'if' instead if you prefer.

Yup, the "if" grouping could be clearer, I think... (either one large "if" check, or separated but with all of the "auto-flow" checks grouped into the same "if").

It would also help to have a comment directly alongside the auto-flow/auto-rows comparisons (perhaps even inside the "if (...)" parens, if you opt for the one-giant-if-check route). Something like:

  /* Check if grid-auto-flow is row-flavored, AND either it or
     grid-auto-rows have non-default values (which makes them worthy
     of expressing inside the shorthand) */

Comment 9

[Daniel Holbert [:dholbert]]

I think your existing (comment before these "if" checks) is kind-of intending to express the same thing as my suggested-comment in comment 8 -- but in the current patch, the comment is just documenting what's being checked, without expressing why it matters, and that makes it harder to understand, for me at least.

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8764248 [details] [diff] [review]
fix+tests

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

r=me with comment 5 and comment 8 addressed.

(disregarding the bitmask thing in comment 5, which you've already clarified here)

Thanks!

Comment 11

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> ::: layout/style/nsCSSParser.cpp
> @@ +9537,5 @@
> > +{
> > +  MOZ_ASSERT(aAutoFlowAxis == NS_STYLE_GRID_AUTO_FLOW_ROW ||
> > +             aAutoFlowAxis == NS_STYLE_GRID_AUTO_FLOW_COLUMN);
> > +  if (!GetToken(true)) {
> > +    return CSSParseResult::Error;
> 
> Nit: please return NotFound for this early-return (for the immediate "no
> more tokens at all" case).

Fixed.

(In reply to Daniel Holbert [:dholbert] from comment #8)

I extended the comments there; hopefully they are clearer now.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/edfba309d405
[css-grid] Update the 'grid' shorthand syntax to the latest spec.  r=dholbert

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/edfba309d405