Skip to content
This repository has been archived by the owner on Sep 20, 2019. It is now read-only.

removeChild followed by appendChild results in no detached/attached callbacks #311

Closed
kevinpschaaf opened this issue May 22, 2015 · 5 comments

Comments

@kevinpschaaf
Copy link
Contributor

Repro in test case here: https://github.com/Polymer/polymer/pull/1591/files#r30874592

Removing takeRecords in following code results in no callbacks being called:

        domBind.parentElement.removeChild(domBind);
        // TODO(kschaaf): detached/attached not called if takeRecords isn't
        // called between remove & insert on polyfill... seems bad?
        CustomElements.takeRecords();
        container.appendChild(domBind);
@jmesserly
Copy link
Contributor

I think this is by design? due to the async nature of MutationObserver, it's impossible to deliver attached/detached the same way spec'd CE will. So by the time we'd deliver the detached/attached, the callback wouldn't reflect current state of the doc, e.g. we'd say "detached" when it's in the document or "attached" when it's not. That seems bad too. Hence the guard: https://github.com/webcomponents/webcomponentsjs/blob/master/src/CustomElements/observe.js#L55
https://github.com/webcomponents/webcomponentsjs/blob/master/src/CustomElements/observe.js#L143

Not sure how we can do better given constraints of existing browsers. Maybe we could special case deatched->attached and make it do something different from attached->detached. Or maybe a polyfill only callback to mean "node moved" ...

@kevinpschaaf
Copy link
Contributor Author

Hmmm, it's ironic, because in my code snippit above, what I really wanted to do was just container.appendChild(domBind) (without the remove) to move the node to a new location, but I had to split it into removeChild -> takeRecords -> appendChild on polyfill, otherwise the element would get no callback indicating anything happened w.r.t. its position in the DOM (this is a case where the element needs to take specific action based on its position in the DOM changing).

I say ironic, because in the case that you just appendChild a node to the new location with native CE on Chrome, you actually do get detached called while it's in the new position in the DOM (i.e. with its new parentElement): http://jsbin.com/jenaraxeje/3/edit So it's already a little weird.

I guess the choice is, given the fact that we can't match the exact behavior on polyfill, in the case that you remove-then-append in a turn, is it better to eat all the callbacks (current), or give a detached with wrong position? Given that's exactly what you get when you don't do the (otherwise unnecessary) remove before appending, the latter seems slightly better given the loss of notification.

That said, I realize there are other cases like append-then-remove, and in that case not being able to rely on the parent et.al. being correct seems bad.

Will discuss with @sorvell on other ideas...

@jmesserly
Copy link
Contributor

I say ironic, because in the case that you just appendChild a node to the new location with native CE on Chrome, you actually do get detached called while it's in the new position in the DOM (i.e. with its new parentElement): http://jsbin.com/jenaraxeje/3/edit So it's already a little weird.

ah, good point

I guess the choice is, given the fact that we can't match the exact behavior on polyfill, in the case that you remove-then-append in a turn, is it better to eat all the callbacks (current), or give a detached with wrong position? Given that's exactly what you get when you don't do the (otherwise unnecessary) remove before appending, the latter seems slightly better given the loss of notification.

yeah, that makes sense. +1 from me, fwiw :)

sorvell pushed a commit that referenced this issue Jul 7, 2015
Ensure attached & detached occur for moves in the same turn. Fixes #311
@jmesserly
Copy link
Contributor

fix LGTM 👍

@paulie4
Copy link

paulie4 commented Jul 24, 2015

Is this a duplicate of #18?

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

No branches or pull requests

3 participants