a follow up issue of : #1099062: autocomplete.js micro optimization
This is a bug fix as well (jQuery 1.6.x+ use prop) :
$(this).attr('checked')
to
this.checked
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | core-js-dom_0-1420706-9.patch | 3.55 KB | nod_ |
| #5 | dom_5.patch | 4.36 KB | droplet |
| dom_.patch | 4.9 KB | droplet |
Comments
Comment #1
droplet commentedtagging
Comment #2
nod_Might want to add a
!!in the lastifto keep the coherence with the other varchecked = !!this.checked.Haven't tested the patch yet.
Comment #3
damien tournoud commentedIsn't this slightly weird? How do we know that the jQuery object does have this property?
Comment #4
nod_same thing for
this.fieldset.idshould be more likethis.fieldset[0].id.Comment #5
droplet commented$input.id fixed in other patch.
Comment #6
droplet commentedchanged to reviews.
Comment #7
nod_Can you name your patches properly please? that'd be nice. I'll review that later today :)
Comment #8
nod_Looks good, but jQuery does check extra things for the
tabindexattribute, we shouldn't replace that.Same for
checked, jQuery makes sure a boolean is returned that's not the case for this.checked.Comment #9
nod_reroll without the checked and tabindex replacements.
There are a lot of files where that could be used as well for some other properties. I'm not sure how we want to go about that. changing for id is fine, but it'll introduce inconsistencies if we use that for other attributes.
Comment #10
pascalduez commented#9 Reviewed and tested, looks good.
Comment #11
seutje commentedWe should probably first make a list of attributes jQuery massages
Pretty much straight grab from jQuery source: all the boolean attributes (autofocus, autoplay, async, checked, controls, defer, disabled, hidden, loop, multiple, open, readOnly, required, scoped, selected), ones that are sort of bugged pretty much across the board (tabIndex) and others that secretly have a different spelling (htmlFor, className, maxLength, cellSpacing, cellPadding, rowSpan, colSpan, frameBorder, contentEditable).
For the last group (differently spelled ones): we could access these directly, but only when using the "real" spelling of them. But I can live with leaving them as using .attr(), since it might seem rather weird to some people. If we wouldn't use .attr() for them, I feel like it would need a comment line like
// Accessing "for" attribute as "htmlFor".or something like that.Also, perhaps it's a good idea to incorporate this into the refactoring of each file, instead of making a single issue to do it everywhere, as that would probably break all the refactor patches and vice-versa.
Comment #12
oxyc commentedHmm regarding the boolean attributes, are you sure it's not safe to use the dom properties directly? From my understanding it's best practice to use prop() instead of attr() nowadays right? By looking at jQuery source the boolean check is only done on attr(), not on prop() at all.
Also attr() apparently doesn't even return the correct value for checked, but returns the default value.
Comment #13
oxyc commentedThought I'd reference some code so it'll be easier to review my previous comment :)
First of all properties are in the DOM, while attributes are on the HTML elements. If one is changed usually the other changes as well, with the exception of attr always returning the default value on some input elements.
prop():
attr():
This is basically where jQuery massages the different inconsistencies for prop() and attr().
In prop() case it will:
In attr() case it will:
In val() case it will: black magic!
Conclusion
Properties which are NOT safe to access through the DOM directly:
tabIndex (get)
selected (get)
value (get/set, differs a lot between node types and browsers, .val() is really useful here)
innerHTML (quite minor problems, source: quirksmode)
innerText, outerHTML, outerText, textContent (quirksmode)
rowIndex, rows (Opera, source quirksmode)
The rest of them should be perfectly safe.
Attributes
I don't actually see when we would prefer attributes over properties? Anyway, if we ever would there are a ton of inconsistencies here and jQuery is probably the way to go. Also most of this seem to be IE 6-7 specific.
Properties with spelling differences:
tabindex: "tabIndex",
readonly: "readOnly",
"for": "htmlFor",
"class": "className",
maxlength: "maxLength",
cellspacing: "cellSpacing",
cellpadding: "cellPadding",
rowspan: "rowSpan",
colspan: "colSpan",
usemap: "useMap",
frameborder: "frameBorder",
contenteditable: "contentEditable"
enctype: "encoding" (IE)
Comment #14
nod_Not worth the trouble.
Comment #14.0
nod_Updated issue summary.