Skip to content
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

Consider adding a childrenChangedCallback or something #619

Closed
dominiccooney opened this issue Dec 26, 2016 · 18 comments
Closed

Consider adding a childrenChangedCallback or something #619

dominiccooney opened this issue Dec 26, 2016 · 18 comments

Comments

@dominiccooney
Copy link
Contributor

Chromium has got feedback from developers (for example Issues 676365, 672732, 658119, 639847, etc.) which indicates some confusion about when child nodes are accessible in a custom element's connectedCallback around construction time. Some confusion is probably because Blink had bugs, but this is inherently a bit confusing—sometimes the connectedCallback after upgrade has the child nodes (fragment parsing, upgrade on insertion) and sometimes it doesn't (parsing.)

There's some relevant discussion on Issue 551, which includes the suggestion to add a "children changed" callback.

I suspect some use case elicitation is in order here, for example, I've also heard people discuss a "parser done adding children" callback which is semantically very different to "children changed", and I note mutation observers can observe not just children but also subtrees.

Another idea could be to let MutationObservers have an option to deliver changes at CEReactions time. I haven't really thought hard about the implementation feasibility of this, but it might be worth considering as an alternative to developing callbacks piecemeal, and the dynamism of disconnect may be useful.

@tnikolai2
Copy link

tnikolai2 commented Dec 26, 2016

Now in almost all implementations connectedCallback behave as connectedAndParsedChildrensCallback and child nodes are fully accessible. I think need left as is.
In Chrome/Canary if custom element define in markup - connectedCallback has empty child list, but if custom element added from js script - child nodes are fully accessible.

For example if need header/footer can write this.innerHTML="Header ..."+this.innerHTML+"Footer...";
With new specification this simple task will be impossible or need complicated code with childrenChangedCallback/MutationObservers.
And if child content will be "children1 children2 children3 ... children_final" - need somehow determine moment when children_final added. If childrens has nested childrens - it can be very complicated task.

If custom component has nested element, and connectedCallback has empty child list - this event absolutelly useless for web developers.

Custom elemets are very simple, but if read specification - very very complicated.

Must be so:
Firstly all dom structure must be created without any CEReactions(ecxeptions - adopt/upgrade), for custom elements need invoke only constructor or use HTMLUnknownElement.
CEReaction for element starts only after parent element finished connectedCallback event. Also first CEReaction for custom element must be connectedCallback(ecxeptions - adopt/upgrade).

@matthewp
Copy link

Now in almost all implementations connectedCallback behave as connectedAndParsedChildrensCallback and child nodes are fully accessible. I think need left as is.

Whether that's true or not, I think a childrenChangedCallback would be useful in cases where your element needs to do work any time children change (which can be after connectedCallback). I've added such a callback (even named the same 😉) in my projects (using MO).

@dominiccooney
Copy link
Contributor Author

Now in almost all implementations connectedCallback behave as connectedAndParsedChildrensCallback

Let's assume that's true. As far as I know there's only two mostly complete implementations of this. Something happening in "almost all" of two isn't that persuasive to me.

For example if need header/footer can write this.innerHTML="Header ..."+this.innerHTML+"Footer..."; With new specification this simple task will be impossible or need complicated code with childrenChangedCallback/MutationObservers.

Is there a specific proposal you're referring to? I don't think anyone is proposing the behavior of innerHTML will change. That uses fragment parsing. Custom elements get upgraded and see populated child nodes (unless another custom element modifies them.)

I think there's a slightly bigger point here, and that's what an element should do. The built-in elements can all be created by document parsing, fragment parsing (such as innerHTML), createElement, etc. and you can modify their children and they respond to those modifications. Jan is codifying some of these patterns in this checklist; "detached instantiation" and "content changes" are relevant to this discussion. One side effect of having main document and innerHTML parsing be so different means you pretty much have to respond to content changes, although it's more work.

Another argument I have heard against responding to content changes is performance. Some authors want to process custom elements like macros and it's inefficient for them to continue to get connectedCallbacks. HTML has a legacy wart like that—isindex.

Ideally we want something where it's possible, even easy, to do the right thing (which probably means responding to content changes.)

@treshugart
Copy link

treshugart commented Dec 28, 2016

If there is such an implementation, I think it should be called both on when the children are first added as well as when children change.

There's a similar proposal in SkateJS and I've been playing around with implementations in the corresponding PR. The problem arose from needing to get nodes for a certain slot so that we could render() out information based on those nodes, such as a TodoMVC implementation where you need to show the todo count based on the number of items (in this case, added as light DOM).

It seems that if a childrenChangedCallback() existed, that we'd be able to optimise this implementation. Currently, the PR for that issue has to completely subvert slotchange and use a MutationObserver because that event doesn't fire when the children are first attached to the host, but only on subsequent updates. This is problematic for reasons mentioned in the issue.

@treshugart
Copy link

treshugart commented Dec 28, 2016

Just sketching out how a polyfill (used as a mixin) for this might look: http://www.webpackbin.com/EJ66Qma4G.

@rektide
Copy link

rektide commented Jan 4, 2017

i'm hugely in favor of not leaving MutationObserver partially re-implemented in Web Components (only having a attributeChangedCallback). I'd like to see either not duplication between functionality (as in, remove attributeChangedCallback entirely, and drop a 2.0), or add all capabilities of MutationObserver.

I don't care about characterData, but it seems odd to fix the childrenChanged/attributeChanged discrepancy between MutationObserver and Custom Elements without fixing the characterDataChanged discrepancy. Getting 2/3 of the way to parity looks like progress to me- really good progress- but it still looks funky when some people are going to end up breaking out MutationObserver anyway. (Not that I have much idea what a good characterData use case is). For sheer consistency sake, if we're going to be covering the exact same turf as MutationObserver (which we already are) we should do it well and consistently so as to avoid PR's like this.

@treshugart
Copy link

@rektide to add to that, the slotchange event is also covering similar functionality for slotting (which can be polyfilled by using a mutation observer on the host and checking the slot attribute).

@tnikolai2
Copy link

parsedCallback

@rektide
Copy link

rektide commented Jan 4, 2017

@treshugart rather than introduce a new kind of eventing- slotChange- i'd much rather see parity with what already exists carried forward. it makes sense to me to make a consistent childrenChanged addition to CE than to add an entirely new concept. i'd like to see a new issue or PR opened/forked off for slotChanged if you think something is an ideal replacement for what is requested here, which is childrenChanged (and perhaps maybe additionally characterDataChanged).

I'm simply not familiar with what a slot is, so trying to interpret the language in skatejs/skatejs#863 is rather hard. If it has a pertinent impact on Custom Elements, I'd like to see it clearly defined in it's own area, rather than jumbled into this current discussion, although I'm fine with this issue referencing that specific topic & vice verse.

@treshugart
Copy link

@rektide slotchange is already a thing. More details in the DOM standard mutation observers section and the Shadow DOM spec.

That issue in Skate was just a reference for a more specific implementation we were spiking that is related to the same use-cases that a childrenChangedCallback would solve. I was hoping that it could serve as proof where an abstract such as this callback could prove very useful. I don't feel it's detracted from this discussion.

@trusktr
Copy link
Contributor

trusktr commented Oct 8, 2017

I've implemented this myself, and basically #668 makes it very difficult to get it just right.

For instance, during parsing, the order of connectedCallback calls in a tree of elements is from parent to leaf-most child. However, the order of connectedCallbacks when using innerHTML or just waiting for the document to be ready before defining elements is from leaf-most child to parent (backwards!).

These sort of inconsistencies make it difficult to write clean programs.

In React, Vue, Angular, etc, the order of callbacks are guaranteed. People coming from those libraries will stumble on this.

@trusktr
Copy link
Contributor

trusktr commented Oct 8, 2017

For reference, here's my implementation: WebComponent.js (on these lines).

Mine is childConnectedCallback and childDisconnectedCallback though, which fire once per child rather than for all children at once, so slightly different. If childrenChangedCallback existed, then it seems like it would be merely a shortcut for a MutationObserver, and that I'd end up implementing childConnected and childDisconnected on top of it anyways.

Using setTimeout there is not reliable though. It's better to wait for document ready (I'm updating that in a new branch soon). Maybe I can take this out and make it a shippable polyfill-like implementation of the idea in this issue.

@trusktr
Copy link
Contributor

trusktr commented Oct 8, 2017

@tnikolai2

Now in almost all implementations connectedCallback behave as connectedAndParsedChildrensCallback and child nodes are fully accessible.

This isn't true, as I've found out in #668 (see my above comment).

@treshugart Have you encountered any issues as I described in #668? The ordering of connectedCallbacks for children of an element is up-the-tree or down-the-tree depending on situations. Also, when the direction is down-the-tree, I'm not certain that detected elements will be upgraded when I observe them.

@robdodson
Copy link

Currently, the PR for that issue has to completely subvert slotchange and use a MutationObserver because that event doesn't fire when the children are first attached to the host, but only on subsequent updates.

@treshugart I think I've sent this your way before, but for the benefit of others on the thread, the above mentioned behavior is changing so slotchange will always fire. See whatwg/dom#447.

If slotchange fired as described in that thread, would that solve this issue? Your custom element would need to at least have a shadow root with a slot, otherwise I think you'd need to depend on a mutation observer.

@annevk
Copy link
Collaborator

annevk commented Feb 19, 2018

Is this different from #550?

@rniwa
Copy link
Collaborator

rniwa commented Feb 19, 2018

I think this a duplicate of the issue #550.

@rniwa rniwa closed this as completed Feb 19, 2018
@trusktr
Copy link
Contributor

trusktr commented Feb 19, 2018

This issue might be slightly different, in that #550 talks about just when children are added/removed, but this issue talks about other types of mutations as well (like character data change, and changes on child elements besides adding/removing). Maybe both issues can converge somehow.

@franktopel
Copy link

For anyone stumbling across this, help is on the way: https://github.com/WebReflection/html-parsed-element

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

No branches or pull requests

10 participants