Permalink
Browse files

Fix #12009. $().find( DOMElement ) should pushStack properly. Close g…

  • Loading branch information...
mikesherov authored and dmethvin committed Oct 30, 2012
1 parent c78a3ba commit e8f91514a6f353e85d8117998087903dc7331210
Showing with 37 additions and 28 deletions.
  1. +24 −17 src/traversing.js
  2. +13 −11 test/unit/traversing.js
View
@@ -12,39 +12,46 @@ var runtil = /Until$/,
jQuery.fn.extend({
find: function( selector ) {
- var i, l, length, n, r, ret,
- self = this;
+ var prevLength, n, r, ret,
+ i = 0,
+ self = this,
+ selfLength = this.length;
if ( typeof selector !== "string" ) {
- return jQuery( selector ).filter(function() {
- for ( i = 0, l = self.length; i < l; i++ ) {
+
+ ret = jQuery( selector ).filter(function() {
+ for ( ; i < selfLength; i++ ) {
if ( jQuery.contains( self[ i ], this ) ) {
return true;
}
}
});
- }
- ret = this.pushStack( [] );
-
- for ( i = 0, l = this.length; i < l; i++ ) {
- length = ret.length;
- jQuery.find( selector, this[i], ret );
+ } else {
- if ( i > 0 ) {
- // Make sure that the results are unique
- for ( n = length; n < ret.length; n++ ) {
- for ( r = 0; r < length; r++ ) {
- if ( ret[r] === ret[n] ) {
- ret.splice(n--, 1);
+ ret = [];
+ for ( ; i < selfLength; i++ ) {
+ prevLength = ret.length;
+ jQuery.find( selector, this[ i ], ret );
+
+ if ( i > 0 ) {
+ // Make sure that the results are unique
+ // by comparing the newly added elements on the ith
+ // iteration to the elements added by the previous iterations
+ for ( n = prevLength; n < ret.length; n++ ) {
+ for ( r = 0; r < prevLength; r++ ) {
+ if ( ret[ r ] === ret[ n ] ) {
+ ret.splice( n--, 1 );

This comment has been minimized.

Show comment Hide comment
@gibson042

gibson042 Oct 31, 2012

Member

Seems like we'd be better off with jQuery.unique than this.

@gibson042

gibson042 Oct 31, 2012

Member

Seems like we'd be better off with jQuery.unique than this.

This comment has been minimized.

Show comment Hide comment
@mikesherov

mikesherov Oct 31, 2012

Member

I presumed this was faster because in this case we already knew the prevLength.

@mikesherov

mikesherov Oct 31, 2012

Member

I presumed this was faster because in this case we already knew the prevLength.

This comment has been minimized.

Show comment Hide comment
@dmethvin

dmethvin Oct 31, 2012

Owner

The previous code wasn't using it either, and i made the same assumption as @mikesherov. I have absolutely no evidence to back up whether it's faster or whether it being somewhat faster makes a difference, but it FEELS like it should. 👻

@dmethvin

dmethvin Oct 31, 2012

Owner

The previous code wasn't using it either, and i made the same assumption as @mikesherov. I have absolutely no evidence to back up whether it's faster or whether it being somewhat faster makes a difference, but it FEELS like it should. 👻

This comment has been minimized.

Show comment Hide comment
@gibson042

gibson042 Oct 31, 2012

Member

True, but it also checks the entirety of prevLength on every outer iteration. I can't say for sure that jQuery.unique would be faster in the average case (not that I would be surprised, either), but it would be smaller, clearer, and always return elements in DOM order (see http://jsfiddle.net/EAtqe/ for a demonstration of the current behavior).

@gibson042

gibson042 Oct 31, 2012

Member

True, but it also checks the entirety of prevLength on every outer iteration. I can't say for sure that jQuery.unique would be faster in the average case (not that I would be surprised, either), but it would be smaller, clearer, and always return elements in DOM order (see http://jsfiddle.net/EAtqe/ for a demonstration of the current behavior).

This comment has been minimized.

Show comment Hide comment
@mikesherov

mikesherov Oct 31, 2012

Member

If this is a correctness issue (re: DOM order), we should fix it.

@mikesherov

mikesherov Oct 31, 2012

Member

If this is a correctness issue (re: DOM order), we should fix it.

This comment has been minimized.

Show comment Hide comment
@mikesherov

mikesherov Oct 31, 2012

Member

Although, we should open a separate issue for that :-)

@mikesherov

mikesherov Oct 31, 2012

Member

Although, we should open a separate issue for that :-)

This comment has been minimized.

Show comment Hide comment
break;
+ }
}
}
}
}
}
- // Needed because $( "selector", context ) becomes $( context ).find( "selector" )
+ // Needed because $( selector, context ) becomes $( context ).find( selector )
+ ret = this.pushStack( ret );
ret.selector = ( this.selector ? this.selector + " " : "" ) + selector;

This comment has been minimized.

Show comment Hide comment
@gibson042

gibson042 Oct 31, 2012

Member

.selector is only relevant for the string case... why not early return this.pushStack( jQuery( selector ).filter(function() { ... }) ) if selector is not a string and skip the else block entirely?

@gibson042

gibson042 Oct 31, 2012

Member

.selector is only relevant for the string case... why not early return this.pushStack( jQuery( selector ).filter(function() { ... }) ) if selector is not a string and skip the else block entirely?

This comment has been minimized.

Show comment Hide comment
@mikesherov

mikesherov Oct 31, 2012

Member

Not a bad idea.

@mikesherov

mikesherov Oct 31, 2012

Member

Not a bad idea.

This comment has been minimized.

Show comment Hide comment
@dmethvin

dmethvin Oct 31, 2012

Owner

Yeah I agree that makes more sense. I can do a followup.

@dmethvin

dmethvin Oct 31, 2012

Owner

Yeah I agree that makes more sense. I can do a followup.

return ret;
},
View
@@ -1,20 +1,21 @@
module("traversing", { teardown: moduleTeardown });
-test("find(String)", function() {
- expect(5);
+test( "find(String)", function() {
+ expect( 6 );
equal( "Yahoo", jQuery("#foo").find(".blogTest").text(), "Check for find" );
// using contents will get comments regular, text, and comment nodes
var j = jQuery("#nonnodes").contents();
equal( j.find("div").length, 0, "Check node,textnode,comment to find zero divs" );
+ equal( j.find("div").andSelf().length, 3, "Check node,textnode,comment to find zero divs, but preserves pushStack" );
- deepEqual( jQuery("#qunit-fixture").find("> div").get(), q("foo", "moretests", "tabindex-tests", "liveHandlerOrder", "siblingTest", "fx-test-group"), "find child elements" );
- deepEqual( jQuery("#qunit-fixture").find("> #foo, > #moretests").get(), q("foo", "moretests"), "find child elements" );
- deepEqual( jQuery("#qunit-fixture").find("> #foo > p").get(), q("sndp", "en", "sap"), "find child elements" );
+ deepEqual( jQuery("#qunit-fixture").find("> div").get(), q( "foo", "moretests", "tabindex-tests", "liveHandlerOrder", "siblingTest", "fx-test-group" ), "find child elements" );
+ deepEqual( jQuery("#qunit-fixture").find("> #foo, > #moretests").get(), q( "foo", "moretests" ), "find child elements" );
+ deepEqual( jQuery("#qunit-fixture").find("> #foo > p").get(), q( "sndp", "en", "sap" ), "find child elements" );
});
-test("find(node|jQuery object)", function() {
- expect( 11 );
+test( "find(node|jQuery object)", function() {
+ expect( 12 );
var $foo = jQuery("#foo"),
$blog = jQuery(".blogTest"),
@@ -23,18 +24,19 @@ test("find(node|jQuery object)", function() {
$fooTwo = $foo.add( $blog );
equal( $foo.find( $blog ).text(), "Yahoo", "Find with blog jQuery object" );
- equal( $foo.find( $blog[0] ).text(), "Yahoo", "Find with blog node" );
+ equal( $foo.find( $blog[ 0 ] ).text(), "Yahoo", "Find with blog node" );
equal( $foo.find( $first ).length, 0, "#first is not in #foo" );
- equal( $foo.find( $first[0]).length, 0, "#first not in #foo (node)" );
+ equal( $foo.find( $first[ 0 ]).length, 0, "#first not in #foo (node)" );
ok( $foo.find( $two ).is(".blogTest"), "Find returns only nodes within #foo" );
ok( $fooTwo.find( $blog ).is(".blogTest"), "Blog is part of the collection, but also within foo" );
- ok( $fooTwo.find( $blog[0] ).is(".blogTest"), "Blog is part of the collection, but also within foo(node)" );
+ ok( $fooTwo.find( $blog[ 0 ] ).is(".blogTest"), "Blog is part of the collection, but also within foo(node)" );
equal( $two.find( $foo ).length, 0, "Foo is not in two elements" );
- equal( $two.find( $foo[0] ).length, 0, "Foo is not in two elements(node)" );
+ equal( $two.find( $foo[ 0 ] ).length, 0, "Foo is not in two elements(node)" );
equal( $two.find( $first ).length, 0, "first is in the collection and not within two" );
equal( $two.find( $first ).length, 0, "first is in the collection and not within two(node)" );
+ equal( $two.find( $foo[ 0 ] ).andSelf().length, 2, "find preserves the pushStack, see #12009" );
});
test("is(String|undefined)", function() {

0 comments on commit e8f9151

Please sign in to comment.