Skip to main content

Bug Tracker

Side navigation

#2548 closed enhancement (fixed)

Opened March 18, 2008 11:31PM UTC

Closed May 13, 2008 12:37AM UTC

.attr() proposal

Reported by: flesler Owned by:
Priority: major Milestone: 1.2.4
Component: core Version: 1.2.3
Keywords: attr Cc:
Blocked by: Blocking:
Description

I really think something is not right, with the way attr works.

You need a huge hash ($.props) to store names, where most of them are ''xyz: xyz'', still, many users complain that attr doesn't return the expected value.

I'll attach a proposal that gives priority to DOM 0 methods, that are, in the end, oftenly used (scrollLeft, parentNode, etc).

It also includes a fix for ticket 2537, where attr returns undefined when it should return 0.

Also slightly optimizes the amount of code.

Attachments (1)
  • attr.diff (3.4 KB) - added by flesler March 19, 2008 01:41PM UTC.

    Proposal

Change History (8)

Changed March 18, 2008 11:37PM UTC by flesler comment:1

This is not a bullet-proof patch, more like a draft of what I think it should look like.

It surely skips some situations, that were already patched, that needs to be merged in, if this gets included.

With this patch, new things will work like:

$('#foo').attr('parentNode');
$('#foo').attr('selectedIndex',5);

Also, I could now safely remove from jQuery.props, all the foo:foo options.

Changed March 18, 2008 11:55PM UTC by flesler comment:2

Related Tickets: 2537, 2521, 2453, 2170, 2119, 2099.

Changed March 29, 2008 01:35PM UTC by scott.gonzal comment:3

One thing that stood out to me was this line:

get = value != undefined,//whether we are getting (or setting)

which should be:

get = value !== undefined,//whether we are getting (or setting)

in order to prevent mistaking null for undefined.

Changed April 01, 2008 03:33AM UTC by flesler comment:4

Thanks for checking, but I disagree.

$().attr('foo',null) is not a normal call, null shouldn't be handled like data, but what you say is logic.

I'll let others decide on that.

Good catch.

Changed May 12, 2008 03:20AM UTC by flesler comment:5

need: ReviewCommit

Yeah, the null should considered setting not getting.

Will change that if this gets approved.

Changed May 12, 2008 03:32AM UTC by brandon comment:6

Nice cleanup patch. Make sure null is setting the value and this is ready for commit.

Changed May 12, 2008 09:03PM UTC by brandon comment:7

One more issue I noticed ... we need to avoid the expando if-branch if the node is from an XML document. It would probably be good to go ahead and store the value of jQuery.isXMLDoc(elem) in a var since it is used multiple times within the attr method.

Changed May 13, 2008 12:37AM UTC by flesler comment:8

resolution: → fixed
status: newclosed

Implemented at [5574].