Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use removeAttribute to forcefully remove properties from the DOM #1510

Merged
merged 1 commit into from
Jan 29, 2016
Merged

Use removeAttribute to forcefully remove properties from the DOM #1510

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 6 additions & 38 deletions src/renderers/dom/shared/DOMProperty.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,12 @@ var DOMPropertyInjection = {
* Mapping from normalized, camelcased property names to a configuration that
* specifies how the associated DOM property should be accessed or rendered.
*/
MUST_USE_ATTRIBUTE: 0x1,
MUST_USE_PROPERTY: 0x2,
HAS_SIDE_EFFECTS: 0x4,
HAS_BOOLEAN_VALUE: 0x8,
HAS_NUMERIC_VALUE: 0x10,
HAS_POSITIVE_NUMERIC_VALUE: 0x20 | 0x10,
HAS_OVERLOADED_BOOLEAN_VALUE: 0x40,
MUST_USE_PROPERTY: 0x1,
HAS_SIDE_EFFECTS: 0x2,
HAS_BOOLEAN_VALUE: 0x4,
HAS_NUMERIC_VALUE: 0x8,
HAS_POSITIVE_NUMERIC_VALUE: 0x10 | 0x8,
HAS_OVERLOADED_BOOLEAN_VALUE: 0x20,

/**
* Inject some specialized knowledge about the DOM. This takes a config object
Expand Down Expand Up @@ -91,7 +90,6 @@ var DOMPropertyInjection = {
propertyName: propName,
mutationMethod: null,

mustUseAttribute: checkMask(propConfig, Injection.MUST_USE_ATTRIBUTE),
mustUseProperty: checkMask(propConfig, Injection.MUST_USE_PROPERTY),
hasSideEffects: checkMask(propConfig, Injection.HAS_SIDE_EFFECTS),
hasBooleanValue: checkMask(propConfig, Injection.HAS_BOOLEAN_VALUE),
Expand All @@ -102,11 +100,6 @@ var DOMPropertyInjection = {
checkMask(propConfig, Injection.HAS_OVERLOADED_BOOLEAN_VALUE),
};

invariant(
!propertyInfo.mustUseAttribute || !propertyInfo.mustUseProperty,
'DOMProperty: Cannot require using both attribute and property: %s',
propName
);
invariant(
propertyInfo.mustUseProperty || !propertyInfo.hasSideEffects,
'DOMProperty: Properties that have side effects must use property: %s',
Expand Down Expand Up @@ -148,7 +141,6 @@ var DOMPropertyInjection = {
}
},
};
var defaultValueCache = {};

var ATTRIBUTE_NAME_START_CHAR = ':A-Z_a-z\\u00C0-\\u00D6\\u00D8-\\u00F6\\u00F8-\\u02FF\\u0370-\\u037D\\u037F-\\u1FFF\\u200C-\\u200D\\u2070-\\u218F\\u2C00-\\u2FEF\\u3001-\\uD7FF\\uF900-\\uFDCF\\uFDF0-\\uFFFD';

Expand Down Expand Up @@ -186,9 +178,6 @@ var DOMProperty = {
* mutationMethod:
* If non-null, used instead of the property or `setAttribute()` after
* initial render.
* mustUseAttribute:
* Whether the property must be accessed and mutated using `*Attribute()`.
* (This includes anything that fails `<propName> in <element>`.)
* mustUseProperty:
* Whether the property must be accessed and mutated as an object property.
* hasSideEffects:
Expand Down Expand Up @@ -237,27 +226,6 @@ var DOMProperty = {
return false;
},

/**
* 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 better to grab all the possible properties when creating an
* element to avoid having to create the same element twice?
*/
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];
},

injection: DOMPropertyInjection,
};

Expand Down
43 changes: 22 additions & 21 deletions src/renderers/dom/shared/DOMPropertyOperations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,17 @@ var DOMPropertyOperations = {
mutationMethod(node, value);
} else if (shouldIgnoreValue(propertyInfo, value)) {
this.deleteValueForProperty(node, name);
} else if (propertyInfo.mustUseAttribute) {
} else if (propertyInfo.mustUseProperty) {
var propName = propertyInfo.propertyName;
// Must explicitly cast values for HAS_SIDE_EFFECTS-properties to the
// property type before comparing; only `value` does and is string.
if (!propertyInfo.hasSideEffects ||
('' + node[propName]) !== ('' + value)) {
// Contrary to `setAttribute`, object properties are properly
// `toString`ed by IE8/9.
node[propName] = value;
}
} else {
var attributeName = propertyInfo.attributeName;
var namespace = propertyInfo.attributeNamespace;
// `setAttribute` with objects becomes only `[object]` in IE8/9,
Expand All @@ -181,16 +191,6 @@ var DOMPropertyOperations = {
} else {
node.setAttribute(attributeName, '' + value);
}
} else {
var propName = propertyInfo.propertyName;
// Must explicitly cast values for HAS_SIDE_EFFECTS-properties to the
// property type before comparing; only `value` does and is string.
if (!propertyInfo.hasSideEffects ||
('' + node[propName]) !== ('' + value)) {
// Contrary to `setAttribute`, object properties are properly
// `toString`ed by IE8/9.
node[propName] = value;
}
}
} else if (DOMProperty.isCustomAttribute(name)) {
DOMPropertyOperations.setValueForAttribute(node, name, value);
Expand Down Expand Up @@ -236,18 +236,19 @@ var DOMPropertyOperations = {
var mutationMethod = propertyInfo.mutationMethod;
if (mutationMethod) {
mutationMethod(node, undefined);
} else if (propertyInfo.mustUseAttribute) {
node.removeAttribute(propertyInfo.attributeName);
} else {
} else if (propertyInfo.mustUseProperty) {
var propName = propertyInfo.propertyName;
var defaultValue = DOMProperty.getDefaultValueForProperty(
node.nodeName,
propName
);
if (!propertyInfo.hasSideEffects ||
('' + node[propName]) !== defaultValue) {
node[propName] = defaultValue;
if (propertyInfo.hasBooleanValue) {
// No HAS_SIDE_EFFECTS logic here, only `value` has it and is string.
node[propName] = false;
} else {
if (!propertyInfo.hasSideEffects ||
('' + node[propName]) !== '') {
node[propName] = '';
}
}
} else {
node.removeAttribute(propertyInfo.attributeName);
}
} else if (DOMProperty.isCustomAttribute(name)) {
node.removeAttribute(name);
Expand Down
Loading