From 8ae867e6b59dc281b35bfb991737892f4cc88037 Mon Sep 17 00:00:00 2001 From: Sophie Alpert Date: Fri, 9 Nov 2018 15:21:47 -0800 Subject: [PATCH] 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. --- .../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 e695a845ff505..c1f66a33a0341 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 4fded23f31a7d..27ac918a87336 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 48659d3f4c846..663d2cd958ab3 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 0000000000000..11f9a9ecd643e --- /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); + }); + } +}