From a3362e1f38c5ee130a7cbbd07e79ddda230844bf Mon Sep 17 00:00:00 2001 From: Nicolas Gallagher Date: Sun, 23 Apr 2017 21:24:27 -0700 Subject: [PATCH] [fix] setNativeProps inline styles Inline styles are preserved when using 'setNativeProps'. Adds unit tests for the resolution logic required by 'setNativeProps'/'resolveStateful' in a DOM context. Fix #439 --- performance/webpack.config.js | 2 +- src/apis/StyleSheet/StyleRegistry.js | 75 ++++++++++++------- .../__tests__/StyleRegistry-test.js | 42 ++++++++--- .../__snapshots__/StyleRegistry-test.js.snap | 58 +++++++------- src/modules/NativeMethodsMixin/index.js | 7 +- 5 files changed, 117 insertions(+), 67 deletions(-) diff --git a/performance/webpack.config.js b/performance/webpack.config.js index 21c11038..1ac15030 100644 --- a/performance/webpack.config.js +++ b/performance/webpack.config.js @@ -49,7 +49,7 @@ module.exports = { ], resolve: { alias: { - 'react-native': path.join(__dirname, '../src/module') + 'react-native': path.join(__dirname, '../src') } } }; diff --git a/src/apis/StyleSheet/StyleRegistry.js b/src/apis/StyleSheet/StyleRegistry.js index c12fb11b..58bdfe39 100644 --- a/src/apis/StyleSheet/StyleRegistry.js +++ b/src/apis/StyleSheet/StyleRegistry.js @@ -11,6 +11,8 @@ import prefixInlineStyles from './prefixInlineStyles'; import ReactNativePropRegistry from '../../modules/ReactNativePropRegistry'; import StyleManager from './StyleManager'; +const vendorPrefixPattern = /^(webkit|moz|ms)/; + const createCacheKey = id => { const prefix = I18nManager.isRTL ? 'rtl' : 'ltr'; return `${prefix}-${id}`; @@ -81,36 +83,59 @@ class StyleRegistry { /** * Resolves a React Native style object to DOM attributes, accounting for - * the existing styles applied to the DOM node + * the existing styles applied to the DOM node. + * + * To determine the next style, some of the existing DOM state must be + * converted back into React Native styles. */ - resolveStateful(reactNativeStyle, domClassList) { - const previousReactNativeStyle = {}; - const preservedClassNames = []; + resolveStateful(rnStyleNext, domStyleProps) { + // Convert the DOM classList back into a React Native form + // Preserves unrecognized class names. + const rnStyleProps = domStyleProps.classList.reduce( + (styleProps, className) => { + const { prop, value } = this.styleManager.getDeclaration(className); + if (prop) { + styleProps.style[prop] = value; + } else { + styleProps.classList.push(className); + } + return styleProps; + }, + { classList: [], style: {} } + ); - // Convert the existing classList to a React Native style and preserve any - // unrecognized classNames. - domClassList.forEach(className => { - const { prop, value } = this.styleManager.getDeclaration(className); - if (prop) { - previousReactNativeStyle[prop] = value; - } else { - preservedClassNames.push(className); + // DOM style may include vendor prefixes and properties set by other libraries. + // Preserve it but transform back into React DOM style. + const rdomStyle = Object.keys(domStyleProps.style).reduce((acc, styleName) => { + const value = domStyleProps.style[styleName]; + if (value !== '') { + const reactStyleName = vendorPrefixPattern.test(styleName) + ? styleName.charAt(0).toUpperCase() + styleName.slice(1) + : styleName; + acc[reactStyleName] = value; + } + return acc; + }, {}); + + // Create next DOM style props from current and next RN styles + const { classList: rdomClassListNext, style: rdomStyleNext } = this.resolve([ + rnStyleProps.style, + rnStyleNext + ]); + // Add the current class names not managed by React Native + rdomClassListNext.push(...rnStyleProps.classList); + // Next class names take priority over current inline styles + const style = { ...rdomStyle }; + rdomClassListNext.forEach(className => { + const { prop } = this.styleManager.getDeclaration(className); + if (style[prop]) { + style[prop] = ''; } }); + // Next inline styles take priority over current inline styles + Object.assign(style, rdomStyleNext); + const className = classListToString(rdomClassListNext); - // Resolve the two React Native styles. - const { classList, style = {} } = this.resolve([previousReactNativeStyle, reactNativeStyle]); - - // Because this is used in stateful operations we need to remove any - // existing inline styles that would override the classNames. - classList.forEach(className => { - const { prop } = this.styleManager.getDeclaration(className); - style[prop] = null; - }); - - classList.push(preservedClassNames); - - const className = classListToString(classList); return { className, style }; } diff --git a/src/apis/StyleSheet/__tests__/StyleRegistry-test.js b/src/apis/StyleSheet/__tests__/StyleRegistry-test.js index bddb4f0b..4dc5d088 100644 --- a/src/apis/StyleSheet/__tests__/StyleRegistry-test.js +++ b/src/apis/StyleSheet/__tests__/StyleRegistry-test.js @@ -58,19 +58,37 @@ describe('apis/StyleSheet/StyleRegistry', () => { }); }); - test('resolveStateful', () => { - // generate a classList to act as pre-existing DOM state - const mockStyle = styleRegistry.register({ - borderWidth: 0, - borderColor: 'red', - width: 100 + describe('resolveStateful', () => { + test('preserves unrelated class names', () => { + const domStyleProps = { classList: ['unknown-class-1', 'unknown-class-2'], style: {} }; + const domStyleNextProps = styleRegistry.resolveStateful({}, domStyleProps); + expect(domStyleNextProps).toMatchSnapshot(); }); - const { classList: domClassList } = styleRegistry.resolve(mockStyle); - domClassList.unshift('external-className'); - expect(domClassList).toMatchSnapshot(); - // test result - const result = styleRegistry.resolveStateful({ borderWidth: 1, opacity: 1 }, domClassList); - expect(result).toMatchSnapshot(); + test('preserves unrelated inline styles', () => { + const domStyleProps = { classList: [], style: { fontSize: '20px' } }; + const domStyleNextProps = styleRegistry.resolveStateful({ opacity: 1 }, domStyleProps); + expect(domStyleNextProps).toMatchSnapshot(); + }); + + test('next class names have priority over current inline styles', () => { + const domStyleProps = { classList: [], style: { opacity: 0.5 } }; + const nextStyle = styleRegistry.register({ opacity: 1 }); + const domStyleNextProps = styleRegistry.resolveStateful(nextStyle, domStyleProps); + expect(domStyleNextProps).toMatchSnapshot(); + }); + + test('next inline styles have priority over current inline styles', () => { + // note: this also checks for correctly uppercasing the first letter of DOM vendor prefixes + const domStyleProps = { + classList: [], + style: { opacity: 0.5, webkitTransform: 'scale(1)', transform: 'scale(1)' } + }; + const domStyleNextProps = styleRegistry.resolveStateful( + { opacity: 1, transform: [{ scale: 2 }] }, + domStyleProps + ); + expect(domStyleNextProps).toMatchSnapshot(); + }); }); }); diff --git a/src/apis/StyleSheet/__tests__/__snapshots__/StyleRegistry-test.js.snap b/src/apis/StyleSheet/__tests__/__snapshots__/StyleRegistry-test.js.snap index 6b75e698..90e75dcf 100644 --- a/src/apis/StyleSheet/__tests__/__snapshots__/StyleRegistry-test.js.snap +++ b/src/apis/StyleSheet/__tests__/__snapshots__/StyleRegistry-test.js.snap @@ -192,35 +192,39 @@ Object { } `; -exports[`apis/StyleSheet/StyleRegistry resolveStateful 1`] = ` -Array [ - "external-className", - "rn-borderTopColor-1gxhl28", - "rn-borderRightColor-knoah9", - "rn-borderBottomColor-1ani3fp", - "rn-borderLeftColor-ribj9x", - "rn-borderTopWidth-13yce4e", - "rn-borderRightWidth-fnigne", - "rn-borderBottomWidth-ndvcnb", - "rn-borderLeftWidth-gxnn5r", - "rn-width-b8lwoo", -] -`; - -exports[`apis/StyleSheet/StyleRegistry resolveStateful 2`] = ` +exports[`apis/StyleSheet/StyleRegistry resolveStateful next class names have priority over current inline styles 1`] = ` Object { - "className": "rn-borderBottomColor-1ani3fp rn-borderBottomWidth-ndvcnb rn-borderLeftColor-ribj9x rn-borderLeftWidth-gxnn5r rn-borderRightColor-knoah9 rn-borderRightWidth-fnigne rn-borderTopColor-1gxhl28 rn-borderTopWidth-13yce4e rn-width-b8lwoo external-className", + "className": "rn-opacity-6dt33c", "style": Object { - "borderBottomColor": null, - "borderBottomWidth": null, - "borderLeftColor": null, - "borderLeftWidth": null, - "borderRightColor": null, - "borderRightWidth": null, - "borderTopColor": null, - "borderTopWidth": null, - "opacity": 1, - "width": null, + "opacity": "", + }, +} +`; + +exports[`apis/StyleSheet/StyleRegistry resolveStateful next inline styles have priority over current inline styles 1`] = ` +Object { + "className": "", + "style": Object { + "WebkitTransform": "scale(2)", + "opacity": 1, + "transform": "scale(2)", + }, +} +`; + +exports[`apis/StyleSheet/StyleRegistry resolveStateful preserves unrelated class names 1`] = ` +Object { + "className": "unknown-class-1 unknown-class-2", + "style": Object {}, +} +`; + +exports[`apis/StyleSheet/StyleRegistry resolveStateful preserves unrelated inline styles 1`] = ` +Object { + "className": "", + "style": Object { + "fontSize": "20px", + "opacity": 1, }, } `; diff --git a/src/modules/NativeMethodsMixin/index.js b/src/modules/NativeMethodsMixin/index.js index 2febc3fc..09135021 100644 --- a/src/modules/NativeMethodsMixin/index.js +++ b/src/modules/NativeMethodsMixin/index.js @@ -67,12 +67,15 @@ const NativeMethodsMixin = { * the initial styles from the DOM node and merge them with incoming props. */ setNativeProps(nativeProps: Object) { - // DOM state + // Copy of existing DOM state const node = findNodeHandle(this); const classList = Array.prototype.slice.call(node.classList); + const style = { ...node.style }; + const domStyleProps = { classList, style }; + // Next DOM state const domProps = createDOMProps(nativeProps, style => - StyleRegistry.resolveStateful(style, classList) + StyleRegistry.resolveStateful(style, domStyleProps) ); UIManager.updateView(node, domProps, this); }