From 3ffc005a7bb389d34ae87564fa0fee3fcb6e8073 Mon Sep 17 00:00:00 2001 From: Nicolas Gallagher Date: Sun, 8 Jan 2017 14:49:54 -0800 Subject: [PATCH] [fix] setNativeProps resolving logic Since styles are set using both class names and inline styles, 'setNativeProps' needs an additional resolving step that accounts for the pre-existing state of RN-managed styles on the DOM node. Fix #321 --- docs/guides/direct-manipulation.md | 2 +- src/apis/StyleSheet/registry.js | 4 ++ src/apis/UIManager/__tests__/index-test.js | 7 ++- src/apis/UIManager/index.js | 8 +-- src/modules/NativeMethodsMixin/index.js | 57 +++++++++++++++++++++- 5 files changed, 65 insertions(+), 13 deletions(-) diff --git a/docs/guides/direct-manipulation.md b/docs/guides/direct-manipulation.md index f6dad3e6..d2778dea 100644 --- a/docs/guides/direct-manipulation.md +++ b/docs/guides/direct-manipulation.md @@ -4,7 +4,7 @@ It is sometimes necessary to make changes directly to a component without using state/props to trigger a re-render of the entire subtree – in the browser, this is done by directly modifying a DOM node. `setNativeProps` is the React Native equivalent to setting properties directly on a DOM node. Use direct -manipulation when frequent re-rendering creates a performance bottleneck Direct +manipulation when frequent re-rendering creates a performance bottleneck. Direct manipulation will not be a tool that you reach for frequently. ## `setNativeProps` and `shouldComponentUpdate` diff --git a/src/apis/StyleSheet/registry.js b/src/apis/StyleSheet/registry.js index a35f5a04..79dd58d8 100644 --- a/src/apis/StyleSheet/registry.js +++ b/src/apis/StyleSheet/registry.js @@ -95,6 +95,7 @@ const resolveProps = (reactNativeStyle) => { style: prefixInlineStyles(style) }; + /* if (process.env.__REACT_NATIVE_DEBUG_ENABLED__) { console.groupCollapsed('[StyleSheet] resolving uncached styles'); console.log( @@ -106,6 +107,7 @@ const resolveProps = (reactNativeStyle) => { console.log('resolve => \n', props); console.groupEnd(); } + */ return props; }; @@ -131,6 +133,7 @@ const StyleRegistry = { initialize(classNames) { injectedClassNames = classNames; + /* if (process.env.__REACT_NATIVE_DEBUG_ENABLED__) { if (global.__REACT_NATIVE_DEBUG_ENABLED__styleRegistryTimer) { clearInterval(global.__REACT_NATIVE_DEBUG_ENABLED__styleRegistryTimer); @@ -142,6 +145,7 @@ const StyleRegistry = { console.groupEnd(); }, 30000); } + */ }, reset() { diff --git a/src/apis/UIManager/__tests__/index-test.js b/src/apis/UIManager/__tests__/index-test.js index 35eab851..19afc74c 100644 --- a/src/apis/UIManager/__tests__/index-test.js +++ b/src/apis/UIManager/__tests__/index-test.js @@ -126,17 +126,16 @@ describe('apis/UIManager', () => { } }; - test('add new className to existing className', () => { + test('supports className alias for class', () => { const node = createNode(); - node.className = 'existing'; const props = { className: 'extra' }; UIManager.updateView(node, props, componentStub); - expect(node.getAttribute('class')).toEqual('existing extra'); + expect(node.getAttribute('class')).toEqual('extra'); }); test('adds correct DOM styles to existing style', () => { const node = createNode({ color: 'red' }); - const props = { style: { marginVertical: 0, opacity: 0 } }; + const props = { style: { marginTop: 0, marginBottom: 0, opacity: 0 } }; UIManager.updateView(node, props, componentStub); expect(node.getAttribute('style')).toEqual('color: red; margin-top: 0px; margin-bottom: 0px; opacity: 0;'); }); diff --git a/src/apis/UIManager/index.js b/src/apis/UIManager/index.js index 78e82bc5..3e93a7eb 100644 --- a/src/apis/UIManager/index.js +++ b/src/apis/UIManager/index.js @@ -1,8 +1,5 @@ import asap from 'asap'; -import createReactDOMStyle from '../StyleSheet/createReactDOMStyle'; -import flattenStyle from '../StyleSheet/flattenStyle'; import CSSPropertyOperations from 'react-dom/lib/CSSPropertyOperations'; -import prefixInlineStyles from '../StyleSheet/prefixInlineStyles'; const _measureLayout = (node, relativeToNativeNode, callback) => { asap(() => { @@ -47,13 +44,12 @@ const UIManager = { const value = props[prop]; switch (prop) { case 'style': { - const style = prefixInlineStyles(createReactDOMStyle(flattenStyle(value))); - CSSPropertyOperations.setValueForStyles(node, style, component._reactInternalInstance); + CSSPropertyOperations.setValueForStyles(node, value, component._reactInternalInstance); break; } case 'class': case 'className': { - node.classList.add(value); + node.setAttribute('class', value); break; } case 'text': diff --git a/src/modules/NativeMethodsMixin/index.js b/src/modules/NativeMethodsMixin/index.js index ba50f9bc..d519442d 100644 --- a/src/modules/NativeMethodsMixin/index.js +++ b/src/modules/NativeMethodsMixin/index.js @@ -7,8 +7,21 @@ */ import findNodeHandle from '../findNodeHandle'; +import StyleRegistry from '../../apis/StyleSheet/registry'; import UIManager from '../../apis/UIManager'; +const REGEX_CLASSNAME_SPLIT = /\s+/; +const REGEX_STYLE_PROP = /rn-(.*):.*/; + +const classNameFilter = (className) => { return className !== ''; }; +const classNameToList = (className = '') => className.split(REGEX_CLASSNAME_SPLIT).filter(classNameFilter); +const getStyleProp = (className) => { + const match = className.match(REGEX_STYLE_PROP); + if (match) { + return match[1]; + } +}; + const NativeMethodsMixin = { /** * Removes focus from an input or view. This is the opposite of `focus()`. @@ -45,7 +58,7 @@ const NativeMethodsMixin = { * - height * * Note that these measurements are not available until after the rendering - * has been completed in native. + * has been completed. */ measureInWindow(callback) { UIManager.measureInWindow(findNodeHandle(this), callback); @@ -60,9 +73,49 @@ const NativeMethodsMixin = { /** * This function sends props straight to the underlying DOM node. + * This works as if all styles were set as inline styles. Since a DOM node + * may aleady be styled with class names and inline styles, we need to get + * the initial styles from the DOM node and merge them with incoming props. */ setNativeProps(nativeProps: Object) { - UIManager.updateView(findNodeHandle(this), nativeProps, this); + // DOM state + const node = findNodeHandle(this); + const domClassList = [ ...node.classList ]; + + // Resolved state + const resolvedProps = StyleRegistry.resolve(nativeProps.style); + const resolvedClassList = classNameToList(resolvedProps.className); + + // Merged state + const classList = []; + const style = { ...resolvedProps.style }; + + // The node has class names that we need to override. + // Only pass on a class name when the style is unchanged. + domClassList.forEach((c) => { + const prop = getStyleProp(c); + if (resolvedProps.className.indexOf(prop) === -1) { + classList.push(c); + } + }); + + // The node has styles that we need to override. + // Remove any inline style that may collide with a new class name. + resolvedClassList.forEach((c) => { + const prop = getStyleProp(c); + classList.push(c); + style[prop] = null; + }); + + const className = `\n${classList.sort().join('\n')}`; + + const props = { + ...nativeProps, + className, + style + }; + + UIManager.updateView(node, props, this); } };