Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use setProperty when setting style properties #9302

Merged
merged 5 commits into from
Apr 20, 2017

Conversation

aweary
Copy link
Contributor

@aweary aweary commented Mar 31, 2017

Work in progress. Resolves #6411

Using setProperty does incur the cost of parsing the style name as setProperty requires the hyphenated property name (background-color not backgroundColor). I'm also not sure if we want to special case IE given the performance benefits mentioned #6411 (comment)

cc @trueadm @sebmarkbage @spicyj

@sebmarkbage
Copy link
Collaborator

It seems like going down this route would inevitably involve a change from camelCase to hyphenation by convention. Probably including React Native then.

@trueadm
Copy link
Contributor

trueadm commented Mar 31, 2017

@sebmarkbage I'm a fan of dropping the camelCase conventions if we can going forward (we can transform them for initial releases with warnings) for RN too. Keeping them closer to true "CSS" helps reduce friction.

@gaearon
Copy link
Collaborator

gaearon commented Mar 31, 2017

It’s more awkward to type though unless you provide some compile-time helper. Also more awkward to transform in JS.

@trueadm
Copy link
Contributor

trueadm commented Mar 31, 2017

@gaearon We could do something nice at compile-time to help people here. We could also simply avoid passing an object literal too and let people pass in a block of cssText.

@aweary
Copy link
Contributor Author

aweary commented Mar 31, 2017

Personally, I prefer the camelCase convention as its consistent with the DOM prop APIs like className and htmlFor. If this change necessitates changing the style API I would be less enthusiastic about adopting it, I don't think the churn it would require in React styling community projects would be worth it.

@trueadm
Copy link
Contributor

trueadm commented Mar 31, 2017

@aweary In my opinion, this has always been one of the harder things for newcomers to understand, especially className vs class. It's led to countless bugs and confusion in the teams I've worked in when using React in big companies at scale. I'm mixed on this.

@milesj
Copy link
Contributor

milesj commented Apr 1, 2017

Why not do the name conversion at the JSX compiler level?

<span style={{ backgroundColor: 'red', 'border-color': 'blue' }}>
  Foo
</span>
React.createElement('span', {
  style: { 'background-color': 'red', 'border-color': 'blue' },
}, 'Foo');

@gaearon
Copy link
Collaborator

gaearon commented Apr 1, 2017

Because you can't know something is a style if it's passed around.

@aweary
Copy link
Contributor Author

aweary commented Apr 4, 2017

Can we consider accepting this without updating the public API for now? Assuming the cost of transforming the names is minimal (they're memoized so it should be in most cases). It would close #6411

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Apr 4, 2017

We need to benchmark this. I don't think we'll want to take a significant hit. The memoized forms still needs to be looked up in a map which is not free. If we want to support #6411 a safer bet might be to just check if it starts with --.

@aweary
Copy link
Contributor Author

aweary commented Apr 4, 2017

@sebmarkbage I'll benchmark before we move any further. setProperty can also perform better per discussion in #6411 so the cost of the map lookup might be offset. But we still might want to do env detection or just use style as setProperty is significantly slower in <IE11 and some others

@sophiebits
Copy link
Collaborator

I think for now it makes sense to call .setProperty for names starting with -- and leave everything else the same.

@aweary
Copy link
Contributor Author

aweary commented Apr 9, 2017

@spicyj @sebmarkbage I've updated it so that only properties starting with -- are set using setProperty. I also added a test ensuring it doesn't warn. I tried testing that the variable was actually being set correctly, but jsdom doesn't appear to fully support CSS variables yet.

@gaearon
Copy link
Collaborator

gaearon commented Apr 18, 2017

Have you verified this works in a browser?

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me but please verify it works in the browser.

@gaearon gaearon added this to the 15-hipri milestone Apr 18, 2017
@flarnie flarnie mentioned this pull request Apr 18, 2017
49 tasks
@aweary
Copy link
Contributor Author

aweary commented Apr 20, 2017

@gaearon I did, I tested in in Chrome, Firefox and Safari on macOS and both styles with CSS variables and styles without were set as expected. I also tested in IE9-11 and Android 4 and verified the styles that didn't use CSS variables were being set as expected.

@sophiebits
Copy link
Collaborator

great, feel free to merge :)

@aweary aweary merged commit 42bc28b into facebook:master Apr 20, 2017
@thysultan
Copy link

thysultan commented Apr 24, 2017

@aweary You could do the following to also support both camelCase and dash-cased styles.

var style = props.style;

for (...) {
    if (name in DOMStyleObject) {
        DOMStyleObject[name] = style[name];
    } else {
        DOMStyleObject.setProperty(name, style[name]);
    } 
}

Edit: though ^^ might not be the best for performance.

flarnie added a commit that referenced this pull request Jun 6, 2017
Just keeping things updated.

#9398
flarnie pushed a commit to flarnie/react that referenced this pull request Jun 7, 2017
* Use setProperty when setting style properties

setProperty is faster in all/most modern browsers. It also lets us support CSS variables.

* Only use setProperty when setting CSS variables

* Add test to ensure setting CSS variables do not warn

* Make this PR pretty again

* Run fiber test script
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants