diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index 884b8c3dc5e..fdde0f21ec2 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -37,6 +37,9 @@ src/renderers/dom/shared/__tests__/ReactRenderDocument-test.js * should throw on full document render w/ no markup * supports findDOMNode on full-page components +src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js +* should control a value in reentrant events + src/renderers/shared/__tests__/ReactPerf-test.js * should count no-op update as waste * should count no-op update in child as waste diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index e1415a100ab..21c98427165 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -546,6 +546,7 @@ src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js * findDOMNode should find dom element after expanding a fragment * should bubble events from the portal to the parent * should not onMouseLeave when staying in the portal +* should not update event handlers until commit * should not crash encountering low-priority tree * throws if non-element passed to top-level render * throws if something other than false, null, or an element is returned from render @@ -959,7 +960,6 @@ src/renderers/dom/shared/wrappers/__tests__/ReactDOMIframe-test.js src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js * should properly control a value even if no event listener exists -* should control a value in reentrant events * should control values in reentrant events with different targets * should display `defaultValue` of number 0 * only assigns defaultValue if it changes diff --git a/src/renderers/art/ReactARTFiber.js b/src/renderers/art/ReactARTFiber.js index ee9aa7d8591..fcdcc957e0f 100644 --- a/src/renderers/art/ReactARTFiber.js +++ b/src/renderers/art/ReactARTFiber.js @@ -42,6 +42,8 @@ const TYPES = { TEXT: 'Text', }; +const UPDATE_SIGNAL = {}; + /** Helper Methods */ function addEventListeners(instance, type, listener) { @@ -418,7 +420,7 @@ const ARTRenderer = ReactFiberReconciler({ // Noop }, - commitUpdate(instance, type, oldProps, newProps) { + commitUpdate(instance, updatePayload, type, oldProps, newProps) { instance._applyProps(instance, newProps, oldProps); }, @@ -482,7 +484,7 @@ const ARTRenderer = ReactFiberReconciler({ }, prepareUpdate(domElement, type, oldProps, newProps) { - return true; + return UPDATE_SIGNAL; }, removeChild(parentInstance, child) { diff --git a/src/renderers/dom/fiber/ReactDOMFiber.js b/src/renderers/dom/fiber/ReactDOMFiber.js index fc7cb30e83a..487013cbc19 100644 --- a/src/renderers/dom/fiber/ReactDOMFiber.js +++ b/src/renderers/dom/fiber/ReactDOMFiber.js @@ -35,9 +35,13 @@ var { createElement, getChildNamespace, setInitialProperties, + diffProperties, updateProperties, } = ReactDOMFiberComponent; -var { precacheFiberNode } = ReactDOMComponentTree; +var { + precacheFiberNode, + updateFiberEventHandlers, +} = ReactDOMComponentTree; if (__DEV__) { var validateDOMNesting = require('validateDOMNesting'); @@ -184,6 +188,7 @@ var DOMRenderer = ReactFiberReconciler({ } const domElement : Instance = createElement(type, props, rootContainerInstance, parentNamespace); precacheFiberNode(internalInstanceHandle, domElement); + updateFiberEventHandlers(domElement, props); return domElement; }, @@ -206,8 +211,9 @@ var DOMRenderer = ReactFiberReconciler({ type : string, oldProps : Props, newProps : Props, + rootContainerInstance : Container, hostContext : HostContext, - ) : boolean { + ) : null | Array { if (__DEV__) { const hostContextDev = ((hostContext : any) : HostContextDev); if (typeof newProps.children !== typeof oldProps.children && ( @@ -218,14 +224,13 @@ var DOMRenderer = ReactFiberReconciler({ validateDOMNesting(null, String(newProps.children), null, ownAncestorInfo); } } - return true; + return diffProperties(domElement, type, oldProps, newProps, rootContainerInstance); }, commitMount( domElement : Instance, type : string, newProps : Props, - rootContainerInstance : Container, internalInstanceHandle : Object, ) : void { ((domElement : any) : HTMLButtonElement | HTMLInputElement | HTMLSelectElement | HTMLTextAreaElement).focus(); @@ -233,16 +238,17 @@ var DOMRenderer = ReactFiberReconciler({ commitUpdate( domElement : Instance, + updatePayload : Array, type : string, oldProps : Props, newProps : Props, - rootContainerInstance : Container, internalInstanceHandle : Object, ) : void { - // Update the internal instance handle so that we know which props are - // the current ones. - precacheFiberNode(internalInstanceHandle, domElement); - updateProperties(domElement, type, oldProps, newProps, rootContainerInstance); + // Update the props handle so that we know which props are the ones with + // with current event handlers. + updateFiberEventHandlers(domElement, newProps); + // Apply the diff to the DOM node. + updateProperties(domElement, updatePayload, type, oldProps, newProps); }, shouldSetTextContent(props : Props) : boolean { diff --git a/src/renderers/dom/fiber/ReactDOMFiberComponent.js b/src/renderers/dom/fiber/ReactDOMFiberComponent.js index f75c0ad9ebb..58d0a166d2a 100644 --- a/src/renderers/dom/fiber/ReactDOMFiberComponent.js +++ b/src/renderers/dom/fiber/ReactDOMFiberComponent.js @@ -301,69 +301,15 @@ function isCustomComponent(tagName, props) { return tagName.indexOf('-') >= 0 || props.is != null; } -/** - * Reconciles the properties by detecting differences in property values and - * updating the DOM as necessary. This function is probably the single most - * critical path for performance optimization. - * - * TODO: Benchmark whether checking for changed values in memory actually - * improves performance (especially statically positioned elements). - * TODO: Benchmark the effects of putting this at the top since 99% of props - * do not change for a given reconciliation. - * TODO: Benchmark areas that can be improved with caching. - */ -function updateDOMProperties( +function setInitialDOMProperties( domElement : Element, rootContainerElement : Element, - lastProps : null | Object, nextProps : Object, - wasCustomComponentTag : boolean, isCustomComponentTag : boolean, ) : void { - var propKey; - var styleName; - var styleUpdates; - for (propKey in lastProps) { - if (nextProps.hasOwnProperty(propKey) || - !lastProps.hasOwnProperty(propKey) || - lastProps[propKey] == null) { - continue; - } - if (propKey === STYLE) { - var lastStyle = lastProps[propKey]; - for (styleName in lastStyle) { - if (lastStyle.hasOwnProperty(styleName)) { - styleUpdates = styleUpdates || {}; - styleUpdates[styleName] = ''; - } - } - } else if (propKey === DANGEROUSLY_SET_INNER_HTML || - propKey === CHILDREN) { - // TODO: Clear innerHTML. This is currently broken in Fiber because we are - // too late to clear everything at this point because new children have - // already been inserted. - } else if (propKey === SUPPRESS_CONTENT_EDITABLE_WARNING) { - // Noop - } else if (registrationNameModules.hasOwnProperty(propKey)) { - // Do nothing for deleted listeners. - } else if (wasCustomComponentTag) { - DOMPropertyOperations.deleteValueForAttribute( - domElement, - propKey - ); - } else if ( - DOMProperty.properties[propKey] || - DOMProperty.isCustomAttribute(propKey)) { - DOMPropertyOperations.deleteValueForProperty(domElement, propKey); - } - } - for (propKey in nextProps) { + for (var propKey in nextProps) { var nextProp = nextProps[propKey]; - var lastProp = - lastProps != null ? lastProps[propKey] : undefined; - if (!nextProps.hasOwnProperty(propKey) || - nextProp === lastProp || - nextProp == null && lastProp == null) { + if (!nextProps.hasOwnProperty(propKey)) { continue; } if (propKey === STYLE) { @@ -374,38 +320,16 @@ function updateDOMProperties( Object.freeze(nextProp); } } - if (lastProp) { - // Unset styles on `lastProp` but not on `nextProp`. - for (styleName in lastProp) { - if (lastProp.hasOwnProperty(styleName) && - (!nextProp || !nextProp.hasOwnProperty(styleName))) { - styleUpdates = styleUpdates || {}; - styleUpdates[styleName] = ''; - } - } - // Update styles that changed since `lastProp`. - for (styleName in nextProp) { - if (nextProp.hasOwnProperty(styleName) && - lastProp[styleName] !== nextProp[styleName]) { - styleUpdates = styleUpdates || {}; - styleUpdates[styleName] = nextProp[styleName]; - } - } - } else { - // Relies on `updateStylesByID` not mutating `styleUpdates`. - styleUpdates = nextProp; - } + // Relies on `updateStylesByID` not mutating `styleUpdates`. + // TODO: call ReactInstrumentation.debugTool.onHostOperation in DEV. + CSSPropertyOperations.setValueForStyles( + domElement, + nextProp, + ); } else if (propKey === DANGEROUSLY_SET_INNER_HTML) { var nextHtml = nextProp ? nextProp[HTML] : undefined; - var lastHtml = lastProp ? lastProp[HTML] : undefined; - // Intentional use of != to avoid catching zero/false. if (nextHtml != null) { - if (lastHtml !== nextHtml) { - setInnerHTML(domElement, '' + nextHtml); - } - } else { - // TODO: It might be too late to clear this if we have children - // inserted already. + setInnerHTML(domElement, nextHtml); } } else if (propKey === CHILDREN) { if (typeof nextProp === 'string') { @@ -433,18 +357,57 @@ function updateDOMProperties( // brings us in line with the same behavior we have on initial render. if (nextProp != null) { DOMPropertyOperations.setValueForProperty(domElement, propKey, nextProp); + } + } + } +} + +function updateDOMProperties( + domElement : Element, + updatePayload : Array, + wasCustomComponentTag : boolean, + isCustomComponentTag : boolean, +) : void { + // TODO: Handle wasCustomComponentTag + for (var i = 0; i < updatePayload.length; i+=2) { + var propKey = updatePayload[i]; + var propValue = updatePayload[i + 1]; + if (propKey === STYLE) { + // TODO: call ReactInstrumentation.debugTool.onHostOperation in DEV. + CSSPropertyOperations.setValueForStyles( + domElement, + propValue, + ); + } else if (propKey === DANGEROUSLY_SET_INNER_HTML) { + setInnerHTML(domElement, propValue); + } else if (propKey === CHILDREN) { + setTextContent(domElement, propValue); + } else if (isCustomComponentTag) { + if (propValue != null) { + DOMPropertyOperations.setValueForAttribute( + domElement, + propKey, + propValue + ); + } else { + DOMPropertyOperations.deleteValueForAttribute( + domElement, + propKey + ); + } + } else if ( + DOMProperty.properties[propKey] || + DOMProperty.isCustomAttribute(propKey)) { + // If we're updating to null or undefined, we should remove the property + // from the DOM node instead of inadvertently setting to a string. This + // brings us in line with the same behavior we have on initial render. + if (propValue != null) { + DOMPropertyOperations.setValueForProperty(domElement, propKey, propValue); } else { DOMPropertyOperations.deleteValueForProperty(domElement, propKey); } } } - if (styleUpdates) { - // TODO: call ReactInstrumentation.debugTool.onHostOperation in DEV. - CSSPropertyOperations.setValueForStyles( - domElement, - styleUpdates, - ); - } } // Assumes there is no parent namespace. @@ -592,12 +555,10 @@ var ReactDOMFiberComponent = { assertValidProps(tag, props); - updateDOMProperties( + setInitialDOMProperties( domElement, rootContainerElement, - null, props, - false, isCustomComponentTag ); @@ -626,35 +587,42 @@ var ReactDOMFiberComponent = { } }, - updateProperties( + // Calculate the diff between the two objects. + diffProperties( domElement : Element, tag : string, lastRawProps : Object, nextRawProps : Object, rootContainerElement : Element, - ) : void { + ) : null | Array { if (__DEV__) { validatePropertiesInDevelopment(tag, nextRawProps); } + var updatePayload : null | Array = null; + var lastProps : Object; var nextProps : Object; switch (tag) { case 'input': lastProps = ReactDOMFiberInput.getHostProps(domElement, lastRawProps); nextProps = ReactDOMFiberInput.getHostProps(domElement, nextRawProps); + updatePayload = []; break; case 'option': lastProps = ReactDOMFiberOption.getHostProps(domElement, lastRawProps); nextProps = ReactDOMFiberOption.getHostProps(domElement, nextRawProps); + updatePayload = []; break; case 'select': lastProps = ReactDOMFiberSelect.getHostProps(domElement, lastRawProps); nextProps = ReactDOMFiberSelect.getHostProps(domElement, nextRawProps); + updatePayload = []; break; case 'textarea': lastProps = ReactDOMFiberTextarea.getHostProps(domElement, lastRawProps); nextProps = ReactDOMFiberTextarea.getHostProps(domElement, nextRawProps); + updatePayload = []; break; default: lastProps = lastRawProps; @@ -668,17 +636,154 @@ var ReactDOMFiberComponent = { } assertValidProps(tag, nextProps); - var wasCustomComponentTag = isCustomComponent(tag, lastProps); - var isCustomComponentTag = isCustomComponent(tag, nextProps); + + var propKey; + var styleName; + var styleUpdates = null; + for (propKey in lastProps) { + if (nextProps.hasOwnProperty(propKey) || + !lastProps.hasOwnProperty(propKey) || + lastProps[propKey] == null) { + continue; + } + if (propKey === STYLE) { + var lastStyle = lastProps[propKey]; + for (styleName in lastStyle) { + if (lastStyle.hasOwnProperty(styleName)) { + if (!styleUpdates) { + styleUpdates = {}; + } + styleUpdates[styleName] = ''; + } + } + } else if (propKey === DANGEROUSLY_SET_INNER_HTML || + propKey === CHILDREN) { + // Noop. This is handled by the clear text mechanism. + } else if (propKey === SUPPRESS_CONTENT_EDITABLE_WARNING) { + // Noop + } else if (registrationNameModules.hasOwnProperty(propKey)) { + // This is a special case. If any listener updates we need to ensure + // that the "current" fiber pointer gets updated so we need a commit + // to update this element. + if (!updatePayload) { + updatePayload = []; + } + } else { + // For all other deleted properties we add it to the queue. We use + // the whitelist in the commit phase instead. + (updatePayload = updatePayload || []).push(propKey, null); + } + } + for (propKey in nextProps) { + var nextProp = nextProps[propKey]; + var lastProp = + lastProps != null ? lastProps[propKey] : undefined; + if (!nextProps.hasOwnProperty(propKey) || + nextProp === lastProp || + nextProp == null && lastProp == null) { + continue; + } + if (propKey === STYLE) { + if (__DEV__) { + if (nextProp) { + // Freeze the next style object so that we can assume it won't be + // mutated. We have already warned for this in the past. + Object.freeze(nextProp); + } + } + if (lastProp) { + // Unset styles on `lastProp` but not on `nextProp`. + for (styleName in lastProp) { + if (lastProp.hasOwnProperty(styleName) && + (!nextProp || !nextProp.hasOwnProperty(styleName))) { + if (!styleUpdates) { + styleUpdates = {}; + } + styleUpdates[styleName] = ''; + } + } + // Update styles that changed since `lastProp`. + for (styleName in nextProp) { + if (nextProp.hasOwnProperty(styleName) && + lastProp[styleName] !== nextProp[styleName]) { + if (!styleUpdates) { + styleUpdates = {}; + } + styleUpdates[styleName] = nextProp[styleName]; + } + } + } else { + // Relies on `updateStylesByID` not mutating `styleUpdates`. + if (!styleUpdates) { + if (!updatePayload) { + updatePayload = []; + } + updatePayload.push(propKey, styleUpdates); + } + styleUpdates = nextProp; + } + } else if (propKey === DANGEROUSLY_SET_INNER_HTML) { + var nextHtml = nextProp ? nextProp[HTML] : undefined; + var lastHtml = lastProp ? lastProp[HTML] : undefined; + if (nextHtml != null) { + if (lastHtml !== nextHtml) { + (updatePayload = updatePayload || []).push(propKey, '' + nextHtml); + } + } else { + // TODO: It might be too late to clear this if we have children + // inserted already. + } + } else if (propKey === CHILDREN) { + if (lastProp !== nextProp && ( + typeof nextProp === 'string' || typeof nextProp === 'number' + )) { + (updatePayload = updatePayload || []).push(propKey, '' + nextProp); + } + } else if (propKey === SUPPRESS_CONTENT_EDITABLE_WARNING) { + // Noop + } else if (registrationNameModules.hasOwnProperty(propKey)) { + if (nextProp) { + // We eagerly listen to this even though we haven't committed yet. + ensureListeningTo(rootContainerElement, propKey); + } + if (!updatePayload && lastProp !== nextProp) { + // This is a special case. If any listener updates we need to ensure + // that the "current" props pointer gets updated so we need a commit + // to update this element. + updatePayload = []; + } + } else { + // For any other property we always add it to the queue and then we + // filter it out using the whitelist during the commit. + (updatePayload = updatePayload || []).push(propKey, nextProp); + } + } + if (styleUpdates) { + (updatePayload = updatePayload || []).push(STYLE, styleUpdates); + } + return updatePayload; + }, + + // Apply the diff. + updateProperties( + domElement : Element, + updatePayload : Array, + tag : string, + lastRawProps : Object, + nextRawProps : Object + ) : void { + var wasCustomComponentTag = isCustomComponent(tag, lastRawProps); + var isCustomComponentTag = isCustomComponent(tag, nextRawProps); + // Apply the diff. updateDOMProperties( domElement, - rootContainerElement, - lastProps, - nextProps, + updatePayload, wasCustomComponentTag, isCustomComponentTag ); + // TODO: Ensure that an update gets scheduled if any of the special props + // changed. switch (tag) { case 'input': // Update the wrapper around inputs *after* updating props. This has to diff --git a/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js b/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js index ea7f39da297..41c7f129b79 100644 --- a/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js +++ b/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js @@ -994,6 +994,89 @@ describe('ReactDOMFiber', () => { ]); }); + it('should not update event handlers until commit', () => { + let ops = []; + const handlerA = () => ops.push('A'); + const handlerB = () => ops.push('B'); + + class Example extends React.Component { + state = { flip: false, count: 0 }; + flip() { + this.setState({ flip: true, count: this.state.count + 1 }); + } + tick() { + this.setState({ count: this.state.count + 1 }); + } + render() { + const useB = !this.props.forceA && this.state.flip; + return ( +
+ ); + } + } + + class Click extends React.Component { + constructor() { + super(); + click(node); + } + render() { + return null; + } + } + + let inst; + ReactDOM.render([ inst = n} />], container); + const node = container.firstChild; + expect(node.tagName).toEqual('DIV'); + + function click(target) { + var fakeNativeEvent = {}; + ReactTestUtils.simulateNativeEventOnNode( + 'topClick', + target, + fakeNativeEvent + ); + } + + click(node); + + expect(ops).toEqual(['A']); + ops = []; + + // Render with the other event handler. + inst.flip(); + + click(node); + + expect(ops).toEqual(['B']); + ops = []; + + // Rerender without changing any props. + inst.tick(); + + click(node); + + expect(ops).toEqual(['B']); + ops = []; + + // Render a flip back to the A handler. The second component invokes the + // click handler during render to simulate a click during an aborted + // render. I use this hack because at current time we don't have a way to + // test aborted ReactDOM renders. + ReactDOM.render([, ], container); + + // Because the new click handler has not yet committed, we should still + // invoke B. + expect(ops).toEqual(['B']); + ops = []; + + // Any click that happens after commit, should invoke A. + click(node); + expect(ops).toEqual(['A']); + + }); + it('should not crash encountering low-priority tree', () => { ReactDOM.render(