From 705ce56694823fcfdc5075e6a0e5f1350725c1fd Mon Sep 17 00:00:00 2001 From: Ben Alpert Date: Tue, 11 Jun 2013 16:44:51 -0700 Subject: [PATCH] Correctly remove attributes when deleting props The most obvious manifestation of this bug is visible here: http://jsfiddle.net/spicyj/zzGas/. In short, when props are removed from a component, the underlying HTML element doesn't have the attribute removed. This change should fix it, but unfortunately it (presumably) makes _updateDOMProperties a bit slower. --- src/core/ReactComponent.js | 12 ++++ src/core/ReactDOMIDOperations.js | 21 +++++- src/core/ReactEventEmitter.js | 2 + src/core/ReactNativeComponent.js | 72 ++++++++++++++++--- .../__tests__/ReactNativeComponent-test.js | 49 +++++++++++++ src/dom/CSSProperty.js | 56 ++++++++++++++- src/dom/CSSPropertyOperations.js | 28 ++++++-- src/dom/DOMProperty.js | 33 +++++++-- src/dom/DOMPropertyOperations.js | 31 +++++++- src/event/EventPluginHub.js | 1 + 10 files changed, 279 insertions(+), 26 deletions(-) diff --git a/src/core/ReactComponent.js b/src/core/ReactComponent.js index fe704dfbbf..949f82f805 100644 --- a/src/core/ReactComponent.js +++ b/src/core/ReactComponent.js @@ -35,6 +35,13 @@ var merge = require('merge'); */ var OWNER = '{owner}'; +/** + * Internal properties that shouldn't be considered when modifying node + * attributes. + */ +var internalPropNames = {children: true}; +internalPropNames[OWNER] = true; + /** * Every React component is in one of these life cycles. */ @@ -131,6 +138,11 @@ function appendNestedChildren(parentKey, sourceArray, targetArray) { */ var ReactComponent = { + /** + * @internal + */ + internalPropNames: internalPropNames, + /** * @param {?object} object * @return {boolean} True if `object` is a valid component. diff --git a/src/core/ReactDOMIDOperations.js b/src/core/ReactDOMIDOperations.js index da779d352f..b51ffbd019 100644 --- a/src/core/ReactDOMIDOperations.js +++ b/src/core/ReactDOMIDOperations.js @@ -75,6 +75,24 @@ var ReactDOMIDOperations = { DOMPropertyOperations.setValueForProperty(node, name, value); }, + /** + * Updates a DOM node to remove a property. This should only be used to remove + * DOM properties in `DOMProperty`. + * + * @param {string} id ID of the node to update. + * @param {string} name A property name to remove, see `DOMProperty`. + * @internal + */ + deletePropertyByID: function(id, name, value) { + var node = ReactDOMNodeCache.getCachedNodeByID(id); + invariant( + !INVALID_PROPERTY_ERRORS.hasOwnProperty(name), + 'updatePropertyByID(...): %s', + INVALID_PROPERTY_ERRORS[name] + ); + DOMPropertyOperations.deleteValueForProperty(node, name, value); + }, + /** * This should almost never be used instead of `updatePropertyByID()` due to * the extra object allocation required by the API. That said, this is useful @@ -95,7 +113,8 @@ var ReactDOMIDOperations = { }, /** - * Updates a DOM node with new style values. + * Updates a DOM node with new style values. If a value is specified as '', + * the corresponding style property will be unset. * * @param {string} id ID of the node to update. * @param {object} styles Mapping from styles to values. diff --git a/src/core/ReactEventEmitter.js b/src/core/ReactEventEmitter.js index ccd74c08a8..aa302815bc 100644 --- a/src/core/ReactEventEmitter.js +++ b/src/core/ReactEventEmitter.js @@ -323,6 +323,8 @@ var ReactEventEmitter = { getListener: EventPluginHub.getListener, + deleteListener: EventPluginHub.deleteListener, + deleteAllListeners: EventPluginHub.deleteAllListeners, trapBubbledEvent: trapBubbledEvent, diff --git a/src/core/ReactNativeComponent.js b/src/core/ReactNativeComponent.js index 82bfcf7002..15f614beb1 100644 --- a/src/core/ReactNativeComponent.js +++ b/src/core/ReactNativeComponent.js @@ -32,7 +32,10 @@ var keyOf = require('keyOf'); var merge = require('merge'); var mixInto = require('mixInto'); +var internalPropNames = ReactComponent.internalPropNames; + var putListener = ReactEventEmitter.putListener; +var deleteListener = ReactEventEmitter.deleteListener; var registrationNames = ReactEventEmitter.registrationNames; // For quickly matching children type, to test if can be treated as content. @@ -206,9 +209,46 @@ ReactNativeComponent.Mixin = { */ _updateDOMProperties: function(nextProps) { var lastProps = this.props; - for (var propKey in nextProps) { - var nextProp = nextProps[propKey]; - var lastProp = lastProps[propKey]; + var propKey; + var nextProp; + var lastProp; + var styleName; + var styleUpdates; + for (propKey in lastProps) { + nextProp = nextProps[propKey]; + lastProp = lastProps[propKey]; + if (!lastProps.hasOwnProperty(propKey) || nextProp) { + continue; + } + if (propKey === STYLE) { + for (styleName in lastProp) { + if (!lastProp.hasOwnProperty(styleName)) { + continue; + } + if (!styleUpdates) { + styleUpdates = {}; + } + styleUpdates[styleName] = ''; + } + } else if (propKey === DANGEROUSLY_SET_INNER_HTML || + propKey === CONTENT) { + ReactComponent.DOMIDOperations.updateTextContentByID( + this._rootNodeID, + '' + ); + } else if (internalPropNames[propKey]) { + } else if (registrationNames[propKey]) { + deleteListener(this._rootNodeID, propKey); + } else { + ReactComponent.DOMIDOperations.deletePropertyByID( + this._rootNodeID, + propKey + ); + } + } + for (propKey in nextProps) { + nextProp = nextProps[propKey]; + lastProp = lastProps[propKey]; if (!nextProps.hasOwnProperty(propKey) || nextProp === lastProp) { continue; } @@ -216,8 +256,17 @@ ReactNativeComponent.Mixin = { if (nextProp) { nextProp = nextProps.style = merge(nextProp); } - var styleUpdates; - for (var styleName in nextProp) { + if (lastProp) { + for (styleName in lastProp) { + if (lastProp.hasOwnProperty(styleName) && !nextProp[styleName]) { + if (!styleUpdates) { + styleUpdates = {}; + } + styleUpdates[styleName] = ''; + } + } + } + for (styleName in nextProp) { if (!nextProp.hasOwnProperty(styleName)) { continue; } @@ -228,12 +277,6 @@ ReactNativeComponent.Mixin = { styleUpdates[styleName] = nextProp[styleName]; } } - if (styleUpdates) { - ReactComponent.DOMIDOperations.updateStylesByID( - this._rootNodeID, - styleUpdates - ); - } } else if (propKey === DANGEROUSLY_SET_INNER_HTML) { var lastHtml = lastProp && lastProp.__html; var nextHtml = nextProp && nextProp.__html; @@ -248,6 +291,7 @@ ReactNativeComponent.Mixin = { this._rootNodeID, '' + nextProp ); + } else if (internalPropNames[propKey]) { } else if (registrationNames[propKey]) { putListener(this._rootNodeID, propKey, nextProp); } else { @@ -258,6 +302,12 @@ ReactNativeComponent.Mixin = { ); } } + if (styleUpdates) { + ReactComponent.DOMIDOperations.updateStylesByID( + this._rootNodeID, + styleUpdates + ); + } }, /** diff --git a/src/core/__tests__/ReactNativeComponent-test.js b/src/core/__tests__/ReactNativeComponent-test.js index 2219567ffc..ee7e986593 100644 --- a/src/core/__tests__/ReactNativeComponent-test.js +++ b/src/core/__tests__/ReactNativeComponent-test.js @@ -100,6 +100,55 @@ describe('ReactNativeComponent', function() { expect(stubStyle.display).toEqual('block'); }); + it("should remove attributes", function() { + var stub = ReactTestUtils.renderIntoDocument(); + + expect(stub.getDOMNode().hasAttribute('height')).toBe(true); + stub.receiveProps({}, transaction); + expect(stub.getDOMNode().hasAttribute('height')).toBe(false); + }); + + it("should remove properties", function() { + var stub = ReactTestUtils.renderIntoDocument(
); + + expect(stub.getDOMNode().className).toEqual('monkey'); + stub.receiveProps({}, transaction); + expect(stub.getDOMNode().className).toEqual(''); + }); + + it("should clear a single style prop when changing 'style'", function() { + var styles = {display: 'none', color: 'red'}; + var stub = ReactTestUtils.renderIntoDocument(
); + + var stubStyle = stub.getDOMNode().style; + + styles = {color: 'green'}; + stub.receiveProps({ style: styles }, transaction); + expect(stubStyle.display).toEqual(''); + expect(stubStyle.color).toEqual('green'); + }); + + it("should clear all the styles when removing 'style'", function() { + var styles = {display: 'none', color: 'red'}; + var stub = ReactTestUtils.renderIntoDocument(
); + + var stubStyle = stub.getDOMNode().style; + + stub.receiveProps({}, transaction); + expect(stubStyle.display).toEqual(''); + expect(stubStyle.color).toEqual(''); + }); + + it("should empty element when removing innerHTML", function() { + var stub = ReactTestUtils.renderIntoDocument( +
+ ); + + expect(stub.getDOMNode().innerHTML).toEqual(':)'); + stub.receiveProps({}, transaction); + expect(stub.getDOMNode().innerHTML).toEqual(''); + }); + }); describe('createOpenTagMarkup', function() { diff --git a/src/dom/CSSProperty.js b/src/dom/CSSProperty.js index 95586d5a7c..619fc2c3bf 100644 --- a/src/dom/CSSProperty.js +++ b/src/dom/CSSProperty.js @@ -18,6 +18,8 @@ "use strict"; +var keyMirror = require('keyMirror'); + /** * CSS properties which accept numbers but are not in units of "px". */ @@ -30,8 +32,60 @@ var isUnitlessNumber = { zoom: true }; +/** + * Most style properties can be unset by doing .style[prop] = '' but IE8 + * doesn't like doing that with shorthand properties so for the properties that + * IE8 breaks on, which are listed here, we instead unset each of the + * individual properties. See http://bugs.jquery.com/ticket/12385. + * The 4-value 'clock' properties like margin, padding, border-width seem to + * behave without any problems. Curiously, list-style works too without any + * special prodding. + */ +var shorthandPropertyExpansions = { + background: { + backgroundImage: true, + backgroundPosition: true, + backgroundRepeat: true, + backgroundColor: true + }, + border: { + borderWidth: true, + borderStyle: true, + borderColor: true + }, + borderBottom: { + borderBottomWidth: true, + borderBottomStyle: true, + borderBottomColor: true + }, + borderLeft: { + borderLeftWidth: true, + borderLeftStyle: true, + borderLeftColor: true + }, + borderRight: { + borderRightWidth: true, + borderRightStyle: true, + borderRightColor: true + }, + borderTop: { + borderTopWidth: true, + borderTopStyle: true, + borderTopColor: true + }, + font: { + fontStyle: true, + fontVariant: true, + fontWeight: true, + fontSize: true, + lineHeight: true, + fontFamily: true + } +}; + var CSSProperty = { - isUnitlessNumber: isUnitlessNumber + isUnitlessNumber: isUnitlessNumber, + shorthandPropertyExpansions: shorthandPropertyExpansions }; module.exports = CSSProperty; diff --git a/src/dom/CSSPropertyOperations.js b/src/dom/CSSPropertyOperations.js index 4c67a4cbe8..186043589a 100644 --- a/src/dom/CSSPropertyOperations.js +++ b/src/dom/CSSPropertyOperations.js @@ -19,10 +19,12 @@ "use strict"; -var dangerousStyleValue = require('dangerousStyleValue'); -var escapeTextForBrowser = require('escapeTextForBrowser'); -var hyphenate = require('hyphenate'); -var memoizeStringOnly = require('memoizeStringOnly'); +var CSSProperty = require('CSSProperty'); + +var dangerousStyleValue = require('./dangerousStyleValue'); +var escapeTextForBrowser = require('./escapeTextForBrowser'); +var hyphenate = require('./hyphenate'); +var memoizeStringOnly = require('./memoizeStringOnly'); var processStyleName = memoizeStringOnly(function(styleName) { return escapeTextForBrowser(hyphenate(styleName)); @@ -60,7 +62,8 @@ var CSSPropertyOperations = { }, /** - * Sets the value for multiple styles on a node. + * Sets the value for multiple styles on a node. If a value is specified as + * '' (empty string), the corresponding style property will be unset. * * @param {DOMElement} node * @param {object} styles @@ -72,7 +75,20 @@ var CSSPropertyOperations = { continue; } var styleValue = styles[styleName]; - style[styleName] = dangerousStyleValue(styleName, styleValue); + if (!styleValue) { + var expansion = CSSProperty.shorthandPropertyExpansions[styleName]; + if (expansion) { + // Shorthand property that IE8 won't like unsetting, so unset each + // component to placate it + for (var individualStyleName in expansion) { + style[individualStyleName] = ''; + } + } else { + style[styleName] = ''; + } + } else { + style[styleName] = dangerousStyleValue(styleName, styleValue); + } } } diff --git a/src/dom/DOMProperty.js b/src/dom/DOMProperty.js index f1950b7fed..a08f628ef3 100644 --- a/src/dom/DOMProperty.js +++ b/src/dom/DOMProperty.js @@ -23,6 +23,8 @@ var invariant = require('invariant'); +var defaultValueCache = {}; + /** * DOMProperty exports lookup objects that can be used like functions: * @@ -98,7 +100,29 @@ var DOMProperty = { */ isCustomAttribute: RegExp.prototype.test.bind( /^(data|aria)-[a-z_][a-z\d_.\-]*$/ - ) + ), + + /** + * Returns the default property value for a DOM property (i.e., not an + * attribute). Most default values are '' or false, but not all. Worse yet, + * some (in particular, `type`) vary depending on the type of element. + * + * TODO: Is it worth caching the test elements? Caching the properties + * ourselves (as opposed to accessing from a cached test element every time) + * looks probably worth it: http://jsperf.com/object-vs-element + */ + getDefaultValueForProperty: function(nodeName, prop) { + var nodeDefaults = defaultValueCache[nodeName]; + var testElement; + if (!nodeDefaults) { + defaultValueCache[nodeName] = nodeDefaults = {}; + } + if (!(prop in nodeDefaults)) { + testElement = document.createElement(nodeName); + nodeDefaults[prop] = testElement[prop]; + } + return nodeDefaults[prop]; + } }; /** @@ -132,7 +156,7 @@ var Properties = { dir: null, disabled: MustUseProperty | HasBooleanValue, enctype: null, - height: null, + height: MustUseAttribute, href: null, htmlFor: null, max: null, @@ -158,7 +182,7 @@ var Properties = { title: null, type: null, value: MustUseProperty | HasSideEffects, - width: null, + width: MustUseAttribute, wmode: MustUseAttribute, /** * SVG Properties @@ -212,7 +236,8 @@ var DOMPropertyNames = { }; /** - * Properties that require special mutation methods. + * Properties that require special mutation methods. If `value` is undefined, + * the mutation method should unset the property. */ var DOMMutationMethods = { /** diff --git a/src/dom/DOMPropertyOperations.js b/src/dom/DOMPropertyOperations.js index 0e2c2c3e8f..d280e1fde3 100644 --- a/src/dom/DOMPropertyOperations.js +++ b/src/dom/DOMPropertyOperations.js @@ -19,10 +19,10 @@ "use strict"; -var DOMProperty = require('DOMProperty'); +var DOMProperty = require("./DOMProperty"); -var escapeTextForBrowser = require('escapeTextForBrowser'); -var memoizeStringOnly = require('memoizeStringOnly'); +var escapeTextForBrowser = require("./escapeTextForBrowser"); +var memoizeStringOnly = require("./memoizeStringOnly"); var processAttributeNameAndPrefix = memoizeStringOnly(function(name) { return escapeTextForBrowser(name) + '="'; @@ -86,6 +86,31 @@ var DOMPropertyOperations = { } else if (DOMProperty.isCustomAttribute(name)) { node.setAttribute(name, value); } + }, + + /** + * Deletes the value for a property on a node. + * + * @param {DOMElement} node + * @param {string} name + */ + deleteValueForProperty: function(node, name) { + if (DOMProperty.isStandardName[name]) { + var mutationMethod = DOMProperty.getMutationMethod[name]; + if (mutationMethod) { + mutationMethod(node, undefined); + } else if (DOMProperty.mustUseAttribute[name]) { + node.removeAttribute(DOMProperty.getAttributeName[name]); + } else { + var propName = DOMProperty.getPropertyName[name]; + node[propName] = DOMProperty.getDefaultValueForProperty( + node.nodeName, + name + ); + } + } else if (DOMProperty.isCustomAttribute(name)) { + node.removeAttribute(name); + } } }; diff --git a/src/event/EventPluginHub.js b/src/event/EventPluginHub.js index 99ce2b008b..7fd1100d8d 100644 --- a/src/event/EventPluginHub.js +++ b/src/event/EventPluginHub.js @@ -312,6 +312,7 @@ var EventPluginHub = { registrationNamesArr: registrationNamesArr, putListener: CallbackRegistry.putListener, getListener: CallbackRegistry.getListener, + deleteListener: CallbackRegistry.deleteListener, deleteAllListeners: deleteAllListeners, extractAbstractEvents: extractAbstractEvents, enqueueAbstractEvents: enqueueAbstractEvents,