Remove popover content with .children().detach() instead of .empty()

[programcsharp]

Using .empty() is bad because it blows up the content. If you're planning on using it later or swapping it around, you can't. jQuery won't let you reattach the content or rewire events to it. Instead, do .children().detach().

This is a refinement of the fix in #13165.

[hnrch02]

No need to split this up into separate lines, you can use .end() after the .detach() and get the original set back.

[programcsharp]

Good point. Updated accordingly.

[hnrch02]

/cc @fat

[fat]

cool thanks!

[cvrebert]

@programcsharp Do you have an example of something that broke with the empty() approach? I'm trying to belatedly add a regression test for this pull request.

[programcsharp]

I believe this was an issue of reuse of the same content in a popover over
multiple open/close cycles causing issues, particularly with events wired
to those elements.

Empty destroys jQuery elements in such a way that they can't properly be
readded -- even if you try to rewire events they never fire. It (and
remove) are designed to be used when the elements will never be used again.
It properly and fully destroys them to keep memory leaks from happening:
http://www.bennadel.com/blog/1822-learning-jquery-1-4-remove-vs-detach.htm

@programcsharp https://github.com/programcsharp Do you have an example of
something that broke with the empty() approach? I'm trying to belatedly add
a regression test for this pull request.

—
Reply to this email directly or view it on GitHub
#14244 (comment).

[programcsharp]

The specific scenario was a popover using the body container layout,
triggered on click. Inside the popover was custom HTML with click handlers
wired. With the empty method, it works fine the first time and appears to
work subsequently (HTML looks fine), but click handlers never fire. I tried
rewiring them on popover show but that didn't work either.

If you can't get a repro, let me know and I'll hunt up an example.
On Dec 18, 2014 10:16 PM, "Chris Hynes" [email protected] wrote:

I believe this was an issue of reuse of the same content in a popover over
multiple open/close cycles causing issues, particularly with events wired
to those elements.

Empty destroys jQuery elements in such a way that they can't properly be
readded -- even if you try to rewire events they never fire. It (and
remove) are designed to be used when the elements will never be used again.
It properly and fully destroys them to keep memory leaks from happening:
http://www.bennadel.com/blog/1822-learning-jquery-1-4-remove-vs-detach.htm

@programcsharp https://github.com/programcsharp Do you have an example
of something that broke with the empty() approach? I'm trying to
belatedly add a regression test for this pull request.

—
Reply to this email directly or view it on GitHub
#14244 (comment).

[cvrebert]

@programcsharp Yeah, I grok what the issue is, and tried to write a testcase, but was unable to make the test fail after reverting your fix... 😕
Would very much appreciate a concrete example using the circa-v3.2.0 popover plugin, if you have the time.

[programcsharp]

Here you are: http://jsfiddle.net/1frd7kgu/

Click the "repro .empty() issue" button once. Popover comes up, button inside works. Close popover. Click button again. Button inside doesn't work now, or ever again.

[cvrebert]

@programcsharp Thanks a million!