Bug Tracker

Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#13545 closed bug (wontfix)

.find breaks with documentfragments

Reported by: sahbeewah@… Owned by:
Priority: low Milestone: None
Component: selector Version: 2.0b2
Keywords: Cc: dmethvin, timmywil
Blocked by: Blocking:

Description

Unexpected behaviour when dealing with DocumentFragments...

var $d = $(document.createDocumentFragment());
$d.append($('<div><a>wee</a></div>')).append($('<p>my paragraph</p>'));

var response;
response = 'I found ' + $d.children().length + ' children.<br>';
response += 'I found ' + $d.children('div').length + ' div children.<br>';
$d.children().each(function() { 
    var escapedChildren = this.outerHTML.replace(/</g, '&lt;').replace(/>/g, '&gt;');
    response += 'Child: ' + escapedChildren + '<br>';
});
response += 'But ' + $d.find('a').length + ' As using find...<br>';
response += 'But ' + $d.find('div').length + ' DIVs using find...<br>';
response += 'Wat?';
$('#report').html(response);

returns:

I found 2 children.
I found 1 div children.
Child: <div><a>wee</a></div>
Child: <p>my paragraph</p>
But 0 As using find...
But 0 DIVs using find...
Wat?

Tested on Chrome 24.0.1312.70 on Debian

Change History (9)

comment:1 Changed 7 years ago by sahbeewah@…

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

My Bad.

http://jsfiddle.net/w7KFN/

comment:2 Changed 7 years ago by bjohn465

.find() uses Sizzle, which appears to always return an empty array when a document fragment is used: https://github.com/jquery/sizzle/blob/4bd24fccc7c00191d57c09f111b074f516203369/sizzle.js#L239-L241

In the test case, when this bit of Sizzle code is executed, context is the document fragment, which has a nodeType of 11, therefore zero results are returned.

Looking further down in sizzle.js, it appears document fragments are excluded because they don't have some methods used by the shortcuts, such as getElementById (line 250), getElementsByClassName (line 278), and, what would probably be used in the given test case if this were not a document fragment, getElementsByTagName (line 274). There could be other methods Sizzle expects from context that document fragments do not include.

comment:3 Changed 7 years ago by gibson042

Cc: dmethvin timmywil added
Component: unfiledselector

Absolutely correct. And looking at that block of code with fresh eyes, I think it's due for some tweaking... instead of returning empty for an invalid context, we ought to throw an exception.

comment:4 Changed 7 years ago by dmethvin

I'm not a big fan of throwing exceptions but it seems appropriate for this case since it emphasizes that document fragments are not supported.

comment:5 Changed 7 years ago by timmywil

Priority: undecidedlow
Status: newopen
Version: git2.0b2

Let's not throw an exception. Consistency is the issue. I'm not sure which way to lean for consistency though.

comment:6 Changed 6 years ago by dmethvin

I should also point out here that document fragments are not considered to be valid input to any of the jQuery APIs and we don't have test cases for them. So "notabug" could also be the correct answer, and if we're going to fix this we'll need a lot more test cases.

comment:7 Changed 6 years ago by timmywil

Resolution: wontfix
Status: openclosed

I don't think we should make a change here. Sometimes document fragments need to work (because we created them) and sometimes they don't (because we didn't). ;) Besides, document fragments aren't supported. I don't think we need to _ensure_ no support for them.

comment:9 in reply to:  7 Changed 6 years ago by tenbits

Replying to timmywil:

I don't think we should make a change here. Sometimes document fragments need to work (because we created them) and sometimes they don't (because we didn't). ;) Besides, document fragments aren't supported. I don't think we need to _ensure_ no support for them.

Really wired behaviour:

fragment = document.createDocumentFragment();
fragment.appendChild(document.createElement('div'));

$(fragment).find('div') //> []
fragment.querySelectorAll('div') //> [HTMLDivElement]

Native querySelector works as expected, but the jQuery doesn't. As for me, this should be considered as a bug.

Note: See TracTickets for help on using tickets.