Skip to content

Commit baa89c8

Browse files
sophiebitssophiebits
committed
Warn about conflicting style values during updates
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.
1 parent 2dd4ba1 commit baa89c8

File tree

4 files changed

+502
-0
lines changed

4 files changed

+502
-0
lines changed

‎packages/react-dom/src/__tests__/ReactDOMComponent-test.js

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,147 @@ describe('ReactDOMComponent', () => {
146146
}
147147
});
148148

149+
fit('should warn for conflicting CSS shorthand updates', () => {
150+
const container = document.createElement('div');
151+
ReactDOM.render(
152+
<div style={{font: 'foo', fontStyle: 'bar'}} />,
153+
container,
154+
);
155+
expect(() =>
156+
ReactDOM.render(<div style={{font: 'foo'}} />, container),
157+
).toWarnDev(
158+
'Warning: Removing a style property during rerender (fontStyle) ' +
159+
'when a conflicting property is set (font) can lead to styling ' +
160+
"bugs. To avoid this, don't mix shorthand and non-shorthand " +
161+
'properties for the same value; instead, replace the shorthand ' +
162+
'with separate values.' +
163+
'\n in div (at **)',
164+
);
165+
166+
// These updates are OK and don't warn:
167+
ReactDOM.render(
168+
<div style={{font: 'qux', fontStyle: 'bar'}} />,
169+
container,
170+
);
171+
ReactDOM.render(
172+
<div style={{font: 'foo', fontStyle: 'baz'}} />,
173+
container,
174+
);
175+
176+
expect(() =>
177+
ReactDOM.render(
178+
<div style={{font: 'qux', fontStyle: 'baz'}} />,
179+
container,
180+
),
181+
).toWarnDev(
182+
'Warning: Updating a style property during rerender (font) when ' +
183+
'a conflicting property is set (fontStyle) can lead to styling ' +
184+
"bugs. To avoid this, don't mix shorthand and non-shorthand " +
185+
'properties for the same value; instead, replace the shorthand ' +
186+
'with separate values.' +
187+
'\n in div (at **)',
188+
);
189+
expect(() =>
190+
ReactDOM.render(<div style={{fontStyle: 'baz'}} />, container),
191+
).toWarnDev(
192+
'Warning: Removing a style property during rerender (font) when ' +
193+
'a conflicting property is set (fontStyle) can lead to styling ' +
194+
"bugs. To avoid this, don't mix shorthand and non-shorthand " +
195+
'properties for the same value; instead, replace the shorthand ' +
196+
'with separate values.' +
197+
'\n in div (at **)',
198+
);
199+
200+
// A bit of a special case: backgroundPosition isn't technically longhand
201+
// (it expands to backgroundPosition{X,Y} but so does background)
202+
ReactDOM.render(
203+
<div style={{background: 'yellow', backgroundPosition: 'center'}} />,
204+
container,
205+
);
206+
expect(() =>
207+
ReactDOM.render(<div style={{background: 'yellow'}} />, container),
208+
).toWarnDev(
209+
'Warning: Removing a style property during rerender ' +
210+
'(backgroundPosition) when a conflicting property is set ' +
211+
"(background) can lead to styling bugs. To avoid this, don't mix " +
212+
'shorthand and non-shorthand properties for the same value; ' +
213+
'instead, replace the shorthand with separate values.' +
214+
'\n in div (at **)',
215+
);
216+
ReactDOM.render(
217+
<div style={{background: 'yellow', backgroundPosition: 'center'}} />,
218+
container,
219+
);
220+
// But setting them at the same time is OK:
221+
ReactDOM.render(
222+
<div style={{background: 'green', backgroundPosition: 'top'}} />,
223+
container,
224+
);
225+
expect(() =>
226+
ReactDOM.render(
227+
<div style={{backgroundPosition: 'top'}} />,
228+
container,
229+
),
230+
).toWarnDev(
231+
'Warning: Removing a style property during rerender (background) ' +
232+
'when a conflicting property is set (backgroundPosition) can lead ' +
233+
"to styling bugs. To avoid this, don't mix shorthand and " +
234+
'non-shorthand properties for the same value; instead, replace the ' +
235+
'shorthand with separate values.' +
236+
'\n in div (at **)',
237+
);
238+
239+
// A bit of an even more special case: borderLeft and borderStyle overlap.
240+
ReactDOM.render(
241+
<div style={{borderStyle: 'dotted', borderLeft: '1px solid red'}} />,
242+
container,
243+
);
244+
expect(() =>
245+
ReactDOM.render(
246+
<div style={{borderLeft: '1px solid red'}} />,
247+
container,
248+
),
249+
).toWarnDev(
250+
'Warning: Removing a style property during rerender (borderStyle) ' +
251+
'when a conflicting property is set (borderLeft) can lead to ' +
252+
"styling bugs. To avoid this, don't mix shorthand and " +
253+
'non-shorthand properties for the same value; instead, replace the ' +
254+
'shorthand with separate values.' +
255+
'\n in div (at **)',
256+
);
257+
expect(() =>
258+
ReactDOM.render(
259+
<div style={{borderStyle: 'dashed', borderLeft: '1px solid red'}} />,
260+
container,
261+
),
262+
).toWarnDev(
263+
'Warning: Updating a style property during rerender (borderStyle) ' +
264+
'when a conflicting property is set (borderLeft) can lead to ' +
265+
"styling bugs. To avoid this, don't mix shorthand and " +
266+
'non-shorthand properties for the same value; instead, replace the ' +
267+
'shorthand with separate values.' +
268+
'\n in div (at **)',
269+
);
270+
// But setting them at the same time is OK:
271+
ReactDOM.render(
272+
<div style={{borderStyle: 'dotted', borderLeft: '2px solid red'}} />,
273+
container,
274+
);
275+
expect(() =>
276+
ReactDOM.render(
277+
<div style={{borderStyle: 'dotted'}} />,
278+
container,
279+
),
280+
).toWarnDev(
281+
'Warning: Removing a style property during rerender (borderLeft) ' +
282+
'when a conflicting property is set (borderStyle) can lead to ' +
283+
"styling bugs. To avoid this, don't mix shorthand and " +
284+
'non-shorthand properties for the same value; instead, replace the ' +
285+
'shorthand with separate values.' +
286+
'\n in div (at **)',
287+
);
288+
});
289+
149290
it('should warn for unknown prop', () => {
150291
const container = document.createElement('div');
151292
expect(() =>

‎packages/react-dom/src/client/ReactDOMComponent.js

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -766,6 +766,9 @@ export function diffProperties(
766766
}
767767
}
768768
if (styleUpdates) {
769+
if (__DEV__) {
770+
CSSPropertyOperations.validateShorthandPropertyCollisionInDev(styleUpdates, nextProps[STYLE]);
771+
}
769772
(updatePayload = updatePayload || []).push(STYLE, styleUpdates);
770773
}
771774
return updatePayload;

‎packages/react-dom/src/shared/CSSPropertyOperations.js

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,16 @@
55
* LICENSE file in the root directory of this source tree.
66
*/
77

8+
import {
9+
overlappingShorthandsInDev,
10+
longhandToShorthandInDev,
11+
shorthandToLonghandInDev,
12+
} from './CSSShorthandProperty';
13+
814
import dangerousStyleValue from './dangerousStyleValue';
915
import hyphenateStyleName from './hyphenateStyleName';
1016
import warnValidStyle from './warnValidStyle';
17+
import warning from 'shared/warning';
1118

1219
/**
1320
* Operations for dealing with CSS properties.
@@ -78,3 +85,103 @@ export function setValueForStyles(node, styles) {
7885
}
7986
}
8087
}
88+
89+
function isValueEmpty(value) {
90+
return value == null || typeof value === 'boolean' || value === '';
91+
}
92+
93+
/**
94+
* When mixing shorthand and longhand property names, we warn during updates if
95+
* we expect an incorrect result to occur. In particular, we warn for:
96+
*
97+
* Updating a shorthand property (longhand gets overwritten):
98+
* {font: 'foo', fontVariant: 'bar'} -> {font: 'baz', fontVariant: 'bar'}
99+
* becomes .style.font = 'baz'
100+
* Removing a shorthand property (longhand gets lost too):
101+
* {font: 'foo', fontVariant: 'bar'} -> {fontVariant: 'bar'}
102+
* becomes .style.font = ''
103+
* Removing a longhand property (should revert to shorthand; doesn't):
104+
* {font: 'foo', fontVariant: 'bar'} -> {font: 'foo'}
105+
* becomes .style.fontVariant = ''
106+
*/
107+
export function validateShorthandPropertyCollisionInDev(
108+
styleUpdates,
109+
nextStyles,
110+
) {
111+
if (!nextStyles) {
112+
return;
113+
}
114+
115+
// Map from each individual property to which shorthand defined it.
116+
// For example, styleUpdates = {flex: 'a', flexBasis: 'b'} gives
117+
// expandedUpdates = {
118+
// flexBasis: 'flexBasis',
119+
// flexGrow: 'flex',
120+
// flexShrink: 'flex',
121+
// }
122+
var expandedUpdates = {};
123+
for (const key in styleUpdates) {
124+
const longhands = shorthandToLonghandInDev[key];
125+
}
126+
127+
for (var key in styleUpdates) {
128+
const isEmpty = isValueEmpty(styleUpdates[key]);
129+
if (isEmpty) {
130+
// Property removal; check if we're removing a longhand property
131+
const shorthands = longhandToShorthandInDev[key];
132+
if (shorthands) {
133+
const conflicting = shorthands.filter(
134+
s => !isValueEmpty(nextStyles[s]),
135+
);
136+
if (conflicting.length) {
137+
warning(
138+
false,
139+
'Removing a style property during rerender (%s) when a ' +
140+
'conflicting property is set (%s) can lead to styling bugs. To ' +
141+
"avoid this, don't mix shorthand and non-shorthand properties " +
142+
'for the same value; instead, replace the shorthand with ' +
143+
'separate values.',
144+
key,
145+
conflicting.join(', '),
146+
);
147+
}
148+
}
149+
}
150+
151+
// Updating or removing a property; check if it's a shorthand property
152+
const longhands = shorthandToLonghandInDev[key];
153+
const overlapping = overlappingShorthandsInDev[key];
154+
var conflicting = new Set();
155+
if (longhands) {
156+
longhands.forEach(l => {
157+
if (isValueEmpty(styleUpdates[l]) && !isValueEmpty(nextStyles[l])) {
158+
// ex: key = 'font', l = 'fontStyle'
159+
conflicting.add(l);
160+
}
161+
});
162+
}
163+
if (overlapping) {
164+
overlapping.forEach(l => {
165+
if (isValueEmpty(styleUpdates[l]) && !isValueEmpty(nextStyles[l])) {
166+
// ex: key = 'borderLeft', l = 'borderStyle'
167+
conflicting.add(l);
168+
}
169+
});
170+
}
171+
if (conflicting.size) {
172+
warning(
173+
false,
174+
'%s a style property during rerender (%s) when a ' +
175+
'conflicting property is set (%s) can lead to styling bugs. To ' +
176+
"avoid this, don't mix shorthand and non-shorthand properties " +
177+
'for the same value; instead, replace the shorthand with ' +
178+
'separate values.',
179+
isEmpty ? 'Removing' : 'Updating',
180+
key,
181+
Array.from(conflicting)
182+
.sort()
183+
.join(', '),
184+
);
185+
}
186+
}
187+
}

0 commit comments

Comments
 (0)