Consider making empty and attributeIf SVG safe

[lydell]

I know that this package is called HTML extra – not SVG extra – but unfortunately the Elm compiler allows using for example attributeIf on an SVG element, so it’s easy to make mistakes.

Here’s the problem:

attributeIf uses empty if the condition is False. empty is implemented as Html.Attributes.classList []. classList is implemented in terms of class, so Html.Attributes.classList [] is equivalent to Html.Attributes.class "". In other words, element.className = "" will be executed in JS. As documented that’s fine in practice.

Except in SVG. .className is a read-only property on SVG elements. Elm adds 'use strict'; to its JS output, and in strict mode trying to write to a read-only property throws an error.

Me and two colleagues recently spent two hours hunting down why Elm’s virtual DOM crashed. No external JS, no browser extensions, just Elm. After commenting out half of the code repeatedly, we found the culprit:

FeatherIcons.filter
    |> Icons.toHtml
        [ Html.Attributes.Extra.attributeIf (countFilters > 0) <| fill "currentColor"
        ]

attributeIf on an SVG element. Really hard to find since the code above is not even clear that there’s any SVG involved!

Worse, we didn’t notice this during development. We use parcel (v1), and it seems to remove the 'use strict'; so no error was thrown. Until we tried to deploy production.

So, what do you think? Is there a way we could try to make these functions less nuclear?

Requirements:

• empty should not cause unwanted effects
• empty should not cause performance problems

Ideas:

• empty = Html.Attributes.style "elmEmptyAttribute" "". After quick check it looks like it’s hard to tell that element.style.elmEmptyAttribute = "" has been called, which is good!
• empty = Html.Attributes.property "__elmEmptyAttribute" (Json.Encode.string ""). element.__elmEmptyAttribute will be readable afterwards, but does it matter? Can it affect performance?

Needs more investigation, but it could work!

Somewhat related: elm/html#141 and elm/html#232

[adamdicarlo]

This seems like a bigger issue... both elm/html and elm/svg use type aliases of a single Attribute type found in elm/virtual-dom. Long-term it seems like it should be fixed in all of these packages -- so, maybe file an issue for better attribute types in elm/virtual-dom?

[lydell]

Feel free to create such an issue, but shorter-term this issue is still useful!