-
Notifications
You must be signed in to change notification settings - Fork 707
[css-values-5][various] Better handling of arguments with commas #9539
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
Naked Using A generic function would not be bad, but not as good as naked |
You mention .subgrid-2 {
grid-column: span 2;
grid-template-columns: subgrid var(--lines-1, [start]) var(--lines-2, [middle]) var(--lines-3, [end]);
} Currently computes to |
Nah, I mention var() as an example, but it doesn't need to use this syntax. (But if we do |
Like I said, the WG already attempted to use naked |
I'd be happy with either bare Sebastian |
Another possibility is continuing to use |
Yeah, I think the "allow comma or semi anywhere" works pretty reasonably, actually. We'd amend the grammar a bit so that commas in functions can also be semicolons (all at once, not on an individual level), but commas are already pretty magical in the grammar anyway. I'm happy with either approach - "upgradeable" commas or a wrapper. We should get the WG's opinion and fix the Values 5 features accordingly. |
Regarding the upgradeable commas. Let's say a property has grammar But later on we relax the grammar to allow So even if not ambiguous, I would prefer to forbid values from having a top-level comma when the author uses |
@Loirooriol 100% agree. If there are any commas within the values, you should have to use semicolon for the top level. |
We simply wouldn't design such a grammar, so any questions about how it would behave aren't relevant, right? |
Well, let me just tweak it a bit: This is not ambiguous: 1st value must be |
@Loirooriol I think what happens in that case is that the comma is interpreted as a top-level comma (i.e. separating toggle() arguments). Since we don't hit a semicolon, it parses cleanly and we're done. If the author wants to include commas in their value, then they have to use semicolons to separate top-level arguments. We notice that's happening because as we're parsing the function, we hit a semicolon; that requires us to go back and reparse. It does mean that you can't pass to a variable-length function a single argument that includes commas, but... that's probably fine? |
Right, I think that @Loirooriol is just saying that they'd prefer we explicitly state that, in cases where you could potentially include a value with commas in it, you can only do so if you use semicolons; we will never parse a production that includes commas alongside other comma-separated arguments. We'd probably address this at the level of where we define functional notation; we'd say that, unless otherwise specified (such as in (Or maybe we actually reify the concept of "arguments" and say that commas are solely for separating arguments, unless semicolons are used instead. But that'd require a larger review of existing spec text. Might still be a good idea, tho.) |
I think a new generic function is the least weird of the proposed solutions, and the most forwards compatible (I propose
I don’t like this at all.
|
Would you require |
This is answered in the OP. |
This applies to a "wrap the arguments in a function" case too. What you've written also might be correct, or not, depending on the exact same thing - whether the variables contain a comma or not. In either case, an author can defensively mark the function, as
I'm also not sure how this doesn't apply to your preferred solution as well. If authors see a |
I lean towards a new generic function as well. If
|
Putting this up on the F2F agenda to discuss. Summarizing the two options:
|
We should consider the user experience not only when writing code, but also when reading code. The former is only done once, but the latter is done many times. I’m quite concerned that 2 is prioritizing the former over the latter. A grouping construct makes it obvious what is going on, whereas using a different separator only some of the time makes parsing hard not just for parsers, but humans too. Can we use plain parentheses, and special case the case when the first character is |
I noticed in the agenda that this issue is mentioned alongside #6705, but neither issue contains an explicit link to another, so I feel it is worth it to mention it explicitly here, as they essentially talk about the same thing. Regarding the potential solution, I would prefer the
|
Are plain parens an option? Parsing-wise, it should be pretty simple to say "if the first character is a paren, the argument does not end before its closing paren". |
Just mentioned in the call, but to highlight this, the argument against bare parentheses is in the first comment above:
|
The CSS Working Group just discussed
The full IRC log of that discussion<fantasai> -> https://github.com//issues/9539#issuecomment-1918019555<fantasai> scribe+ <fantasai> TabAtkins: General problem is, we have several function in-flight which can take argument values that are the full set of CSS value syntax including comma-separated lists. <fantasai> TabAtkins: example might be var(), which isn't a problem because we intentionally put fallback as the last part, so you can know whether the commas are top-level or part of the argument <fantasai> TabAtkins: but for several other new things, that's not possible <fantasai> TabAtkins: Right now the proposals for these, when your function could contain commas, we switch to using semicolons to separate arguments in those functions. <fantasai> TabAtkins: but it does mean that during design phase of a function, we have to decide whether to use commas or semicolons <fantasai> TabAtkins: it's also a bit weird to have two distinct syntaxes, especially when most uses of these functions won't need semicolons -- most of the time will not contain commas as part of the arguement, just a keyword or whatever <fantasai> TabAtkins: So using semicolon for these when 99.9% of cases don't need it is odd <fantasai> TabAtkins: We came up with several options <fantasai> TabAtkins: 1. Don't use semicolons. Instead, allow functions to have some way to wrap arguments e.g. function, brackets, etc. <fantasai> TabAtkins: For example, 'item()' which just wraps its contents and means its contents. <fantasai> TabAtkins: we could use backets, like Grid already does <fantasai> TabAtkins: or curly braces, which aren't allowed top-level but can use inside a function <astearns> q+ <fantasai> TabAtkins: Alternatively, could make the semicolons an optional upgrade <fantasai> TabAtkins: That is, you start parsing assuming comma separation <fantasai> TabAtkins: but if you hit a semicolon, reparse the function as requiring semicolons between arguments and treating commas as part of the arguments <florian> q+ <fantasai> TabAtkins: I am mildly inclined to go with option 2, where semicolons are an optional upgrade <fantasai> TabAtkins: can be done across all of CSS, so don't have to think about which functions <fantasai> TabAtkins: Doesn't require extra level of nesting which we try to minimize <fantasai> TabAtkins: that said, I'm OK with any of these options <fantasai> TabAtkins: Opinions? <Rossen_> q? <oriol> q+ <Rossen_> ack astearns <fantasai> astearns: Lea has asked a few times, instead of generic function, why not bare parentheses? <florian> q- <fantasai> TabAtkins: I responded -- previously purposely avoided bare parens. We used them originally e.g. for grid names, but switched away, because that would mess up SASS. <fantasai> TabAtkins: those arguments still apply <fantasai> astearns: I have a slight preference for option 1 with short function name <fantasai> astearns: I also think square brackets would be fine <fantasai> oriol: Personally I'm fine with current situation of just using semicolons. <florian> [removed myself from the queue because I wanted to ask and say the same thing as astearns] <fantasai> oriol: It's true that we need to think about it at the beginning of the function design, but it's ok to me <Rossen_> ack oriol <fantasai> oriol: I'm also OK with option 2 <fantasai> oriol: among the others, they seem strange <fantasai> oriol: I would be OK with parens, but the others seem a bit strange to me <fantasai> oriol: Also argument about SASS, I wonder if there would be a possibility for them to wrap... This is a 3rd party tool, we shouldn't constrain CSS development to match outside tooling. <TabAtkins> fantasai: like Oriol, I'm fine with current situation of desinging it form the beginning. also fine with option 2. i could live with braces, but I think the function option reads as if it's part of CSS rather than an escaping mechanism. <TabAtkins> fantasai: so i'd prefer avoiding using a function. <TabAtkins> fantasai: For brackets, we already use that in other parts of CSS (Grid) so it's potentially confusing there. <TabAtkins> fantasai: But we'd *never* use braces in that same way, so we won't have the same problem <TabAtkins> fantasai: So mild pref for semicolon; their identity is a stronger comma and it's fitting we use it for that purpose <Rossen_> ack fantasai <fantasai> Rossen_: Other opinions? <fantasai> TabAtkins: Straw poll? <TabAtkins> 1. No change (design functions to use ; when it's needed) <TabAtkins> 2: use [] wrapper. <TabAtkins> 3: use {} wrapper. <TabAtkins> 4. use function wrapper (item() or similar) <TabAtkins> 5. Upgradeable semicolon (comma in general, but authors can choose to use ; instead when necessary) <TabAtkins> 5, 3 <astearns> 4, 2 <vmpstr> 4, 2/3 <fantasai> 1, 5, 3 <oriol> 1, 5 <schenney> 2,4,1 <miriam> 5,3,1 <flackr> 5, 4 <florian> 3,5 <kbabbitt> 2/3, 4, 1 <iank_> 5, 1 maybe <Rossen_> 4,3 <fantasai> astearns: Tab you had a worry that {} might break parsing, is that still a concern? <fantasai> TabAtkins: for naive (regex) parsing yeah, but all good tools should have a real parser <TabAtkins> fantasai: If you can't do bracket matching your tool is already gonna break everywhere, so minimum you need to bea ble to bracket match <dholbert> (abstain) <schenney> 5 is also most popular 2nd option, I think <flackr> q+ <fantasai> Total counts: 5.5 for 1, 2 for 2, 7 for 3, 6 for 4, 7 for 5 <Rossen_> ack flackr <fantasai> flackr: Is there a problem with nesting the functions? <fantasai> TabAtkins: that's fine, because the function itself will be the wrapper <fantasai> ^Rossen: More first-place votes for option 5 <florian> WFM <fantasai> Rossen_: Let's resolve on 5. <florian> +1 <fantasai> TabAtkins: Was close, but 5 ekes out, and if any strong objections can re-discuss <fantasai> Rossen_: any objections to #5? <fantasai> RESOLVED: Make semicolons an optional upgrade to commas in CSS functions. |
So if a function accepts a single parameter, then |
Would |
Strong +1 to what Tab is proposing here.
Good point, but this would only be a problem for functions that accept
This currently resolves to Related question: it may be too late for
I don't think arbitrary substitution functions should be allowed to mess with the grammar of the calling function itself, just like you're not allowed to paste e.g. a fallback into another So when When |
I'm not sure what you mean by this. What's the grammar for that first argument? If it's a comma-containing production, then The presence or absence of a second argument, optional or not, has nothing to do with this, and I'm not sure how you're reading that into what I wrote. |
(edited to add: per my next comment, I'm probably slightly wrong here.)
Per my proposal above, the But as Anders said, we might be able to (and hopefully can) make var() more subtle, and allow enforcing the {} rules like other locations. (But, as they say, there's no way we can make it require {} around commas; it'll still need to allow top-level commas in its fallback for back-compat.) If that's the case, then the behavior instead becomes:
This defensive coding is slightly unfortunate but a required part of how var() (and all other arbitrary-substitution functions) work; they are intrinsically passing around raw syntax, not higher-level values. For example, a custom property could be |
Oh, hm, you're probably right actually. You indeed can't sub in arbitrary grammar like that, since var() is actually parsed while its surrounding context is left as unparsed tokens. I'll need to give this a little thought. |
Exactly. By default, I'd expect the same from any arbitrary substitution function. I don't think we should deviate for But this is a bit off-topic (we can ask roughly the same questions under upgradeable commas), so maybe we should open a new issue if more discussion is needed on that. |
Yeah sorry I misread what you said, I thought you were proposing something different. |
Yup, #10947 |
The CSS Working Group just discussed
The full IRC log of that discussion<emeyer> TabAtkins: We discussed how to deal with argument values that might have commas in them<emeyer> …We did a straw poll that ended in a tie, and did a runoff <emeyer> …That resulted in upgradeable commas, where you can replace commas as semicolons <emeyer> …This works, but upon more thought, we would like revisit this decision and go with the other thing that got first place <emeyer> …The option we want to go with instead is to use a curly brace wrapper around the values <emeyer> …So if you write an if with a comma-separated condition, you write it like this (shows syntax from comment on issue) <emeyer> …So it can either be a single curly brace block containing anything that’s valid, or it’s a sequence of component values <emeyer> …This way as long as you’re doing anything fancy, you can just write the fuction exactly as you would elsewhere <emeyer> …An argument with commas gets marked as being a special thing <emeyer> …In the original version, you use quoted strings separated by commas <emeyer> …YOu can end up with one thing being valid and one not valid, and you can only fix it with a trailing comma <emeyer> …This is bad <emeyer> s/YOu/You/ <emeyer> …It looks bad and it’s inconsistent with other things <emeyer> …The curly braces make it clear where the complicated bits start and stop <emeyer> …This also makes other issues easier <emeyer> …There are a few parsing questions about how this would apply to var() <emeyer> …They’re arcane <oriol> q+ <emeyer> …So we’d like to change to curly brace wrapping and resolve on that instead <astearns> ack oriol <emeyer> oriol: I agree the commas were not a great decision, but I have a mild preference for the original idea to require semicolons <emeyer> …It avoids authors thinking if they use a var(), maybe the value is something typical <emeyer> …But the semicolons has some downsides so I’m okay with doing this <emeyer> TabAtkins: Oriol’s pointing out authors had to memorize the special-casde syntax even if they didn’t often use it <emeyer> …This is what turned us against it; we wanted something lower-touch <emeyer> TabAtkins: (shows an example of the older syntax) <emeyer> s/casde/case/ <lea> q? <emeyer> TabAtkins: This would have required defensive coding, but that’s kind of true of any syntax we decide to use <emeyer> …The only one that avoids it is “always use semicolons” but we rejected that for other reasons <lea> q+ to say agree that a wrapping construct is better than upgradable commas. The natural option here is bare parens, and I don't buy the argument that we should contort our syntax *forever* to accommodate a preprocessor, especially now that its popularity is dwindling <astearns> ack lea <Zakim> lea, you wanted to say agree that a wrapping construct is better than upgradable commas. The natural option here is bare parens, and I don't buy the argument that we should contort <Zakim> ... our syntax *forever* to accommodate a preprocessor, especially now that its popularity is dwindling <emeyer> lea: I agree that upgradable commas are weird and wrapping constructs are much better <emeyer> …For the best wrapping construct would be, we can discuss options but I agree that bare parentheses are the natural option <emeyer> …We should not contort CSS syntax forever <emeyer> …The choice to prioritize Sass syntax over natural syntax seems weird <emeyer> TabAtkins: We used quare brackets in Grid because it was easier at the time <emeyer> …Here, bare parentheses were proposed and it was pointed out we’d decided against that in the past and didn’t want to revisit that <kizu> q+ <miriam> q+ <emeyer> …I don’t think there’s an overriding reason to go back and revisit our decision to avoid those <astearns> ack kizu <emeyer> kizu: +1 to Lea <emeyer> …I think plain parentheses would be better <emeyer> …Curly braces mean a block in CSS <dbaron> (I agree that wrapping is better than trailing semicolons as a defensive technique, for what it's worth.) <lea> bare parens are the quintessential grouping construct. No braces, brackets, or function compares to the natural ergonomics of bare parens for this. IMO <emeyer> (scribe having trouble making out what’s being said) <emeyer> …I do want to use commas with braces <astearns> ack miriam <emeyer> miriam: I think the same can be said the other way about parentheses in CSS< which are only used on function and calculations <lea> q? <emeyer> …This stands out as being different and looking different <astearns> ack fantasai <lea> q+ to say in every prog lang parens are used for functions and grouping, this is not new <emeyer> fantasai: The advantage of curly braces is it leaves parentheses to be used for something more useful than “we needed to escape commas here” <tantek> +1 dbaron, lea <emeyer> …I don’t think we should take a syntax that could be much more useful here <ntim> `toggle(« Arial, Helvetica, sans-serif », « Times, serif »)` :D <emeyer> lea: I don’t see it as escaping, I see it as grouping <emeyer> …In every popular language, parentheses are used for grouping <emeyer> …We do that inside our own calc() <emeyer> …If we end up deciding other constructs are better, regardless of the Sass problem, there’s no Sass problem in the first place <astearns> q? <astearns> ack lea <Zakim> lea, you wanted to say in every prog lang parens are used for functions and grouping, this is not new <emeyer> TabAtkins: Because the reasons for the original Sass allowances are valid and are even more so here, Chrome would object to using parentheses for this case regardless <emeyer> q? <emeyer> astearns: We have two options, each with champions, and one has an objection <emeyer> …Is there anyone who would object to curly braces <emeyer> lea: There are other options like square brackets <emeyer> fantasai: Square brackets aren’t an option because you’d have to escape them <emeyer> TabAtkins: You wouldn’t but other options lost to curly braces <emeyer> …in the original poll <fantasai> s/escape them/escape them in grid/ <TabAtkins> TabAtkins: Chrome doesn't object to the other options, but {} won the original poll <romain> Curly brackets look quite nice in this example by kizu for inline conditions : https://github.com//issues/10064#issuecomment-2373483709 <lea> Option 1: Bare () <lea> Option 2: {} <lea> Option 3: [] <lea> Option 4: Function e.g. value(), val(), arg(), item() <emeyer> astearns: It’s not clear to me that the options for parentheses transfeer to square brackets <emeyer> …It’s another grouping mechanism <ntim> 2 <fantasai> 2 <romain> 2 <kbabbitt> 2 <lea> 3 <emeyer> s/transfeer/transfer/ <ethanjv> 2 <moonira> 2 <chrishtr> 2 <alisonmaher> 2 <kizu> 4 <emeyer> …Looks like we’re doing a straw poll; Lea put options into IRC <miriam> 2 <TabAtkins> 2 <keithamus> 2/3 <Penny> 2 <emeyer> …Please put in a number corresponding to your preference <astearns> 2 <futhark> 4 <lea> actually 3/4 for me <dbaron> 2 <vmpstr> abstain <rachelandrew> 2 <dholbert> abstain <ydaniv> abstain <emeyer> abstain <castastrophe> 2 <masonf> abstain <emeyer> astearns: Poll looks pretty clear toward curly brackets <oriol> Preference for requiring semicolons. But 2 among the above. <emeyer> …Is there anyone who would object to that option? <emeyer> (silence) <emeyer> …Given the straw poll, I suggest we resolve on using curly braces <emeyer> …Objections? <emeyer> astearns: Hearing none, we will undo the previous resolution <emeyer> RESOLVED: Undo previous decision and move forward with optional curly-brace wrapping of complex arguments |
We resolved on using semicolons in |
I quite liked @kizu's solution in #10064 which I'm surprised wasn't suggested here.
|
@nt1m That was specifically for |
Yes. I'm a little annoyed that we went with a different syntax for |
… to allowing {} wrappers, rather than semicolon upgrades. #9539
There's no reason we can't change it now though, is it? It hasn't shipped anywhere(?) |
In theory no, but we literally decided on both of these (the new syntax for comma-containing arguments, and the syntax for if()) at the same meeting a few weeks ago, and explicitly handled the former first so it could influence the latter's discussion. The WG just resolved differently than what I wanted, is all. It happens. (I'm a little confused why @fantasai is asking if we're still happy about this, for exactly this reason. The two topics were discussed on the same day, immediately following each other, with explicit references between them, so I don't understand why this resolution would be in question.) |
I hope you do not mind that I did not open a separate issue for this. |
Ah yes, you are completely correct. I accidentally dropped the context that this only applies within functions when I rewrote the text, thanks.
Yeah I was wondering which way I should write it. You're probably right that I should just do treat it as a block, since grammars never see the { tokens themselves. |
Based on the current definition of a non-strict comma-containing production:
Both are valid in the current version of Chrome and Firefox. @tabatkins, could you please tell me if breaking backwards-compatibility is intentional? I am not sure I fully understand the previous comments but I suspect that only a standalone {}-block should act differently. |
We've got several in-flight proposals for functions that can take "whole property value" arguments, like
mix()
,random-item()
, etc. (And now custom functions and mixins, which @mirisuzanne and I are looking into more seriously again.) Because these arguments can potentially contain commas (say, mixing between two background-image lists), we can't use commas as the argument separator, and the proposals currently all use semicolons as the separator instead, as this is guaranteed to never show up at the top-level of a property value.Even tho I'm the one that started this trend, I don't like it. ^_^ I'd like to find a different solution. The problems are:
var()
function already lets you take this kind of argument, but because its syntax was designed to take only one, and as the final argument, it can continue to use comma separation. (That is, you can writevar(--foo, blue, red, black)
- that's two args, a-foo
and ablue, red, black
list.)The obvious solution is to add an optional wrapper around arguments, which will "hide" the commas from the outer function. It would only be necessary when someone is actually passing a comma-separated argument.
I have two possible suggestions, and am happy with either:
[]
, likebackground: mix(50%, url(start), [ url(foo), url(bar) ]);
item()
:background: mix(50%, url(start), item( url(foo), url(bar) ));
This notation would only be allowed in the functions that can take these problematic values, and if present, would be stripped for the purpose of the actual value. No nesting - if you were mixing a subgridded "line names only" value for grid-template-columns, for example, you'd write it as
grid-template-columns: mix(50%, [[foo bar]], [[baz qux]])
, so the outer[]
gets stripped and the inner[]
is part of the actual value.(I'm rejecting using naked
()
for the same reason we ended up switching away from them for grid lines - it would mess up Sass syntax, and possibly others, hardcore, since in Sass it indicates math. I'm rejecting{}
for "naive userland tools" reasons; again, many just split the text with regexes, and stray{}
can screw them up. They're already potentially broken for custom props containing these chars, of course, but that's not an official Part Of CSS like this would be.)Thoughts?
The text was updated successfully, but these errors were encountered: