Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Warn about conflicting style values during updates #14181

Merged
merged 1 commit into from Nov 9, 2018
Merged

Warn about conflicting style values during updates #14181

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
135 changes: 135 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMComponent-test.js
View file
Open in desktop
Expand Up @@ -146,6 +146,141 @@ describe('ReactDOMComponent', () => {
}
});

it('should warn for conflicting CSS shorthand updates', () => {
const container = document.createElement('div');
ReactDOM.render(
<div style={{font: 'foo', fontStyle: 'bar'}} />,
container,
);
expect(() =>
ReactDOM.render(<div style={{font: 'foo'}} />, 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(
<div style={{font: 'qux', fontStyle: 'bar'}} />,
container,
);
ReactDOM.render(
<div style={{font: 'foo', fontStyle: 'baz'}} />,
container,
);

expect(() =>
ReactDOM.render(
<div style={{font: 'qux', fontStyle: 'baz'}} />,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, this is subtle. Had to read these tests a couple times. Nice to have a warning for it.

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(<div style={{fontStyle: 'baz'}} />, 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(
<div style={{background: 'yellow', backgroundPosition: 'center'}} />,
container,
);
expect(() =>
ReactDOM.render(<div style={{background: 'yellow'}} />, 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(
<div style={{background: 'yellow', backgroundPosition: 'center'}} />,
container,
);
// But setting them at the same time is OK:
ReactDOM.render(
<div style={{background: 'green', backgroundPosition: 'top'}} />,
container,
);
expect(() =>
ReactDOM.render(<div style={{backgroundPosition: 'top'}} />, 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(
<div style={{borderStyle: 'dotted', borderLeft: '1px solid red'}} />,
container,
);
expect(() =>
ReactDOM.render(
<div style={{borderLeft: '1px solid red'}} />,
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(
<div style={{borderStyle: 'dashed', borderLeft: '1px solid red'}} />,
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(
<div style={{borderStyle: 'dotted', borderLeft: '2px solid red'}} />,
container,
);
expect(() =>
ReactDOM.render(<div style={{borderStyle: 'dotted'}} />, 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(() =>
Expand Down
6 changes: 6 additions & 0 deletions packages/react-dom/src/client/ReactDOMComponent.js
View file
Open in desktop
Expand Up @@ -766,6 +766,12 @@ export function diffProperties(
}
}
if (styleUpdates) {
if (__DEV__) {
CSSPropertyOperations.validateShorthandPropertyCollisionInDev(
styleUpdates,
nextProps[STYLE],
);
}
(updatePayload = updatePayload || []).push(STYLE, styleUpdates);
}
return updatePayload;
Expand Down
96 changes: 96 additions & 0 deletions packages/react-dom/src/shared/CSSPropertyOperations.js
View file
Open in desktop
Expand Up @@ -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.
Expand Down Expand Up @@ -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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this could get noisy if we don't do any kind of de-duping. I wonder if this is something you considered and ruled out?

Guess de-duping would have a small impact on tests too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about it but I don't think this will be very common, it is a bad bug, and it is pretty easy to fix – so I think it's OK not to.

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(', '),
);
}
}
}