fixed the issue that inline-styles updating could lose style properties

[linmic]

I've encountered an issue that when I tried to update the inline styles of a node, it could lose some of the style properties which may lead to unintentional layout broke.

Say I want to replace an inline-style:

{
    'background': 'url(http://abc.com/a.jpg) no-repeat center',
    'backgroundSize': 'cover',
    'backgroundColor': 'red'
}

with

{
    'background': 'url(http://abc.com/b.jpg) no-repeat center',
    'backgroundSize': 'cover',
    'backgroundColor': 'red'
}

on an onChange event. And the properties won't be completely replaced/copied during the process.

[sophiebits]

We won't take this as-is since setting all of the styles regardless of if they've changed will be bad for perf. It would probably be okay if we do this only in the case that the keys change, though it might be even better to do it only for shorthand properties (though I'm unsure if there's a way to do that without hardcoding them all). Maybe we could do something sneaky like looking for a prefix match.

[sophiebits]

(Could've sworn we had an issue for this but I can't find it.)

[zpao]

#2407 is the other issue.

[sophiebits]

I was probably thinking of #2013.

[linmic]

I've revised a bit to do "setting all styles regardless of the changes" to only the ones with shorthand properties. This might as well resolve #2013 and #2407.

[syranide]

@spicyj Even only testing for shorthands seems unreasonably expensive, consider {border: '...'}, any time it updates it has to test for (and possibly update) all 31 related border-properties! A subset of that is required when unsetting any of the related properties too... there's also the problem of {borderLeft: '...', border: '...'}, unless we make that illegal then any change to any border property must reapply all border properties.

IMHO, all things considered it's better to just disallow overlapping and warn in dev.

[sebmarkbage]

I agree with @syranide

[linmic]

I'd say the cases that overwriting shorthand properties are quite common while styling. Rather than disable overlapping I personally think it's better to just disable the shorthand properties and only allow the non-shorthand to be set and precisely written in doc.

[dheuermann]

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

background is NOT shorthand for backgroundSize.

[syranide]

background is NOT shorthand for backgroundSize.

Yes it is. https://developer.mozilla.org/en-US/docs/Web/CSS/background#Formal_syntax

[dheuermann]

Thank you for the correction. It looks like background IS shorthand for backgroundSize on newer browsers (IE 9+, Chrome 21+) but not on many browsers that are currently in use, including the mobile browser for which I am developing. That aside, the following:

background: "transparent url('image.png') no-repeat left top",
backgroundSize: "100% 100%",

Gets rendered as:

url(image.png) 0% 0% no-repeat transparent

I am able to work around it by specifying each of the background properties individually.

backgroundColor: "transparent",
backgroundImage: "url('image.png')",
backgroundRepeat: "no-repeat",
backgroundPosition: "left top",
backgroundSize: "100% 100%",

I am not sure why the properties cannot be rendered as specified. This is leading to extra output that is unnecessary.

[syranide]

@dheuermann It seems it behaves entirely as it should (it being part of background), it's just not serialized into the background property when read for some reason, but it is serialized into the property if backgroundSize is set first and then changed it seems, in chrome at least. It doesn't really matter to React though, but it's a really weird discovery you made...

[facebook-github-bot]

@linmic updated the pull request.

[linmic]

Sorry, I accidentally pushed a merge. Working on it. Please add the needs-revision tag back.

[gaearon]

Thank you for sending the PR! I’m closing it as it hasn’t been updated in a few months, and it seems to me from this and other discussions that the consensus is that we should warn on mixing shorthand with normal properties. I opened #6348 so we can all agree on the implementation strategy that wouldn’t compromise performance, and discuss what we learned from other libraries like Radium and React Native, before diving into a specific implementation. I’m sorry this pull request didn’t get through but I hope to hear your thoughts in the discussion in #6348. Cheers!