From 399ac962a6716a0bfddf0ea219c82bfe369e8425 Mon Sep 17 00:00:00 2001 From: Sophie Alpert Date: Fri, 9 Nov 2018 14:27:58 -0800 Subject: [PATCH] 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. --- .../src/__tests__/ReactDOMComponent-test.js | 135 ++++++++++ .../react-dom/src/client/ReactDOMComponent.js | 6 + .../src/shared/CSSPropertyOperations.js | 96 +++++++ .../src/shared/CSSShorthandProperty.js | 255 ++++++++++++++++++ 4 files changed, 492 insertions(+) create mode 100644 packages/react-dom/src/shared/CSSShorthandProperty.js diff --git a/packages/react-dom/src/__tests__/ReactDOMComponent-test.js b/packages/react-dom/src/__tests__/ReactDOMComponent-test.js index e695a845ff50..c1f66a33a034 100644 --- a/packages/react-dom/src/__tests__/ReactDOMComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMComponent-test.js @@ -146,6 +146,141 @@ describe('ReactDOMComponent', () => { } }); + it('should warn for conflicting CSS shorthand updates', () => { + const container = document.createElement('div'); + ReactDOM.render( +
, + container, + ); + expect(() => + ReactDOM.render(
, 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( +
, + container, + ); + ReactDOM.render( +
, + container, + ); + + expect(() => + ReactDOM.render( +
, + 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(
, 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( +
, + container, + ); + expect(() => + ReactDOM.render(
, 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( +
, + container, + ); + // But setting them at the same time is OK: + ReactDOM.render( +
, + container, + ); + expect(() => + ReactDOM.render(
, 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( +
, + container, + ); + expect(() => + ReactDOM.render( +
, + 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( +
, + 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( +
, + container, + ); + expect(() => + ReactDOM.render(
, 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(() => diff --git a/packages/react-dom/src/client/ReactDOMComponent.js b/packages/react-dom/src/client/ReactDOMComponent.js index 4fded23f31a7..27ac918a8733 100644 --- a/packages/react-dom/src/client/ReactDOMComponent.js +++ b/packages/react-dom/src/client/ReactDOMComponent.js @@ -766,6 +766,12 @@ export function diffProperties( } } if (styleUpdates) { + if (__DEV__) { + CSSPropertyOperations.validateShorthandPropertyCollisionInDev( + styleUpdates, + nextProps[STYLE], + ); + } (updatePayload = updatePayload || []).push(STYLE, styleUpdates); } return updatePayload; diff --git a/packages/react-dom/src/shared/CSSPropertyOperations.js b/packages/react-dom/src/shared/CSSPropertyOperations.js index 48659d3f4c84..663d2cd958ab 100644 --- a/packages/react-dom/src/shared/CSSPropertyOperations.js +++ b/packages/react-dom/src/shared/CSSPropertyOperations.js @@ -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. @@ -78,3 +85,92 @@ 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; + } + + for (const 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]; + // eslint-disable-next-line no-var + 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(', '), + ); + } + } +} diff --git a/packages/react-dom/src/shared/CSSShorthandProperty.js b/packages/react-dom/src/shared/CSSShorthandProperty.js new file mode 100644 index 000000000000..11f9a9ecd643 --- /dev/null +++ b/packages/react-dom/src/shared/CSSShorthandProperty.js @@ -0,0 +1,255 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +// List derived from Gecko source code: +// https://github.com/mozilla/gecko-dev/blob/4e638efc71/layout/style/test/property_database.js +export const shorthandToLonghandInDev = __DEV__ + ? { + animation: [ + 'animationDelay', + 'animationDirection', + 'animationDuration', + 'animationFillMode', + 'animationIterationCount', + 'animationName', + 'animationPlayState', + 'animationTimingFunction', + ], + background: [ + 'backgroundAttachment', + 'backgroundClip', + 'backgroundColor', + 'backgroundImage', + 'backgroundOrigin', + 'backgroundPositionX', + 'backgroundPositionY', + 'backgroundRepeat', + 'backgroundSize', + ], + backgroundPosition: ['backgroundPositionX', 'backgroundPositionY'], + border: [ + 'borderBottomColor', + 'borderBottomStyle', + 'borderBottomWidth', + 'borderImageOutset', + 'borderImageRepeat', + 'borderImageSlice', + 'borderImageSource', + 'borderImageWidth', + 'borderLeftColor', + 'borderLeftStyle', + 'borderLeftWidth', + 'borderRightColor', + 'borderRightStyle', + 'borderRightWidth', + 'borderTopColor', + 'borderTopStyle', + 'borderTopWidth', + ], + borderBlockEnd: [ + 'borderBlockEndColor', + 'borderBlockEndStyle', + 'borderBlockEndWidth', + ], + borderBlockStart: [ + 'borderBlockStartColor', + 'borderBlockStartStyle', + 'borderBlockStartWidth', + ], + borderBottom: [ + 'borderBottomColor', + 'borderBottomStyle', + 'borderBottomWidth', + ], + borderColor: [ + 'borderBottomColor', + 'borderLeftColor', + 'borderRightColor', + 'borderTopColor', + ], + borderImage: [ + 'borderImageOutset', + 'borderImageRepeat', + 'borderImageSlice', + 'borderImageSource', + 'borderImageWidth', + ], + borderInlineEnd: [ + 'borderInlineEndColor', + 'borderInlineEndStyle', + 'borderInlineEndWidth', + ], + borderInlineStart: [ + 'borderInlineStartColor', + 'borderInlineStartStyle', + 'borderInlineStartWidth', + ], + borderLeft: ['borderLeftColor', 'borderLeftStyle', 'borderLeftWidth'], + borderRadius: [ + 'borderBottomLeftRadius', + 'borderBottomRightRadius', + 'borderTopLeftRadius', + 'borderTopRightRadius', + ], + borderRight: ['borderRightColor', 'borderRightStyle', 'borderRightWidth'], + borderStyle: [ + 'borderBottomStyle', + 'borderLeftStyle', + 'borderRightStyle', + 'borderTopStyle', + ], + borderTop: ['borderTopColor', 'borderTopStyle', 'borderTopWidth'], + borderWidth: [ + 'borderBottomWidth', + 'borderLeftWidth', + 'borderRightWidth', + 'borderTopWidth', + ], + columnRule: ['columnRuleColor', 'columnRuleStyle', 'columnRuleWidth'], + columns: ['columnCount', 'columnWidth'], + flex: ['flexBasis', 'flexGrow', 'flexShrink'], + flexFlow: ['flexDirection', 'flexWrap'], + font: [ + 'fontFamily', + 'fontFeatureSettings', + 'fontKerning', + 'fontLanguageOverride', + 'fontSize', + 'fontSizeAdjust', + 'fontStretch', + 'fontStyle', + 'fontVariant', + 'fontVariantAlternates', + 'fontVariantCaps', + 'fontVariantEastAsian', + 'fontVariantLigatures', + 'fontVariantNumeric', + 'fontVariantPosition', + 'fontWeight', + 'lineHeight', + ], + fontVariant: [ + 'fontVariantAlternates', + 'fontVariantCaps', + 'fontVariantEastAsian', + 'fontVariantLigatures', + 'fontVariantNumeric', + 'fontVariantPosition', + ], + gap: ['columnGap', 'rowGap'], + grid: [ + 'gridAutoColumns', + 'gridAutoFlow', + 'gridAutoRows', + 'gridTemplateAreas', + 'gridTemplateColumns', + 'gridTemplateRows', + ], + gridArea: [ + 'gridColumnEnd', + 'gridColumnStart', + 'gridRowEnd', + 'gridRowStart', + ], + gridColumn: ['gridColumnEnd', 'gridColumnStart'], + gridColumnGap: ['columnGap'], + gridGap: ['columnGap', 'rowGap'], + gridRow: ['gridRowEnd', 'gridRowStart'], + gridRowGap: ['rowGap'], + gridTemplate: [ + 'gridTemplateAreas', + 'gridTemplateColumns', + 'gridTemplateRows', + ], + listStyle: ['listStyleImage', 'listStylePosition', 'listStyleType'], + margin: ['marginBottom', 'marginLeft', 'marginRight', 'marginTop'], + marker: ['markerEnd', 'markerMid', 'markerStart'], + mask: [ + 'maskClip', + 'maskComposite', + 'maskImage', + 'maskMode', + 'maskOrigin', + 'maskPositionX', + 'maskPositionY', + 'maskRepeat', + 'maskSize', + ], + maskPosition: ['maskPositionX', 'maskPositionY'], + outline: ['outlineColor', 'outlineStyle', 'outlineWidth'], + overflow: ['overflowX', 'overflowY'], + padding: ['paddingBottom', 'paddingLeft', 'paddingRight', 'paddingTop'], + placeContent: ['alignContent', 'justifyContent'], + placeItems: ['alignItems', 'justifyItems'], + placeSelf: ['alignSelf', 'justifySelf'], + textDecoration: [ + 'textDecorationColor', + 'textDecorationLine', + 'textDecorationStyle', + ], + textEmphasis: ['textEmphasisColor', 'textEmphasisStyle'], + transition: [ + 'transitionDelay', + 'transitionDuration', + 'transitionProperty', + 'transitionTimingFunction', + ], + wordWrap: ['overflowWrap'], + } + : {}; + +export const longhandToShorthandInDev = {}; + +export const overlappingShorthandsInDev = {}; + +/** + * obj[key].push(val), handling new keys. + */ +function pushToValueArray(obj, key, val) { + let arr = obj[key]; + if (!arr) { + obj[key] = arr = []; + } + arr.push(val); +} + +if (__DEV__) { + // eslint-disable-next-line no-var + var shorthand; + + // We want to treat properties like 'backgroundPosition' as one of the + // properties that 'background' expands to, even though that's technically + // incorrect. + for (shorthand in shorthandToLonghandInDev) { + const longhands = shorthandToLonghandInDev[shorthand]; + for (const shorthand2 in shorthandToLonghandInDev) { + // eslint-disable-next-line no-var + var longhands2 = shorthandToLonghandInDev[shorthand2]; + if (shorthand <= shorthand2) { + continue; + } + if (longhands.every(l => l === shorthand || longhands2.includes(l))) { + // ex: shorthand = 'backgroundPosition', shorthand2 = 'background' + longhands2.push(shorthand); + } else if ( + longhands.some(l => l !== shorthand && longhands2.includes(l)) + ) { + // ex: shorthand = 'borderLeft', shorthand2 = 'borderStyle' + pushToValueArray(overlappingShorthandsInDev, shorthand, shorthand2); + pushToValueArray(overlappingShorthandsInDev, shorthand2, shorthand); + } + } + } + + // Flip into {maskPositionX: ['mask', 'maskPosition']} for reverse lookups + for (shorthand in shorthandToLonghandInDev) { + const longhands = shorthandToLonghandInDev[shorthand]; + longhands.forEach(longhand => { + pushToValueArray(longhandToShorthandInDev, longhand, shorthand); + }); + } +}