Fix for #2808; inconsistent .not/.filter #2809
Conversation
|
By analyzing the blame information on this pull request, we identified @gibson042, @timmywil and @markelog to be potential reviewers |
|
Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA). 📝 Please visit http://contribute.jquery.org/CLA/ to sign. After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know. If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check. |
There was a problem hiding this comment.
the cheaper check should be evaled first
There was a problem hiding this comment.
It may be a cheaper check on it's own, but it's a far less discriminating check in most use cases as it would only ever be false when using non-element nodes.
As far as I know, only the .contents() function can return non-element nodes, so not a very common situation.
Placing the nodeType check last would shortcut it in most cases.
Placing the nodeType check first would mean both checks would have to be evaluated in most cases.
There was a problem hiding this comment.
True, the nodeType check is pretty rarely going to fail so the order isn't too important here.
Inconsistent ehaviour for non-element nodes when using multiple recursive pseudo-selectors on `*` in `.not()` and `.filter()`.
|
The code change looks good, but it needs |
|
Unittests should be simple enough. |
|
Added two unittests which cover the issue. |
There was a problem hiding this comment.
Can you combine these tests together into something like "not(Selector) excludes non-element nodes (gh-2808)", and use simpler selectors? E.g.,
var mixedContents = jQuery( "#nonnodes" ).contents(),
childElements = q( "nonnodesElement" );
assert.deepEqual( mixedContents.not( "*" ).get(), [], "not *" );
assert.deepEqual( mixedContents.not( "[id=a],[id=b]" ).get(), childElements, "not [id=a],[id=b]" );
assert.deepEqual( mixedContents.not( "[id=a],*,[id=b]" ).get(), [], "not [id=a],*,[id=b]" );
6 participants
Inconsistent ehaviour for non-element nodes when using multiple recursive pseudo-selectors on
*in.not()and.filter().Fixes gh-2808