@samitouri / QOS-React / commits / 8ae867e6b5

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.

Sophie Alpert committed Nov 9, 2018 at 15:21 UTC 8ae867e6b59dc281b35bfb991737892f4cc88037
4 files changed +492
packages/react-dom/src/__tests__/ReactDOMComponent-test.js
+135
@@ -146,6 +146,141 @@ describe('ReactDOMComponent', () => {
146 }
147 });
148
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 +
284 it('should warn for unknown prop', () => {
285 const container = document.createElement('div');
286 expect(() =>
packages/react-dom/src/client/ReactDOMComponent.js
+6
@@ -766,6 +766,12 @@ export function diffProperties(
766 }
767 }
768 if (styleUpdates) {
769 + if (__DEV__) {
770 + CSSPropertyOperations.validateShorthandPropertyCollisionInDev(
771 + styleUpdates,
772 + nextProps[STYLE],
773 + );
774 + }
775 (updatePayload = updatePayload || []).push(STYLE, styleUpdates);
776 }
777 return updatePayload;
packages/react-dom/src/shared/CSSPropertyOperations.js
+96
@@ -5,9 +5,16 @@
5 * LICENSE file in the root directory of this source tree.
6 */
7
8 +import {
9 + overlappingShorthandsInDev,
10 + longhandToShorthandInDev,
11 + shorthandToLonghandInDev,
12 +} from './CSSShorthandProperty';
13 +
14 import dangerousStyleValue from './dangerousStyleValue';
15 import hyphenateStyleName from './hyphenateStyleName';
16 import warnValidStyle from './warnValidStyle';
17 +import warning from 'shared/warning';
18
19 /**
20 * Operations for dealing with CSS properties.
@@ -78,3 +85,92 @@ export function setValueForStyles(node, styles) {
85 }
86 }
87 }
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 +}
packages/react-dom/src/shared/CSSShorthandProperty.js new
+255
@@ -0,0 +1,255 @@
1 +/**
2 + * Copyright (c) Facebook, Inc. and its affiliates.
3 + *
4 + * This source code is licensed under the MIT license found in the
5 + * LICENSE file in the root directory of this source tree.
6 + */
7 +
8 +// List derived from Gecko source code:
9 +// https://github.com/mozilla/gecko-dev/blob/4e638efc71/layout/style/test/property_database.js
10 +export const shorthandToLonghandInDev = __DEV__
11 + ? {
12 + animation: [
13 + 'animationDelay',
14 + 'animationDirection',
15 + 'animationDuration',
16 + 'animationFillMode',
17 + 'animationIterationCount',
18 + 'animationName',
19 + 'animationPlayState',
20 + 'animationTimingFunction',
21 + ],
22 + background: [
23 + 'backgroundAttachment',
24 + 'backgroundClip',
25 + 'backgroundColor',
26 + 'backgroundImage',
27 + 'backgroundOrigin',
28 + 'backgroundPositionX',
29 + 'backgroundPositionY',
30 + 'backgroundRepeat',
31 + 'backgroundSize',
32 + ],
33 + backgroundPosition: ['backgroundPositionX', 'backgroundPositionY'],
34 + border: [
35 + 'borderBottomColor',
36 + 'borderBottomStyle',
37 + 'borderBottomWidth',
38 + 'borderImageOutset',
39 + 'borderImageRepeat',
40 + 'borderImageSlice',
41 + 'borderImageSource',
42 + 'borderImageWidth',
43 + 'borderLeftColor',
44 + 'borderLeftStyle',
45 + 'borderLeftWidth',
46 + 'borderRightColor',
47 + 'borderRightStyle',
48 + 'borderRightWidth',
49 + 'borderTopColor',
50 + 'borderTopStyle',
51 + 'borderTopWidth',
52 + ],
53 + borderBlockEnd: [
54 + 'borderBlockEndColor',
55 + 'borderBlockEndStyle',
56 + 'borderBlockEndWidth',
57 + ],
58 + borderBlockStart: [
59 + 'borderBlockStartColor',
60 + 'borderBlockStartStyle',
61 + 'borderBlockStartWidth',
62 + ],
63 + borderBottom: [
64 + 'borderBottomColor',
65 + 'borderBottomStyle',
66 + 'borderBottomWidth',
67 + ],
68 + borderColor: [
69 + 'borderBottomColor',
70 + 'borderLeftColor',
71 + 'borderRightColor',
72 + 'borderTopColor',
73 + ],
74 + borderImage: [
75 + 'borderImageOutset',
76 + 'borderImageRepeat',
77 + 'borderImageSlice',
78 + 'borderImageSource',
79 + 'borderImageWidth',
80 + ],
81 + borderInlineEnd: [
82 + 'borderInlineEndColor',
83 + 'borderInlineEndStyle',
84 + 'borderInlineEndWidth',
85 + ],
86 + borderInlineStart: [
87 + 'borderInlineStartColor',
88 + 'borderInlineStartStyle',
89 + 'borderInlineStartWidth',
90 + ],
91 + borderLeft: ['borderLeftColor', 'borderLeftStyle', 'borderLeftWidth'],
92 + borderRadius: [
93 + 'borderBottomLeftRadius',
94 + 'borderBottomRightRadius',
95 + 'borderTopLeftRadius',
96 + 'borderTopRightRadius',
97 + ],
98 + borderRight: ['borderRightColor', 'borderRightStyle', 'borderRightWidth'],
99 + borderStyle: [
100 + 'borderBottomStyle',
101 + 'borderLeftStyle',
102 + 'borderRightStyle',
103 + 'borderTopStyle',
104 + ],
105 + borderTop: ['borderTopColor', 'borderTopStyle', 'borderTopWidth'],
106 + borderWidth: [
107 + 'borderBottomWidth',
108 + 'borderLeftWidth',
109 + 'borderRightWidth',
110 + 'borderTopWidth',
111 + ],
112 + columnRule: ['columnRuleColor', 'columnRuleStyle', 'columnRuleWidth'],
113 + columns: ['columnCount', 'columnWidth'],
114 + flex: ['flexBasis', 'flexGrow', 'flexShrink'],
115 + flexFlow: ['flexDirection', 'flexWrap'],
116 + font: [
117 + 'fontFamily',
118 + 'fontFeatureSettings',
119 + 'fontKerning',
120 + 'fontLanguageOverride',
121 + 'fontSize',
122 + 'fontSizeAdjust',
123 + 'fontStretch',
124 + 'fontStyle',
125 + 'fontVariant',
126 + 'fontVariantAlternates',
127 + 'fontVariantCaps',
128 + 'fontVariantEastAsian',
129 + 'fontVariantLigatures',
130 + 'fontVariantNumeric',
131 + 'fontVariantPosition',
132 + 'fontWeight',
133 + 'lineHeight',
134 + ],
135 + fontVariant: [
136 + 'fontVariantAlternates',
137 + 'fontVariantCaps',
138 + 'fontVariantEastAsian',
139 + 'fontVariantLigatures',
140 + 'fontVariantNumeric',
141 + 'fontVariantPosition',
142 + ],
143 + gap: ['columnGap', 'rowGap'],
144 + grid: [
145 + 'gridAutoColumns',
146 + 'gridAutoFlow',
147 + 'gridAutoRows',
148 + 'gridTemplateAreas',
149 + 'gridTemplateColumns',
150 + 'gridTemplateRows',
151 + ],
152 + gridArea: [
153 + 'gridColumnEnd',
154 + 'gridColumnStart',
155 + 'gridRowEnd',
156 + 'gridRowStart',
157 + ],
158 + gridColumn: ['gridColumnEnd', 'gridColumnStart'],
159 + gridColumnGap: ['columnGap'],
160 + gridGap: ['columnGap', 'rowGap'],
161 + gridRow: ['gridRowEnd', 'gridRowStart'],
162 + gridRowGap: ['rowGap'],
163 + gridTemplate: [
164 + 'gridTemplateAreas',
165 + 'gridTemplateColumns',
166 + 'gridTemplateRows',
167 + ],
168 + listStyle: ['listStyleImage', 'listStylePosition', 'listStyleType'],
169 + margin: ['marginBottom', 'marginLeft', 'marginRight', 'marginTop'],
170 + marker: ['markerEnd', 'markerMid', 'markerStart'],
171 + mask: [
172 + 'maskClip',
173 + 'maskComposite',
174 + 'maskImage',
175 + 'maskMode',
176 + 'maskOrigin',
177 + 'maskPositionX',
178 + 'maskPositionY',
179 + 'maskRepeat',
180 + 'maskSize',
181 + ],
182 + maskPosition: ['maskPositionX', 'maskPositionY'],
183 + outline: ['outlineColor', 'outlineStyle', 'outlineWidth'],
184 + overflow: ['overflowX', 'overflowY'],
185 + padding: ['paddingBottom', 'paddingLeft', 'paddingRight', 'paddingTop'],
186 + placeContent: ['alignContent', 'justifyContent'],
187 + placeItems: ['alignItems', 'justifyItems'],
188 + placeSelf: ['alignSelf', 'justifySelf'],
189 + textDecoration: [
190 + 'textDecorationColor',
191 + 'textDecorationLine',
192 + 'textDecorationStyle',
193 + ],
194 + textEmphasis: ['textEmphasisColor', 'textEmphasisStyle'],
195 + transition: [
196 + 'transitionDelay',
197 + 'transitionDuration',
198 + 'transitionProperty',
199 + 'transitionTimingFunction',
200 + ],
201 + wordWrap: ['overflowWrap'],
202 + }
203 + : {};
204 +
205 +export const longhandToShorthandInDev = {};
206 +
207 +export const overlappingShorthandsInDev = {};
208 +
209 +/**
210 + * obj[key].push(val), handling new keys.
211 + */
212 +function pushToValueArray(obj, key, val) {
213 + let arr = obj[key];
214 + if (!arr) {
215 + obj[key] = arr = [];
216 + }
217 + arr.push(val);
218 +}
219 +
220 +if (__DEV__) {
221 + // eslint-disable-next-line no-var
222 + var shorthand;
223 +
224 + // We want to treat properties like 'backgroundPosition' as one of the
225 + // properties that 'background' expands to, even though that's technically
226 + // incorrect.
227 + for (shorthand in shorthandToLonghandInDev) {
228 + const longhands = shorthandToLonghandInDev[shorthand];
229 + for (const shorthand2 in shorthandToLonghandInDev) {
230 + // eslint-disable-next-line no-var
231 + var longhands2 = shorthandToLonghandInDev[shorthand2];
232 + if (shorthand <= shorthand2) {
233 + continue;
234 + }
235 + if (longhands.every(l => l === shorthand || longhands2.includes(l))) {
236 + // ex: shorthand = 'backgroundPosition', shorthand2 = 'background'
237 + longhands2.push(shorthand);
238 + } else if (
239 + longhands.some(l => l !== shorthand && longhands2.includes(l))
240 + ) {
241 + // ex: shorthand = 'borderLeft', shorthand2 = 'borderStyle'
242 + pushToValueArray(overlappingShorthandsInDev, shorthand, shorthand2);
243 + pushToValueArray(overlappingShorthandsInDev, shorthand2, shorthand);
244 + }
245 + }
246 + }
247 +
248 + // Flip into {maskPositionX: ['mask', 'maskPosition']} for reverse lookups
249 + for (shorthand in shorthandToLonghandInDev) {
250 + const longhands = shorthandToLonghandInDev[shorthand];
251 + longhands.forEach(longhand => {
252 + pushToValueArray(longhandToShorthandInDev, longhand, shorthand);
253 + });
254 + }
255 +}