elm-community / typed-svg

Typed SVG library written for Elm
BSD 3-Clause "New" or "Revised" License
59 stars 16 forks source link

Cannot set property className of #<SVGElement> which has only a getter #52

Open gilesbowkett opened 2 years ago

gilesbowkett commented 2 years ago

I was able to cause a bug using TypedSvg with Html.Attributes.

in particular, using Html.Attributes.class on a TypedSvg SVG's shape elements went badly.

here's what it looked like:

Screen Shot 2022-11-28 at 7 23 32 PM

(additionally, it broke other functionality in my app.)

this code triggered it:

import Html.Attributes exposing (class)
import TypedSvg exposing (circle, path, svg) 
import TypedSvg.Attributes exposing (cx, cy, d, fill, r, viewBox)

svg [ viewBox 0 0 24 24 ] 
    [ circle [ cx (Px 12), cy (Px 12), r (Px 10), fill backgroundColor, class "primary" ] [] 
    , path 
        [ d data 
        , class "secondary"
        , fill foregroundColor
        ]    
        []   
    ]

removing the class attributes fixed the problem.

off the top of my head, I'm not sure what the solution should be for this bug. but I can see that TypedSvg.Attribute uses elm/virtual-dom attributes under the hood, which is probably why I'm able to throw incorrect attributes onto my SVGs here. so maybe there's a way to articulate within TypedSvg.Attribute which virtual DOM attributes can pass through and which ones need to be re-implemented or restricted in some way.

gilesbowkett commented 2 years ago

more context: based on some hints from Stack Overflow, I think HTML uses a className property under the hood, whereas SVG uses a proper class attribute. looks like a relic from the long and quirky evolution of JavaScript and the DOM.

rupertlssmith commented 2 years ago

Looking at the code, Html.Attribute.class uses:

https://github.com/elm/html/blob/1.0.0/src/Html/Attributes.elm#L162

stringProperty : String -> String -> Attribute msg
stringProperty key string =
  Elm.Kernel.VirtualDom.property key (Json.string string)

And TypedSvg.Attribute.class uses:

https://github.com/elm-community/typed-svg/blob/7.0.0/src/TypedSvg/Core.elm#L58

attribute : String -> String -> Attribute msg
attribute =
    VirtualDom.attribute

Both define Attribute as an alias onto VirtualDom.Attribute:

type alias Attribute msg = VirtualDom.Attribute msg

So long as TypedSvg is just a set of typed constructors built directly on top of VirtualDom, there isn't really a way to prevent this. TypedSvg would need some kind of intermediate representation to prevent you mixing in Html.Attribute stuff.

Its unfortunate this causes a runtime exception. The best way to fix this might be for VirtualDom to do better checking and ignore invalid attributes. This was also mentioned on the Elm discourse recently: https://discourse.elm-lang.org/t/valid-code-with-runtime-exception/8787

Workaround: User TypedSvg.Attribute.class and not Html.Attribute.class

rupertlssmith commented 2 years ago

The distinction between the two different ways of defining the class could also be mentioned in the docs and is something that might be worth adding to this package.