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.

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

Conversation

syranide
Copy link
Contributor

@syranide syranide commented Sep 4, 2014

Proper implementation of #1512

It does not get rid of MUST_USE_PROPERTY (see #2140, #2202) and MUST_USE_ATTRIBUTE because I'm not 100% sure if we might want to use it or not, but I assume there's no need for it.

I also removed a test that I'm not sure what to do about. It relied on really hacky behavior and I don't really see a simple way of keeping it alive (not really familiar with testing), but please shout if you think otherwise.

This is beneficial for a lot of reasons, one being that we ensure DOM attributes always reflect the intended value. MUST_USE_PROPERTY was previously used for some attributes not supported by all browsers/elements, which when updated would just add a custom property to the node object rather than the intended attribute.

Half-related: #1848 and #1657

@syranide
Copy link
Contributor Author

syranide commented Sep 4, 2014

cc @zpao

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

@syranide syranide changed the title Automatically detect property/attribute for HTMLDOMPropertyConfig [idea] Automatically detect property/attribute for HTMLDOMPropertyConfig Sep 29, 2014
@sebmarkbage
Copy link
Collaborator

Since properties can have all kind of weird behaviors and formats that attributes usually doesn't, I think that properties should probably be opt-in on a whitelist. Browsers add crazy new properties all the time. It seems fragile to use those.

Instead, we could, by default, opt to always use attributes for unknown props. That way we're compatible with most of the arbitrary attributes without a whitelist of special cases.

@syranide
Copy link
Contributor Author

syranide commented Oct 2, 2014

@sebmarkbage I expanded on this in a related issue a short while back: #1449 (comment), which I think is a preferable direction. Full auto detection is, as you say, probably rather fragile.

@gaearon
Copy link
Collaborator

gaearon commented Mar 26, 2016

I’m closing this as it doesn’t appear that we want to go through with it, or at least not at the moment. Thank you for your work on this PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants