@samitouri / QOS-React-2 / commits / 705ce56694
Fork 0

The library for web and native user interfaces.

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.

Ben Alpert committed Jun 11, 2013 at 16:44 UTC 705ce56694823fcfdc5075e6a0e5f1350725c1fd
10 files changed +279 -26
src/core/ReactComponent.js
+12
@@ -35,6 +35,13 @@ var merge = require('merge');
35 */
36 var OWNER = '{owner}';
37
38 +/**
39 + * Internal properties that shouldn't be considered when modifying node
40 + * attributes.
41 + */
42 +var internalPropNames = {children: true};
43 +internalPropNames[OWNER] = true;
44 +
45 /**
46 * Every React component is in one of these life cycles.
47 */
@@ -131,6 +138,11 @@ function appendNestedChildren(parentKey, sourceArray, targetArray) {
138 */
139 var ReactComponent = {
140
141 + /**
142 + * @internal
143 + */
144 + internalPropNames: internalPropNames,
145 +
146 /**
147 * @param {?object} object
148 * @return {boolean} True if `object` is a valid component.
src/core/ReactDOMIDOperations.js
+20 -1
@@ -75,6 +75,24 @@ var ReactDOMIDOperations = {
75 DOMPropertyOperations.setValueForProperty(node, name, value);
76 },
77
78 + /**
79 + * Updates a DOM node to remove a property. This should only be used to remove
80 + * DOM properties in `DOMProperty`.
81 + *
82 + * @param {string} id ID of the node to update.
83 + * @param {string} name A property name to remove, see `DOMProperty`.
84 + * @internal
85 + */
86 + deletePropertyByID: function(id, name, value) {
87 + var node = ReactDOMNodeCache.getCachedNodeByID(id);
88 + invariant(
89 + !INVALID_PROPERTY_ERRORS.hasOwnProperty(name),
90 + 'updatePropertyByID(...): %s',
91 + INVALID_PROPERTY_ERRORS[name]
92 + );
93 + DOMPropertyOperations.deleteValueForProperty(node, name, value);
94 + },
95 +
96 /**
97 * This should almost never be used instead of `updatePropertyByID()` due to
98 * the extra object allocation required by the API. That said, this is useful
@@ -95,7 +113,8 @@ var ReactDOMIDOperations = {
113 },
114
115 /**
98 - * Updates a DOM node with new style values.
116 + * Updates a DOM node with new style values. If a value is specified as '',
117 + * the corresponding style property will be unset.
118 *
119 * @param {string} id ID of the node to update.
120 * @param {object} styles Mapping from styles to values.
src/core/ReactEventEmitter.js
+2
@@ -323,6 +323,8 @@ var ReactEventEmitter = {
323
324 getListener: EventPluginHub.getListener,
325
326 + deleteListener: EventPluginHub.deleteListener,
327 +
328 deleteAllListeners: EventPluginHub.deleteAllListeners,
329
330 trapBubbledEvent: trapBubbledEvent,
src/core/ReactNativeComponent.js
+61 -11
@@ -32,7 +32,10 @@ var keyOf = require('keyOf');
32 var merge = require('merge');
33 var mixInto = require('mixInto');
34
35 +var internalPropNames = ReactComponent.internalPropNames;
36 +
37 var putListener = ReactEventEmitter.putListener;
38 +var deleteListener = ReactEventEmitter.deleteListener;
39 var registrationNames = ReactEventEmitter.registrationNames;
40
41 // For quickly matching children type, to test if can be treated as content.
@@ -206,9 +209,46 @@ ReactNativeComponent.Mixin = {
209 */
210 _updateDOMProperties: function(nextProps) {
211 var lastProps = this.props;
209 - for (var propKey in nextProps) {
210 - var nextProp = nextProps[propKey];
211 - var lastProp = lastProps[propKey];
212 + var propKey;
213 + var nextProp;
214 + var lastProp;
215 + var styleName;
216 + var styleUpdates;
217 + for (propKey in lastProps) {
218 + nextProp = nextProps[propKey];
219 + lastProp = lastProps[propKey];
220 + if (!lastProps.hasOwnProperty(propKey) || nextProp) {
221 + continue;
222 + }
223 + if (propKey === STYLE) {
224 + for (styleName in lastProp) {
225 + if (!lastProp.hasOwnProperty(styleName)) {
226 + continue;
227 + }
228 + if (!styleUpdates) {
229 + styleUpdates = {};
230 + }
231 + styleUpdates[styleName] = '';
232 + }
233 + } else if (propKey === DANGEROUSLY_SET_INNER_HTML ||
234 + propKey === CONTENT) {
235 + ReactComponent.DOMIDOperations.updateTextContentByID(
236 + this._rootNodeID,
237 + ''
238 + );
239 + } else if (internalPropNames[propKey]) {
240 + } else if (registrationNames[propKey]) {
241 + deleteListener(this._rootNodeID, propKey);
242 + } else {
243 + ReactComponent.DOMIDOperations.deletePropertyByID(
244 + this._rootNodeID,
245 + propKey
246 + );
247 + }
248 + }
249 + for (propKey in nextProps) {
250 + nextProp = nextProps[propKey];
251 + lastProp = lastProps[propKey];
252 if (!nextProps.hasOwnProperty(propKey) || nextProp === lastProp) {
253 continue;
254 }
@@ -216,8 +256,17 @@ ReactNativeComponent.Mixin = {
256 if (nextProp) {
257 nextProp = nextProps.style = merge(nextProp);
258 }
219 - var styleUpdates;
220 - for (var styleName in nextProp) {
259 + if (lastProp) {
260 + for (styleName in lastProp) {
261 + if (lastProp.hasOwnProperty(styleName) && !nextProp[styleName]) {
262 + if (!styleUpdates) {
263 + styleUpdates = {};
264 + }
265 + styleUpdates[styleName] = '';
266 + }
267 + }
268 + }
269 + for (styleName in nextProp) {
270 if (!nextProp.hasOwnProperty(styleName)) {
271 continue;
272 }
@@ -228,12 +277,6 @@ ReactNativeComponent.Mixin = {
277 styleUpdates[styleName] = nextProp[styleName];
278 }
279 }
231 - if (styleUpdates) {
232 - ReactComponent.DOMIDOperations.updateStylesByID(
233 - this._rootNodeID,
234 - styleUpdates
235 - );
236 - }
280 } else if (propKey === DANGEROUSLY_SET_INNER_HTML) {
281 var lastHtml = lastProp && lastProp.__html;
282 var nextHtml = nextProp && nextProp.__html;
@@ -248,6 +291,7 @@ ReactNativeComponent.Mixin = {
291 this._rootNodeID,
292 '' + nextProp
293 );
294 + } else if (internalPropNames[propKey]) {
295 } else if (registrationNames[propKey]) {
296 putListener(this._rootNodeID, propKey, nextProp);
297 } else {
@@ -258,6 +302,12 @@ ReactNativeComponent.Mixin = {
302 );
303 }
304 }
305 + if (styleUpdates) {
306 + ReactComponent.DOMIDOperations.updateStylesByID(
307 + this._rootNodeID,
308 + styleUpdates
309 + );
310 + }
311 },
312
313 /**
src/core/__tests__/ReactNativeComponent-test.js
+49
@@ -100,6 +100,55 @@ describe('ReactNativeComponent', function() {
100 expect(stubStyle.display).toEqual('block');
101 });
102
103 + it("should remove attributes", function() {
104 + var stub = ReactTestUtils.renderIntoDocument(<img height='17' />);
105 +
106 + expect(stub.getDOMNode().hasAttribute('height')).toBe(true);
107 + stub.receiveProps({}, transaction);
108 + expect(stub.getDOMNode().hasAttribute('height')).toBe(false);
109 + });
110 +
111 + it("should remove properties", function() {
112 + var stub = ReactTestUtils.renderIntoDocument(<div className='monkey' />);
113 +
114 + expect(stub.getDOMNode().className).toEqual('monkey');
115 + stub.receiveProps({}, transaction);
116 + expect(stub.getDOMNode().className).toEqual('');
117 + });
118 +
119 + it("should clear a single style prop when changing 'style'", function() {
120 + var styles = {display: 'none', color: 'red'};
121 + var stub = ReactTestUtils.renderIntoDocument(<div style={styles} />);
122 +
123 + var stubStyle = stub.getDOMNode().style;
124 +
125 + styles = {color: 'green'};
126 + stub.receiveProps({ style: styles }, transaction);
127 + expect(stubStyle.display).toEqual('');
128 + expect(stubStyle.color).toEqual('green');
129 + });
130 +
131 + it("should clear all the styles when removing 'style'", function() {
132 + var styles = {display: 'none', color: 'red'};
133 + var stub = ReactTestUtils.renderIntoDocument(<div style={styles} />);
134 +
135 + var stubStyle = stub.getDOMNode().style;
136 +
137 + stub.receiveProps({}, transaction);
138 + expect(stubStyle.display).toEqual('');
139 + expect(stubStyle.color).toEqual('');
140 + });
141 +
142 + it("should empty element when removing innerHTML", function() {
143 + var stub = ReactTestUtils.renderIntoDocument(
144 + <div dangerouslySetInnerHTML={{__html: ":)"}} />
145 + );
146 +
147 + expect(stub.getDOMNode().innerHTML).toEqual(':)');
148 + stub.receiveProps({}, transaction);
149 + expect(stub.getDOMNode().innerHTML).toEqual('');
150 + });
151 +
152 });
153
154 describe('createOpenTagMarkup', function() {
src/dom/CSSProperty.js
+55 -1
@@ -18,6 +18,8 @@
18
19 "use strict";
20
21 +var keyMirror = require('keyMirror');
22 +
23 /**
24 * CSS properties which accept numbers but are not in units of "px".
25 */
@@ -30,8 +32,60 @@ var isUnitlessNumber = {
32 zoom: true
33 };
34
35 +/**
36 + * Most style properties can be unset by doing .style[prop] = '' but IE8
37 + * doesn't like doing that with shorthand properties so for the properties that
38 + * IE8 breaks on, which are listed here, we instead unset each of the
39 + * individual properties. See http://bugs.jquery.com/ticket/12385.
40 + * The 4-value 'clock' properties like margin, padding, border-width seem to
41 + * behave without any problems. Curiously, list-style works too without any
42 + * special prodding.
43 + */
44 +var shorthandPropertyExpansions = {
45 + background: {
46 + backgroundImage: true,
47 + backgroundPosition: true,
48 + backgroundRepeat: true,
49 + backgroundColor: true
50 + },
51 + border: {
52 + borderWidth: true,
53 + borderStyle: true,
54 + borderColor: true
55 + },
56 + borderBottom: {
57 + borderBottomWidth: true,
58 + borderBottomStyle: true,
59 + borderBottomColor: true
60 + },
61 + borderLeft: {
62 + borderLeftWidth: true,
63 + borderLeftStyle: true,
64 + borderLeftColor: true
65 + },
66 + borderRight: {
67 + borderRightWidth: true,
68 + borderRightStyle: true,
69 + borderRightColor: true
70 + },
71 + borderTop: {
72 + borderTopWidth: true,
73 + borderTopStyle: true,
74 + borderTopColor: true
75 + },
76 + font: {
77 + fontStyle: true,
78 + fontVariant: true,
79 + fontWeight: true,
80 + fontSize: true,
81 + lineHeight: true,
82 + fontFamily: true
83 + }
84 +};
85 +
86 var CSSProperty = {
34 - isUnitlessNumber: isUnitlessNumber
87 + isUnitlessNumber: isUnitlessNumber,
88 + shorthandPropertyExpansions: shorthandPropertyExpansions
89 };
90
91 module.exports = CSSProperty;
src/dom/CSSPropertyOperations.js
+22 -6
@@ -19,10 +19,12 @@
19
20 "use strict";
21
22 -var dangerousStyleValue = require('dangerousStyleValue');
23 -var escapeTextForBrowser = require('escapeTextForBrowser');
24 -var hyphenate = require('hyphenate');
25 -var memoizeStringOnly = require('memoizeStringOnly');
22 +var CSSProperty = require('CSSProperty');
23 +
24 +var dangerousStyleValue = require('./dangerousStyleValue');
25 +var escapeTextForBrowser = require('./escapeTextForBrowser');
26 +var hyphenate = require('./hyphenate');
27 +var memoizeStringOnly = require('./memoizeStringOnly');
28
29 var processStyleName = memoizeStringOnly(function(styleName) {
30 return escapeTextForBrowser(hyphenate(styleName));
@@ -60,7 +62,8 @@ var CSSPropertyOperations = {
62 },
63
64 /**
63 - * Sets the value for multiple styles on a node.
65 + * Sets the value for multiple styles on a node. If a value is specified as
66 + * '' (empty string), the corresponding style property will be unset.
67 *
68 * @param {DOMElement} node
69 * @param {object} styles
@@ -72,7 +75,20 @@ var CSSPropertyOperations = {
75 continue;
76 }
77 var styleValue = styles[styleName];
75 - style[styleName] = dangerousStyleValue(styleName, styleValue);
78 + if (!styleValue) {
79 + var expansion = CSSProperty.shorthandPropertyExpansions[styleName];
80 + if (expansion) {
81 + // Shorthand property that IE8 won't like unsetting, so unset each
82 + // component to placate it
83 + for (var individualStyleName in expansion) {
84 + style[individualStyleName] = '';
85 + }
86 + } else {
87 + style[styleName] = '';
88 + }
89 + } else {
90 + style[styleName] = dangerousStyleValue(styleName, styleValue);
91 + }
92 }
93 }
94
src/dom/DOMProperty.js
+29 -4
@@ -23,6 +23,8 @@
23
24 var invariant = require('invariant');
25
26 +var defaultValueCache = {};
27 +
28 /**
29 * DOMProperty exports lookup objects that can be used like functions:
30 *
@@ -98,7 +100,29 @@ var DOMProperty = {
100 */
101 isCustomAttribute: RegExp.prototype.test.bind(
102 /^(data|aria)-[a-z_][a-z\d_.\-]*$/
101 - )
103 + ),
104 +
105 + /**
106 + * Returns the default property value for a DOM property (i.e., not an
107 + * attribute). Most default values are '' or false, but not all. Worse yet,
108 + * some (in particular, `type`) vary depending on the type of element.
109 + *
110 + * TODO: Is it worth caching the test elements? Caching the properties
111 + * ourselves (as opposed to accessing from a cached test element every time)
112 + * looks probably worth it: http://jsperf.com/object-vs-element
113 + */
114 + getDefaultValueForProperty: function(nodeName, prop) {
115 + var nodeDefaults = defaultValueCache[nodeName];
116 + var testElement;
117 + if (!nodeDefaults) {
118 + defaultValueCache[nodeName] = nodeDefaults = {};
119 + }
120 + if (!(prop in nodeDefaults)) {
121 + testElement = document.createElement(nodeName);
122 + nodeDefaults[prop] = testElement[prop];
123 + }
124 + return nodeDefaults[prop];
125 + }
126 };
127
128 /**
@@ -132,7 +156,7 @@ var Properties = {
156 dir: null,
157 disabled: MustUseProperty | HasBooleanValue,
158 enctype: null,
135 - height: null,
159 + height: MustUseAttribute,
160 href: null,
161 htmlFor: null,
162 max: null,
@@ -158,7 +182,7 @@ var Properties = {
182 title: null,
183 type: null,
184 value: MustUseProperty | HasSideEffects,
161 - width: null,
185 + width: MustUseAttribute,
186 wmode: MustUseAttribute,
187 /**
188 * SVG Properties
@@ -212,7 +236,8 @@ var DOMPropertyNames = {
236 };
237
238 /**
215 - * Properties that require special mutation methods.
239 + * Properties that require special mutation methods. If `value` is undefined,
240 + * the mutation method should unset the property.
241 */
242 var DOMMutationMethods = {
243 /**
src/dom/DOMPropertyOperations.js
+28 -3
@@ -19,10 +19,10 @@
19
20 "use strict";
21
22 -var DOMProperty = require('DOMProperty');
22 +var DOMProperty = require("./DOMProperty");
23
24 -var escapeTextForBrowser = require('escapeTextForBrowser');
25 -var memoizeStringOnly = require('memoizeStringOnly');
24 +var escapeTextForBrowser = require("./escapeTextForBrowser");
25 +var memoizeStringOnly = require("./memoizeStringOnly");
26
27 var processAttributeNameAndPrefix = memoizeStringOnly(function(name) {
28 return escapeTextForBrowser(name) + '="';
@@ -86,6 +86,31 @@ var DOMPropertyOperations = {
86 } else if (DOMProperty.isCustomAttribute(name)) {
87 node.setAttribute(name, value);
88 }
89 + },
90 +
91 + /**
92 + * Deletes the value for a property on a node.
93 + *
94 + * @param {DOMElement} node
95 + * @param {string} name
96 + */
97 + deleteValueForProperty: function(node, name) {
98 + if (DOMProperty.isStandardName[name]) {
99 + var mutationMethod = DOMProperty.getMutationMethod[name];
100 + if (mutationMethod) {
101 + mutationMethod(node, undefined);
102 + } else if (DOMProperty.mustUseAttribute[name]) {
103 + node.removeAttribute(DOMProperty.getAttributeName[name]);
104 + } else {
105 + var propName = DOMProperty.getPropertyName[name];
106 + node[propName] = DOMProperty.getDefaultValueForProperty(
107 + node.nodeName,
108 + name
109 + );
110 + }
111 + } else if (DOMProperty.isCustomAttribute(name)) {
112 + node.removeAttribute(name);
113 + }
114 }
115
116 };
src/event/EventPluginHub.js
+1
@@ -312,6 +312,7 @@ var EventPluginHub = {
312 registrationNamesArr: registrationNamesArr,
313 putListener: CallbackRegistry.putListener,
314 getListener: CallbackRegistry.getListener,
315 + deleteListener: CallbackRegistry.deleteListener,
316 deleteAllListeners: deleteAllListeners,
317 extractAbstractEvents: extractAbstractEvents,
318 enqueueAbstractEvents: enqueueAbstractEvents,