Error in function argument parsing

[Mangara]

After parsing, the function translateY(-.1em) is output as translatey(- .1em). There should not be a space between the minus sign and the length.

[Mangara]

Changing the grammar to treat the (optional) MINUS token as part of the number, dimension, etc., works for this use case, but breaks calc. (see this commit.)

Perhaps it would be best to do some post-processing that merges the MINUS operator with subsequent terms?

[radkovo]

I mean the parsing works fine, the problem is in output formatting. Unfortunately, the functions may have very different kinds of arguments (lengths, identifiers, comma-separated lists, space-separated lists, etc.) and therefore, the operators are treated separately from the values (e.g. lengths). For the same reason, it is not easy to find a unified way of formatting the arguments for output. My proposed solution for this particular issue is in this commit. But perhaps the real solution would be to treat each function individually (as calc() is being treated now). This would allow to validate and format the arguments properly. On the other hand, I am not sure if there is a complete list of existing CSS functions available.

[Mangara]

You're right, there are multiple places this could be fixed. But I would argue that -10deg should be parsed as TermAngle(10, deg, -1), instead of [ TermOperator('-'), TermAngle(10, deg, 1) ]. This makes everything we want to do with it afterwards easier, including fixing the output formatting.

Going through the list of functions at MDN, only calc treats - as an operator, so merging it into subsequent values is something we could do generically for all other functions (perhaps in an overridden setValue of TermFunction?).

And while full validation for each function is a nice goal, the list is extensive, and new functions seem to be added fairly frequently, so treating each separately will involve a lot of work.

[Mangara]

This commit implements the merging that I mentioned earlier. What do you think?

[radkovo]

Yes, this seems like a good direction. I have made some comments to the commit itself. My idea was to go even one step further: Not to create just a universal TermFunction but to create more specific derived terms such as TermTransformFunction and later even more specific TermScaleXFunction, etc. This would allow to parse the arguments according to the actual function syntax. Then, we could use your approach for simple functions with one argument and some different solution for different functions (such as translate3d). In the same time, the parser would correctly recognize syntax errors in function arguments. Currently, it a accepts any arguments for any function which is not a correct behavior from the point of view of CSS standards. I will try to create some basic framework for this and later, we may add the individual functions.

[Mangara]

That sounds like a very good long-term solution. Do you want to merge my changes as a short-term fix, or just start on the long-term framework?

[Mangara]

I made a PR for my changes, in case you want to use them.

[radkovo]

I tried to create a list of all functions mentioned on MDN grouped by categories: css_functions.txt. There are many of them. The plan is to support the most common ones (such as <shape>, <basic-shape>, <filter-function>, <transform-function> and <gradient>) by specific terms and to use the current generic TermFunction as a fallback for the remaining ones. It may take some time to make this work.

[hrj]

Sorry for jumping in without reading up fully, but I tried the latest master branch of jstyleParser with gngr, and there's only one regression. Not sure if it's relevant to this issue. The test case uses a counter as follows:

 #test span:before { content: counter(c, none) "z"; }

It used to work earlier (in the 3.2 series). Note that simple pseudo elements still work, hence I am suspecting a problem in the counter function area.

[radkovo]

Yes, it is related, thanks for reporting this. There was a bug in counter() function parsing. It should be fixed by this commit.