Skip to content
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

Operators in "calc" not parsed correctly. #12

Open
pmwhite opened this issue Aug 31, 2021 · 2 comments
Open

Operators in "calc" not parsed correctly. #12

pmwhite opened this issue Aug 31, 2021 · 2 comments

Comments

@pmwhite
Copy link
Contributor

pmwhite commented Aug 31, 2021

The following is a test which demonstrates a bug in the parser:

let%expect_test _ =
  sexp_print {| div { height: calc(100vh - 20px); } |};
  [%expect
    {|
    ((Style_rule
      ((prelude ((Ident div)))
       (block
        ((Declaration
          ((name height)
           (value
            ((Function
              (calc
               ((Float_dimension (100 vh Length)) (Delim -)
                (Float_dimension (20 px Length)))))))
           (important false)))))))) |}];
  sexp_print {| div { height: calc(100vh-20px); } |};
  [%expect
    {|
    ((Style_rule
      ((prelude ((Ident div)))
       (block
        ((Declaration
          ((name height) (value ((Function (calc ((Dimension (100 vh-20px)))))))
           (important false)))))))) |}];

The two bugs demonstrated are:

  • the operator "-" is interpreted as a "Delim" rather than a "Operator" token, which means that it gets printed without any spaces between the parts of the expression, which is not valid CSS.
  • the parser does not reject the version without spaces but instead parses "vh-20px" as the unit of the dimension.

The first of these is far more important than the second. I took a bit of a stab at trying to fix the grammar/lexer, but I'm unfamiliar with the structure of the grammar, so I got lost. Any idea what the right way to solve this is?

@astrada astrada added bug Something isn't working and removed bug Something isn't working labels Sep 1, 2021
@astrada
Copy link
Owner

astrada commented Sep 1, 2021

Sorry, but how is sexp_print defined?

In my library there is only a parsing function, not a printing one. And the OPERATOR token is used to parse operators in selectors (~=, |=, ^=, $=, *=, ||). It's a trick I used to overcome my probably poor understanding of sedlex/menhir, I don't know if it's strictly needed, as it's not in the W3C standard (https://www.w3.org/TR/css-syntax-3/). In the standard everything is a DELIM token.

@pmwhite
Copy link
Contributor Author

pmwhite commented Sep 7, 2021

Ah, right. This test is in a library that basically copies all the types in ocaml-css-parser, and also adds [@@deriving sexp] them. It's probably not worth actually trying to run the above test - I only show it as a read-only illustration of the bug.

I don't have time right now to work on fixing this bug, and it's not super critical to me that it ever gets fixed, so I probably won't pursue finding a solution any further. It's up to you whether you want to close the issue, or just leave it open (it's still a bug, after all).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants