Presence of disconnectedCallback can expedite an invocation of connectedCallback

[rniwa]

When two custom elements are enqueued of connected callbacks, and the first connectedCallback removes the second one and inserts it back, whether connectedCallback is invoked during the removal or the insertion depends on the presence of disconnectedCallback.

This seems like a very unintuitive behavior for developers. Why should adding a disconnectedCallback which doesn't do anything change the timing at which connectedCallback is invoked?

See https://gist.github.com/rniwa/537d2f7f29cc536f0731be1ff27258c8 for an example. In this example, the timing at which connectedCallback is invoked is different for Child1 which doesn't have disconnectedCallback and Child2 which does have disconnectedCallback.

[rniwa]

@domenic @justinfagnani @annevk @smaug---- @bzbarsky @hober

[rniwa]

The problem comes down to step 3 of the concept to enqueue a custom element callback reaction exits early when there is no matching callback without enqueueing an element on the appropriate element queue.

WebKit happens to have a bug to not skip the step to enqueue an element on the appropriate element queue even when there is no matching callback, and as a result doesn't exhibit this behavior.

[rniwa]

Rough consensus at TPAC F2F: Always enqueue a custom element regardless of whether it has a specific callback to avoid this changing of the behavior when a new callback is added.

Action Item: Update the spec to fix this problem.

[rniwa]

@tomalec suggested that we also add an informal note saying connectedCallback may be called when a custom element is not connected.

[smaug----]

https://bugzilla.mozilla.org/show_bug.cgi?id=1501968

[rniwa]

Action Item(@rniwa): Make a PR for fixing WPT.

[domenic]

I'm unassigning myself as I won't have time to work on this in the near future. I'd be happy to review a pull request, though.

[tomalec]

Use-case which needs a note/warning in spec or at least MDN https://jsbin.com/secexuq/edit?html,console,output

class ParentElement extends HTMLElement {
    connectedCallback() {
        const child = this.childNodes[0];

        log('Child removal begin');
        this.removeChild(child);
        log('Child removal end');
    }
}
customElements.define('parent-element', ParentElement);

class Child1 extends HTMLElement {
    connectedCallback() {
        log('Child connectedCallback isConnected=' + this.isConnected.toString());
    }

}
customElements.define('child-1', Child1);

const container = document.createElement('div');
container.innerHTML = '<parent-element><child-1></child-1></parent-element>';
customElements.upgrade(container);
document.body.appendChild(container); 
// Child connectedCallback isConnected=false

[rniwa]

Pulling my comment from the PR, I'm having second thoughts about this. If we made this change, presumably, all new reaction callback we would add should do the same. But that would mean that we would enqueue a custom element into the reaction queue at timings we currently don't do. That seems like a serious forward compatibility issue to me.

Another somewhat different fix here might be that avoid invoking connectedCallback or disconnectedCallback when the condition isn't met. As @tomalec mentioned during TPAC, this is a rather very confusion behavior, and unlike MutationObserver, the purpose of custom element reaction queue isn't to record everything that had happened. So it might better to avoid invoking connected or disconnected callbacks when those conditions are not met.

What do you all think?

[domenic]

That seems like a serious forward compatibility issue to me.

Thanks for the careful analysis; I think I agree.

Another somewhat different fix here might be that avoid invoking connectedCallback or disconnectedCallback when the condition isn't met.

The idea here would be, that we'd add something to https://html.spec.whatwg.org/#invoke-custom-element-reactions which is like "If the callback reaction's name is 'connectedCallback', and the element is not connected, skip this callback function"?

I see the attraction. However, I'm unsure of the implications for custom element authors, and for the logic they put in their connectedCallback/disconnectedCallback. The element was briefly connected---even if it isn't right now. But the connected logic just gets ignored? Maybe these cases of "quickly connected and disconnected" shouldn't really count as being connected, in which case this change is fine. But I worry about introducing some other unexpected case, where authors are wondering why their callback isn't firing, and their important setup logic not happening.

I don't have good intuition one way or the other here, and would love to hear more from others (especially authors).

Also I wonder if you have similar thoughts about attributeChangedCallback? Like, if the attribute's current value is not equal to the newValue argument, should we avoid calling attributeChangedCallback?

[rniwa]

I see the attraction. However, I'm unsure of the implications for custom element authors, and for the logic they put in their connectedCallback/disconnectedCallback. The element was briefly connected---even if it isn't right now. But the connected logic just gets ignored? Maybe these cases of "quickly connected and disconnected" shouldn't really count as being connected, in which case this change is fine. But I worry about introducing some other unexpected case, where authors are wondering why their callback isn't firing, and their important setup logic not happening.

Right. That's definitely a risk here. But that kind of mutation analysis is probably best done with MutationObserver instead so perhaps we're better off providing that capability to MutationObserver.

Also I wonder if you have similar thoughts about attributeChangedCallback?

I think attributeChangedCallback is less problematic because there aren't many DOM APIs which mutate multiple element's attributes at once, which is the root cause of the out-of-order reaction delivery being discussed in this issue.

[rniwa]

@JanMiksovsky, @justinfagnani, @treshugart @WebReflection, @sorvell, WDYT?

[treshugart]

Maybe these cases of "quickly connected and disconnected" shouldn't really count as being connected, in which case this change is fine

I can't think of any use cases where this would cause any issues, especially since you'd theoretically also skip the disconnected callback which should undo any side effects in connected.

Also I wonder if you have similar thoughts about attributeChangedCallback? Like, if the attribute's current value is not equal to the newValue argument, should we avoid calling attributeChangedCallback?

I haven't come across any use cases that this would break, but am unsure how comfortable I would be making this change. The word "changed" in the callback does imply that the value is different, though.

Cheers for the mention.

[justinfagnani]

Another somewhat different fix here might be that avoid invoking connectedCallback or disconnectedCallback when the condition isn't met. As @tomalec mentioned during TPAC, this is a rather very confusion behavior, and unlike MutationObserver, the purpose of custom element reaction queue isn't to record everything that had happened. So it might better to avoid invoking connected or disconnected callbacks when those conditions are not met.

I'm personally perfectly fine with MutationObserver-like queuing of callbacks even if the underlying condition isn't met anymore - it seems ultimately simpler to explain if necessary and matches attributeChangedCallback currently. Let me ask around on my team though.

[justinfagnani]

Talking though it here, I'm also aware that this behavior could be weird to devs, so maybe skipping the callbacks is fine in this case too. @bicknellr might have more to say

[WebReflection]

@rniwa I don't have objections in skipping callbacks if conditions aren't met, I would however argue developers should also be capable of understanding when "conditions" are met (i.e. the node is fully known, its tag was closed, the node is ready, all callbacks are trustable).

[rniwa]

Yeah, so the idea would be that if connectedCallback or disconnectedCallback is enqueued, then before invoking those callbacks, we would check for any redundancy; e.g. if the element is currently connected, then we can get rid of disconnected & connected callbacks pairs (in that chronological order) in the reaction queue. If the element is currently disconnected, then we can remove a sequence of pairs of connected and disconnected callbacks.

This way, when you receive connected or disconnected callbacks, you know that the element is currently connected or disconnected. One downside is that you wouldn't be able to tell whether the element had temporarily got connected / disconnected or not by simply listening to reaction callbacks.

[WebReflection]

One downside is that you wouldn't be able to tell whether the element had temporarily got connected / disconnected or not by simply listening to reaction callbacks.

that works for me, I've never even expected these to be synchronous anyway

[annevk]

So you cannot tell if the element was moved? (This means you cannot do <iframe>, which might be a feature, but also seems problematic for anything that has a relation to its parent, e.g., <picture>/<source>.)

[treshugart]

I think in most cases the parent should be handling the relationship with the children and not the other way around.

In the picture / source example, the picture would handle any child tree updates where a source was added or removed.

What is the iframe example you're thinking of @annevk?

Maybe there's room for something similar to the adoptedCallback where a callback is invoked when a custom element is re-parented? Or is it worth discussing the fact that maybe an element should have their connected / disconnected callbacks invoked if it's moved, but not if it's added then removed very quickly?

[domenic]

I think in most cases the parent should be handling the relationship with the children and not the other way around.

You can't do that though with today's technology, given that callbacks are triggered on the child, not the parent. If we wanted to change that, it would be pretty awkward to implement and spec; triggering callbacks on X when X gets connected/disconnected is easy, but having X's parent watch all of X's children (or descendants, in some cases!) requires a lot more machinery.

Or is it worth discussing the fact that maybe an element should have their connected / disconnected callbacks invoked if it's moved, but not if it's added then removed very quickly?

Right now moving is defined as removing and then adding, so introducing this distinction would be an unfortunate complication to the model.

Overall I'm leaning back toward the status quo, just because it's more primitive and fundamental, and you can implement semantic batching on top of it. I'm also worried about implementing batching at this level for connect/disconnect but not attributeChanged, although as @rniwa points out it might not be observable in practice given the current set of DOM APIs.

[treshugart]

but having X's parent watch all of X's children (or descendants, in some cases!) requires a lot more machinery.

True. I think I'm spoiled by abstractions for this.

Overall I'm leaning back toward the status quo, just because it's more primitive and fundamental, and you can implement semantic batching on top of it.

In retrospect, this is probably the safest option.

[annevk]

To answer the earlier question, if you move an iframe around, you keep destroying/creating its associated "inner" browsing context and global environment and such.

[rniwa]

Overall I'm leaning back toward the status quo, just because it's more primitive and fundamental, and you can implement semantic batching on top of it. I'm also worried about implementing batching at this level for connect/disconnect but not attributeChanged, although as @rniwa points out it might not be observable in practice given the current set of DOM APIs.

Yeah, the states quo might not be the worst behavior here. The currently spec'ed behavior is definitely weird but only when you start mutating DOM inside a custom element reaction, which is arguably already discouraged. I think we need to update MDN, etc... to make that discouragement more clear & prominent.

[rniwa]

I've updated WebKit's implementation to match the status quo in https://trac.webkit.org/changeset/239096