elm / html

Use HTML in Elm!
https://package.elm-lang.org/packages/elm/html/latest/
BSD 3-Clause "New" or "Revised" License
394 stars 99 forks source link

Bug: CSS Custom Properties can't be set #177

Open hpate-omicron opened 6 years ago

hpate-omicron commented 6 years ago

CSS Variables cannot be used from Elm at the moment, style "--myvar" "value" is not applied to DOM.

Elm version: 0.19 Browsers: All

Here are some SSCEs, if CSS variables were applied you would see a 200x200 blue box in this Ellie: https://ellie-app.com/3fpSpnv8nyNa1

Instead, the box is empty image

Vanilla HTML + CSS example https://codepen.io/hpate-omicron/pen/BOZzgZ image

caniuse data: https://caniuse.com/#feat=css-variables W3 spec: https://www.w3.org/TR/css-variables/

brasilikum commented 6 years ago

Possible duplicate: https://github.com/elm/html/issues/129

hpate-omicron commented 6 years ago

To address some of the notes on the previous issue:

I'm not sure the exact purpose of CSS variables when you are working in a language that has variables. It seems like this is a 2nd way to do things we can already do differently, except now without any compiler help for types and typos.

I'm not against doing this particularly, but so far I have not seen any cases that definitively show that this is a good thing to have. I prefer to use GitHub for bugs, not feature requests, so I will close.

The main benefit is that the variables can be referenced from other CSS, for instance, I have some code to allow me to make boxes at different aspect ratios. Another use is in theming components.

@supports (--custom: property) {
  [style*="--aspect-ratio"]::before {
    content: "";
    float: left;
    height: 0;
    width: 1px;
    margin-left: -1px;
    padding-bottom: calc(100% / var(--aspect-ratio));
  }
  [style*="--aspect-ratio"]::after {
    content: "";
    display: table;
    clear: both;
  }
}

To the second point, I believe this to be a bug because it doesn't not support the spec, a feature request would be to add something not supported in the CSS spec.

hpate-omicron commented 6 years ago

Turns out there is already a PR that fixes this as well - https://github.com/elm/virtual-dom/pull/127

brasilikum commented 6 years ago

@hpate-omicron So do you think you can close this as discussion is going on elsewhere anyways?

hpate-omicron commented 6 years ago

This one should remain open, the previous issue was marked as a feature request and closed, and the pull request hasn't had any activity on it since it was opened, so this is still an open bug.

Lattyware commented 5 years ago

I'd like to note this is a really important bug to fix for my uses.

One of the core benefits of CSS is that style can be swapped out without changing the content - this means users can do things like having custom style-sheets. If you force the use of elm variables - that no longer works - the style is now embedded in the application and can't be changed in the same way.

If this feature is excluded, it basically means that Elm is dictating the use of inline styles, otherwise you have to refactor styles into elm code whenever you discover you need a variable. You then end up with duplication in your elm and CSS (e.g: if you have a media query that would interact with that variable, you now have to match that query with elm code to do the same thing, and keep it in sync).

harrysarson commented 5 years ago

You can make a port and call out to it when custom variables need updating. That is how I am working around the bug currently.

In js


  app.ports.setCssProp.subscribe(([selector, prop, value]) => {
    for (const $el of document.querySelectorAll(selector)) {
      $el.style.setProperty(prop, value);
    }
  });

In elm


update : Msg -> Model -> ( Model, Cmd Msg )
update msg model =
    case msg of
        ChangeStage change ->
            let
                newStage =
                    max 0 <| min Config.maxStage (model.stage + change)
            in
            { model | stage = newStage }
                |> Cmd.Extra.with
                    (setCssProp ( ".display-panel", "--show-stage", String.fromInt newStage ))
        ...
berenddeboer commented 5 years ago

Great workaround, but flabbergasted elm won't fix this.

Lattyware commented 5 years ago

As noted in https://github.com/Lattyware/elm-fontawesome-generator/issues/4 this is now a blocking issue for elm-fontawesome.

Custom properties can't simply be replaced with Elm code as suggested elsewhere. Given this seems like a small change that is already implemented, the cost of supporting this seems very low, and as far as I can see, is being held up based on misunderstanding more than anything else.

Furthermore, this is only going to get more important: future tools are being built on top of custom properties, and those will also break without them.

ChristophP commented 3 years ago

Yes, there are tons of libraries that are built on this the standard of CSS custom properties, such as this one. Each component can be customized with custom properties. https://shoelace.style/components/image-comparer?id=css-custom-properties

There are even new specs building on top of it such as the @property keyword in CSS. Example: https://css-tricks.com/using-property-for-css-custom-properties/ Spec: https://www.w3.org/TR/css-properties-values-api-1/#example-2

z5h commented 3 years ago

Turns out there is already a PR that fixes this as well - elm/virtual-dom#127

Seeing as there is a fix, can we apply some "persuasion" to get this applied? Perhaps a writeup on Discourse that would help people understand the problem?

Lattyware commented 3 years ago

Turns out there is already a PR that fixes this as well - elm/virtual-dom#127

Seeing as there is a fix, can we apply some "persuasion" to get this applied? Perhaps a writeup on Discourse that would help people understand the problem?

From April last year: https://discourse.elm-lang.org/t/css-custom-properties/5554

Radio silence after asking for the example. I lobbied for this one as hard as I could, but just no response. Evan seems to have decided that the language is frozen for now, so I wouldn't bother engaging until after things are moving again.

ChristophP commented 3 years ago

I remember back then that @Lattyware did a great job explaining why CSS custom properties should be supported and had lots of details in his response in discourse, which sadly never got any reactions from Evan.

However, I think the whole questioning whether it makes sense to support because elm already has variables is a bit like saying: Why should we support web components since you can already make components with Elm/React/Vue/Angular? The fact is that CSS Custom properties are a web standard that is seeing increasing usage and enables animating thins which weren't possible before in many areas including libs that Elm users may rely on.

So I think a better question than Should Elm support this? would be Will any priority be assigned to this given that it's an easy fix and it's impacting many users?

harrysarson commented 3 years ago

Are the any insurmountable issues with patching this in the compiled js?

elm make ... --output elm-needs-patching.js
sed s/domNodeStyle[key] = styles[key]/domNodeStyle.setProperty(key, styles[key])/ elm-needs-patching.js > elm.js
# minify etc

otherwise that should unblock this for you

ChristophP commented 3 years ago

@harrysarson I'm using a css class which sets whatever property needs to be set.

.color-red {
  color: var(--my-red); 
}

So I have a workaround for my use cases. I just see how much CSS vars are being used in all of the frontend world and I think we moved past the point of pondering whether this should be supported or not. To help the situation I think this issue should be addressed for Elm or if it's not an issue of whether this should be adopted it should be communicated that this simply is not a priority for the foreseeable future. I think that kind of communication would be fair to respect the efforts of @Lattyware and others who have tried to provide the information but never got a response.

SuPythony commented 2 years ago

Following on @harrysarson 's suggestion, I have created a batch script -

elm make %1 --optimize --output=unpatched.js
powershell -Command "(gc unpatched.js) -replace 'domNodeStyle\[key\] = styles\[key\]', 'domNodeStyle.setProperty(key, styles[key])' | Out-File -encoding ASCII main.js"

It can be used like this - compile.bat src/Main.elm

SuPythony commented 2 years ago

I am new to Elm and I can't understand why the pull request is not being merged. The solution for this problem is just to change that one line!

ChristophP commented 2 years ago

@SuPythony this is where the last conversation went silent https://discourse.elm-lang.org/t/css-custom-properties/5554. There seemed to be concerns about performance implications of making the change (elm is usually benchmarked pretty well for performance) as well as seemingly concerns about whether CSS vars is in conflict with other kinsd of module systems. IMO the web has moved on to fully embrace CSS custom properties and not supporting it at this point is a bummer for Elm users who want to use CSS vars or have to use them because of dependencies or component libs that rely on them. I wish had support for it without workarounds.

SuPythony commented 2 years ago

@ChristophP So does making this change make it slow? Also is there any other way to do it instead of just setting the style directly in elm? I was working with a stylesheet that was already made and wanted to change the css variables reactively using the model.

harrysarson commented 2 years ago

See my PR https://github.com/elm/virtual-dom/pull/127, there are back compat concerns so it cannot be merged as is. I don't think it would be impossible to resolve those though if someone wanted to try and drive this forward. Obviously involvement from maintainers is also needed for merge and this repo isn't under active dev at this point in time.

SuPythony commented 2 years ago

@harrysarson So for now, will changing the generated js like you said cause any problems or will it be fine?

ChristophP commented 2 years ago

@harrysarson ah I see, didn't know about those camel-case/kebap-case spelling issues. Yeah, that could lead to code breaking and would thus warrant a new major version of elm/html I guess. So handling this would require some more time and attention, it's just a bit frustrating that solving this issue is so low on the priority list since quite a large number of people are bugged by this.

Lattyware commented 2 years ago

Would having a distinct customProperty function solve this? It could require/add the -- and therefore only work for custom properties, and do the right thing, thus avoiding performance issues and breaking compatibility.

It does make these things more distinct, but I'm not sure that's really a problem?

SuPythony commented 2 years ago

@Lattyware That's a great idea!

ChristophP commented 2 years ago

@Lattyware I think that would be pretty nice and it be possible to add support without breaking changes. But I wonder if how well that fits into Elm's API design philosophy of the elm/html package, where everything basically mirrors HTML. One downside could be that one has to know about customProperty and could be suprised if style does not work as one would expect. Might make the design a little less coherent, wouldn't bother me personally though.

dboitnot commented 1 year ago

This got me too. I was able to work around it by using Html.Attribute.attribute:

    H.span
        [ A.attribute "style" ("font-variation-settings:" ++ fontVariationSettings attrs)
        , A.class "material-symbols-outlined"
        ]
        [ H.text name ]
ChristophP commented 1 year ago

Yeah, that works. One downside of this this way of adding Styles is that it doesn't stack, i.e. calling it multiple times will override Styles instead of combining them.

arowM commented 1 year ago

elm-mixin is another work around without the downside. The Mixin.style function can handle multiple style attributes well.

dwayne commented 1 year ago

In elm-2048 I used the following snippet,

H.node "style"
    []
    [ H.text <| ":root { --grid-tile-move-duration: " ++ String.fromFloat moveDuration ++ "ms;" ++ " }"
    ]

re: https://github.com/dwayne/elm-2048/blob/607a1219024d8422263ed3bec5bb1b4bdc040035/src/App/View/Grid.elm#L115-L118

to set the CSS custom property --grid-tile-move-duration.

re: https://github.com/dwayne/elm-2048/blob/607a1219024d8422263ed3bec5bb1b4bdc040035/sass/_config.scss#L20

mauroc8 commented 1 year ago

I have the same issue but a different use-case for CSS variables. I'll add here an example of what I would want to do hoping it helps:

.hover-background-color:hover {
  background-color: var(--hover-background-color);
}
hoverBackgroundColor : Color.Color -> List (Html.Attribute msg)
hoverBackgroundColor color =
  [ Html.Attributes.class "hover-background-color"
  , Html.Attributes.style "--hover-background-color" (Color.toCssString color)
  ]

I think CSS variables are really cool. We get to use CSS pseudo-classes with vainilla elm-html (without elm-css).

(The workaround, as mentioned, is to use elm-mixin)

dwayne commented 1 year ago

I've collected some notes on all the acceptable workarounds I've found.