ocaml-css-parser icon indicating copy to clipboard operation
ocaml-css-parser copied to clipboard

Operators in "calc" not parsed correctly.

Open pmwhite opened this issue 4 years ago • 2 comments

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?

pmwhite avatar Aug 31 '21 17:08 pmwhite

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.

astrada avatar Sep 01 '21 19:09 astrada

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).

pmwhite avatar Sep 07 '21 15:09 pmwhite