Skip to content

Commit

Permalink
Warn about conflicting style values during updates
Browse files Browse the repository at this point in the history 
This is one of the most insidious quirks of React DOM that people run into. Now we warn when we think an update is dangerous.

We still allow rendering `{background, backgroundSize}` with unchanging values, for example. But once you remove either one or change `background` (without changing `backgroundSize` at the same time), that's bad news. So we warn.
  • Loading branch information
@sophiebits
sophiebits committed Nov 9, 2018
1 parent 2dd4ba1 commit baa89c8
Show file tree
Hide file tree
Showing 4 changed files with 502 additions and 0 deletions.
141 changes: 141 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMComponent-test.js
View file
Expand Up @@ -146,6 +146,147 @@ describe('ReactDOMComponent', () => {
}
});

fit('should warn for conflicting CSS shorthand updates', () => {
const container = document.createElement('div');
ReactDOM.render(
<div style={{font: 'foo', fontStyle: 'bar'}} />,
container,
);
expect(() =>
ReactDOM.render(<div style={{font: 'foo'}} />, container),
).toWarnDev(
'Warning: Removing a style property during rerender (fontStyle) ' +
'when a conflicting property is set (font) can lead to styling ' +
"bugs. To avoid this, don't mix shorthand and non-shorthand " +
'properties for the same value; instead, replace the shorthand ' +
'with separate values.' +
'\n in div (at **)',
);

// These updates are OK and don't warn:
ReactDOM.render(
<div style={{font: 'qux', fontStyle: 'bar'}} />,
container,
);
ReactDOM.render(
<div style={{font: 'foo', fontStyle: 'baz'}} />,
container,
);

expect(() =>
ReactDOM.render(
<div style={{font: 'qux', fontStyle: 'baz'}} />,
container,
),
).toWarnDev(
'Warning: Updating a style property during rerender (font) when ' +
'a conflicting property is set (fontStyle) can lead to styling ' +
"bugs. To avoid this, don't mix shorthand and non-shorthand " +
'properties for the same value; instead, replace the shorthand ' +
'with separate values.' +
'\n in div (at **)',
);
expect(() =>
ReactDOM.render(<div style={{fontStyle: 'baz'}} />, container),
).toWarnDev(
'Warning: Removing a style property during rerender (font) when ' +
'a conflicting property is set (fontStyle) can lead to styling ' +
"bugs. To avoid this, don't mix shorthand and non-shorthand " +
'properties for the same value; instead, replace the shorthand ' +
'with separate values.' +
'\n in div (at **)',
);

// A bit of a special case: backgroundPosition isn't technically longhand
// (it expands to backgroundPosition{X,Y} but so does background)
ReactDOM.render(
<div style={{background: 'yellow', backgroundPosition: 'center'}} />,
container,
);
expect(() =>
ReactDOM.render(<div style={{background: 'yellow'}} />, container),
).toWarnDev(
'Warning: Removing a style property during rerender ' +
'(backgroundPosition) when a conflicting property is set ' +
"(background) can lead to styling bugs. To avoid this, don't mix " +
'shorthand and non-shorthand properties for the same value; ' +
'instead, replace the shorthand with separate values.' +
'\n in div (at **)',
);
ReactDOM.render(
<div style={{background: 'yellow', backgroundPosition: 'center'}} />,
container,
);
// But setting them at the same time is OK:
ReactDOM.render(
<div style={{background: 'green', backgroundPosition: 'top'}} />,
container,
);
expect(() =>
ReactDOM.render(
<div style={{backgroundPosition: 'top'}} />,
container,
),
).toWarnDev(
'Warning: Removing a style property during rerender (background) ' +
'when a conflicting property is set (backgroundPosition) can lead ' +
"to styling bugs. To avoid this, don't mix shorthand and " +
'non-shorthand properties for the same value; instead, replace the ' +
'shorthand with separate values.' +
'\n in div (at **)',
);

// A bit of an even more special case: borderLeft and borderStyle overlap.
ReactDOM.render(
<div style={{borderStyle: 'dotted', borderLeft: '1px solid red'}} />,
container,
);
expect(() =>
ReactDOM.render(
<div style={{borderLeft: '1px solid red'}} />,
container,
),
).toWarnDev(
'Warning: Removing a style property during rerender (borderStyle) ' +
'when a conflicting property is set (borderLeft) can lead to ' +
"styling bugs. To avoid this, don't mix shorthand and " +
'non-shorthand properties for the same value; instead, replace the ' +
'shorthand with separate values.' +
'\n in div (at **)',
);
expect(() =>
ReactDOM.render(
<div style={{borderStyle: 'dashed', borderLeft: '1px solid red'}} />,
container,
),
).toWarnDev(
'Warning: Updating a style property during rerender (borderStyle) ' +
'when a conflicting property is set (borderLeft) can lead to ' +
"styling bugs. To avoid this, don't mix shorthand and " +
'non-shorthand properties for the same value; instead, replace the ' +
'shorthand with separate values.' +
'\n in div (at **)',
);
// But setting them at the same time is OK:
ReactDOM.render(
<div style={{borderStyle: 'dotted', borderLeft: '2px solid red'}} />,
container,
);
expect(() =>
ReactDOM.render(
<div style={{borderStyle: 'dotted'}} />,
container,
),
).toWarnDev(
'Warning: Removing a style property during rerender (borderLeft) ' +
'when a conflicting property is set (borderStyle) can lead to ' +
"styling bugs. To avoid this, don't mix shorthand and " +
'non-shorthand properties for the same value; instead, replace the ' +
'shorthand with separate values.' +
'\n in div (at **)',
);
});

it('should warn for unknown prop', () => {
const container = document.createElement('div');
expect(() =>
Expand Down
3 changes: 3 additions & 0 deletions packages/react-dom/src/client/ReactDOMComponent.js
View file
Expand Up @@ -766,6 +766,9 @@ export function diffProperties(
}
}
if (styleUpdates) {
if (__DEV__) {
CSSPropertyOperations.validateShorthandPropertyCollisionInDev(styleUpdates, nextProps[STYLE]);
}
(updatePayload = updatePayload || []).push(STYLE, styleUpdates);
}
return updatePayload;
Expand Down
107 changes: 107 additions & 0 deletions packages/react-dom/src/shared/CSSPropertyOperations.js
View file
Expand Up @@ -5,9 +5,16 @@
* LICENSE file in the root directory of this source tree.
*/

import {
overlappingShorthandsInDev,
longhandToShorthandInDev,
shorthandToLonghandInDev,
} from './CSSShorthandProperty';

import dangerousStyleValue from './dangerousStyleValue';
import hyphenateStyleName from './hyphenateStyleName';
import warnValidStyle from './warnValidStyle';
import warning from 'shared/warning';

/**
* Operations for dealing with CSS properties.
Expand Down Expand Up @@ -78,3 +85,103 @@ export function setValueForStyles(node, styles) {
}
}
}

function isValueEmpty(value) {
return value == null || typeof value === 'boolean' || value === '';
}

/**
* When mixing shorthand and longhand property names, we warn during updates if
* we expect an incorrect result to occur. In particular, we warn for:
*
* Updating a shorthand property (longhand gets overwritten):
* {font: 'foo', fontVariant: 'bar'} -> {font: 'baz', fontVariant: 'bar'}
* becomes .style.font = 'baz'
* Removing a shorthand property (longhand gets lost too):
* {font: 'foo', fontVariant: 'bar'} -> {fontVariant: 'bar'}
* becomes .style.font = ''
* Removing a longhand property (should revert to shorthand; doesn't):
* {font: 'foo', fontVariant: 'bar'} -> {font: 'foo'}
* becomes .style.fontVariant = ''
*/
export function validateShorthandPropertyCollisionInDev(
styleUpdates,
nextStyles,
) {
if (!nextStyles) {
return;
}

// Map from each individual property to which shorthand defined it.
// For example, styleUpdates = {flex: 'a', flexBasis: 'b'} gives
// expandedUpdates = {
// flexBasis: 'flexBasis',
// flexGrow: 'flex',
// flexShrink: 'flex',
// }
var expandedUpdates = {};
for (const key in styleUpdates) {
const longhands = shorthandToLonghandInDev[key];
}

for (var key in styleUpdates) {
const isEmpty = isValueEmpty(styleUpdates[key]);
if (isEmpty) {
// Property removal; check if we're removing a longhand property
const shorthands = longhandToShorthandInDev[key];
if (shorthands) {
const conflicting = shorthands.filter(
s => !isValueEmpty(nextStyles[s]),
);
if (conflicting.length) {
warning(
false,
'Removing a style property during rerender (%s) when a ' +
'conflicting property is set (%s) can lead to styling bugs. To ' +
"avoid this, don't mix shorthand and non-shorthand properties " +
'for the same value; instead, replace the shorthand with ' +
'separate values.',
key,
conflicting.join(', '),
);
}
}
}

// Updating or removing a property; check if it's a shorthand property
const longhands = shorthandToLonghandInDev[key];
const overlapping = overlappingShorthandsInDev[key];
var conflicting = new Set();
if (longhands) {
longhands.forEach(l => {
if (isValueEmpty(styleUpdates[l]) && !isValueEmpty(nextStyles[l])) {
// ex: key = 'font', l = 'fontStyle'
conflicting.add(l);
}
});
}
if (overlapping) {
overlapping.forEach(l => {
if (isValueEmpty(styleUpdates[l]) && !isValueEmpty(nextStyles[l])) {
// ex: key = 'borderLeft', l = 'borderStyle'
conflicting.add(l);
}
});
}
if (conflicting.size) {
warning(
false,
'%s a style property during rerender (%s) when a ' +
'conflicting property is set (%s) can lead to styling bugs. To ' +
"avoid this, don't mix shorthand and non-shorthand properties " +
'for the same value; instead, replace the shorthand with ' +
'separate values.',
isEmpty ? 'Removing' : 'Updating',
key,
Array.from(conflicting)
.sort()
.join(', '),
);
}
}
}

0 comments on commit baa89c8

Please sign in to comment.