Bug Tracker

Modify

Ticket #2548 (closed enhancement: fixed)

Opened 5 years ago

Last modified 5 years ago

.attr() proposal

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

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

attr.diff Download (3.4 KB) - added by flesler 5 years ago.
Proposal

Change History

comment:1 Changed 5 years ago by flesler

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.

comment:2 Changed 5 years ago by flesler

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

Changed 5 years ago by flesler

Proposal

comment:3 Changed 5 years ago by scott.gonzal

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.

comment:4 Changed 5 years ago by flesler

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.

comment:5 Changed 5 years ago by flesler

  • need changed from Review to Commit

Yeah, the null should considered setting not getting. Will change that if this gets approved.

comment:6 Changed 5 years ago by brandon

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

comment:7 Changed 5 years ago by brandon

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.

comment:8 Changed 5 years ago by flesler

  • Status changed from new to closed
  • Resolution set to fixed

Implemented at [5574].

Please follow the  bug reporting guidlines and use  jsFiddle when providing test cases and demonstrations instead of pasting the code in the ticket.

View

Add a comment

Modify Ticket

Action
as closed
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.