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

CommentFileSizeAuthor
#9 core-js-dom_0-1420706-9.patch3.55 KBnod_
#5 dom_5.patch4.36 KBdroplet
dom_.patch4.9 KBdroplet

Comments

droplet’s picture

tagging

nod_’s picture

Might want to add a !! in the last if to keep the coherence with the other var checked = !!this.checked.

Haven't tested the patch yet.

damien tournoud’s picture

-          .attr('id', $input.attr('id') + '-autocomplete-aria-live')
+          .attr('id', $input.id + '-autocomplete-aria-live')

Isn't this slightly weird? How do we know that the jQuery object does have this property?

nod_’s picture

Status: Needs review » Needs work

same thing for this.fieldset.id should be more like this.fieldset[0].id.

droplet’s picture

StatusFileSize
new4.36 KB

$input.id fixed in other patch.

droplet’s picture

Status: Needs work » Needs review

changed to reviews.

nod_’s picture

Can you name your patches properly please? that'd be nice. I'll review that later today :)

nod_’s picture

Status: Needs review » Needs work

Looks good, but jQuery does check extra things for the tabindex attribute, we shouldn't replace that.
Same for checked, jQuery makes sure a boolean is returned that's not the case for this.checked.

nod_’s picture

Status: Needs work » Needs review
StatusFileSize
new3.55 KB

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.

pascalduez’s picture

#9 Reviewed and tested, looks good.

seutje’s picture

We 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.

oxyc’s picture

Hmm 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.

oxyc’s picture

Thought 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():

if ( notxml ) {
  // Fix name and attach hooks
  name = jQuery.propFix[ name ] || name;
  hooks = jQuery.propHooks[ name ];
}

attr():

if ( notxml ) {
  name = name.toLowerCase();
  hooks = jQuery.attrHooks[ name ] || ( rboolean.test( name ) ? boolHook : nodeHook );
}

This is basically where jQuery massages the different inconsistencies for prop() and attr().

In prop() case it will:

  1. look for speling differences as seutje described earlier. But there's also a fix for enctype -> encoding in IEs case
  2. look for hooks to fix inconsistencies, there are only two propHooks, getter for selected (safari specific) and the getter for tabindex (totally messed up)
  3. Call either of the two mentioned hooks and return OR get/set the value directly through the dom consistently (this.checked/etc)

In attr() case it will:

  1. Look for hooks to fix inconsistencies: type (set), value (get/set), width (set), height (set), contenteditable (get/set), href/src/width/height (get), style (get/set)
  2. Use a boolean hook on all the boolean values seutje mentioned. This is probably what nod_ was referencing.
  3. Use a custom get/setter function for IE

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)

nod_’s picture

Status: Needs review » Closed (won't fix)

Not worth the trouble.

nod_’s picture

Issue summary: View changes

Updated issue summary.