New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Presence of disconnectedCallback can expedite an invocation of connectedCallback #760

Open
rniwa opened this Issue Aug 15, 2018 · 25 comments

Comments

Projects
None yet
8 participants
@rniwa
Contributor

rniwa commented Aug 15, 2018

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

This comment has been minimized.

Contributor

rniwa commented Aug 15, 2018

@rniwa

This comment has been minimized.

Contributor

rniwa commented Aug 15, 2018

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

This comment has been minimized.

Contributor

rniwa commented Oct 25, 2018

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

This comment has been minimized.

Contributor

rniwa commented Oct 25, 2018

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

@smaug----

This comment has been minimized.

@rniwa

This comment has been minimized.

Contributor

rniwa commented Oct 25, 2018

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

@domenic

This comment has been minimized.

Contributor

domenic commented Oct 25, 2018

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.

@domenic domenic removed their assignment Oct 25, 2018

@tomalec

This comment has been minimized.

Contributor

tomalec commented Oct 25, 2018

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

annevk added a commit to whatwg/html that referenced this issue Oct 26, 2018

@rniwa

This comment has been minimized.

Contributor

rniwa commented Nov 29, 2018

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

This comment has been minimized.

Contributor

domenic commented Nov 29, 2018

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

This comment has been minimized.

Contributor

rniwa commented Nov 29, 2018

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

This comment has been minimized.

Contributor

rniwa commented Nov 29, 2018

@treshugart

This comment has been minimized.

treshugart commented Nov 30, 2018

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

This comment has been minimized.

justinfagnani commented Nov 30, 2018

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

This comment has been minimized.

justinfagnani commented Nov 30, 2018

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

This comment has been minimized.

WebReflection commented Nov 30, 2018

@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

This comment has been minimized.

Contributor

rniwa commented Dec 5, 2018

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

This comment has been minimized.

WebReflection commented Dec 5, 2018

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

This comment has been minimized.

Member

annevk commented Dec 8, 2018

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

This comment has been minimized.

treshugart commented Dec 9, 2018

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

This comment has been minimized.

Contributor

domenic commented Dec 9, 2018

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

This comment has been minimized.

treshugart commented Dec 10, 2018

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

This comment has been minimized.

Member

annevk commented Dec 10, 2018

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

This comment has been minimized.

Contributor

rniwa commented Dec 10, 2018

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.

kisg pushed a commit to paul99/webkit-mips that referenced this issue Dec 12, 2018

rniwa@webkit.org
connectedCallback is invoked during the removal of the element inside…
… another element's connectedCallback

https://bugs.webkit.org/show_bug.cgi?id=183586
<rdar://problem/38403504>

Reviewed by Frédéric Wang.

Source/WebCore:

Align WebKit's behavior with Chrome/Firefox with regards to w3c/webcomponents#760

After much discussion, it's unclear that there is a clear path forward to fixing the oddness that
the presence of a custom element reaction changes the timing at which another reaction callback gets invoked.
So matching Chrome/Firefox behaviors in this case seems the path of the least resistance to interoperability.

Namely, this patch makes WebKit not insert a custom element to the appropriate element queue when the element
does not have a matching reaction callback. Put it another way, steps 3-5 in would be done before step 6 in:
https://html.spec.whatwg.org/multipage/custom-elements.html#enqueue-a-custom-element-callback-reaction
    1. Let definition be element's custom element definition.
    2. Let callback be the value of the entry in definition's lifecycle callbacks with key callbackName.
    3. If callback is null, then return
    4. If callbackName is "attributeChangedCallback", then:
        1. Let attributeName be the first element of args.
        2. If definition's observed attributes does not contain attributeName, then return.
    5. Add a new callback reaction to element's custom element reaction queue, with callback function callback
       and arguments args.
    6. Enqueue an element on the appropriate element queue given element.

Test: fast/custom-elements/enqueue-custom-element-callback-reactions-inside-another-callback.html

* dom/CustomElementReactionQueue.cpp:
(WebCore::CustomElementReactionQueue::enqueueElementUpgrade):
(WebCore::CustomElementReactionQueue::enqueueConnectedCallbackIfNeeded):
(WebCore::CustomElementReactionQueue::enqueueDisconnectedCallbackIfNeeded):
(WebCore::CustomElementReactionQueue::enqueueAdoptedCallbackIfNeeded):
(WebCore::CustomElementReactionQueue::enqueueAttributeChangedCallbackIfNeeded):
(WebCore::CustomElementReactionQueue::enqueuePostUpgradeReactions):
(WebCore::CustomElementReactionQueue::enqueueElementOnAppropriateElementQueue): Renamed from ensureCurrentQueue.
* dom/CustomElementReactionQueue.h:

LayoutTests:

Added a W3C style testharness test.

* fast/custom-elements/enqueue-custom-element-callback-reactions-inside-another-callback-expected.txt: Added.
* fast/custom-elements/enqueue-custom-element-callback-reactions-inside-another-callback.html: Added.


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@239096 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@rniwa

This comment has been minimized.

Contributor

rniwa commented Dec 13, 2018

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

annevk added a commit to whatwg/html that referenced this issue Dec 13, 2018

annevk added a commit to whatwg/html that referenced this issue Dec 13, 2018

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