Skip to content

[idea] Automatically detect property/attribute for HTMLDOMPropertyConfig #2141

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

Closed
wants to merge 1 commit into from
Closed

[idea] Automatically detect property/attribute for HTMLDOMPropertyConfig #2141

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
28 changes: 3 additions & 25 deletions src/browser/ui/__tests__/ReactDOMComponent-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,11 +117,11 @@ describe('ReactDOMComponent', function() {
});

it("should remove attributes", function() {
var stub = ReactTestUtils.renderIntoDocument(<img height='17' />);
var stub = ReactTestUtils.renderIntoDocument(<div role='abc' />);

expect(stub.getDOMNode().hasAttribute('height')).toBe(true);
expect(stub.getDOMNode().hasAttribute('role')).toBe(true);
stub.receiveComponent({props: {}}, transaction);
expect(stub.getDOMNode().hasAttribute('height')).toBe(false);
expect(stub.getDOMNode().hasAttribute('role')).toBe(false);
});

it("should remove properties", function() {
Expand Down Expand Up @@ -187,28 +187,6 @@ describe('ReactDOMComponent', function() {
stub.receiveComponent({props: {children: 'adieu'}}, transaction);
expect(stub.getDOMNode().innerHTML).toEqual('adieu');
});

it("should not incur unnecessary DOM mutations", function() {
var stub = ReactTestUtils.renderIntoDocument(<div value="" />);

var node = stub.getDOMNode();
var nodeValue = ''; // node.value always returns undefined
var nodeValueSetter = mocks.getMockFunction();
Object.defineProperty(node, 'value', {
get: function() {
return nodeValue;
},
set: nodeValueSetter.mockImplementation(function(newValue) {
nodeValue = newValue;
})
});

stub.receiveComponent({props: {value: ''}}, transaction);
expect(nodeValueSetter.mock.calls.length).toBe(0);

stub.receiveComponent({props: {}}, transaction);
expect(nodeValueSetter.mock.calls.length).toBe(1);
});
});

describe('createOpenTagMarkup', function() {
Expand Down
22 changes: 22 additions & 0 deletions src/browser/ui/dom/DOMProperty.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,8 @@ var DOMPropertyInjection = {
}
}
};

var hasPropertyAccessorCache = {};
var defaultValueCache = {};

/**
Expand Down Expand Up @@ -267,6 +269,26 @@ var DOMProperty = {
return false;
},

hasPropertyAccessor: function(nodeName, name) {
if (DOMProperty.mustUseProperty[name]) {
return true;
}
if (DOMProperty.mustUseAttribute[name]) {
return false;
}
var prop = DOMProperty.getPropertyName[name];
var nodeHasPropertyAccessors = hasPropertyAccessorCache[nodeName];
var testElement;
if (!nodeHasPropertyAccessors) {
hasPropertyAccessorCache[nodeName] = nodeHasPropertyAccessors = {};
}
if (!(prop in nodeHasPropertyAccessors)) {
testElement = document.createElement(nodeName);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updates are going to get slow until this cache is populated, which I think might be a showstopper. One thing we can do is hold onto our testElement for each node type so we aren't recreating each time we call the function. That's still going to be slower than what we have now though. The tradeoff is a bit more correctness and not having to special case things (as much).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zpao Last I checked, document.createElement is crazy fast, even on IE8, we're talking 100K/s+ unless my memory fails me. We talked about caching nodes for getDefaultForProperty but cancelled that because it seemed pointless.

I don't mind caching, but I really really doubt the cost of this can be measured at all unless there is thrashing involved. If that is, then it could probably be avoided by simply keeping a reference to the last element which should provide sufficient caching. Your call :)

Additionally, the cost of this should be immeasurable when compared to the cost of actually figuring out what to update.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm suspicious of any call to document.*, especially in a tight loop. I'd like to see perf in a real-life pathological case with lots of different types of elements and properties that all need to be looked up.

also, cc @yungsters

nodeHasPropertyAccessors[prop] = prop in testElement;
}
return nodeHasPropertyAccessors[prop];
},

/**
* 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,
Expand Down
44 changes: 24 additions & 20 deletions src/browser/ui/dom/DOMPropertyOperations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,19 +137,21 @@ var DOMPropertyOperations = {
mutationMethod(node, value);
} else if (shouldIgnoreValue(name, value)) {
this.deleteValueForProperty(node, name);
} else if (DOMProperty.mustUseAttribute[name]) {
// `setAttribute` with objects becomes only `[object]` in IE8/9,
// ('' + value) makes it output the correct toString()-value.
node.setAttribute(DOMProperty.getAttributeName[name], '' + value);
} else {
var propName = DOMProperty.getPropertyName[name];
// Must explicitly cast values for HAS_SIDE_EFFECTS-properties to the
// property type before comparing; only `value` does and is string.
if (!DOMProperty.hasSideEffects[name] ||
('' + node[propName]) !== ('' + value)) {
// Contrary to `setAttribute`, object properties are properly
// `toString`ed by IE8/9.
node[propName] = value;
if (DOMProperty.hasPropertyAccessor(node.nodeName, propName)) {
// Must explicitly cast values for HAS_SIDE_EFFECTS-properties to the
// property type before comparing; only `value` does and is string.
if (!DOMProperty.hasSideEffects[name] ||
('' + node[propName]) !== ('' + value)) {
// Contrary to `setAttribute`, object properties are properly
// `toString`ed by IE8/9.
node[propName] = value;
}
} else {
// `setAttribute` with objects becomes only `[object]` in IE8/9,
// ('' + value) makes it output the correct toString()-value.
node.setAttribute(DOMProperty.getAttributeName[name], '' + value);
}
}
} else if (DOMProperty.isCustomAttribute(name)) {
Expand All @@ -175,17 +177,19 @@ var DOMPropertyOperations = {
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];
var defaultValue = DOMProperty.getDefaultValueForProperty(
node.nodeName,
propName
);
if (!DOMProperty.hasSideEffects[name] ||
('' + node[propName]) !== defaultValue) {
node[propName] = defaultValue;
if (DOMProperty.hasPropertyAccessor(node.nodeName, propName)) {
var defaultValue = DOMProperty.getDefaultValueForProperty(
node.nodeName,
propName
);
if (!DOMProperty.hasSideEffects[name] ||
('' + node[propName]) !== defaultValue) {
node[propName] = defaultValue;
}
} else {
node.removeAttribute(DOMProperty.getAttributeName[name]);
}
}
} else if (DOMProperty.isCustomAttribute(name)) {
Expand Down
64 changes: 32 additions & 32 deletions src/browser/ui/dom/HTMLDOMPropertyConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,8 @@ var HTMLDOMPropertyConfig = {
accept: null,
accessKey: null,
action: null,
allowFullScreen: MUST_USE_ATTRIBUTE | HAS_BOOLEAN_VALUE,
allowTransparency: MUST_USE_ATTRIBUTE,
allowFullScreen: HAS_BOOLEAN_VALUE,
allowTransparency: null,
alt: null,
async: HAS_BOOLEAN_VALUE,
autoComplete: null,
Expand All @@ -68,79 +68,79 @@ var HTMLDOMPropertyConfig = {
autoPlay: HAS_BOOLEAN_VALUE,
cellPadding: null,
cellSpacing: null,
charSet: MUST_USE_ATTRIBUTE,
checked: MUST_USE_PROPERTY | HAS_BOOLEAN_VALUE,
classID: MUST_USE_ATTRIBUTE,
charSet: null,
checked: HAS_BOOLEAN_VALUE,
classID: null,
// To set className on SVG elements, it's necessary to use .setAttribute;
// this works on HTML elements too in all browsers except IE8. Conveniently,
// IE8 doesn't support SVG and so we can simply use the attribute in
// browsers that support SVG and the property in browsers that don't,
// regardless of whether the element is HTML or SVG.
className: hasSVG ? MUST_USE_ATTRIBUTE : MUST_USE_PROPERTY,
cols: MUST_USE_ATTRIBUTE | HAS_POSITIVE_NUMERIC_VALUE,
className: null,
cols: HAS_POSITIVE_NUMERIC_VALUE,
colSpan: null,
content: null,
contentEditable: null,
contextMenu: MUST_USE_ATTRIBUTE,
controls: MUST_USE_PROPERTY | HAS_BOOLEAN_VALUE,
contextMenu: null,
controls: HAS_BOOLEAN_VALUE,
coords: null,
crossOrigin: null,
data: null, // For `<object />` acts as `src`.
dateTime: MUST_USE_ATTRIBUTE,
dateTime: null,
defer: HAS_BOOLEAN_VALUE,
dir: null,
disabled: MUST_USE_ATTRIBUTE | HAS_BOOLEAN_VALUE,
disabled: HAS_BOOLEAN_VALUE,
download: HAS_OVERLOADED_BOOLEAN_VALUE,
draggable: null,
encType: null,
form: MUST_USE_ATTRIBUTE,
form: null,
formNoValidate: HAS_BOOLEAN_VALUE,
frameBorder: MUST_USE_ATTRIBUTE,
height: MUST_USE_ATTRIBUTE,
hidden: MUST_USE_ATTRIBUTE | HAS_BOOLEAN_VALUE,
frameBorder: null,
height: null,
hidden: HAS_BOOLEAN_VALUE,
href: null,
hrefLang: null,
htmlFor: null,
httpEquiv: null,
icon: null,
id: MUST_USE_PROPERTY,
id: null,
label: null,
lang: null,
list: null,
loop: MUST_USE_PROPERTY | HAS_BOOLEAN_VALUE,
loop: HAS_BOOLEAN_VALUE,
max: null,
maxLength: MUST_USE_ATTRIBUTE,
maxLength: null,
mediaGroup: null,
method: null,
min: null,
multiple: MUST_USE_PROPERTY | HAS_BOOLEAN_VALUE,
muted: MUST_USE_PROPERTY | HAS_BOOLEAN_VALUE,
multiple: HAS_BOOLEAN_VALUE,
muted: HAS_BOOLEAN_VALUE,
name: null,
noValidate: HAS_BOOLEAN_VALUE,
pattern: null,
placeholder: null,
poster: null,
preload: null,
radioGroup: null,
readOnly: MUST_USE_PROPERTY | HAS_BOOLEAN_VALUE,
readOnly: HAS_BOOLEAN_VALUE,
rel: null,
required: HAS_BOOLEAN_VALUE,
role: MUST_USE_ATTRIBUTE,
rows: MUST_USE_ATTRIBUTE | HAS_POSITIVE_NUMERIC_VALUE,
role: null,
rows: HAS_POSITIVE_NUMERIC_VALUE,
rowSpan: null,
sandbox: null,
scope: null,
scrollLeft: MUST_USE_PROPERTY,
scrolling: null,
scrollTop: MUST_USE_PROPERTY,
seamless: MUST_USE_ATTRIBUTE | HAS_BOOLEAN_VALUE,
selected: MUST_USE_PROPERTY | HAS_BOOLEAN_VALUE,
seamless: HAS_BOOLEAN_VALUE,
selected: HAS_BOOLEAN_VALUE,
shape: null,
size: MUST_USE_ATTRIBUTE | HAS_POSITIVE_NUMERIC_VALUE,
size: HAS_POSITIVE_NUMERIC_VALUE,
span: HAS_POSITIVE_NUMERIC_VALUE,
spellCheck: null,
src: null,
srcDoc: MUST_USE_PROPERTY,
srcDoc: null,
srcSet: null,
start: HAS_NUMERIC_VALUE,
step: null,
Expand All @@ -151,17 +151,17 @@ var HTMLDOMPropertyConfig = {
type: null,
useMap: null,
value: MUST_USE_PROPERTY | HAS_SIDE_EFFECTS,
width: MUST_USE_ATTRIBUTE,
wmode: MUST_USE_ATTRIBUTE,
width: null,
wmode: null,

/**
* Non-standard Properties
*/
autoCapitalize: null, // Supported in Mobile Safari for keyboard hints
autoCorrect: null, // Supported in Mobile Safari for keyboard hints
itemProp: MUST_USE_ATTRIBUTE, // Microdata: http://schema.org/docs/gs.html
itemScope: MUST_USE_ATTRIBUTE | HAS_BOOLEAN_VALUE, // Microdata: http://schema.org/docs/gs.html
itemType: MUST_USE_ATTRIBUTE, // Microdata: http://schema.org/docs/gs.html
itemProp: null, // Microdata: http://schema.org/docs/gs.html
itemScope: HAS_BOOLEAN_VALUE, // Microdata: http://schema.org/docs/gs.html
itemType: null, // Microdata: http://schema.org/docs/gs.html
property: null // Supports OG in meta tags
},
DOMAttributeNames: {
Expand Down
7 changes: 3 additions & 4 deletions src/browser/ui/dom/__tests__/DOMPropertyOperations-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,10 +253,9 @@ describe('DOMPropertyOperations', function() {
// Suppose 'foobar' is a property that corresponds to the underlying
// 'className' property:
DOMProperty.injection.injectDOMPropertyConfig({
Properties: {foobar: DOMProperty.injection.MUST_USE_PROPERTY},
DOMPropertyNames: {
foobar: 'className'
}
Properties: {foobar: null},
DOMAttributeNames: {foobar: 'class'},
DOMPropertyNames: {foobar: 'className'}
});

DOMPropertyOperations.setValueForProperty(
Expand Down