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.

Sign up for GitHub

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

Jump to bottom

Fix #1357. Don't append "px" suffix to CSS string values. #5140

Merged
merged 1 commit into from
Oct 14, 2015
+77 −4
Merged

Fix #1357. Don't append "px" suffix to CSS string values. #5140

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/renderers/dom/shared/CSSPropertyOperations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,9 +125,10 @@ var CSSPropertyOperations = {
* The result should be HTML-escaped before insertion into the DOM.
*
* @param {object} styles
* @param {ReactDOMComponent} component
* @return {?string}
*/
createMarkupForStyles: function(styles) {
createMarkupForStyles: function(styles, component) {
var serialized = '';
for (var styleName in styles) {
if (!styles.hasOwnProperty(styleName)) {
Expand All @@ -139,7 +140,7 @@ var CSSPropertyOperations = {
}
if (styleValue != null) {
serialized += processStyleName(styleName) + ':';
serialized += dangerousStyleValue(styleName, styleValue) + ';';
serialized += dangerousStyleValue(styleName, styleValue, component) + ';';
}
}
return serialized || null;
Expand Down
2 changes: 1 addition & 1 deletion src/renderers/dom/shared/ReactDOMComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -714,7 +714,7 @@ ReactDOMComponent.Mixin = {
}
propValue = this._previousStyleCopy = assign({}, props.style);
}
propValue = CSSPropertyOperations.createMarkupForStyles(propValue);
propValue = CSSPropertyOperations.createMarkupForStyles(propValue, this);
}
var markup = null;
if (this._tag != null && isCustomComponent(this._tag, props)) {
Expand Down
40 changes: 40 additions & 0 deletions src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,46 @@ describe('ReactDOMComponent', function() {
expect(console.error.argsForCall.length).toBe(2);
});

it('should warn about styles with numeric string values for non-unitless properties', function() {
spyOn(console, 'error');

var div = document.createElement('div');
var One = React.createClass({
render: function() {
return this.props.inline ? <span style={{fontSize: '1'}} /> : <div style={{fontSize: '1'}} />;
},
});
var Two = React.createClass({
render: function() {
return <div style={{fontSize: '1'}} />;
},
});
ReactDOM.render(<One inline={false} />, div);
expect(console.error.calls.length).toBe(1);
expect(console.error.argsForCall[0][0]).toBe(
'Warning: a `div` tag (owner: `One`) was passed a numeric string value ' +
'for CSS property `fontSize` (value: `1`) which will be treated ' +
'as a unitless number in a future version of React.'
);

// Don't warn again for the same component
ReactDOM.render(<One inline={true} />, div);
expect(console.error.calls.length).toBe(1);

// Do warn for different components
ReactDOM.render(<Two />, div);
expect(console.error.calls.length).toBe(2);
expect(console.error.argsForCall[1][0]).toBe(
'Warning: a `div` tag (owner: `Two`) was passed a numeric string value ' +
'for CSS property `fontSize` (value: `1`) which will be treated ' +
'as a unitless number in a future version of React.'
);

// Really don't warn again for the same component
ReactDOM.render(<One inline={true} />, div);
expect(console.error.calls.length).toBe(2);
});

it('should warn semi-nicely about NaN in style', function() {
spyOn(console, 'error');

Expand Down
34 changes: 33 additions & 1 deletion src/renderers/dom/shared/dangerousStyleValue.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,10 @@
'use strict';

var CSSProperty = require('CSSProperty');
var warning = require('warning');

var isUnitlessNumber = CSSProperty.isUnitlessNumber;
var styleWarnings = {};

/**
* Convert a value into the proper css writable value. The style name `name`
Expand All @@ -23,9 +25,10 @@ var isUnitlessNumber = CSSProperty.isUnitlessNumber;
*
* @param {string} name CSS property name such as `topMargin`.
* @param {*} value CSS property value such as `10px`.
* @param {ReactDOMComponent} component
* @return {string} Normalized style value with dimensions applied.
*/
function dangerousStyleValue(name, value) {
function dangerousStyleValue(name, value, component) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably take in a component name, since some components will be stateless and won't have instances but should still have names (I think).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would I need to change to do that? Just pass in component._currentElement._owner.getName() directly?

Copy link
Contributor

Choose a reason for hiding this comment

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

Eeeh, now I'm having mixed feelings about this one. If we're going to make a change, we might as well add the name of the current component in addition to the owner component (to provide more context). So directly passing in something like component._currentElement._owner.getName() and component.._tag. We could do that, but it also means wiring it through createMarkupForStyles and I'm not sure it's worth it at this point.

In a perfect world, we would have the full path, which would require merging #5167 first, but we need to decide on the fate of that PR before we can merge it, and it might not be worth delaying this PR even longer.

I'm tempted to say that this is fine for now, and we can make it better after #5167. Unless @spicyj wants it changed now (or unless we merge #5167 real fast), my intuition is that maybe you just ignore this comment and we deal with stateless components and/or giving the full parent path when those things actually happen.

// Note that we've removed escapeTextForBrowser() calls here since the
// whole string will be escaped when the attribute is injected into
// the markup. If you provide unsafe user data here they can inject
Expand All @@ -48,6 +51,35 @@ function dangerousStyleValue(name, value) {
}

if (typeof value === 'string') {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this check is enough; we should not fire the warning if the string already contains the 'px' at the end. Or actually, it might not always be px (could be 'em' or something). We should only print the warning if the string value can be parsed as an integer.

if (__DEV__) {
if (component) {
var owner = component._currentElement._owner;
var ownerName = owner ? owner.getName() : null;
if (ownerName && !styleWarnings[ownerName]) {
styleWarnings[ownerName] = {};
}
var warned = false;
if (ownerName) {
var warnings = styleWarnings[ownerName];
warned = warnings[name];
if (!warned) {
warnings[name] = true;
}
}
if (!warned) {
warning(
false,
'a `%s` tag (owner: `%s`) was passed a numeric string value ' +
'for CSS property `%s` (value: `%s`) which will be treated ' +
'as a unitless number in a future version of React.',
component._currentElement.type,
ownerName || 'unknown',
name,
value
);
}
}
}
value = value.trim();
}
return value + 'px';
Copy link
Contributor

Choose a reason for hiding this comment

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

We should only append 'px' if the value can be parsed as an integer. If they specify units, we should honor those units to avoid printing '5pxpx' or '5empx' which would make no sense. We can make that change in 0.15 (ie. now), since it should break any old code since presumably no one is doing that yet (because the old code would break).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That behaviour hasn't actually changed. Otherwise the tests would be falling, anyway. If you look at my first commit you'll see that the only actual change in logic is that the reassignment has been replaced with a return statement. This only affects unitless numeric string values.

Expand Down