Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for CSS variables in style attributes #6411

Closed
henrikhelmers opened this issue Apr 5, 2016 · 44 comments
Closed

Add support for CSS variables in style attributes #6411

henrikhelmers opened this issue Apr 5, 2016 · 44 comments

Comments

@henrikhelmers
Copy link

CSS variables is now supported in Chromium, which we use for rendering. They enable us to write cleaner, more flexible code. Sadly, I cannot seem to use them in React without resorting to various tricks. Ideally, I would like to be able to use them like <div style={{"--color": "hotpink"}} />, which would make the variable available inside the scope of the div.

I am able to add them using the following syntax <div style={{["--color"]: "hotpink"}} />, but then they aren't updated if I try assigning a new value—which ruins much of the point of using a variable.

I am able to add and remove them using ReactDOM and ReactDOM.findDOMNode(this).style.setProperty("--color", "hotpink"), but that gets it out of sync with the DOM updates, in addition to not being pretty.

If there are any questions on the usefulness of CSS variables I'll be more than happy to explain.

@gaearon
Copy link
Collaborator

gaearon commented Apr 5, 2016

What are your thoughts on using JavaScript for this? Conceptually React’s experimental-and-subject-to-change context feature allows you to define variables local to a subtree of components. Your parent might look like

class BlueBar extends Component {
  getChildContext() {
    return {
      color: '#f00'
    };
  }

  render() {
    return (
      <div>
        <Background>
          <NotificationBadge />
          <SearchBox />
        </Background>
      </div>
    );
  }
}
BlueBar.childContextTypes = {
  color: PropTypes.string.isRequired
};

and your child components could read it like this:

class Background extends Component {
  render() {
    return (
      <div style={{ backgroundColor: this.context.color }}>
        {this.props.children}
      </div>
    );
  }
}  
Background.contextTypes = {
  color: PropTypes.string.isRequired
};

I understand this is orthogonal to whether we want to support CSS variables, but it could be a good solution in the meantime.

@henrikhelmers
Copy link
Author

We have a large codebase(user interface for the Vivaldi browser) in Less. I have looked at some JavaScript-ways of managing our styles, but so far we've decided to not make the switch. Using context looks nice, and would solve some issues, but not all. Using CSS variables would let us simplify our existing Less codebase without a full rewrite.

@gaearon
Copy link
Collaborator

gaearon commented Apr 5, 2016

Thanks for sharing the context (pun totally intended).
Let’s wait for my DOM-savvy colleagues to reply now.
Have you had a chance to look at what doesn’t work now and what it would take to fix it?

@zpao
Copy link
Member

zpao commented Apr 5, 2016

As long as the spec is stable enough (haven't looked at it recently), it seems like this should be doable, but not sure at what cost. Basically we can use node.style.setProperty('--color', 'hotpink'). But if we need to look at every style property to see if the key starts with '--' then it could get costly.

I am able to add them using the following syntax <div style={{["--color"]: "hotpink"}} />, but then they aren't updated if I try assigning a new value—which ruins much of the point of using a variable.

Yea, that's a result of us using innerHTML to do the first render and then updating properties after that. This will stop working in v15, even for initial render, so heads up.

@henrikhelmers
Copy link
Author

We're in a bit of a unique position perhaps, since we only need to relate to Chromium. But support for CSS variables is maturing, according to http://caniuse.com/#feat=css-variables. I would not mind setting the variables differently from the rest of the inline style declaration, I would just love a way to do this with React.

@zpao
Copy link
Member

zpao commented Apr 5, 2016

I think the short-term plan for this is probably to just leave the status quo and not change what we do for styles. As we continue to explore js styles, I think we're likely to end up changing how we end up doing styles. !important is another feature which has the same problem where it would work for initial render when using innerHTML but wouldn't update and now doesn't work at all v15 for client rendered. That is also possible to set using a DOM API but would be expensive to look at every value, so we might want an alternative way to express that.

For now if you want to use CSS variables you'll want to just break out of the declarative approach and manage setting these yourself with the lifecycle hooks (eg set refs and then in componentDidMount componentDidUpdate, use the ref to call node.style.setProperty(...) directly)

@henrikhelmers
Copy link
Author

By using the lifecycle hooks correctly, I got it working. It does seem to cause trouble with using React style attributes, but setting style with JS instead worked fine.

@kevinSuttle
Copy link
Contributor

From what I understand, the trick is hooking the refs up so that the non-virtual DOM nodes will be known at the time the style is applied.

Relevant thread: postcss/postcss-custom-properties#1

@henrikhelmers
Copy link
Author

I've successfully converted most of our internal variables to CSS variables, using a wrapper to calculate them before they're applied. By giving the user control of 4 higher level colors, I can calculate relevant palettes. This allowed for theming our product (https://vivaldi.net/en-US/teamblog/121-snapshot-1-3-501-6-customizable-ui-themes).

I insert the variables at the top of each window, and can optionally override them further down the DOM tree. The top component is passed theme as a prop, and the the componentDidUpdate function looks like so:

componentDidUpdate: function(prevProps, prevState) {
  if (this.props.theme) {
    Object.keys(this.props.theme).forEach((key) => {
      const cssKey = `--${key}`;
      const cssValue = this.props.theme[key];

      document.body.style.removeProperty(cssKey);
      document.body.style.setProperty(cssKey, cssValue);
    });
  }
},

I found that setting the property would not replace the old one, so it needs to be removed first. The theme prop only contains the changed variables, keeping overhead down. The performance of the variables are good, and it has allowed us to remove a large amount of CSS.

@kevinSuttle
Copy link
Contributor

kevinSuttle commented Oct 10, 2016

Having the ability to have <style> tags inside a React component that respec CSS Custom Properties and @apply would be ideal. This is how the Shadow DOM spec currently handles it, and which Polymer 2.0 will support.

https://developers.google.com/web/fundamentals/getting-started/primers/shadowdom#styling
https://github.com/polymer/polymer/tree/2.0-preview#scoped-styling
https://github.com/webcomponents/shadycss

This would also solve the issues of

  1. missing out on @-selectors and pseudo-classes in JS.
  2. varying syntax and casing between CSS and JS

I've looked through the issues here and have seen a bunch of related issues, but the latest Web Components spec has since been updated.

What it ends up coming down to is dangerouslySetInnerHTML.

@kevinSuttle
Copy link
Contributor

Perhaps sanitation via ES2015 tagged template strings could work for parsing styles. A bit more elegant than say, XML CDATA escapes.

@jide
Copy link

jide commented Dec 26, 2016

I did this, which can be useful until this is fixed : https://github.com/jide/react-css-variables

@JawsomeJason
Copy link

Could still really use this. @jide's solution is great for most cases, but my case is for a Modal that is injected outside of the app.

@davidkpiano
Copy link
Contributor

davidkpiano commented Mar 30, 2017

For reference, here's a small yet non-trivial use-case.

Let's say you want to toggle between light and dark themes for any text nodes in a certain element:

const LightDarkSpan = ({ children }) => (
  <span style={{
    color: 'var(--text-color, black)'
  }}>
    {children}
  </span>
);

Then, any parent can set that variable, and it effectively acts as context to any child element that explicitly chooses to use that specific CSS variable:

// rendered
<div style={{
  backgroundColor: 'black',
  '--text-color': 'white'
}}>
  <article>
    <LightDarkSpan>testing</LightDarkSpan>
  </article>
</div>

No extra JS indirection needed 👍

cc. @spicyj

@sophiebits
Copy link
Collaborator

sophiebits commented Mar 30, 2017

This seems to me like something we may as well support unless @sebmarkbage / @gaearon have an objection. It sounds like it would be only an additional two lines of code in our style logic.

@gaearon
Copy link
Collaborator

gaearon commented Mar 30, 2017

👍

@sebmarkbage
Copy link
Collaborator

I think we'd want to special case the way we set it based on the key. So we'd have to materialize, compare and inspect the key string. (Un)fortunately we already do that with floats (which already makes everything slower). I guess it's fine. It'll always be a slow-path. If you want to be on the slow-path, sure.

@sebmarkbage
Copy link
Collaborator

Curious if @trueadm has done any benchmarking on style[name] = value; vs. style.setProperty(name, value);

@davidkpiano
Copy link
Contributor

... any benchmarking on style[name] = value; vs. style.setProperty(name, value);

Some relevant benchmarking info done here: https://jsperf.com/dom-style-vs-setproperty

Source: https://github.com/elm-lang/virtual-dom/pull/44

It seems to be faster in some environments.

@aweary
Copy link
Contributor

aweary commented Mar 31, 2017

I ran that benchmark in IE9-11 and in all cases setProperty was slower. Though we only need setProperty for browsers that support CSS variables anyways, so we could always do environment detection for IE like we do with DOMLazyTree and use style[name] = value in IE/Edge

@sebmarkbage
Copy link
Collaborator

Looks like CSS Typed OM is all about those (well typed, lol) strings too so seems pretty future proof direction.

@trueadm
Copy link
Contributor

trueadm commented Mar 31, 2017

setProperty has always been fastest and this is what we should use. I had a discussion a while back with some guys at Mozilla at JSConf about some optimizations that they added to setProperty that, at the time, they couldn't add to the property accessing route due to legacy web reasons or something.

Furthermore, in regards to Edge/MS, they should be also applying the fast path optimization for setProperty just as other browsers are doing. They're behind in a bunch of web related quirks and I'm sure if we can nudge them, they can apply the same fast paths in this scenario as they are doing so in others.

@pixelass
Copy link

I created this testcase comparing setProperty and style setter with custom and native properties.

Strangely setProperty is faster when using custom properties (Chrome 57.x) which obviously makes me happy.

https://jsperf.com/style-setter-vs-setproperty

jsperf

I looked left and right to check if I messed anything up in the test because the difference in speed is so immense. Feedback welcome.

@LucaColonnello
Copy link

I looked into it and I reckon there's still an issue with this.
These variables are all scoped but not isolated.

So if the parent set a variable that one child want to use is fine.

But if that child imports a react component that is using the same variable name, that would be affected as well.

Web Components solve this issue cause they are isolated by default...

Is there a way we can get React to create custom elements if needed?

@LucaColonnello
Copy link

That should solve isolation for styles...

@davidkpiano
Copy link
Contributor

davidkpiano commented Apr 7, 2017

But if that child imports a react component that is using the same variable name, that would be affected as well.

This is no different than name collisions on contextTypes. That is, in the above example, if you have an imported React component that provides the same this.context.color, you'll have the same overwriting issue.

It's also not hard to namespace or obfuscate (a la CSS Modules) CSS variables. It's just a string, after all, e.g.: '--button-color-abc123': 'green'.

@LucaColonnello
Copy link

Maybe obfuscate could be a way, but is not really standard...

@pixelass
Copy link

pixelass commented Apr 8, 2017

@LucaColonnello This is just the way it works. It's the same issue with components and classnames.
If you don't make sure your className is obfuscated it could suffer from other components that use the same classNames. But that's no reason to not enable a webstandard in React.

but is not really standard...

Wait what? Where did you read that?

@LucaColonnello
Copy link

The standard is now about using web components in order to have an isolated and scoped style using :host.

If I'm going to obfuscate the variables my component is using, it would be hard to change from outside the variable value when my component is packed and compiled into node modules.

Obfuscation is a good technique we use for example with css-modules, but the standard is coming with a build in solution foe conflict that is scoped CSS.

I'm trying to say that a solution already exists in the browser, although we can do what we prefer and what's more sensitive for us.

Correct me if I'm wrong anyway :)

@pixelass
Copy link

pixelass commented Apr 8, 2017

I just don't think this is related to the actual feature request to simply allow the usage.

The request is a feature. You are talking about scoped styles for webcomponents.

@LucaColonnello
Copy link

Yeah yeah that's true. This was to just to say that we have not everything we need for CSS variables also with this feature.

But is a feature I'd like to have anyway...

@FezVrasta
Copy link

I'm trying this but the CSS Custom prop doesn't get applied?

https://codepen.io/FezVrasta/pen/LLPpGe

@TrySound
Copy link
Contributor

TrySound commented Jun 1, 2017

It wasn't released yet

@pixelass
Copy link

pixelass commented Jun 1, 2017

15.5.4 (April 11, 2017)

The PR was merged on April, 20

https://github.com/facebook/react/releases

@JawsomeJason
Copy link

@FezVrasta I forked your pen to 15.5.4: https://codepen.io/thejase/pen/owvQNe

I'm still not seeing the style being applied to the div
image

@pixelass
Copy link

pixelass commented Jun 2, 2017

@TheJase See my comment. 15.5.4 was released 0n April 11th. The merge was done 9 days later.

@JawsomeJason
Copy link

JawsomeJason commented Jun 2, 2017

@pixelass Ah, now I understand. Sorry, wasn't thinking straight. Nonetheless, I had to correct a selector on the previous fork (#root would never see the CSS Property value anyway).

@DanielRamosAcosta
Copy link

So we have to wait until the next release right?

@sophiebits
Copy link
Collaborator

sophiebits commented Jun 5, 2017

This is in 15.6.0-rc.1 (release candidate on npm) and will be in 15.6 final.

@DanielRamosAcosta
Copy link

Thanks @spicyj!

@DanielRamosAcosta
Copy link

I'm trying to use the variables in a boolean way to change some things in CSS:

.foo {
  height: calc(46px * var(--open))
}
.bar {
  transform: translateY(calc(46px * (1 - var(--open))))
}

The ideal way would be to set style={{'--open': this.state.open}}... But my workaround doesn't work either: style={{'--open': this.state.open * 1}}. This sets open to 1px or 0px, instead to 1 or 0.

I manage to do it with:

<div style={{'--open': `calc(${this.state.open * 1})`}}>
...
</div>

Is there a way to disable units in from inline styles?

@FezVrasta
Copy link

I think you can pass them as string

@DanielRamosAcosta
Copy link

@FezVrasta doesn't work, but I get the warning:

Warning: a `div` tag (owner: `TimeBar`) was passed a numeric string value for CSS property `--open` (value: `1`) which will be treated as a unitless number in a future version of React.

So I think I will have to wait 😋

@goornaz
Copy link

goornaz commented Sep 29, 2017

Hi guys, sorry but i have a problem. I'm trying to make my application with 2 themes but without fortune.

This is the method i call in componentDidMount of my application component.

loadTheme = () => { const selectedTheme = configuration.appTheme === 'dark' ? theme.dark : theme.light; selectedTheme.map(value => { document.body.style.removeProperty(value.prop); document.body.style.setProperty(value.prop, value.value, 'important'); }); };

Where value.prop === '--color-01' and value.value(i will change) is #ff0000.
But nothings happens.

This is my default theme variables:

`:root {
/* -------- COLORS -------- */

--color-01: #192635;
--color-02: #f8f8f8;
--color-03: #203141;
--color-04: #24364a;
--color-05: #abb4c0;
--color-06: #3fa2f7;
--color-07: #23cf5f;
--color-08: #ff5900;
--color-09: #00ff7e;
--color-10: #00e8ff;
--color-11: #ffcb00;

}`

Thanks in advance and have a nice day.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests