elm / virtual-dom

The foundation of HTML and SVG in Elm.
https://package.elm-lang.org/packages/elm/virtual-dom/latest
BSD 3-Clause "New" or "Revised" License
209 stars 80 forks source link

Allow CSS custom properties by using style.setProperty() #127

Open harrysarson opened 6 years ago

harrysarson commented 6 years ago

When using custom css properties the syntax style.property = 'value'does not work for custom css properties. Instead, using style.setProperty() allows custom css properties to be set.

I would like to argue that this is a bug fix rather than an enhancement as currently elm silently fails to add these CSS properties. I believe that, if this PR is not the way to go and elm does not want to support custom css properties, then it should throw an error rather than silently failing.

Documentation for style.setProperty(): https://developer.mozilla.org/en-US/docs/Web/API/CSSStyleDeclaration/setProperty

Fixes elm/html/issues/129

berenddeboer commented 5 years ago

Great, would love to have this.

tj commented 5 years ago

šŸ‘ this would be nice, elm-css and others are cool but I think writing regular CSS is easier, much more flexible, and more portable. Personally I like to keep styles outside of my components aside from "structural" styles as well.

For reference React allows as well: https://github.com/facebook/react/pull/9302/files#diff-61273f1ad4383d5a3a802ac95031ade4R221

evancz commented 4 years ago

The question in https://github.com/elm/html/issues/129 is about the fact that this seems to duplicate the existing ability to use variables. The first example given in the spec is:

:root {
  --main-color: #06c;
  --accent-color: #006;
}
/* The rest of the CSS file */
#foo h1 {
  color: var(--main-color);
}

You would probably then say h1 [ id "foo" ] [ text "header" ] in your code. Would the following code be equivalent though?

import Html exposing (..)
import Html.Attributes exposing (..)

mainColor = "#06c"
accentColor = "#006"

foo = h1 [ id "foo", style "color" mainColor ] [ text "header" ]

I think one of the weaknesses of HTML/CSS/JS is that they each have separate systems for modules and variables, and it would be a shame to duplicate that duplication in Elm.

What is the use case you have in mind that requires a second mechanism for variables?

berenddeboer commented 4 years ago

The use case for me is interfacing with existing CSS frameworks. From a pure Elm perspective I agree it duplicates functionality. But I think we will be using CSS frameworks for a long time. For example I use elm-mdc: we use the CSS from Google's framework, but have rewritten the JavaScript in Elm. The CSS in Google's Material Design for the Web is huge, and the amount of engineering and testing effort that goes into that is much more than the JavaScript side. Duplicating that in Elm does not seem to serve a purpose.

From a user perspective, silently failing, does not seem to be "by design", rather a side-effect of picking one way of setting a property instead of another.

I haven't looked at performance implications: if setProperty() is measurably slower, we can always call JavaScript from Elm for the cases where we need this. If setProperty() is faster, we should use it.

charukiewicz commented 4 years ago

Would the following code be equivalent though?

Setting an inline style is not equivalent to using CSS variables. Yes, the styles applied to the elements may be the same, but there are other trade offs beyond what the element looks like. Setting inline styles in Elm may give you the benefit of the compiler, but makes it impossible to use a secure Content-Security-Policy, since inline styles requires including style-src: unsafe-inline in your CSP.

Here's more info about Content Security Policy from Mozilla. They consider a CSP "mandatory for new website" and "recommended for existing websites". Let me quote their recommendation as well:

It is recommended to start with a reasonably locked down policy such as default-src 'none'; img-src 'self'; script-src 'self'; style-src 'self' and then add in sources as revealed during testing.

The site observatory.mozilla.org is used for grading your website's overall security configuration. It's impossible to even get a perfect score if you include any unsafe CSP rules.

harrysarson commented 4 years ago

As I said in the description:

I would like to argue that this is a bug fix rather than an enhancement as currently elm silently fails to add these CSS properties. I believe that, if this PR is not the way to go and elm does not want to support custom css properties, then it should throw an error rather than silently failing.

(Emphasis added).


I would completely understand if using css-variables in elm is considered "bad" and doing so causes an error. However, that elm silently fails in this case is definately a bug.

Lattyware commented 4 years ago

There is a discussion where I have listed my arguments in favour of supporting custom properties on the Elm discourse, and there I gave this example of interoperability with web components that is impossible without custom properties.

annaghi commented 4 years ago

Passing variables to CSS

This is another example of passing variables to CSS which I can share with limited access for now.

If I want to scroll horizontally in a grid table with sticky first column, I have to specify the width explicitly. This pen demonstrates the issue and the screencast below: https://codepen.io/annaghi/pen/oNgyLYw

codepen-Peek 2020-04-23 17-52

The following screencast shows the solution written in Elm, and you can see that I need to set the width based on columns count after reorganizing the table to guarantee the sticky first column while scrolling:

elm-Peek 2020-04-23 17-57

lydell commented 3 years ago

Working on a custom virtual dom implementation, I tried .setProperty() out since I think it makes sense. Turns out that change is accidentally backwards incompatible! I noticed that while testing my implementation at work.

A text input was missing its rounded corners with my implementation. Iā€™ve always written it like this (coming from a CSS background):

Html.Attributes.style "border-radius" "5px"

But itā€™s written like this at work (might be natural for someone who have done a lot of vanilla JS):

Html.Attributes.style "borderRadius" "5px"

Turns out doing element.style.borderRadius = "5px" and element.style["border-radius"] = "5px" is equivalent.

But element.style.setProperty("borderRadius", "5px") does not work! It mandates the ā€œCSS spellingā€ with the dash: element.style.setProperty("border-radius", "5px").

How unfortunate.

It might be fixable, though:

if (property in element.style) {
    element.style[property] = value;
} else {
    element.style.setProperty(property, value);
}
harrysarson commented 3 years ago

Ah man! That is so annoying! Javascript isba pita some times.

Thanks for spotting. I worry that the alternative might be slower (I guess up to two times?) as we have to interact with the DOM object twice. Maybe the overhead is lost in the noise though?

Herteby commented 2 years ago

Another workaround btw is to set the css variables like this:

attribute "style" "--color:blue"
ChristophP commented 2 years ago

@lydell @harrysarson is it necessary to check whether the property exists in the DOM or could be just check the property name instead?

if (property[0] === '-' && property[1] === '-') { // check if prop starts with --
        element.style.setProperty(property, value); 
} else {
    element.style[property] = value;
}

If so, that could help avoid touching the DOM twice.

lydell commented 2 years ago

@ChristophP You solution does not help with border-radius vs borderRadius, does it?

ChristophP commented 2 years ago

@lydell oh yeah, it does not. I was just looking at the multi DOM access parts. Guess I missed that it does the camel case / Kebap base checking as well.

lydell commented 2 years ago

Wait, your solution would work I think! (I was wrong.) Because it does the same thing as now for everything except properties starting with --. Sounds promising!

ChristophP commented 2 years ago

@lydell Uhhm, yeah I guess šŸ˜… You're right, if the prop doesn't start with -- everything would work the same.