[CSS Grid Layout] Update named grid lines syntax to the last version of the specs

[CSS Grid Layout] Update named grid lines syntax to the last version of the specs

Names for grid lines are now specified as a list of whitespace separated
idents enclosed in parentheses instead of as a list of strings.

BUG=282452
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162555

[Julien - ping for review, 2013-09-04 21:41:00]

I would like some testing added as part of this change:
* nested parentheses: (foo (bar baz)) 100px
* unbalanced parentheses: (foo bar 100px, foo bar) 100px
* misplaced parentheses: (foo 100px) 200px

https://codereview.chromium.org/23528004/diff/1/Source/core/css/CSSGrammar.y.in
File Source/core/css/CSSGrammar.y.in (right):

https://codereview.chromium.org/23528004/diff/1/Source/core/css/CSSGrammar.y....
Source/core/css/CSSGrammar.y.in:1664: $$->addValue(makeOperatorValue(')'));
I am not totally sure what's the purpose of adding the opening / closing
parentheses here: the only authorized <indent> in <track-list> are for named
grid lines so can't we just drop them be and consume <ident>+ in
CSSParser::parseGridLineNames?

https://codereview.chromium.org/23528004/diff/1/Source/core/css/CSSGrammar.y....
Source/core/css/CSSGrammar.y.in:1685: | expr operator track_names_list {
That's an overly broad change: we will now accept track_names for anything that
takes an expr (any properties, ...). Could you explain why this change is
needed?

[svillar, 2013-09-05 07:05:21]

I'll add some extra checks, I also believe that this change indeed deserves
them.

https://codereview.chromium.org/23528004/diff/1/Source/core/css/CSSGrammar.y.in
File Source/core/css/CSSGrammar.y.in (right):

https://codereview.chromium.org/23528004/diff/1/Source/core/css/CSSGrammar.y....
Source/core/css/CSSGrammar.y.in:1664: $$->addValue(makeOperatorValue(')'));
On 2013/09/04 21:41:00, Julien Chaffraix wrote:
> I am not totally sure what's the purpose of adding the opening / closing
> parentheses here: the only authorized <indent> in <track-list> are for named
> grid lines so can't we just drop them be and consume <ident>+ in
> CSSParser::parseGridLineNames?

My first idea was not to add them but we have cases like the following:

10px (linename) min-content

without any delimiter the parser will conclude that both "linename" and
"min-content" are the names for the last grid line.

We did not have that issue before because we had the line names as strings and
then we have the idents, but now everything will be an ident.

https://codereview.chromium.org/23528004/diff/1/Source/core/css/CSSGrammar.y....
Source/core/css/CSSGrammar.y.in:1685: | expr operator track_names_list {
On 2013/09/04 21:41:00, Julien Chaffraix wrote:
> That's an overly broad change: we will now accept track_names for anything
that
> takes an expr (any properties, ...). Could you explain why this change is
> needed?

Well, we need the grammar to allow <ident>+ enclosed in parentheses, so
something needs to be added in any case, otherwise it will be interpreted as an
invalid parentheses block inside a declaration.

It was added to expr because it's used in the property declaration definition.
It's indeeed true that we're accepting track_names for any expresion at grammar
level but the parser should properly reject them (not 100% sure about that maybe
esprenh could give some advice here, because maybe some code assumes that
parentheses blocks inside declarations are always invalid if they don't belong
to a function call).

I am not an expert in the grammar but I guess that either we do it this way or
we define a specific syntax just for the grid-definition-* properties, something
that also looks a bit weird, doesn't it?

BTW, this last "expr operator track_names_list" is specifically required to
support grid line names inside a repeat().

[Julien - ping for review, 2013-09-09 23:33:54]

https://codereview.chromium.org/23528004/diff/1/Source/core/css/CSSGrammar.y.in
File Source/core/css/CSSGrammar.y.in (right):

https://codereview.chromium.org/23528004/diff/1/Source/core/css/CSSGrammar.y....
Source/core/css/CSSGrammar.y.in:1664: $$->addValue(makeOperatorValue(')'));
> We did not have that issue before because we had the line names as strings and
> then we have the idents, but now everything will be an ident.

It's probably a bad idea but if we stored the line names as <string> it would
solve our issue. Alternatively you can set the CSSValueID of the line name to
CSSValueInvalid and that should enable you to disambiguate between a
<track-size> definition and a line name.

https://codereview.chromium.org/23528004/diff/1/Source/core/css/CSSGrammar.y....
Source/core/css/CSSGrammar.y.in:1685: | expr operator track_names_list {
On 2013/09/05 07:05:22, svillar wrote:
> On 2013/09/04 21:41:00, Julien Chaffraix wrote:
> > That's an overly broad change: we will now accept track_names for anything
> that
> > takes an expr (any properties, ...). Could you explain why this change is
> > needed?
> 
> Well, we need the grammar to allow <ident>+ enclosed in parentheses, so
> something needs to be added in any case, otherwise it will be interpreted as
an
> invalid parentheses block inside a declaration.
> 
> It was added to expr because it's used in the property declaration definition.
> It's indeeed true that we're accepting track_names for any expresion at
grammar
> level but the parser should properly reject them (not 100% sure about that
maybe
> esprenh could give some advice here, because maybe some code assumes that
> parentheses blocks inside declarations are always invalid if they don't belong
> to a function call).
> 
> I am not an expert in the grammar but I guess that either we do it this way or
> we define a specific syntax just for the grid-definition-* properties,
something
> that also looks a bit weird, doesn't it?

From my perspective, this is less weird than allowing this syntax for all cases.
We know that a very small subset of the properties allows this syntax so it's
probably cleaner to just special case them. I haven't tried coding it so I may
be wrong though.

[rune, 2013-09-10 11:30:19]

https://codereview.chromium.org/23528004/diff/7001/Source/core/css/CSSGrammar...
File Source/core/css/CSSGrammar.y.in (right):

https://codereview.chromium.org/23528004/diff/7001/Source/core/css/CSSGrammar...
Source/core/css/CSSGrammar.y.in:1678: $$->extend(*($2));
$2, which comes from is createFloatingValueList() in the ident_list production,
is not kept after this statement. There's no "sink", which means ident_lists
will be added to m_floatValueLists while parsing, but never deleted until the
CSSParser object is deleted.

[rune, 2013-09-10 16:06:11]

https://codereview.chromium.org/23528004/diff/1/Source/core/css/CSSGrammar.y.in
File Source/core/css/CSSGrammar.y.in (right):

https://codereview.chromium.org/23528004/diff/1/Source/core/css/CSSGrammar.y....
Source/core/css/CSSGrammar.y.in:1685: | expr operator track_names_list {
On 2013/09/04 21:41:00, Julien Chaffraix wrote:
> That's an overly broad change: we will now accept track_names for anything
that
> takes an expr (any properties, ...). Could you explain why this change is
> needed?

The alternative would be to use a different grammar for certain properties, so
instead of having a common:

  declaration: property ':' ... expr ...

you would need to have:

  declaration: grid_property ':' ... grid_property_expr ...

which would make the grammar explode if you did the same argument for other
properties. For width: "Why accept strings, functions etc for width?"

Normally the page author wouldn't add parentheses expressions to properties that
don't accept them, so I don't think there should be additional cost of
processing or storage even if it's recognized for all properties.

Note that all functions with a general expr argument are already accepted for
all property declarations.

The track_names_list here is quite close to how the function non-terminal is
recognized. I think I would have considered doing it like it's done for
functions. Note, I say _considered_. I have not looked at how it works in
detail, but there's a setFromFunction() method for handling expr (or multiple
terms) inside the function expression like you need to handling for the list of
idents here.

So, maybe you could add track_names_list (or something more generic when more
properties start to accept parentheses blocks) as a term (like for function)
instead of the extra rules in the expr non-terminal?

There's another important part missing here. Balancing parentheses correctly for
error handling requires some additional rules. I wouldn't bet on error handling
working correctly without any error_recovery in track_names_list/ident_list. See
how it's done in various function non-terminals.

[Julien - ping for review, 2013-11-12 07:32:44]

Sorry for the delay, didn't reply to the answer straight away and this review
dropped off my plate.

On 2013/09/10 16:06:11, rune - CET wrote:
>
https://codereview.chromium.org/23528004/diff/1/Source/core/css/CSSGrammar.y.in
> File Source/core/css/CSSGrammar.y.in (right):
> 
>
https://codereview.chromium.org/23528004/diff/1/Source/core/css/CSSGrammar.y....
> Source/core/css/CSSGrammar.y.in:1685: | expr operator track_names_list {
> On 2013/09/04 21:41:00, Julien Chaffraix wrote:
> > That's an overly broad change: we will now accept track_names for anything
> that
> > takes an expr (any properties, ...). Could you explain why this change is
> > needed?
> 
> The alternative would be to use a different grammar for certain properties, so
> instead of having a common:
> 
>   declaration: property ':' ... expr ...
> 
> you would need to have:
> 
>   declaration: grid_property ':' ... grid_property_expr ...
> 
> which would make the grammar explode if you did the same argument for other
> properties. For width: "Why accept strings, functions etc for width?"
> 
> Normally the page author wouldn't add parentheses expressions to properties
that
> don't accept them, so I don't think there should be additional cost of
> processing or storage even if it's recognized for all properties.
> 
> Note that all functions with a general expr argument are already accepted for
> all property declarations.

That's a fair point. Even if I would be more comfortable with a more
laser-focused change but based on the current code it's probably wishful
thinking;

> The track_names_list here is quite close to how the function non-terminal is
> recognized. I think I would have considered doing it like it's done for
> functions. Note, I say _considered_. I have not looked at how it works in
> detail, but there's a setFromFunction() method for handling expr (or multiple
> terms) inside the function expression like you need to handling for the list
of
> idents here.
> 
> So, maybe you could add track_names_list (or something more generic when more
> properties start to accept parentheses blocks) as a term (like for function)
> instead of the extra rules in the expr non-terminal?

I think this make sense and would yield to more simple code.

[Julien - ping for review, 2013-11-12 07:33:16]

https://chromiumcodereview.appspot.com/23528004/diff/7001/Source/core/css/CSS...
File Source/core/css/CSSGrammar.y.in (right):

https://chromiumcodereview.appspot.com/23528004/diff/7001/Source/core/css/CSS...
Source/core/css/CSSGrammar.y.in:1663: $$->insertValueAt(0,
makeOperatorValue('('));
I wonder if we couldn't simplify the code by making inserting the value list
directly: it seems dumb to lose the already split list of <ident> (not to
mention the shenanigans we do to reconstruct the consumed operators so that we
split it _again_ in CSSParser-in.cpp).

[svillar, 2013-11-12 07:45:04]

https://chromiumcodereview.appspot.com/23528004/diff/7001/Source/core/css/CSS...
File Source/core/css/CSSGrammar.y.in (right):

https://chromiumcodereview.appspot.com/23528004/diff/7001/Source/core/css/CSS...
Source/core/css/CSSGrammar.y.in:1663: $$->insertValueAt(0,
makeOperatorValue('('));
On 2013/11/12 07:33:17, Julien Chaffraix - AEDT wrote:
> I wonder if we couldn't simplify the code by making inserting the value list
> directly: it seems dumb to lose the already split list of <ident> (not to
> mention the shenanigans we do to reconstruct the consumed operators so that we
> split it _again_ in CSSParser-in.cpp).

I can take a look again, and maybe the changes proposed by rune would make it
better, but the reason behind adding the operators is to allow the parser to
distinguish between track names and other types of indents. As I explained in my
first comment

    10px (trackname) max-content

the parser needs to know that the list of indents between parenthesis are track
names and the other indents are track breadths.

[Julien - ping for review, 2013-11-12 07:49:38]

https://chromiumcodereview.appspot.com/23528004/diff/7001/Source/core/css/CSS...
File Source/core/css/CSSGrammar.y.in (right):

https://chromiumcodereview.appspot.com/23528004/diff/7001/Source/core/css/CSS...
Source/core/css/CSSGrammar.y.in:1663: $$->insertValueAt(0,
makeOperatorValue('('));
On 2013/11/12 07:45:04, svillar wrote:
> On 2013/11/12 07:33:17, Julien Chaffraix - AEDT wrote:
> > I wonder if we couldn't simplify the code by making inserting the value list
> > directly: it seems dumb to lose the already split list of <ident> (not to
> > mention the shenanigans we do to reconstruct the consumed operators so that
we
> > split it _again_ in CSSParser-in.cpp).
> 
> I can take a look again, and maybe the changes proposed by rune would make it
> better, but the reason behind adding the operators is to allow the parser to
> distinguish between track names and other types of indents. As I explained in
my
> first comment
> 
>     10px (trackname) max-content
> 
> the parser needs to know that the list of indents between parenthesis are
track
> names and the other indents are track breadths.

Agreed but the proposal was to insert the list as-is: track breadths would
inserted as CSSPrimitiveValue(s) one at a time where track names would be stored
in a nested CSSValueList. This should make it possible to differentiate them in
CSSParser-in.cpp.

[svillar, 2013-11-12 18:21:57]

So this new CL adds track_names_list as a term (following rune's suggestion). We
also add the whole list of idents as a single value (following jchaffraix's
suggestion) so we don't have to split the list twice.

The grammar rules are indeed much simpler and we get also error recovery
support.

[Julien - ping for review, 2013-11-13 02:18:52]

I like the new change. lgtm.

@rune / @darktears, either of you should double check the grammar change
(particularly the error recovery which looks fine to me but I may have missed
something)

https://codereview.chromium.org/23528004/diff/55001/LayoutTests/fast/css-grid...
File LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html (right):

https://codereview.chromium.org/23528004/diff/55001/LayoutTests/fast/css-grid...
LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html:64: }
NIt: How about an empty parentheses case? (e.g. grid-definition-columns: ()
50%;)

[darktears, 2013-11-14 18:01:00]

On 2013/11/13 02:18:52, Julien Chaffraix - AEDT wrote:
> I like the new change. lgtm.
> 
> @rune / @darktears, either of you should double check the grammar change
> (particularly the error recovery which looks fine to me but I may have missed
> something)
> 
>
https://codereview.chromium.org/23528004/diff/55001/LayoutTests/fast/css-grid...
> File LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html (right):
> 
>
https://codereview.chromium.org/23528004/diff/55001/LayoutTests/fast/css-grid...
> LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html:64: }
> NIt: How about an empty parentheses case? (e.g. grid-definition-columns: ()
> 50%;)

I'm not an expert on the recovery part of the grammar but the change lgtm.

[rune, 2013-11-14 20:12:47]

On 2013/11/14 18:01:00, darktears wrote:

> I'm not an expert on the recovery part of the grammar but the change lgtm.

Error recovery lgtm.

[rune, 2013-11-14 20:54:43]

https://codereview.chromium.org/23528004/diff/115001/LayoutTests/fast/css-gri...
File LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html (right):

https://codereview.chromium.org/23528004/diff/115001/LayoutTests/fast/css-gri...
LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html:125:
testGridDefinitionsValues(document.getElementById("gridWithEmptyParentheses"),
"none", "none");

According to the current ED [1], a track list may be an empty list (parentheses
only) [1]. Since the list may be omitted, it's not necessary to allow empty
lists, but this test currently deviates from the spec. Either the spec or our
implementation needs to be fixed.

[1] http://dev.w3.org/csswg/css-grid/#typedef-line-names

[rune, 2013-11-14 21:16:49]

https://codereview.chromium.org/23528004/diff/55001/LayoutTests/fast/css-grid...
File LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html (right):

https://codereview.chromium.org/23528004/diff/55001/LayoutTests/fast/css-grid...
LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html:100:
testGridDefinitionsSetJSValues("(first) 18px", "66px (last)", "first 18px",
"66px last", "first 18px", "66px last");
So this means you didn't change the serialization output to emit valid
declaration values, so they won't make the round-trip in CSSOM. Did you plan to
do that in a follow-up CL?

[svillar, 2013-11-15 08:10:26]

On 2013/11/14 20:54:43, rune - CET wrote:
>
https://codereview.chromium.org/23528004/diff/115001/LayoutTests/fast/css-gri...
> File LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html (right):
> 
>
https://codereview.chromium.org/23528004/diff/115001/LayoutTests/fast/css-gri...
> LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html:125:
> testGridDefinitionsValues(document.getElementById("gridWithEmptyParentheses"),
> "none", "none");
> 
> According to the current ED [1], a track list may be an empty list
(parentheses
> only) [1]. Since the list may be omitted, it's not necessary to allow empty
> lists, but this test currently deviates from the spec. Either the spec or our
> implementation needs to be fixed.
> 
> [1] http://dev.w3.org/csswg/css-grid/#typedef-line-names

Ooops, yeah we explicitly forbid empty lists in the grammar which is the correct
option IMHO. In this case I think the spec makes no sense. I'll notify the WG.

[svillar, 2013-11-15 11:06:21]

On 2013/11/14 21:16:49, rune - CET wrote:
>
https://codereview.chromium.org/23528004/diff/55001/LayoutTests/fast/css-grid...
> File LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html (right):
> 
>
https://codereview.chromium.org/23528004/diff/55001/LayoutTests/fast/css-grid...
> LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html:100:
> testGridDefinitionsSetJSValues("(first) 18px", "66px (last)", "first 18px",
> "66px last", "first 18px", "66px last");
> So this means you didn't change the serialization output to emit valid
> declaration values, so they won't make the round-trip in CSSOM. Did you plan
to
> do that in a follow-up CL?

I could do that in this same CL

[svillar, 2013-11-19 12:29:11]

On 2013/11/14 21:16:49, rune - CET wrote:
>
https://codereview.chromium.org/23528004/diff/55001/LayoutTests/fast/css-grid...
> File LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html (right):
> 
>
https://codereview.chromium.org/23528004/diff/55001/LayoutTests/fast/css-grid...
> LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html:100:
> testGridDefinitionsSetJSValues("(first) 18px", "66px (last)", "first 18px",
> "66px last", "first 18px", "66px last");
> So this means you didn't change the serialization output to emit valid
> declaration values, so they won't make the round-trip in CSSOM. Did you plan
to
> do that in a follow-up CL?

Actually I'm doing this in a follow-up CL because I'm thinking about creating a
CSSTrackListValue class to help with that and simplify a bit the code.

[svillar, 2013-11-19 12:30:32]

On 2013/11/19 12:29:11, svillar wrote:
 
> Actually I'm doing this in a follow-up CL because I'm thinking about creating
a
> CSSTrackListValue class to help with that and simplify a bit the code.

CSSTrackNamesListValue actually

[svillar, 2013-11-20 17:49:31]

Final version of the patch with two main changes: first we now support empty
grid line names declarations and secondly CSSGridLineNamesValue was added to
help with parsing and computed style declarations. Tests were accordingly
updated

[svillar, 2013-11-21 18:45:49]

On 2013/11/20 17:49:31, svillar wrote:
> Final version of the patch with two main changes: first we now support empty
> grid line names declarations and secondly CSSGridLineNamesValue was added to
> help with parsing and computed style declarations. Tests were accordingly
> updated

@jchaffraix, @rune, @darktears any additional comment to my latest changes?
Should I land it anyway?

[Julien - ping for review, 2013-11-21 22:50:34]

lgtm

https://codereview.chromium.org/23528004/diff/235001/LayoutTests/fast/css-gri...
File LayoutTests/fast/css-grid-layout/non-named-grid-line-get-set.html (right):

https://codereview.chromium.org/23528004/diff/235001/LayoutTests/fast/css-gri...
LayoutTests/fast/css-grid-layout/non-named-grid-line-get-set.html:43: }
I would test several contiguous named grid lines:
grid-definition-columns: (foo) (bar) 10px;
grid-definition-columns: 50px (foo bar) (foobar) 10px;

https://codereview.chromium.org/23528004/diff/235001/Source/core/css/CSSGridL...
File Source/core/css/CSSGridLineNamesValue.h (right):

https://codereview.chromium.org/23528004/diff/235001/Source/core/css/CSSGridL...
Source/core/css/CSSGridLineNamesValue.h:32: #define CSSGridLineNamesValue_h
We should prevent git cl from being too smart for its own good and think this is
a copy of a random file (the same applies to CSSGridLineNamesValue.cpp). The
next patches should be uploaded using:

git cl upload --no-find-copies

https://codereview.chromium.org/23528004/diff/235001/Source/core/css/CSSGridL...
Source/core/css/CSSGridLineNamesValue.h:52: explicit
CSSGridLineNamesValue(CSSParserValueList*);
This constructor is not used (nor defined).

https://codereview.chromium.org/23528004/diff/235001/Source/core/css/CSSGridL...
Source/core/css/CSSGridLineNamesValue.h:59: 
Nit: Extra empty space

https://codereview.chromium.org/23528004/diff/235001/Source/core/css/CSSParse...
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/23528004/diff/235001/Source/core/css/CSSParse...
Source/core/css/CSSParser-in.cpp:4831: } while (identList->current());
I would still use a while here as it would easily add a variable for
identList->current().

https://codereview.chromium.org/23528004/diff/235001/Source/core/css/CSSParse...
File Source/core/css/CSSParserValues.h (right):

https://codereview.chromium.org/23528004/diff/235001/Source/core/css/CSSParse...
Source/core/css/CSSParserValues.h:274: this->valueList = valueList.leakPtr();
:(

I am assuming this is because C++ doesn't allow classes with destructor in a
union.

[commit-bot: I haz the power, 2013-11-22 17:02:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/svillar@igalia.com/23528004/495001

[commit-bot: I haz the power, 2013-11-22 18:48:17]

Change committed as 162555