Skip to content

Commit 8ae867e

Browse files
sophiebitssophiebits
authored
Warn about conflicting style values during updates (#14181)
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 d5e1bf0 commit 8ae867e

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)