Closed Bug 1164918 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: dholbert, Assigned: zentner.kyle)

References

Details

Attachments

(1 file, 1 obsolete file)

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. :)
Summary: [css-grid] Update grid-template-areas CSS parsing to accept multiple dots → [css-grid] Update grid-template-areas CSS parsing to accept multiple dots for a single cell
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.
(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
The spec editor's draft has now been updated to reflect this change, BTW:
 https://hg.csswg.org/drafts/rev/a30873c9b62d
(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
)
Attached patch Patch Revision 1 (obsolete) — Splinter Review
Assignee: nobody → kzentner
Status: NEW → ASSIGNED
Attachment #8607846 - Flags: review?(dholbert)
Comment on attachment 8607846 [details] [diff] [review]
Patch Revision 1

Looks good!
Attachment #8607846 - Flags: review?(dholbert) → review+
Keywords: checkin-needed
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.)
Keywords: checkin-needed
(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".)
Alright, the Try run looks good enough now.
Keywords: checkin-needed
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.
Attachment #8607846 - Attachment is obsolete: true
Attachment #8609050 - Flags: review?(dholbert)
Comment on attachment 8609050 [details] [diff] [review]
CSSGridAreaMultipleDots

r=dholbert
Attachment #8609050 - Flags: review?(dholbert) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/374eac1b621b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: