Fix styles to prevent subtle diffing bugs, add support for vendor prefixing #6701

jimfb · 2016-05-04T20:46:02Z

We have several minor/subtle issues with styles, all of which get knocked out by this PR.

For instance, initial render of a component should be identical to [initial render, different render, initial render again]. That is, the only thing that SHOULD matter is the most recent render. However, if with our old code, there are edge cases where this is untrue (eg. when specifying a general style and a more specific style). Example: http://jsfiddle.net/0861av29/ Historically, we've told people "don't do that", but that's not a good answer. For users, the old code made it hard to solve in general, because this situation often arrises when a parent clones a child and append an additional (more specific) style. This creates complex interactions between components that are difficult to debug. Solved by this PR.

Another big fix, this PR also enables vendor prefix for css values, which has been a popular request for a long time. Syntax looks like this:

var styles = {
  padding: 0,
  margin: 0,
  listStyle: 'none',
  display: ['-webkit-box', '-moz-box', '-ms-flexbox', '-webkit-flex'],
  WebkitFlexFlow: 'row wrap',
  justifyContent: 'space-around',
};

sophiebits · 2016-05-04T20:55:17Z

We've said in the past that we might want to support margin: [20, 0] which this would preclude.

jimfb · 2016-05-04T21:07:08Z

@spicyj If we wanted to do that, we could support both within React. Margin would just be a css style that is known to take multiple values (ie. disable vendor prefixing logic for those known styles). That is, if we do desire to support that behavior. So I'd say these approaches aren't mutually exclusive.

sophiebits · 2016-05-04T21:46:28Z

background is a property that would need both.

jimfb · 2016-05-04T21:52:53Z

Ok, well, as per discussion, it sounds like we'll go with something like React.multi(['-webkit-box', '-moz-box', '-ms-flexbox', '-webkit-flex']) which allows us to punt on the question of using an array as a value.

ghost · 2016-05-04T23:39:16Z

@jimfb updated the pull request.

dabbott · 2016-05-05T18:48:11Z

Could we consider a different name for React.CSS.multi? My concern with this one is it's overly general, so people will misuse.

E.g. The naming indicates I should write background: React.CSS.multi('url(...)', 'red') -- but that's the array use case that may be supported in the future: background: ['url(...)', 'red'].

If this method is specifically for CSS vendor prefixes, maybe something like React.CSS.vendorPrefix() would be more clear. People can still abuse it to accomplish the array use case, but it'll be clear that's not the intended usage.

zpao · 2016-05-05T22:14:39Z

We can always bikeshed on the name if we take this route. I also dislike multi and think we could make something clearer if we do this.

jimfb · 2016-05-05T22:16:21Z

@zpao proposal? I'm very open to ideas. I'm not attached to this notation/name, but I would like to find something that is acceptable to everyone so we can drive this to completion.

zpao · 2016-05-05T22:34:50Z

Haven't thought any more about a name beyond that multi seems overly vague.

Regardless of that, can we pull this apart so we can consider the 2 pieces of this distinctly.

use the style attribute instead of the CSSOM
newly proposed support for specifying multiple values that relies on 1

zpao · 2016-05-05T22:36:37Z

src/isomorphic/React.js

@@ -62,6 +63,10 @@ var React = {
    only: onlyChild,
  },

+  CSS: {
+    multi: CSSPropertyOperations.multi,
+  },


This would definitely be a ReactDOM API (nothing in isomorphic/ should require anything in renderers/)

I'd actually even argue that this shouldn't be a part of React's API and should live in a little helper/addon module somewhere on NPM.

gaearon · 2016-05-06T00:59:37Z

src/renderers/dom/shared/CSSPropertyOperations.js

@@ -236,6 +249,10 @@ var CSSPropertyOperations = {
    }
  },

+  multi: function() {
+    arguments.__isMultiCssValue = true;
+    return arguments;


Isn’t leaking arguments bad in general?
https://github.com/petkaantonov/bluebird/wiki/Optimization-killers#32-leaking-arguments

Is it worse than copying the values in to a new array? Happy to change it.

…fixing

ghost · 2016-05-06T03:35:30Z

@jimfb updated the pull request.

syranide · 2016-05-06T07:51:06Z

use the style attribute instead of the CSSOM

From my understanding we really really should benchmark real-life cases to see whether updating style entirely each time is realistic. It's handy, but from my earlier testing every expanded property being set comes with a fixed cost (this includes margin being 4 times as costly as marginTop) EDIT and setting cssText is near equivalent to setting each individually. For mostly static content this is probably largely irrelevant but if React is serious about animations then this could be a huge performance eater. It may be that real-life shows that few enough properties are ever being updated simultaneously so that it's not a problem, but that's not obvious to me.

gaearon · 2016-05-06T11:01:57Z

Maybe worth running some demos from @chenglou’s react-motion.

developit · 2016-05-09T10:12:13Z

Just an addition to @syranide's excellent point about the performance benefit of consolidating style changes into a single DOM operation: it's also worth noting that setting style.cssText is marginally faster than the style attribute (similar to the performance difference between DOM attributes vs properties):

Benchmark.js Test (toggles 4 property values)

syranide · 2016-05-09T11:28:53Z

@developit My point was the exact opposite, that cssText is significantly slower when you have many properties but only a few are actually dynamic/changing. It's imperative to check how performance changes with the number of properties when benchmarking this.

IMHO it's pointless to throw around raw numbers here. It's a question of whether or not performance is acceptable with cssText vs properties in real-life scenarios to maintain 60+ FPS, if it's 500 FPS vs 600 FPS then who cares. No doubt cssText will be faster for the initial frame (it scales linearly with number of properties but has less overhead), but setting via properties to be able to skip unchanged properties might be a big factor in being able to maintain fluid framerate for animations (it scales linearly only with the number of changed properties but has slightly more overhead). Put differently, it could be that cssText is 20% faster in the best-case, but where setting via properties is 1000% faster in the worst-case. If we want to push towards inline styles as well then there may be a huge advantage to setting via properties.

PS. Your benchmark fails to consider the cost of serializing the styles to supplied to cssText. Also, Object.assign(node.style, properties) closes the performance gap by quite a lot.

developit · 2016-05-09T11:47:53Z

Perhaps the better pairing is whichever more closely matches React's preferred format for the style attribute in JSX. If that's heading in the direction of objects by default, then I can see properties making more sense.

syranide · 2016-05-09T11:47:59Z

Let's do a math exercise. I'm getting ~45k/ops on IE11 according to the benchmark. 45k / 60 = 750 ops/frame, that's a big number, but it's not a huge number. Consider that you may have more complex styles, lots of elements animating (all the different parts of a button may animate, so a button is not "just one thing"), need some leeway to maintain fluid animation, cache-misses, real-life overheads and there being other significant costs besides just setting styles then it seems to me that achieving fluid 60 FPS is not trivial, performance is important!

So if we go with cssText then the initial frame may be slightly faster, I would sincerely estimate the real-life improvement to be less than 5% considering all the other costs of setting up the initial frame, add to this the fact that people are not super sensitive to initial rendering times. On the other hand, the cost for CSS animations may be 20%+ in not-at-all-unlikely scenarios.

IMHO React has wisely chosen the "best worst-case" route, it's not the fastest, but it's always fast enough. In this case that is setting via property considering the vast improvements in the worst-case, or so it seems to be at least.

developit · 2016-05-09T12:15:59Z

It's probably worth mentioning that the benchmark is toggling, so all those numbers should be adjusted by a factor of 2. Also, in Chrome I'm seeing ~200k ops/sec, which is over 3000 ops/frame.

In reality, I don't think there is much of an argument to be made for one approach over the other given that the benchmark I linked doesn't actually show the values for differing numbers of properties. It seems that it'd be worth testing the performance stratified by property count - if it turns out (as seems likely) that cssText is faster for large numbers of properties, wouldn't it be reasonable to switch between the two implementations depending on that factor?

Also, do you have an example showing that mutating cssText actually affects in-progress animations any differently than setting properties? Some quick testing seems to indicate there is no difference in chrome:
https://jsfiddle.net/developit/mftxdu44/

syranide · 2016-05-09T12:39:39Z

It's probably worth mentioning that the benchmark is toggling, so all those numbers should be adjusted by a factor of 2. Also, in Chrome I'm seeing ~200k ops/sec, which is over 3000 ops/frame.

Optimizing for the "best best-case" is not really relevant.

In reality, I don't think there is much of an argument to be made for one approach over the other given that the benchmark I linked doesn't actually show the values for differing numbers of properties. It seems that it'd be worth testing the performance stratified by property count would be useful - if it turns out (as seems likely) that cssText is faster for large numbers of properties, wouldn't it be reasonable to switch between the two implementations depending on that factor?

That's not what I said, the two have very difference performance characteristics and it does not come down to the number of properties.

Also, do you have an example showing that mutating cssText actually affects in-progress animations any differently than setting properties? Some quick testing seems to indicate there is no difference in chrome:

I never said that.

esbench broke it seems. Anyway, run the following bench to see what I mean. It shows 35k/ops for cssText and 130k/ops for properties. So to put it in perspective; if I get to choose between having 20% faster in the best-case and 350%+ slower in the worst-case or vice versa then I would optimize for the worst-case. EDIT: Yes it's an imperfect benchmark.

let node = document.createElement('div');
document.body.appendChild(node);

// hoist style definitions so we're not allocating in the tests
let styles = [
    'color:blue; margin:2px 3px; padding:5px; position:relative;',
    'color:red; margin:2px 3px; padding:5px; position:relative;',
    'color:blue; margin:2px 3px; padding:5px; position:relative;'
];
let stylesObj = [
    { color:'blue', margin:'2px 3px', padding:'5px', position:'relative' },
    { color:'red' },
    { color:'blue' }
];

function setViaCssText(css) {
    node.style.cssText = css;
}

function setViaPropertiesAssigned(properties) {
    Object.assign(node.style, properties);
}

setViaCssText(styles[0]);
setViaCssText(styles[1]);
setViaCssText(styles[2]);
setViaCssText(styles[1]);
setViaCssText(styles[2]);
setViaCssText(styles[1]);

setViaPropertiesAssigned(stylesObj[0]);
setViaPropertiesAssigned(stylesObj[1]);
setViaPropertiesAssigned(stylesObj[2]);
setViaPropertiesAssigned(stylesObj[1]);
setViaPropertiesAssigned(stylesObj[2]);
setViaPropertiesAssigned(stylesObj[1]);

kof · 2016-05-15T10:58:50Z

Didn't read everything said in this thread, in response to @spicyj

We've said in the past that we might want to support margin: [20, 0] which this would preclude.

I also want to add something like this to jss and it has already support for fallbacks from the very beginning. So I think a good way would be to do something like this

margin: {top: 20, right: 0}

It is more verbose but also easier to read and understand. Especially when it comes to complex css values where nobody really knows the order of those things.

React 15 has no way to handle fallback CSS values (for example, vendor prefixed 'display:flex' values) in inline styles. This patch drops all fallback values for inline styles at the cost of regressing browser support (those without standard flexbox support will not layout React Native components correctly). Fix #131

wollnyst · 2016-07-15T04:59:09Z

So what is the status here?

This is really a huge issue for everyone who is using inline-styles.

Any help needed?

sophiebits · 2016-07-15T23:42:13Z

We don't have enough agreement on the "multi" approach to move forward on it. It's probably not worth the complexity.

Setting styles always as an attribute is possible but serializing everything to strings is not the direction we want to move in the long term so it would probably be better to find a way to fix the subtle bugs while retaining the strategy of setting individual properties only when necessary.

wollnyst · 2016-07-16T15:32:29Z

Thanks for your response. Maybe there is problem that two issues are mixed up here:

Subtle bugs
Vendor prefixing

I agree that there might be a better solution for the subtle bugs as in the fiddle above. But for vendor prefixing I don’t see any other option then setting styles as an attribute. Obviously we can’t assign multiple values to style['display'].

So maybe it’s worth splitting these two issues. Sorry for focusing on the second one, I’m having a lot of pain with this.
As it’s mandatory to set styles as an attribute, if you want to have vendor-prefixing, we should let the user choose.
For instance, we could have a separate property like styleText which sets the styles as attribute. Or even more explicit: having the option as a separate prop (renderStyleAsAttribute).
This way we could make the decision in ReactDOMComponent and support both.

facebook-github-bot · 2016-12-29T01:37:37Z

Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours has expired.

Before we can review or merge your code, we need you to email cla@fb.com with your details so we can update your status.

nhunzaker · 2017-06-30T02:12:54Z

We don't have enough agreement on the "multi" approach to move forward on it. It's probably not worth the complexity.

@spicyj No news in nearly a year. Was any consensus achieved? Is there anything actionable, or should this be closed?

Otherwise, I'm thinking about closing this out. What do you think, @aweary? How do we usually handle long out of date PRs that aren't making headway?

dfrho · 2017-09-07T03:19:14Z

please don't close. It's the last mile in inline-styling.

sophiebits · 2017-09-07T03:29:02Z

I'm going to close this because it's unlikely we'll merge this exact code. It doesn't mean we won't fix the issue this is trying to fix.

The current status is:

I don't think we want to support display: ['-webkit-box', '-moz-box', '-ms-flexbox', '-webkit-flex'] with that syntax, at least. It would certainly require more conensus and discussion than we have right now.
As I said above: "Setting styles always as an attribute is possible but serializing everything to strings is not the direction we want to move in the long term so it would probably be better to find a way to fix the subtle bugs while retaining the strategy of setting individual properties only when necessary." So if someone wants to propose a PR that does that then we should consider it.

ghost added GH Review: review-needed CLA Signed labels May 4, 2016

jimfb force-pushed the css-styles-text branch 2 times, most recently from aa9a68f to c4463a1 Compare May 4, 2016 23:34

zpao reviewed May 5, 2016
View reviewed changes

jimfb force-pushed the css-styles-text branch 3 times, most recently from a8d7719 to 40c3842 Compare May 6, 2016 00:05

gaearon reviewed May 6, 2016
View reviewed changes

Fix styles to prevent subtle diffing bugs, add support for vendor pre…

2a2067b

…fixing

jimfb force-pushed the css-styles-text branch from 40c3842 to 2a2067b Compare May 6, 2016 03:28

alexlande mentioned this pull request May 10, 2016

Adding userAgent config to Radium makes it not rerender styles that need prefixing FormidableLabs/radium#602

Closed

jimfb mentioned this pull request May 11, 2016

Warn on inline style update with a bad value #6749

Closed

sophiebits mentioned this pull request May 12, 2016

Fallback values Khan/aphrodite#74

Closed

robinweser mentioned this pull request May 14, 2016

Box doesn't work with react 15.0.0 robinweser/react-layout-components#27

Closed

robinweser mentioned this pull request May 24, 2016

Problem with stepper styling mui/material-ui#4280

Closed

alexlande mentioned this pull request Jun 15, 2016

Radium & unexpected effects on layouts FormidableLabs/radium#722

Open

ghost added the CLA Signed label Jul 12, 2016

alexlande mentioned this pull request Jul 19, 2016

The Road to 1.0 FormidableLabs/radium#788

Open

sebmarkbage removed the GH Review: review-needed label Mar 1, 2017

sophiebits closed this Sep 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix styles to prevent subtle diffing bugs, add support for vendor prefixing #6701

Fix styles to prevent subtle diffing bugs, add support for vendor prefixing #6701

jimfb commented May 4, 2016

sophiebits commented May 4, 2016

jimfb commented May 4, 2016

sophiebits commented May 4, 2016

jimfb commented May 4, 2016

ghost commented May 4, 2016

dabbott commented May 5, 2016

zpao commented May 5, 2016

jimfb commented May 5, 2016

zpao commented May 5, 2016

zpao May 5, 2016

jimfb May 5, 2016

jimfb May 5, 2016

gaearon May 6, 2016

jimfb May 6, 2016

sophiebits May 6, 2016

jimfb May 6, 2016

ghost commented May 6, 2016

syranide commented May 6, 2016 •

edited

Loading

gaearon commented May 6, 2016

developit commented May 9, 2016

syranide commented May 9, 2016 •

edited

Loading

developit commented May 9, 2016

syranide commented May 9, 2016 •

edited

Loading

developit commented May 9, 2016 •

edited

Loading

syranide commented May 9, 2016 •

edited

Loading

kof commented May 15, 2016 •

edited

Loading

wollnyst commented Jul 15, 2016

sophiebits commented Jul 15, 2016

wollnyst commented Jul 16, 2016

facebook-github-bot commented Dec 29, 2016

nhunzaker commented Jun 30, 2017

dfrho commented Sep 7, 2017

sophiebits commented Sep 7, 2017

Fix styles to prevent subtle diffing bugs, add support for vendor prefixing #6701

Fix styles to prevent subtle diffing bugs, add support for vendor prefixing #6701

Conversation

jimfb commented May 4, 2016

sophiebits commented May 4, 2016

jimfb commented May 4, 2016

sophiebits commented May 4, 2016

jimfb commented May 4, 2016

ghost commented May 4, 2016

dabbott commented May 5, 2016

zpao commented May 5, 2016

jimfb commented May 5, 2016

zpao commented May 5, 2016

zpao May 5, 2016

Choose a reason for hiding this comment

jimfb May 5, 2016

Choose a reason for hiding this comment

jimfb May 5, 2016

Choose a reason for hiding this comment

gaearon May 6, 2016

Choose a reason for hiding this comment

jimfb May 6, 2016

Choose a reason for hiding this comment

sophiebits May 6, 2016

Choose a reason for hiding this comment

jimfb May 6, 2016

Choose a reason for hiding this comment

ghost commented May 6, 2016

syranide commented May 6, 2016 • edited Loading

gaearon commented May 6, 2016

developit commented May 9, 2016

syranide commented May 9, 2016 • edited Loading

developit commented May 9, 2016

syranide commented May 9, 2016 • edited Loading

developit commented May 9, 2016 • edited Loading

syranide commented May 9, 2016 • edited Loading

kof commented May 15, 2016 • edited Loading

wollnyst commented Jul 15, 2016

sophiebits commented Jul 15, 2016

wollnyst commented Jul 16, 2016

facebook-github-bot commented Dec 29, 2016

nhunzaker commented Jun 30, 2017

dfrho commented Sep 7, 2017

sophiebits commented Sep 7, 2017

syranide commented May 6, 2016 •

edited

Loading

syranide commented May 9, 2016 •

edited

Loading

syranide commented May 9, 2016 •

edited

Loading

developit commented May 9, 2016 •

edited

Loading

syranide commented May 9, 2016 •

edited

Loading

kof commented May 15, 2016 •

edited

Loading