1164953 - [css-grid] Update grid-template-rows / grid-template-columns to use square brackets around lists, instead of parentheses

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

Quoting https://lists.w3.org/Archives/Public/www-style/2015May/0175.html :
====

Brackets for Grid Line Names
----------------------------

  <plinss> https://lists.w3.org/Archives/Public/www-style/2015May/0103.html
  fantasai: I was talking with astearns at the last F2F. Right now
            we're using parenthesis and he pointed out brackets
            would be easier to read instead of nested parenthesis.
            Seemed reasonable, so I suggested it on the mailing list.
  fantasai: I think TabAtkins is in favor here.
  fantasai: I have no strong opinion.
  Rossen: I can live with either.
  fantasai: Should we revisit when he's here?
  astearns: I don't think it matters much either way.
  <astearns> (but I have a slight preference for the brackets)

  plinss: There's a issue with backwards compat if anyone is using
          this already.
  Rossen: Is there any implementations of this?
  plinss: Good question.
  Rossen: We're not implementing it. For the time being it doesn't
          matter. As long as the syntax and behavior makes sense in
          the overall model, it should be fine. Once we get to
          implementation I'm sure we'll revisit a lot of it and I'm
          sure we won't be alone. For the time being let's move
          forward and accept.

  plinss: And this is grouping line names everywhere?
  fantasai: Yes.
  Rossen: Do we have any other incidents of this?
  plinss: I don't think we do anywhere.
  plinss: We use bare brackets as attribute selectors.
  fantasai: That's in selectors.
  plinss: Yeah.

  plinss: So. Objections to switching from parentheses to brackets
          for grid line names?

  RESOLVED: Switch from parenthesis to brackets for grid line names.
====


Quoting the email proposal:
https://lists.w3.org/Archives/Public/www-style/2015May/0103.html
====
[...] the repeat()
syntax in Grid Layout would be easier to read if line names used
brackets instead of parentheses

   repeat([line-name] 10px [line-name] calc(60em+10px))
vs
   repeat((line-name) 10px (line-name) calc(60em+10px))
====


The code in question lives here in nsCSSParser method "ParseGridLineNames":
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSParser.cpp#8005

Also:
 - There's a line of documentation that mentions "paren" up where that method is declared, too; that 'paren' word needs updating as part of this.
 - These (few) tests need updating:
http://mxr.mozilla.org/mozilla-central/search?string=grid-template-columns.*\%28&regexp=on
 - The "other_values" entries here need updating:
http://mxr.mozilla.org/mozilla-central/source/layout/style/test/property_database.js?rev=956a03448bbe#5784

Comment 1

[Daniel Holbert [:dholbert]]

The spec editor's draft has now been updated to reflect this change, BTW:
 https://hg.csswg.org/drafts/rev/df9d097fe9e8

Comment 2

[Kyle Zentner]

Attachment: Patch file

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 8607207 [details] [diff] [review]
Patch file

Looks good! Two nits:
 (1) The commit message didn't make it in somehow. (It should be in the patch headers at the top of the patch file.)
 (2) We still need to do the substitution in property_database.js -- the last link in comment 0.

So, marking "r-" since this isn't quite ready to land yet, but this is nearly r+.

Comment 4

[Kyle Zentner]

Attachment: GridParen2Brace

Patch revision 2. Fixed tests and serialization. Passes mochitest and reftest.

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8607643 [details] [diff] [review]
GridParen2Brace

Looks great! Just one nit:

>Bug 1164953 - Update ParseGridLineNames from parens to brackets. r=dholbert

As you discovered, this turned out to be more involved than just tweaking ParseGridLineNames.  Probably better to have the commit message say:
 "Update CSS grid <line-names> syntax to use brackets instead of parens"

r=me with that.

I'll give this patch a Try run, and if all's well, please upload a new version with the updated commit message (updated with 'hg qref -e'), and add the "checkin-needed" keyword to this bug.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5440e67f019

Comment 6

[Kyle Zentner]

Attachment: Patch Revision 3, with updated commit message.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8607787 [details] [diff] [review]
Patch Revision 3, with updated commit message.

Looks good, thanks!

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/62a8ef975639

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/62a8ef975639