Bug Tracker

Modify

Ticket #1733 (closed bug: fixed)

Opened 6 years ago

Last modified 5 years ago

addClass gives error on textnodes

Reported by: achun Owned by:
Priority: major Milestone: 1.2.2
Component: core Version: 1.2.1
Keywords: addClass Cc:
Blocking: Blocked by:

Description

$("<b>addClass</b>\n<b>addClass</b>").addClass('tmp')

error! in className:add fun

		add: function( elem, c ){
			jQuery.each( (c || "").split(/\s+/), function(i, cur){
				if ( !jQuery.className.has( elem.className, cur ) )
					elem.className += ( elem.className ? " " : "" ) + cur;
			});
		},

fix?

		add: function( elem, c ){
			jQuery.each( (c || "").split(/\s+/), function(i, cur){
				if ( !jQuery.className.has( elem.className||"", cur ) )
					elem.className += ( elem.className ? " " : "" ) + cur;
			});
		},

Attachments

1733.diff Download (2.5 KB) - added by brandon 6 years ago.
Patch + Tests for text nodes in jQuery.className methods

Change History

Changed 6 years ago by brandon

Patch + Tests for text nodes in jQuery.className methods

comment:1 Changed 6 years ago by brandon

  • need changed from Review to Commit

I've attached patch + tests to make jQuery.className.remove/add/has not fail when it gets a non element node.

However, I'm wondering if the better solution (or in addition to) should be that text nodes do not make it into the jQuery collection via jQuery.clean.

comment:2 follow-up: ↓ 3 Changed 6 years ago by davidserduke

I don't think we can keep text nodes out of the jQuery collection. I looked in to that recently and realized there are several ways to get text nodes in. In fact, the function .contents() specifically includes text nodes.

 http://docs.jquery.com/Traversing/contents

With that said, do you think it makes sense to implement the patch? It looks like a good solution to me.

comment:3 in reply to: ↑ 2 Changed 6 years ago by brandon

Replying to davidserduke:

Yeah we definitely can't keep text nodes out now ... however we need to be mindful of this in our manipulation methods such as css, attr and the class methods. We should probably go ahead and be proactive and test/patch the other manipulation methods.

comment:4 Changed 6 years ago by davidserduke

  • Summary changed from addClass? to addClass fails on textnodes

comment:5 Changed 6 years ago by davidserduke

  • Summary changed from addClass fails on textnodes to addClass gives error on textnodes

comment:6 Changed 6 years ago by davidserduke

See #1039 for a similar ticket.

comment:7 Changed 6 years ago by davidserduke

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

Fixed in [4062] taking Brandon's suggestion that text nodes should not break jQuery even when they are in the jQuery object. addClass was fixed along with the other functions in the API.

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.