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)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: dholbert, Assigned: zentner.kyle)
References
Details
Attachments
(1 file, 1 obsolete file)
6.06 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
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. :)
Reporter | ||
Updated•9 years ago
|
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
Reporter | ||
Comment 1•9 years ago
|
||
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.
Reporter | ||
Comment 2•9 years ago
|
||
(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
Reporter | ||
Comment 3•9 years ago
|
||
The spec editor's draft has now been updated to reflect this change, BTW: https://hg.csswg.org/drafts/rev/a30873c9b62d
Reporter | ||
Comment 4•9 years ago
|
||
(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 )
Assignee | ||
Comment 5•9 years ago
|
||
Reporter | ||
Comment 6•9 years ago
|
||
Comment on attachment 8607846 [details] [diff] [review] Patch Revision 1 Looks good!
Attachment #8607846 -
Flags: review?(dholbert) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 7•9 years ago
|
||
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
Assignee | ||
Comment 8•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c12c01359db0
Reporter | ||
Comment 9•9 years ago
|
||
(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".)
Assignee | ||
Comment 10•9 years ago
|
||
Alright, the Try run looks good enough now.
Keywords: checkin-needed
Comment 11•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/580d1022c398
Keywords: checkin-needed
Comment 13•9 years ago
|
||
Backed out for reftest failures. https://treeherder.mozilla.org/logviewer.html#?job_id=10040406&repo=mozilla-inbound
Comment 14•9 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/4c5d42a5e9ad
Assignee | ||
Comment 15•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0863ef4ad856
Assignee | ||
Comment 16•9 years ago
|
||
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)
Reporter | ||
Comment 17•9 years ago
|
||
Comment on attachment 8609050 [details] [diff] [review] CSSGridAreaMultipleDots r=dholbert
Attachment #8609050 -
Flags: review?(dholbert) → review+
Reporter | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 18•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/374eac1b621b
Keywords: checkin-needed
Comment 19•9 years ago
|
||
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.
Description
•