Skip to content

Commit 399ac96

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. Fixes #6348.
1 parent 2dd4ba1 commit 399ac96

File tree

4 files changed

+492
-0
lines changed

4 files changed

+492
-0
lines changed

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

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

149+
it('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(<div style={{backgroundPosition: 'top'}} />, container),
227+
).toWarnDev(
228+
'Warning: Removing a style property during rerender (background) ' +
229+
'when a conflicting property is set (backgroundPosition) can lead ' +
230+
"to styling bugs. To avoid this, don't mix shorthand and " +
231+
'non-shorthand properties for the same value; instead, replace the ' +
232+
'shorthand with separate values.' +
233+
'\n in div (at **)',
234+
);
235+
236+
// A bit of an even more special case: borderLeft and borderStyle overlap.
237+
ReactDOM.render(
238+
<div style={{borderStyle: 'dotted', borderLeft: '1px solid red'}} />,
239+
container,
240+
);
241+
expect(() =>
242+
ReactDOM.render(
243+
<div style={{borderLeft: '1px solid red'}} />,
244+
container,
245+
),
246+
).toWarnDev(
247+
'Warning: Removing a style property during rerender (borderStyle) ' +
248+
'when a conflicting property is set (borderLeft) can lead to ' +
249+
"styling bugs. To avoid this, don't mix shorthand and " +
250+
'non-shorthand properties for the same value; instead, replace the ' +
251+
'shorthand with separate values.' +
252+
'\n in div (at **)',
253+
);
254+
expect(() =>
255+
ReactDOM.render(
256+
<div style={{borderStyle: 'dashed', borderLeft: '1px solid red'}} />,
257+
container,
258+
),
259+
).toWarnDev(
260+
'Warning: Updating a style property during rerender (borderStyle) ' +
261+
'when a conflicting property is set (borderLeft) can lead to ' +
262+
"styling bugs. To avoid this, don't mix shorthand and " +
263+
'non-shorthand properties for the same value; instead, replace the ' +
264+
'shorthand with separate values.' +
265+
'\n in div (at **)',
266+
);
267+
// But setting them at the same time is OK:
268+
ReactDOM.render(
269+
<div style={{borderStyle: 'dotted', borderLeft: '2px solid red'}} />,
270+
container,
271+
);
272+
expect(() =>
273+
ReactDOM.render(<div style={{borderStyle: 'dotted'}} />, container),
274+
).toWarnDev(
275+
'Warning: Removing a style property during rerender (borderLeft) ' +
276+
'when a conflicting property is set (borderStyle) can lead to ' +
277+
"styling bugs. To avoid this, don't mix shorthand and " +
278+
'non-shorthand properties for the same value; instead, replace the ' +
279+
'shorthand with separate values.' +
280+
'\n in div (at **)',
281+
);
282+
});
283+
149284
it('should warn for unknown prop', () => {
150285
const container = document.createElement('div');
151286
expect(() =>

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -766,6 +766,12 @@ export function diffProperties(
766766
}
767767
}
768768
if (styleUpdates) {
769+
if (__DEV__) {
770+
CSSPropertyOperations.validateShorthandPropertyCollisionInDev(
771+
styleUpdates,
772+
nextProps[STYLE],
773+
);
774+
}
769775
(updatePayload = updatePayload || []).push(STYLE, styleUpdates);
770776
}
771777
return updatePayload;

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

Lines changed: 96 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,92 @@ 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+
for (const key in styleUpdates) {
116+
const isEmpty = isValueEmpty(styleUpdates[key]);
117+
if (isEmpty) {
118+
// Property removal; check if we're removing a longhand property
119+
const shorthands = longhandToShorthandInDev[key];
120+
if (shorthands) {
121+
const conflicting = shorthands.filter(
122+
s => !isValueEmpty(nextStyles[s]),
123+
);
124+
if (conflicting.length) {
125+
warning(
126+
false,
127+
'Removing a style property during rerender (%s) when a ' +
128+
'conflicting property is set (%s) can lead to styling bugs. To ' +
129+
"avoid this, don't mix shorthand and non-shorthand properties " +
130+
'for the same value; instead, replace the shorthand with ' +
131+
'separate values.',
132+
key,
133+
conflicting.join(', '),
134+
);
135+
}
136+
}
137+
}
138+
139+
// Updating or removing a property; check if it's a shorthand property
140+
const longhands = shorthandToLonghandInDev[key];
141+
const overlapping = overlappingShorthandsInDev[key];
142+
// eslint-disable-next-line no-var
143+
var conflicting = new Set();
144+
if (longhands) {
145+
longhands.forEach(l => {
146+
if (isValueEmpty(styleUpdates[l]) && !isValueEmpty(nextStyles[l])) {
147+
// ex: key = 'font', l = 'fontStyle'
148+
conflicting.add(l);
149+
}
150+
});
151+
}
152+
if (overlapping) {
153+
overlapping.forEach(l => {
154+
if (isValueEmpty(styleUpdates[l]) && !isValueEmpty(nextStyles[l])) {
155+
// ex: key = 'borderLeft', l = 'borderStyle'
156+
conflicting.add(l);
157+
}
158+
});
159+
}
160+
if (conflicting.size) {
161+
warning(
162+
false,
163+
'%s a style property during rerender (%s) when a ' +
164+
'conflicting property is set (%s) can lead to styling bugs. To ' +
165+
"avoid this, don't mix shorthand and non-shorthand properties " +
166+
'for the same value; instead, replace the shorthand with ' +
167+
'separate values.',
168+
isEmpty ? 'Removing' : 'Updating',
169+
key,
170+
Array.from(conflicting)
171+
.sort()
172+
.join(', '),
173+
);
174+
}
175+
}
176+
}

0 commit comments

Comments
 (0)