Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

element.wrap not working when jQuery is present #3860

Closed
luiscarlosjayk opened this issue Sep 3, 2013 · 5 comments
Closed

element.wrap not working when jQuery is present #3860

luiscarlosjayk opened this issue Sep 3, 2013 · 5 comments

Comments

@luiscarlosjayk
Copy link

Hi, I spent hours trying to figure you why my directive code wasn't working.
I came to make a simple directive example and realized that when I removed jQuery it worked!

http://plnkr.co/edit/JEaYyrhkJXIWCaoUUoFq?p=preview

The element.wrap functions is doing as expected withouth jQuery but when jQuery is present well... :(

Any idea?

@petebacondarwin
Copy link
Contributor

The non-jquery version is not working either... It is not actually wrapping the directive's element.

@cqr
Copy link
Contributor

cqr commented Sep 29, 2013

@Ciul It seems like in this case, neither should work but jqLite is a little bit more forgiving. Because the paragraph in your example is never attached to the DOM explicitly, jQuery is actually doing the right thing. It happens that jqLite is implemented in a way that appends the element to the wrapper if the wrapper is already in the DOM, but this is not the documented behavior.

I'm a little worried about the use of wrap(), because like many of the jqLite methods, you almost never really want to use them. If you want to accomplish what you seem to be trying to do in your plunker, append() is what you want. If your use case is more complicated, you can review the intended behavior of wrap() here: http://plnkr.co/edit/XwdWXpYG64xwkpuMdSzq

@petebacondarwin It looks like you're mistaken. The semantics of wrap() are that the target becomes wrapped in the argument. In this case, the target is a paragraph element and the argument is the element with the directive, so it's working as is documented.

http://api.jquery.com/wrap/

I recommend that this issue be closed.

@cqr
Copy link
Contributor

cqr commented Sep 29, 2013

Also, angular team, I'd be happy to write the patch that brings jqLite's behavior in line with jQuery's if this seems important. Basically, it looks like they clone the wrapNode in the event that the target element has no parentNode, and attach it as the target's parentNode. Pretty close to a no-op unless you then grab the parent, but pretty easy to change so that jqLite does it correctly.

cqr added a commit to cqr/angular.js that referenced this issue Sep 29, 2013
Change jqLite's implementation of wrap() to clone the wrapNode before
wrapping the target element in it.
Match jQuery's wrap() behavior and prevent accidentally attaching
target element to the DOM as a side effect.

Closes angular#3860
@luiscarlosjayk
Copy link
Author

@chrisrhoden so both the target and the wrapper must be already attached to the DOM? If that's the case it should explicitly say so. In my example, as you say, the paragraph is not attached, want to wrap it with the directive's element. But jQuery doesn't do that, while jqLite does it without problem.

Append works fine, but wrap should work too, I think.

Now, no matter if wrap should or shouldn't work, there's still a big issue here. jqLite should work just as jQuery does and it isn't doing that. If that's the case, just think a ridiculous idea where we would need then to do

if (window.jQuery)
   // behave this way
else
   // behave that way

and just because they are working different, which isn't desired as I understand from documentation.

Just something that should be fixed in angular-element.

Regards,
Ciul

@cqr
Copy link
Contributor

cqr commented Sep 29, 2013

@Ciul
No, the target is the only thing that needs to be attached to the DOM. What is passed as an argument to wrap() should have no impact on whether the target is attached to the DOM, though, which is the problem we're seeing. I have submitted a pull request that fixes the behavior so that they both work as documented.

If you want the behavior documented by append(), you should use it. wrap() is specifically for taking elements and wrapping them in some additional markup. The fact that the additional markup you are wrapping them in is already attached to the DOM should not result in this working like append(), which it does now, and I have submitted a patch to resolve it.

cqr added a commit to cqr/angular.js that referenced this issue Aug 1, 2014
Change jqLite's implementation of wrap() to clone the wrapNode before
wrapping the target element in it.
Match jQuery's wrap() behavior and prevent accidentally attaching
target element to the DOM as a side effect.

Closes angular#3860
cqr added a commit to cqr/angular.js that referenced this issue Aug 1, 2014
Change jqLite's implementation of wrap() to clone the wrapNode before
wrapping the target element in it.
Match jQuery's wrap() behavior and prevent accidentally attaching
target element to the DOM as a side effect.

Closes angular#3860
IgorMinar pushed a commit that referenced this issue Aug 19, 2014
Change jqLite's implementation of wrap() to clone the wrapNode before
wrapping the target element in it.
Match jQuery's wrap() behavior and prevent accidentally attaching
target element to the DOM as a side effect.

Closes #3860
Closes #4194
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants