1164918 - [css-grid] Update grid-template-areas CSS parsing to accept multiple dots for a single cell

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 :
====
grid-template-areas and dot sequences
-------------------------------------

  <plinss> https://lists.w3.org/Archives/Public/www-style/2015Jan/0473.html
  fantasai: On the original template syntax each named template was
            a single letter. Then we had the issue of what is a
            letter and there was a proposal to change that to
            identifiers which we accepted. Originally if you place-
            held a spot but without a name it was a single dot, but
            this was a proposal that you allow a sequence of dots
            just so that it's more visible and fills up space.
  fantasai: This is reasonable to me unless there's backwards compat
            issues.
  Rossen: I think this is a good proposal too. It definitely won't
          set us backwards.
  <astearns> I think it's fine too
  plinss: Anyone else? Objections?

  RESOLVED: Accept proposal to allow multiple dots
====


Here's an example from the proposal (2015Jan/0473.html, linked above):
     “.     HEAD  .    .    ”
     “.     MAIN .    PANE ”

     VS

     “.... HEAD .... .... ”
     “.... MAIN .... PANE ”

I may use this as a "good first bug" for an intern starting next week, so don't anyone jump on it yet. :)

Comment 1

[Daniel Holbert [:dholbert]]

I think the code that needs updating here is this case in nsCSSGridTemplateAreaScanner::Next():
> 1363   } else if (ch == '.') {
> 1364     // Null cell token
> 1365     aTokenResult.mName.Truncate();
> 1366     aTokenResult.isTrash = false;
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSScanner.cpp?rev=4e973ce874e2#1363

I think we need to add a "while" loop there, like we have in the clause above, to step forward until we hit a non-period character.

I'm not sure what should happen if we hit something other than a period before we hit whitespace, offhand; I'll email www-style to ask.

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #1)
> I'm not sure what should happen if we hit something other than a period
> before we hit whitespace, offhand; I'll email www-style to ask.

For example, "...-blah"

Re-reading the (not yet updated) spec text on this[1], I'm actually pretty sure this should lead to this token being treated as a "trash token".

http://dev.w3.org/csswg/css-grid/#grid-template-areas-property

Comment 3

[Daniel Holbert [:dholbert]]

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

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> For example, "...-blah"
> 
> Re-reading the (not yet updated) spec text on this[1], I'm actually pretty
> sure this should lead to this token being treated as a "trash token".

(Actually, Kyle realized this is equivalent to "... -blah", because whitespace isn't strictly necessary to separate tokens.  Tab confirmed this on www-style:
 https://lists.w3.org/Archives/Public/www-style/2015May/0239.html
)

Comment 5

[Kyle Zentner]

Attachment: Patch Revision 1

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8607846 [details] [diff] [review]
Patch Revision 1

Looks good!

Comment 7

[Daniel Holbert [:dholbert]]

Removing 'checkin-needed' for the moment -- the tree sheriffs (who triage checkin-needed requests) generally require a Try run of some sort before they'll land stuff.

(This lets them land a handful of checkin-needed patches in a single push, and not have to worry as much about needing to do detective work to assign blame & figure out who to backout when a test fails.)

Comment 8

[Kyle Zentner]

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

Comment 9

[Daniel Holbert [:dholbert]]

(Once that Try run has a green result [or orange-from-an-intermittent-test-failure] for each test-job, on at least one platform, I'd say you're good to add "checkin-needed".)

Comment 10

[Kyle Zentner]

Alright, the Try run looks good enough now.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/580d1022c398

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/580d1022c398

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Backed out for reftest failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=10040406&repo=mozilla-inbound

Comment 14

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c5d42a5e9ad

Comment 15

[Kyle Zentner]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0863ef4ad856

Comment 16

[Kyle Zentner]

Attachment: CSSGridAreaMultipleDots

The previous patch was broken when the fix for Bug 1164953 (which I was also working on) was checked in. This version should be better as indicated by the test runs above.

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 8609050 [details] [diff] [review]
CSSGridAreaMultipleDots

r=dholbert

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/374eac1b621b

Comment 19

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/374eac1b621b