-
Notifications
You must be signed in to change notification settings - Fork 682
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
[css-values-5][various] Better handling of arguments with commas #9539
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
|
And just to be clear about the actual spec text for this, we wouldn't allow {} wrappers just anywhere, so there's no need to upgrade the parsing of all functions, everywhere. Instead, it would be a change to the definition of the comma-containing productions. Currently (with the "upgradeable commas" feature), they're defined to not allow commas, unless the function is being reparsed in semicolon mode. (aka, if the function is currently in comma mode, the production ends at a comma or the With this change, instead they'd be defined in two branches. The production is either:
To aid in clarity, I'd probably swap them all to a new production, maybe |
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. |
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: