Menu ▾ ▴

#11 Patch proposal for calc()

none
closed
RBRi
None
1
2020-10-07
2020-09-26
Christian Morgner
No

Hi,

I would like to propose a patch for https://sourceforge.net/p/cssparser/bugs/59/ that fixes some of the failing tests for calc().

The patch adds support for calc() with (optional) spaces, nesting and sequences of arithmetic expressions. It reduces the number of parse errors in the realword/all.css test from 26 to 4.

I had to change the Java version in pom.xml from 1.5 to 11 in order to be able to build the cssparser on my machine, but I think that a lower version should also work.

There is one warning in the JavaCC code generation which I was not able to get rid of yet.

Warning: Choice conflict in (...)+ construct at line 1722, column 13.
         Expansion nested within construct and expansion following construct
         have common prefixes, one of which is: <S>
         Consider using a lookahead of 2 or more for nested expansion.

Please review the patch and let me know what changes you want to have done to be able to merge it.

I really want to be able to use the CSS parser in Structr (https://structr.com), so I'm trying to get as close to full CSS3 support as possible. Maybe there is someone who can help me with that, because I have some problems with "code too large for try statement" and "code too large" errors, when I try to add support for variables (--my-color) and the var() function.

Best regards,
Christian

1 Attachments
calc-patch.diff

Discussion

  • RBRi

    RBRi - 2020-09-26

    Many thanks, looks good. Will merge this soon.
    Looks like '()' are not supported by your patch ? Will be great if you can add this also.

     

  • Christian Morgner

    Christian Morgner - 2020-09-26

    Thanks for the quick response! :)

    Do you mean parentheses inside of a calc expression? If yes: I will try to add support for that as well.

    What do you think about support for variables and the var() function? I wasn't able to get that working because JavaCC creates uncompilable code (code too large for try statement), so maybe you have a hint?

     

    • RBRi

      RBRi - 2020-09-27

      The size of the genrated code is a real problem with JavaCC, so far i have no idea how to fix that. There are already issues open for Javacc to split the generated methods but i guess there is noone working on that.

       

      • RBRi

        RBRi - 2020-09-27

        With the last commit i have changed

        | < FUNCTION_CALC: <c_letter> <a_letter> <l_letter> <c_letter> <lround> ></lround></c_letter></l_letter></a_letter></c_letter>

        into

        | < FUNCTION_CALC: "calc" <lround> ></lround>

        Not using the x_letter definitions simplifys the lexer - this gives you some room for more lexer rules (there is a tradeoff, not all escaped writings of calc are supported by using this - but i guess nobody will escape letters inside 'calc').

         

  • RBRi

    RBRi - 2020-09-27

    Hi Christian,
    have spend some time to rewrite the calc parser based on the spec.
    It will be great if you can write more test cases for the clalc parser (e.g. negative values, devide by dimensions, whitespace at various places).

     

  • Christian Morgner

    Christian Morgner - 2020-09-27

    Hi,

    I'm working on a better grammar that is closer the the specification and supports parentheses. I also added test cases, but I have not managed to get the grammar working yet, so I need some more time.

    I'm not sure what you mean by "have spend some time to rewrite the calc parser based on the spec."... do you have spent some time, or should I spend some more time (as I'm currently doing)?

     

    Last edit: Christian Morgner 2020-09-27

  • Christian Morgner

    Christian Morgner - 2020-09-27

    Hi, I just saw that you already merged my initial patch proposal and improved the grammar a lot more than I was able to. Thanks a lot! :)

    I will happily provide some more test cases, probably later today / this evening.

     

    • RBRi

      RBRi - 2020-09-27

      Have done another minor update

       

      Last edit: RBRi 2020-09-27

  • Christian Morgner

    Christian Morgner - 2020-09-27

    Thank you very much for the update! I have attached a diff with the test cases I was trying to resolve. Some of them still don't work, but I haven't managed to find a solution :(

    For example, there is a problem with calc(100% / 3 - 40px) where the / 3 part is parsed correctly, but doesn't appear in the final result. I wasn't able to resolve the problem, but it seems that the nextLexicalUnits "/" and "3" are overwritten when adding "-" after exiting calcProduct().

    I also included a test case for CSS variables, which I was not able to implement at all because of the "code too large" errors we talked about earlier.

    Please let me know if I can help in any way, and thank you for your work and time!

     
    tests.diff

  • RBRi

    RBRi - 2020-09-28

    Thanks, will update the parser (again)

     

  • RBRi

    RBRi - 2020-09-28

    Ok, hope have finally fixed the parser, now many more tests passing.
    Your tests are also merged.

    Maybe you can try to find a case where the parser stil not works correct.

     

  • Christian Morgner

    Christian Morgner - 2020-09-28

    Great, thank you for the update again!

    I tested the current version against my CSS test case (which is minified Tailwind UI CSS (https://tailwindui.com)). I found some empty rules where I expected custom properties and var() expressions, so I added grammar rules for that. The grammar is much simpler, and I think I was more successful this time. (I added more test cases as well.)

    You can find the changes in the attached diff, which should add support for custom properties and the var() function.

    Please note that I had to change lang() and not() from x_letter definitions to literal strings to avoid "code too large" errors.

     
    var-and-custom-properties.diff

  • Anonymous

    Anonymous - 2020-09-29

    Looks good, will merge this later today.

     

  • RBRi

    RBRi - 2020-09-29

    patch is applied

     

  • Christian Morgner

    Christian Morgner - 2020-09-29

    Hi, thanks again and sorry for keeping you busy.. :(

    It seems that the merge accidentially removed the @Test annotation of the var() tests. That alone wouldn't be so bad, but your changes to the patch for the grammar seem to introduce an endless recursion that results in a Stack Overflow, and the tests do not catch it because they are disabled by accident:

    Exception occurred in target VM:  
java.lang.StackOverflowError: 
    at java.lang.Math.min(Math.java:1528)
    at java.util.Arrays.copyOf(Arrays.java:3748)
    at java.lang.AbstractStringBuilder.ensureCapacityInternal(AbstractStringBuilder.java:227)
    at java.lang.AbstractStringBuilder.append(AbstractStringBuilder.java:593)
    at java.lang.StringBuilder.append(StringBuilder.java:172)
    at com.steadystate.css.parser.LexicalUnitImpl.appendParams(LexicalUnitImpl.java:638)
    at com.steadystate.css.parser.LexicalUnitImpl.getCssText(LexicalUnitImpl.java:399)
    at com.steadystate.css.parser.LexicalUnitImpl.toString(LexicalUnitImpl.java:411)
    at com.steadystate.css.parser.LexicalUnitImpl.appendParams(LexicalUnitImpl.java:644)
    at com.steadystate.css.parser.LexicalUnitImpl.getCssText(LexicalUnitImpl.java:399)

    I created another patch that fixes the stack overflow by avoiding to overwrite the prev argument of the method.

     
    fix-stackoverflow.diff

    • RBRi

      RBRi - 2020-09-29

      Verdammt - will have a look at this tomorrow. Sorry did it in a hurry...

       

      Last edit: RBRi 2020-09-30
      alternate

  • Christian Morgner

    Christian Morgner - 2020-09-29

    Here is another patch that adds support for the "rem" dimension. The patch deprecates the previous one, as it contains the stackoverflow fix and some more test cases.

    The only thing left in the tailwind.ui CSS is the empty custom property declaration: var(--my-value, ). I added a (failing) test that demonstrates the error; sadly I was not able to find a solution.

    With "rem" support, the number of import errors is down to about 10, which is really awesome because we are very close to have the full set of rules in the database!

     
    stackoverflow-fix-and-rem-dimension.diff

  • RBRi

    RBRi - 2020-09-30

    Ok, next incarnation is online.

     

  • Christian Morgner

    Christian Morgner - 2020-09-30

    Thank you very much!

    The most recent version fixes all warnings and errors in my test case, and all the rules are imported successfully.

    Would it be possible to build a release and publish it on maven central, so we can officially use the new version without the SNAPSHOT suffix?

     

  • Christian Morgner

    Christian Morgner - 2020-10-07

    Hi, would it be possible to publish a 0.9.28 release based on this patch on Maven Central?

     

  • RBRi

    RBRi - 2020-10-07

    Aganin thanks, releae is on the way

     

  • RBRi

    RBRi - 2020-10-07
    • status: open --> closed
    • assigned_to: RBRi
     

Anonymous
Anonymous

Add attachments
Cancel





Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.