Bug Tracker

Ticket #9102 (closed bug: wontfix)

Opened 3 years ago

Last modified 2 years ago

attr() doesn't always return a string in jQuery 1.6 and lower

Reported by: Andy E Owned by: Andy E
Priority: low Milestone: 1.next
Component: attributes Version: 1.6
Keywords: neededdocs Cc:
Blocking: Blocked by:

Description

For Internet Explorer 7 and older, attr() may return a boolean or function type for certain attributes, for example "checked" or "onclick" respectively.

This is due to the old getAttribute bug that returns the DOM property value instead of the attribute string, which was finally fixed in IE 8. As a workaround, jQuery should make use of getAttributeNode rather than getAttribute.

This solution can be demonstrated by the following fiddle:  http://jsfiddle.net/TbjRN/. Run it in IE's compatibility mode or IE 7 or IE 6.

Change History

comment:1 Changed 3 years ago by rwaldron

  • Owner set to timmywil
  • Status changed from new to assigned
  • Component changed from unfiled to attributes

comment:2 Changed 3 years ago by timmywil

  • Priority changed from undecided to low

For onclick, I'm not sure we want to add the code to handle those cases. If we do, we'll want to handle all of the inline event handlers as special cases. hideFocus is not supported in any browser except IE, so I don't think we want to worry about that one either or we'll have to add it to propFix. Regardless, type is already a string and checked will consistently be a string in 1.6.1, although I'm not sure how that one is returning a boolean in 1.6. Anyway, avoiding the use of getAttributeNode wherever possible is a good idea.

comment:3 Changed 3 years ago by timmywil

OK, IE returns a boolean with getAttribute when there's no value. This is moot tho with the boolean attributes changes for 1.6.1.

comment:5 Changed 3 years ago by Andy E

@timmywil,

Why is avoiding the use of getAttributeNode wherever possible a good idea?

I used hidefocus purely as an example of a boolean attribute, but there are many others (checked, selected, contenteditable, disabled, etc...). Casting those to strings, however, would fix the problem just as well.

I filed this bug because I came across  a real-world scenario where jQuery was causing problems with attr and the oncontextmenu attribute. Would it be a problem to check the 0-2 substring of the requested attribute for "on" and use getAttributeNode in that case?

comment:6 Changed 3 years ago by dmethvin

  • Owner changed from timmywil to Andy E
  • Status changed from assigned to pending

What is the practical problem in leaving this the way it is? I'd like to see some pseudo-realistic code that needs this to work cross-browser. As the code stands now it's possible to see if an onXXXX handler is attached for example, especially now that 1.6 supports .prop().

comment:7 Changed 3 years ago by timmywil

The other boolean attributes you mention will return strings in 1.6.1 except for contenteditable, since it's property is actually contentEditable (we might want to add that one to propFix). I see hidefocus as a different matter tho.

As for your question, getAttributeNode should only be used when getAttribute is not sufficient because it is a drop in performance every time it is used. It is debatable whether it is worth the extra code to support unifying types for on* attributes since interacting with those is generally not a good idea anyway. It's unnecessary to edit the attribute when you should be editing the property.

comment:8 Changed 3 years ago by Andy E

  • Status changed from pending to new

Fix it or don't, it doesn't really matter to me. I reported it because someone else ran into the problem and I thought one of the ideas behind jQuery was cross-browser interoperability where possible.

I find it interesting you discuss performance, because attr as a getter doesn't really have much to offer beyond getAttribute - especially if it's not offering a consistent result across all the supported browsers - maybe jQuery should advise that users just use getAttribute instead? It's also one thing to tell me that interacting with those attributes isn't a good idea, but I already know that. jQuery is used by many, many users that don't have the slightest idea of the correct approach to anything (just look for all the users using keyup to detect text input), and it's those people that you need to convey that message to.

For what it's worth, I don't think there would be much code involved and you could keep a minimal performance hit by using feature detection on init, followed by only using getAttributeNode in the browsers that don't return the correct type:

    var getAttributeNodeRequired,
        tmp = document.createElement("div");

    tmp.innerHTML = "<span onclick=void(0)></span>";
    getAttributeNodeRequired = typeof tmp.firstChild.getAttribute("onclick") === "function";

    /* ... */

    $.fn.attr = function (name /*, etc, ... */) {
        /* ... */
        if (getAttributeNodeRequired && name.slice(0,2) == "on")
            return this[0].getAttributeNode(name);
        /* ... */
    }

If it's not to be fixed, then it might be a good idea to warn users in the docs for attr instead.

comment:9 Changed 3 years ago by dmethvin

  • Keywords needsdocs added
  • Status changed from new to closed
  • Resolution set to wontfix

Andy, I think your suggestion for users to use getAttribute for these cases is the best course of action. It just isn't common enough to justify the extra work to fix for a case (inline handlers) that we recommend against anyway.

I've marked the ticket as needing a docs change ("Attributes are generally strings, but in IE6/7 attr may return boolean for checked or a function for onclick.")

Note that people often WANT .attr() to return truthy/falsy for boolean attributes, we tried to change that in 1.6 and had to back it out for 1.6.1. So It's unlikely anyone is complaining about that.

comment:10 Changed 3 years ago by addyosmani

  • Keywords neededdocs added; needsdocs removed

Docs updated as per dmethvin's note:  http://api.jquery.com/attr/. Should we need to expand on this please feel free to re-add the needsdocs tag and I'll update accordingly.

Note: See TracTickets for help on using tickets.