CSS backgroundSize property lost when re-rendering an image URL inside of an adjacent background property

[ggdouglas]

The CSS background property can shorthanded in many various ways. When inlining multiple styles including that property, React seems to optimize them on re-render.

For example, if we define a style like so:

var style = {
    background: `#ffffff url(${this.state.url})`,
    backgroundPosition: `${this.state.x}% ${this.state.y}%`,
    backgroundSize: 'cover'
};

The initial render returns a style in the form:
style="background:#ffffff url([URL]);background-position:[X]% [Y]%;background-size:cover;"

If we change the state (such as this.state.x) to cause a re-render, the style format is changed to:
style="background: url([URL]) [X']% [Y]% / cover rgb(255, 255, 255);"

Where this seems to break down is while changing the state of a URL inside of the background property while also defining a backgroundSize.

var style = {
    background: `#ffffff url(${url})`,
    backgroundSize: 'cover'
};

The initial render will return the expected style:
style="background:#ffffff url(http://i.imgur.com/aPd8qDo.png);background-size:cover;"

However, changing the URL results in rendering a style without background-size:
style="background: url([URL]) rgb(255, 255, 255);"

---

Example Demo

[zpao]

This is a known issue with CSS shorthand expansion. For now the recommendation is to list each explicit property and not rely on the shorthand. Usually these bugs only surface on update because initial render simply creates an inline style as a part of setting innerHTML. Updates work on individual properties so when you set background on the update we notice that you only changed background so only set that property, which since it's a shorthand, clears out the backgroundSize property.

See #4661 for more discussion.

[dheuermann]

background is shorthand for:
background-color
background-image
background-repeat
background-attachment
background-position

background is NOT shorthand for backgroundSize.

[jimfb]

@zpao @spicyj @syranide Could we solve all these correctness issues/concerns if we were to use cssText instead of setting the properties individually?

My understanding is that we've always claimed that diffing the css ourselves was faster than letting the browser do it, but Ben's perf test (http://jsperf.com/style-vs-csstext-vs-setattribute/8) from #929, on chrome, seems to indicate that cssText isn't actually any slower (in fact, maybe faster than diffing ourselves) and I think it would solve all these concerns in one fix. Thoughts?

[syranide]

@zpao @spicyj @syranide Could we solve all these correctness issues/concerns if we were to use cssText instead of setting the properties individually?

Yes, but performance-wise that is near identical to setting each property individual like we do now, except we can cheaply skip unchanged properties, so you definitely do not want to do that for this reason. I could be proven wrong here, but that's the last I remember when messing around with it.

[jimfb]

Right, so if performance is nearly identical but the 'correctness' is easier (since we punt to the browser), it seems like a pure win. We could still cheaply skip if all the properties are unchanged, which covers the common case.

Based on my reading: It seems to me that the performance difference of cssText vs. properties is a toss-up. Sometimes one wins, other times the other wins, it depends on the specifics of your microbenchmark. But overall, it's a tight race. However, there are a bunch of other (very measurable) disadvantages to manually diffing+setting css properties... things like these 'correctness issues', vendor prefixes, unnecessary code/cruft in the core, etc. I guess my intuition is that if performance is nearly identical, the other factors should be taken into account, making cssText seem like a pretty clear win. Anyway, maybe that's a debate for another thread/issue, it sounds like the answer to my initial question is "yes, it would fix the correctness issue/concern".

[syranide]

Right, so if performance is nearly identical but the 'correctness' is easier (since we punt to the browser), it seems like a pure win. We could still cheaply skip if all the properties are unchanged, which covers the common case.

With cssText you cannot skip properties, all properties must be present in the string. That's the problem.

[jimfb]

With cssText you cannot skip properties, all properties must be present in the string. That's the problem.

Yeah, that's true, cssText might loose that pathological case, but it wins in some other cases, so my argument is that it's a wash. Overall, it's not clear that one technique is faster than the other, so I don't think the performance argument holds water here (maybe I'm wrong).

I don't have the numbers, but if we are already updating a css property on that element (ie. repainting that element anyway), I suspect that the additional cost of parsing a couple additional properties out of a cssText really isn't a big deal, especially when the cost is offset by perf wins in other cases.

If it is in fact a wash, then I argue that cssText is better from a simplicity perspective, because then we aren't trying (and failing) to simulate behaviors that the browsers already have implemented/optimized.

[zpao]

Please move any discussion about cssTest to a new issue, it's not helpful to have it buried in the comments of another issue that is already closed.

[huoyingjianshen]

use code follow:

getStyle = () => {
    var style = {
                background: "url(xxxxxxxxxx") no-repeat center",
                backgroundSize: "100% 100%"
            }
    return style;
}
<div style={getStyle}></div>