Bug: styles object using css variables and both a shorthand and a specific property renders incorrectly

[joaomoleiro]

React does not produce the correct css inline styles when using css variables for both the shorthand property and another specific one (like padding and paddingRight).

The styles object:

{
  padding: "calc(var(--spacing) * 1)",
  paddingRight: "calc(var(--spacing) * 3)",
  paddingBottom: "calc(var(--spacing) * 4)"
};

produces the following styles:

and the following html:

<span style="padding-top: ; padding-right: calc(var(--spacing) * 3); padding-bottom: calc(var(--spacing) * 4); padding-left: ;">App</span>

even though the computed properties tab of the dev-tools appear to be correct and the padding is properly rendered in the screen:

If I remove the css-variable, everything works as expected.

React version: From v15.0.0 to 16.12.0

Note: Below v15.0.0 the styles are correctly produced:

<span style="padding:calc(var(--spacing) * 1);padding-right:calc(var(--spacing) * 3);padding-bottom:calc(var(--spacing) * 4);">App</span>

Steps To Reproduce

1. Add a style object to a component that has both a property shorthand and a specific one (like padding and paddingRight) and uses a css variable (like var(--spacing).
2. Render that component and inspect using dev-tools.

Link to code example: https://codesandbox.io/s/heuristic-wood-bjr1y

styles object:

{
  padding: "calc(var(--spacing) * 1)",
  paddingRight: "calc(var(--spacing) * 3)",
  paddingBottom: "calc(var(--spacing) * 4)"
};

The current behavior

React does not produces the correct css inline styles when using css variables for both the shorthand property and another specific one:

<span style="padding-top: ; padding-right: calc(var(--spacing) * 3); padding-bottom: calc(var(--spacing) * 4); padding-left: ;">App</span>

The expected behavior

Inline styles using css variables that have both a shorthand and a specific one should produce the correct styles.

<span style="padding: calc(var(--spacing) * 1); padding-right: calc(var(--spacing) * 3); padding-bottom: calc(var(--spacing) * 4);">App</span>

[bvaughn]

I reported this some time back (#8689) but it didn't get much traction at the time. The response I got was that supporting the expected behavior would be too costly, but maybe React could have a DEV warning.

[joaomoleiro]

In this case it seems that it works OK if I do not use a css variable.

This:

{
  padding: "calc(var(--spacing) * 1)",
  paddingRight: "calc(var(--spacing) * 3)",
  paddingBottom: "calc(var(--spacing) * 4)"
};

translates to:

<span style="padding-top: ; padding-right: calc(var(--spacing) * 3); padding-bottom: calc(var(--spacing) * 4); padding-left: ;">App</span>

But this:

{
  padding: "calc(10px * 1)",
  paddingRight: "calc(10px * 3)",
  paddingBottom: "calc(10px * 4)"
};

translates to:

<span style="padding: calc(10px) calc(30px) calc(40px) calc(10px);">App</span>

Nevertheless a DEV warning would be nice (if supporting the expected behavior is too costly).

[bvaughn]

I don't know the history of this particular part of ReactDOM, so I'm not sure what thought has gone into this before and if there are any good reasons for the current behavior (and lack of warning) other than "no one has got around to building it"

I agree though that a warning would be nice.

I'm going to tag it for discussion and see if others can share more context.

[bvaughn]

cc @syranide in case you can share any more background context.

[syranide]

@bvaughn Hey! I can't speak for the CSS-variable part of this question. But not supporting overlapping styles is simply put a performance/complexity problem. If React does not support overlapping styles it can naively iterate the prev/next and apply any changes as a property, very quick and simple. If overlapping styles are supported then React must have knowledge of how shorthand properties are decomposed and to be able to figure out whether a changed property shadows/overlaps another property and apply them in correct order. This is significantly slower, more complex and rather fragile. I'm not aware of any way of solving this issue without incurring significant overhead costs given the current set of features exposed by browsers, which depending on what kind of performance/use-case one expects out of React makes it unacceptable.

Since I'm not actively involved nowadays I can't speak for why there still isn't a warning, but I would positively think it's because "no one has got around to building it" as you say. The warning should be rather straightforward to implement for at least the current/common set of properties.

[syranide]

Just to illustrate; it used to be, and I assume it still is, that setting a shorthand property incurs the same cost as if you individually set all the properties it decomposes into. So if you have e.g. font + font-size and change either of them, it would effectively incur something like 1+7 property updates instead of just 1, on-top of that you have the additional non-trivial cost of having React check for the overlap. And it is even worse when it comes to e.g. border which decomposes in several steps.

It could be that given the improved state of browsers nowadays that the cost might be negligible when compared to the related rendering costs. But I haven't done any tests lately so I wouldn't know.

[bvaughn]

Understood RE: the performance benefits. Was more curious to know if the DEV warning had been investigated previously and determined to also be too expensive.

[esr360]

I've been facing this bug for the past few hours and finally found some relevant Github discussions. I have a system where styles get added dynamically from various sources, and can demonstrate a flow that shows the issue here: https://codesandbox.io/s/elated-flower-zxvkf?file=/src/App.js. I understand the performance benefits would be too costly but it's just a big shame because to me it seems like quite a big issue.

If I am ultimately rendering two divs with the same styles object passed to the style attribute of each div, regardless of any previously existing styles on the divs, I should expect to get the same result on both divs, imo.