Bug: CSS variables can't be a space character

[itsdouges]

React version: v16/v17

Steps To Reproduce

1. https://codesandbox.io/s/empty-css-value-jmbfl?file=/src/App.js
2. Notice the "should be black border" has a pink border
3. Dangerously setting inner HTML works because it doesn't go through reacts trim() flow
4. I've tracked the suspect code to here https://github.com/facebook/react/blob/6cbb9394d1474e3a728b49dc7f3a11d61a421ce3/packages/react-dom/src/shared/dangerousStyleValue.js#L44

Link to code example: https://codesandbox.io/s/empty-css-value-jmbfl?file=/src/App.js

The current behavior

CSS variables have their value trimmed - resulting in ' ' become '' which then removes the CSS variable from the browser.

The expected behavior

The ' ' value should not be trimmed.

One potential option is to, for any custom properties that have a space value, don't trim.

if (isCustomProperty && value === ' ') {
   return value;
}

return ('' + value).trim();

I'm happy to do the bug fix. I would also love for this to get released on v16.

---

Found a work around:

https://codesandbox.io/s/empty-css-value-forked-g5jud?file=/src/App.js

style={{ "--border-color": "var(--, )" }}

[ctjlewis]

My thoughts are that style values which, when trimmed, become empty strings, should be returned unaltered, i.e.:

if (value.toString().trim() === ‘’) return value;

This would handle the space, series of spaces, and also the case where an empty string is supplied.

[itsdouges]

We've found a work around: https://codesandbox.io/s/empty-css-value-forked-g5jud?file=/src/App.js

style={{ "--border-color": "var(--, )" }}

Not the most ideal - but it works 😄.

[eps1lon]

Interestingly, trimming was intentionally added and considered "correct" (see commit message): https://github.com/facebook/react/commit/2bff5c5c7e776a183ed9b429f2cb46cc044d9bc1.

Considering that whitespace is a valid token for custom properties (2. Defining Custom Properties: the --* family of properties -> 8.2. Defining Arbitrary Contents: the and productions), it doesn't seem like trimming is correct.

Maybe it was added to fix a browser quirk? Would be interesting to see if the described quirk still happens in React 17 without trimming the values.

[itsdouges]

I can't replicate the suffix being added (both v16 + v17 behave the same - see: https://codesandbox.io/s/solitary-pond-v0i69?file=/src/App.js) - I wonder what use case it's wanting to handle?

[eps1lon]

I can't replicate the suffix being added (both v16 + v17 behave the same - see: codesandbox.io/s/solitary-pond-v0i69?file=/src/App.js) - I wonder what use case it's wanting to handle?

Maybe this changed over time. Unitless values as strings aren't working at all on the client. For example:

https://github.com/facebook/react/blob/6cbb9394d1474e3a728b49dc7f3a11d61a421ce3/packages/react-dom/src/tests/CSSPropertyOperations-test.js#L29-L38

right:4 is created on the server but has no effect on the client. It doesn't matter whether the value is trimmed or not.

Have you tried removing the trimming behavior and see what test fails?

Edit: related spec sections: left property definition allows <length> as a value. However, <length> requires a unit unless it is 0. So https://github.com/facebook/react/blob/6cbb9394d1474e3a728b49dc7f3a11d61a421ce3/packages/react-dom/src/tests/CSSPropertyOperations-test.js#L29-L38 is not testing behavior that is relevant to browsers (though there's always the possibility react supports some browser quirk).

[itsdouges]

Ok checking the code out and removing the trim I get only that test you've mentioned failing:

Summary of all failing tests
 FAIL  packages/react-dom/src/__tests__/CSSPropertyOperations-test.js
  ● CSSPropertyOperations › should trim values

    expect(received).toContain(expected) // indexOf

    Expected substring: "\"left:16;opacity:0.5;right:4\""
    Received string:    "<div style=\"left:16 ;opacity:0.5;right: 4 \" data-reactroot=\"\"></div>"

      35 |     const div = <div style={styles} />;
      36 |     const html = ReactDOMServer.renderToString(div);
    > 37 |     expect(html).toContain('"left:16;opacity:0.5;right:4"');
         |                  ^
      38 |   });
      39 |
      40 |   it('should not append `px` to styles that might need a number', () => {

      at Object.<anonymous> (packages/react-dom/src/__tests__/CSSPropertyOperations-test.js:37:18)


Test Suites: 1 failed, 257 passed, 258 total
Tests:       1 failed, 26 skipped, 7009 passed, 7036 total
Snapshots:   83 passed, 83 total
Time:        165.044s
Ran all test suites.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
Command failed: /usr/local/Cellar/yvm/3.3.0/versions/v1.22.4/bin/yarn.js test
➜  react git:(master) ✗

I have to wonder if we can just skip the trim if the value is a space. Or whitespace.

[eps1lon]

As far as I know this test isn't testing anything meaningful for browsers since right:4 and left:16 are invalid i.e. won't have any effect.

I would replace the unitless numbers with a zero (e.g. left: ' 0 ') and test the generated markup in a browser

[itsdouges]

0 still gets applied with extra white space (heres just a quick vanilla sandbox)

https://codesandbox.io/s/naughty-star-y43ey?file=/index.html

do we have enough to go on? what do we want for more evidence or investigation 😄

[eps1lon]

what do we want for more evidence or investigation 😄

I guess propose the change via PR and then we'll see what core members think about this change.

[subhamdas461]

style={{ "--border-color": "black" }}, also solves the problem

[itsdouges]

In this instance yes, however the behaviour wanted is to actually "unset" the CSS variable, having the border change to black is merely the effect of unsetting.

[hamlim]

Ran into a permutation of this issue this week, in our case we were setting the value of a custom property to an empty string:

<div
  style={{
    '--foo': ''
  }}
/>

When server side rendering, it would be stripped so the returned html to the client would look like:

<div></div>

and then cause a client server mismatch because on the client it was being respected as a value, expecting the following html:

<div style="--foo:;"></div>

We were able to workaround this issue by setting the value of the variable to '0' in our case, there is also some client side code (dependent upon useState/useEffect/etc) that updated the value of the custom property later during rendering, so the default of 0 worked for us.

Looking more at the code, which I think moved to here: https://github.com/facebook/react/blob/2cf4352e1c81a5b8c3528519a128c20e8e65531d/packages/react-dom-bindings/src/shared/dangerousStyleValue.js, I wonder if in all cases of the key being a custom property, that the code should let the value pass through as-is, without trimming it/changing it.

In our case, we were running into this branch specifically: https://github.com/facebook/react/blob/2cf4352e1c81a5b8c3528519a128c20e8e65531d/packages/react-dom-bindings/src/shared/dangerousStyleValue.js#L31-L34

So I think the ideal change would look something like:

function dangerousStyleValue(name, value, isCustomProperty) {
  if (isCustomProperty) {
    // I assume we'd want to preserve this check as well, but haven't dug into the code much to see if its necessary
    if (__DEV__) {
      checkCSSPropertyStringCoercion(value, name);
    }
    return value;
  }
  // ... current code here

@madou have you prepared a PR for any of the proposed changes at all? Curious if my snippet above would also work for your case too!