#4087 - insertAfter, insertBefore, etc do not work when destination is original element #1033

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
@PaulBRamos
Contributor

PaulBRamos commented Nov 18, 2012

No description provided.

@@ -272,7 +272,8 @@ jQuery.fn.extend({
// Make sure that the elements are removed from the DOM before they are inserted
// this can help fix replacing a parent with child elements
- if ( !isFunc && typeof value !== "string" ) {
+ // #4087 - don't detach if original and target are the same (element is removed otherwise)

This comment has been minimized.

@gibson042

gibson042 Nov 18, 2012

Member

I think this entire special-case block and corresponding expense of .index can be eliminated if the jQuery( this ).remove() on L296 moves after the .before/.append. Did you try anything like that?

@gibson042

gibson042 Nov 18, 2012

Member

I think this entire special-case block and corresponding expense of .index can be eliminated if the jQuery( this ).remove() on L296 moves after the .before/.append. Did you try anything like that?

This comment has been minimized.

@PaulBRamos

PaulBRamos Nov 18, 2012

Contributor

I didn't, but I can definitely give it a shot.

@PaulBRamos

PaulBRamos Nov 18, 2012

Contributor

I didn't, but I can definitely give it a shot.

This comment has been minimized.

@markelog

markelog Nov 25, 2012

Member

jQuery.remove() not only remove node from DOM, but also remove all data from the element,
so with this change, if we have, for example, events attached to node that replaces itself, all events from that node will be lost.

@PaulBRamos how you feel about this suggestion?

@markelog

markelog Nov 25, 2012

Member

jQuery.remove() not only remove node from DOM, but also remove all data from the element,
so with this change, if we have, for example, events attached to node that replaces itself, all events from that node will be lost.

@PaulBRamos how you feel about this suggestion?

@@ -525,7 +527,7 @@ jQuery.buildFragment = function( args, context, scripts ) {
if ( !fragment ) {
fragment = context.createDocumentFragment();
- jQuery.clean( args, context, fragment, scripts );
+ jQuery.clean( args, context, fragment, scripts, originalContext[0] );

This comment has been minimized.

@gibson042

gibson042 Nov 18, 2012

Member

That's a dangerous expression (see L506)... I'm not saying it's wrong, but you should throw a lot at this to test for robustness.

@gibson042

gibson042 Nov 18, 2012

Member

That's a dangerous expression (see L506)... I'm not saying it's wrong, but you should throw a lot at this to test for robustness.

This comment has been minimized.

@PaulBRamos

PaulBRamos Nov 18, 2012

Contributor

I was a bit worried about it myself, but didn't see any issues during testing. Can you think of a scenario where this might be an issue?

@PaulBRamos

PaulBRamos Nov 18, 2012

Contributor

I was a bit worried about it myself, but didn't see any issues during testing. Can you think of a scenario where this might be an issue?

This comment has been minimized.

@PaulBRamos

PaulBRamos Nov 18, 2012

Contributor

Ahh, nevermind, I missed the reference to L506. I'll explore that.

@PaulBRamos

PaulBRamos Nov 18, 2012

Contributor

Ahh, nevermind, I missed the reference to L506. I'll explore that.

@@ -783,6 +785,11 @@ jQuery.extend({
for ( i = 0; (elem = ret[i]) != null; i++ ) {
// Check if we're done after handling an executable script
if ( !( jQuery.nodeName( elem, "script" ) && handleScript( elem ) ) ) {
+ // #4087 - dom manipulation not working when destination is original element (element is removed)
+ if ( originalContext && originalContext == elem ) {
+ continue;

This comment has been minimized.

@gibson042

gibson042 Nov 18, 2012

Member

This is going to have implications with respect to managing embedded <script>s, so test it extra thoroughly. Also, we use === instead of == except for the special case of window.

@gibson042

gibson042 Nov 18, 2012

Member

This is going to have implications with respect to managing embedded <script>s, so test it extra thoroughly. Also, we use === instead of == except for the special case of window.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Nov 18, 2012

Member

This approach has promise, but also a fair amount of risk and potential pitfalls. I have to wonder about alternatives like creating temporary proxy nodes (that wouldn't get detached when their analogs were pulled into a fragment), or—potentially more performant—a callback for domManip/buildFragment that lets the caller see the set of preexisting nodes about to be swept into a fragment and perform some prep work before that happens. In this case, the callback would look something like:

function( fragmentChildren ) {
    self.each(function() {
        jQuery._data( this, "sub", isDisconnected( this ) || !jQuery.inArray( this, fragmentChildren ) ?
            this :
            this.parentNode.insertBefore( this.ownerDocument.createTextNode(""), this )
        );
    });
}
Member

gibson042 commented Nov 18, 2012

This approach has promise, but also a fair amount of risk and potential pitfalls. I have to wonder about alternatives like creating temporary proxy nodes (that wouldn't get detached when their analogs were pulled into a fragment), or—potentially more performant—a callback for domManip/buildFragment that lets the caller see the set of preexisting nodes about to be swept into a fragment and perform some prep work before that happens. In this case, the callback would look something like:

function( fragmentChildren ) {
    self.each(function() {
        jQuery._data( this, "sub", isDisconnected( this ) || !jQuery.inArray( this, fragmentChildren ) ?
            this :
            this.parentNode.insertBefore( this.ownerDocument.createTextNode(""), this )
        );
    });
}
@PaulBRamos

This comment has been minimized.

Show comment
Hide comment
@PaulBRamos

PaulBRamos Nov 18, 2012

Contributor

I definitely see your concerns. I had considered the method you mentioned above, but went with this solution to minimize the footprint of the change. I'll explore this alternative if you think it's more promising.

Contributor

PaulBRamos commented Nov 18, 2012

I definitely see your concerns. I had considered the method you mentioned above, but went with this solution to minimize the footprint of the change. I'll explore this alternative if you think it's more promising.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Nov 19, 2012

Member

@PaulBRamos Thanks for looking at this ticket! As you can tell it's been sitting around for a while but your solution looks like it could be pretty elegant. It's definitely worth doing some experiments to see if there is a better/smaller way.

Member

dmethvin commented Nov 19, 2012

@PaulBRamos Thanks for looking at this ticket! As you can tell it's been sitting around for a while but your solution looks like it could be pretty elegant. It's definitely worth doing some experiments to see if there is a better/smaller way.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Nov 21, 2012

Member

@PaulBRamos I'd certainly like to see how it goes. Now that I've pulled the rug out from under these changes with 22f58bd anyway, how about giving it a shot with a new branch from master?

Member

gibson042 commented Nov 21, 2012

@PaulBRamos I'd certainly like to see how it goes. Now that I've pulled the rug out from under these changes with 22f58bd anyway, how about giving it a shot with a new branch from master?

@PaulBRamos

This comment has been minimized.

Show comment
Hide comment
@PaulBRamos

PaulBRamos Nov 21, 2012

Contributor

@gibson042 That's exactly what I'm doing. :)

Contributor

PaulBRamos commented Nov 21, 2012

@gibson042 That's exactly what I'm doing. :)

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Nov 28, 2012

Member

Where does this stand now? It seemed like the technique was showing some promise.

Member

dmethvin commented Nov 28, 2012

Where does this stand now? It seemed like the technique was showing some promise.

@PaulBRamos

This comment has been minimized.

Show comment
Hide comment
@PaulBRamos

PaulBRamos Nov 28, 2012

Contributor

I'm about to do a pull request for a newer version of this fix.

Contributor

PaulBRamos commented Nov 28, 2012

I'm about to do a pull request for a newer version of this fix.

@PaulBRamos

This comment has been minimized.

Show comment
Hide comment
@PaulBRamos

PaulBRamos Nov 28, 2012

Contributor

new pull request for this issue: #1047

Contributor

PaulBRamos commented Nov 28, 2012

new pull request for this issue: #1047

@PaulBRamos PaulBRamos closed this Nov 28, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment