-
Notifications
You must be signed in to change notification settings - Fork 647
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] minimum nested pairs of parentheses in calc to 32 #3462
Comments
Same issue in this |
Why? And why only on calc() and not on other (all?) CSS blocks? |
There ought to be a reasonable ceiling limit to everything. 32 seems to me very reasonable.
Which other CSS blocks? I'm not sure I understand. |
I mean why only on |
Another example is CSS Selectors 4 When the spec has no limits, implementations are forced to make their own choices between
|
Ideally the limit should be a minimum so that authors can have guarantee and as technical limits matter less in the future the ceiling can be lifted and use cases may occur we did not realise. A spec should never really have a maximum limit in my opinion. Is there precedence for min/max limits? What about limits on amount of CSS selectors or how many CSS stylesheets you can import or link to? Is a limit here a dangerous precedent or would asking implementations to guarantee some minimum be more reasonable? |
Right now, Firefox fails If we can not reach an agreement on a ceiling limit value on calc nested parentheses, the very least minimum to do will be to adjust such test. |
That test is a WebKit-internal test, I don't see why Firefox would need to pass it. |
CSS has never introduced actual maximums for anything. We have a few places where we've defined minimums that must be supported (with the implication that going over is implementation-defined and allowed to fail, in a particular spec-defined fashion). @fantasai and I have tried to introduce such limits for all the numeric values, but haven't succeeded yet. :( I'm happy to similarly define a maximum required nesting limit here. Presumably a separate nesting limit for rules vs values, since they're generally parsed in different phases?
Crashing is never a reasonable behavior. ^_^ |
I will be submitting that test - most likely modified a bit - to WPT repository soon (say, within 6 weeks). So the test has to be modified somehow so that Firefox is not unfairly penalized (and does not fail) just because it supports 128 nested calc parentheses. |
@tabatkins Did you mean minimum? Otherwise, why add this inconsistency if minimums are used everywhere else? |
I would agree with setting a minimum, but not a maximum, in the spec. As for the test, it could be written with as many parentheses as needed to pass in all known implementations, and also include a comment to the effect that if your UA is failing because you support more, then ask the test to be adjusted. It might also be worth separating out that test into its own file. |
I will eventually adjust that sub-test so that only 32 nested parentheses are used. |
Sorry, yes, that's what I meant. Must support up to X degree of nesting; can support more but are not required to; must fail in a particular standardized way when the limit is exceeded. |
Following-up on the above. I think that 128 pairs of nested parentheses inside a calc() function in that test should be reduced to 32 pairs. Note that there was already a test from @FremyCompany that was testing 24 pairs of nested parentheses inside a calc() function: |
Then François Remy's calc-parenthesis-stack.html test could or would suffice. |
I believe that CSS implementations should support a minimum of 32 nested parentheses. |
agenda+ to confirm #3462 (comment) |
The current spec in CSS Values is
|
Just reviewed the test, and we might also want to make clear whether the parentheses for |
@AmeliaBR Right, but that doesn't restrict the number of parens you have to be able to handle; you could have a single "term" wrapped in a hundred paren pairs and not violate that support requirement. ^_^ Tho I suppose there's no reason to mandate at least 32 nesting levels and mandate less than 32 terms, since the only reason to add parens is to separate terms. We should probably bump the minimum terms requirement to 32 as well, then. I think I also need to rewrite that requirement a bit, as it's implicitly (and vaguely) referring to tokens; as stated, an expression containing a single |
The CSS Working Group just discussed
The full IRC log of that discussion<dael> Topic: minimum nested pairs of parentheses in calc to 32<dael> github: https://github.com//issues/3462#issuecomment-478408798 <dael> TabAtkins: Get a resolution that we should have limit on req. number nested parans that impl must support <dael> TabAtkins: calc imposes min on terms but there's no spot where we allow you to fail with nested <dael> TabAtkins: Prop: Put in a mandatory min for paren depth. Past that you're allowed to fail. Number is set to 32 <dael> fantasai: And this is a min, not a max. <dael> florian: It's setting a min and saying when you fail how you do so? <tantek> regrets+ <dael> TabAtkins: Yes, we have that term in other parts of calc so reuse <dael> AmeliaBR: This is specfically about parsing nesting so this is literal parens not conseptual order of operations? Chrome when serializes adds a lot of parens so from authoring might be confusing. One paren with multi operations is it literal parens? <dael> TabAtkins: It's a parsing problem so literal parens. Further order of operations can only impose one additional level so don't have to worry about that nesting indefinetly. <dael> florian: If chrome inserts unnec parens it's probably wrong. <dael> TabAtkins: Yep <dael> AmeliaBR: True but I think 32 number comes out of chrome so not sure if anybody has tested how the 32 is counted in chrome and maybe has an effect? <dael> florian: Are we saying 32 included or excluded? <dael> TabAtkins: I do not care. I'll put one down. <dael> fremy: Similar question because it's a difference of one. Need to be clear if it's inside the code. <dael> florian: As long as clear doesn't matter <dael> fremy: Agree <dael> astearns: Serialization bug will be an extra problem if people using serialized values to set other prop. Importance of fixing prob. go up <dael> florian: But it's a min not a max. <dael> astearns: We have a limit on terms which is 20, why choose different number? <bkardell_> can someone explain why that is not a good assumption? <bkardell_> (that you should be able to round-trip parse/serialize) <fantasai> bkardell_, because browsers have bugs :) <bkardell_> are these bugs not fixable? <fantasai> bkardell_, in an ideal state, it should otherwise be a good assumption <dael> TabAtkins: 20 terms came because it seemed good. 32 is smallest limit across the browsers; Blink has 32. It would prob. be good to have similar numbers so I suggest raising term to 32 to make them symmertic <fantasai> bkardell_, yes <dael> AmeliaBR: I don't think it's conceptually possible to support 32 nesting and 20 terms. Doesn't bracket count as a term? <bkardell_> fantasai: yes they are fixable, or yes they are not fixable? :) <fantasai> bkardell_, fixable of course :) <dael> TabAtkins: No. It doesn't count functions or parens so I need to update. When it's updated paren groups should count <dael> astearns: bkardell_ asked on IRC about what's not a good assumption. We should not assume people writing serialization code and people writing paren code are talking to each other. Assuming if i's all chrome it'll work is bad. <dael> bkardell_: SO we want to spec so when done it's safe assumption? <dael> florian: We want chrome to fix bugs b/c having more parens in output then in input is not a thing that should happen. By shortest serialization it shouldn't do that <dael> bkardell_: AS we spec it there should be a test that says that is a safe assumption <dael> astearns: Should be a bug for a 32 paren that's parsed, you serialize, reparse, and then it works. <dael> astearns: I believe prop is? <dael> TabAtkins: Make the term limit consistent with nested parens limit <dael> astearns: 2 resolutions. Set a min parens that is set at 32. Then raise min to 32 <AmeliaBR> s/raise min/raise min terms/ <dael> astearns: Obj to set minimum number of parenthesis to 32 <dael> RESOLVED: Set minimum number of parenthesis to 32 <dael> astearns: Obj to raise the number of expressions that must be handled to 32 <dael> RESOLVED: Raise the number of expressions that must be handled to 32 <dael> astearns: Lots of tests for all of this <dael> florian: Writing a test is what prompted it <dael> TabAtkins: For values stuff I'm enjoying writing tests so I'll make sure inputs go in with tests |
@frivoal Someone (it could be you, Florian) will need to approve pull request 16170 now that this issue has been settled. Pull request 16170 involves a specific, dedicated test on a calc() with 32 pairs of parentheses. |
done. |
calc-errors.html test at line 68
At line 68 of the test above, there is a calc() function with 128 nested parentheses. Firefox is able to support 128 pairs of nested parentheses. Blink supports 32 nested pairs of parentheses and then fails when trying 33 nested pairs of parentheses:
trac-webkit-calc-errors.html with 33 nested pairs of parentheses
128 nested pairs of parentheses seems outrageously extreme but there seems nothing in the CSS3 spec and CSS4 spec that indicate ceiling limit for nested parentheses.
Proposal: CSS4 spec explicitly state a limit (ceiling) of support to 32 nested pairs of parentheses in calc() function.
Then the test calc-errors.html should be adjusted accordingly before submitted to wpt.
The text was updated successfully, but these errors were encountered: