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

connectedCallback timing when the document parser creates custom elements #551

Closed
kojiishi opened this issue Aug 23, 2016 · 92 comments
Closed

Comments

@kojiishi
Copy link

From a blink bug, an author reported that connectedCallback is called too early in Blink.

<script>customElements.define('x-x', ...);
<script>customElements.define('y-y', ...);
<x-x>
  <div></div>
  <y-y></y-y>
</x-x>

or a sample code in jsfiddle.

When current Blink impl runs this code, this.children.length is 1 in connectedCallback for x-x. This looks like matching to the spec to me, but I appreciate discussion here to confirm if my understanding is correct, and also to confirm if this is what we should expect.

My reading goes this way:

  1. The parser is in the "in body" insertion mode.
  2. It sees x-x as Any other start tag, it insert an HTML element.
  3. insert a foreign element says to create an element for a token, then insert.
    Note that this "insert" is not a link, but I assume this is insert a node?
  4. In insert a node, step 5.2.1 enqueues connectedCallback.
  5. In enqueue a custom element callback reaction, it adds to the backup element queue.
  6. The parser then reads div. In its create an element for a token, definition is null, so will execute script is false. This div is then inserted.
  7. The parser then reads y-y. In its create an element for a token, definition is non-null, so will execute script is true.
  8. Step 6.2 perform a microtask checkpoint if will execute script is true, so the connectedCallback runs here.

Am I reading the spec correctly? If so, is this what we should expect?

@domenic @dominiccooney @rniwa

@domenic
Copy link
Collaborator

domenic commented Aug 23, 2016

In enqueue a custom element callback reaction, it adds to the backup element queue.

I think you are correct that the spec says this, but this also seems wrong. The backup element queue should be avoidable when we are doing parsing; it should only be used for things like editing. We should be able to just use the element queue created for that element.

That would make connectedCallback be called with zero children, right? That sounds much better.

I am not sure what the best spec fix is for this---it is getting late over here---but maybe it would suffice to just push an element queue in "insert a foreign element", then pop it and run reactions? Better suggestions appreciated.

@kojiishi
Copy link
Author

Yeah, 0 sounds much better than 1, and "insert a foreign element" looks the right place.

I also understand web developers might expect 2 instead of 0, but that's harder, wondering whether the benefit is worth the complexity or not.

@annevk
Copy link
Collaborator

annevk commented Aug 23, 2016

No. We absolutely want zero children in as many places as possible. The whole point is that you should not depend on children, since you cannot depend on them with createElement() either. Web developers should create robust custom element implementations, not half-baked ones.

@kojiishi
Copy link
Author

In constructor, yes, but in connectedCallback, authors can add children before adding the element to a document, no?

@annevk
Copy link
Collaborator

annevk commented Aug 23, 2016

Ah yeah they could. But that should not be required by the custom element.

@ju-lien
Copy link

ju-lien commented Aug 23, 2016

I'am the author of the blink bug,

Ah yeah they could. But that should not be required by the custom element.

Could you explain why ? connectedCallback() is called when the node is inserted in a DOM, so i think it should be aware of its own children ?

If not, how/when a CE can work with its children ?

Also it's very bizare that connectedCallback() doesn't have the same capability when the node is created from the parser or upgraded.

@annevk
Copy link
Collaborator

annevk commented Aug 23, 2016

When the node is created and inserted, it doesn't have any children.

@matthewp
Copy link

So... how do you work with custom elements that have expectations about their children?

@annevk
Copy link
Collaborator

annevk commented Aug 23, 2016

Mutation observers.

@matthewp
Copy link

matthewp commented Aug 23, 2016

So connectedCallback is called with 0 children in the case of upgrades, but if I am to do:

var myEl = document.createElement('my-element');
myEl.appendChild(document.createElement('span'));

var frag = document.createDocumentFragment();
frag.appendChild(myEl);

document.body.appendChild(frag);

Then "my-element"'s connectedCallback is called when the fragment is inserted into the body and it will have 1 children in this case.

So a custom element with expectations on children will need to:

  1. Check if has children in connectedCallback, do stuff
  2. Set up a mutation observer and do stuff when nodes are added (which will happen in the case of upgrades).
  3. Disconnect the mutation observer in disconnectedCallback

No concerns here, just trying to work it out in my head.

@annevk
Copy link
Collaborator

annevk commented Aug 23, 2016

If you set up the mutation observer at construction time, that first appendChild() can trigger do stuff too, but if you only want to observe while connected something like that could work.

@annevk
Copy link
Collaborator

annevk commented Aug 23, 2016

I meant it can cause do stuff to be scheduled. Mutation observers run end-of-task so not immediately.

@rniwa
Copy link
Collaborator

rniwa commented Aug 23, 2016

An alternative is to add childrenChangedCallback: #550

@ju-lien
Copy link

ju-lien commented Aug 23, 2016

Mutation observers.

For now in chrome canary ( i don't know if it matches the spec or not), if i try to get all children with an observer even in constructor(), the observer is fired 2 times as you can see in this jsfiddle.
So if i just want to have a list of initial children, i don't know when i have to disconnect the observer and do the work?

An alternative is to add childrenChangedCallback: #550

Why not if all initial children are returned in one call.

@annevk
Copy link
Collaborator

annevk commented Aug 23, 2016

The whole idea of initial children is flawed. Children are dynamic.

@domenic
Copy link
Collaborator

domenic commented Aug 23, 2016

Indeed. In general the multi-tag designs of HTML (e.g. <select> + <option>, <ul> + <li>, <picture> + <source> + <img>, etc.) are very fragile; the amount of spec machinery involved in making them work correctly is extensive and in some cases still buggy in the current spec. (We don't even have a correct design for <li> yet, this many years later: whatwg/html#1617.) Web devs should be very cautious when trying to set up such designs for their custom elements; you need to be aware of how children change dynamically, and not just use the children as some kind of setup data.

@domenic
Copy link
Collaborator

domenic commented Aug 23, 2016

It's worth noting that in the past we've talked about adding a new callback for this sort of thing, I think someone called it closeTagReachedCallback or something. That would be useful for emulating <script> behavior, I think.

domenic added a commit to whatwg/html that referenced this issue Aug 23, 2016
Fixes WICG/webcomponents#551 by ensuring that
insertions into the DOM trigger connectedCallback immediately, instead
of putting the callback reaction on the the backup element queue and
letting it get triggered at the next microtask checkpoint. This means
connectedCallback will generally be invoked when the element has zero
children, as expected, instead of a random number depending on when the
next custom element is seen.
@rniwa
Copy link
Collaborator

rniwa commented Aug 23, 2016

FWIW, I would object to adding something like closeTagReachedCallback because they're specific to HTML parser behavior. I'd much rather add something like childrenChangedCallback, which can be used either during DOM mutations or during parsers.

@ju-lien
Copy link

ju-lien commented Aug 23, 2016

Thanks domenic for this complete answer.

I thought indeed to this type of case, an element that sort its children or compute a layout.

For example to create an element that compute a sexy layout, its important to access to an initial state of children to avoid re-compute the layout many times(each time a new CE children is parsed) for just the first render...

So i understand we should avoid to use the children as some kind of setup data, but what is the main technical reason to not to do that ?

@domenic
Copy link
Collaborator

domenic commented Aug 23, 2016

For example to create an element that compute a sexy layout, its important to access to an initial state of children to avoid re-compute the layout many times(each time a new CE children is parsed) for just the first render...

What I'd suggest doing is just resorting or re-rendering your children on every requestAnimationFrame. That way you will not only get the post-parsing batching behavior you desire, but you will also synchronize your work with when it's displayed to users.

So i understand we should avoid to use the children as some kind of setup data, but what is the main technical reason to not to do that ?

I thought I explained why that was above, talking about how they are dynamic and can change any time, and this leads to complicated behavior.

@ju-lien
Copy link

ju-lien commented Aug 23, 2016

What I'd suggest doing is just resorting or re-rendering your children on every requestAnimationFrame. That way you will not only get the post-parsing batching behavior you desire, but you will also synchronize your work with when it's displayed to users.

Ok, that's a good idea.
Even it's seems a - little - hacky for a brand new API.

I thought I explained why that was above, talking about how they are dynamic and can change any time, and this leads to complicated behavior.

Ok, thanks, it's bizarre but i trust you :)

@domenic
Copy link
Collaborator

domenic commented Aug 23, 2016

Ok, that's a good idea. Even it's seems a - little - hacky for a brand new API.

Nah. When do you think browsers update rendering for their elements? (The answer is: every animation frame callback.)

@annevk
Copy link
Collaborator

annevk commented Aug 24, 2016

Only if there is something to update though.

annevk pushed a commit to whatwg/html that referenced this issue Aug 24, 2016
Fixes WICG/webcomponents#551 by ensuring that
insertions into the DOM trigger connectedCallback immediately, instead
of putting the callback reaction on the the backup element queue and
letting it get triggered at the next microtask checkpoint. This means
connectedCallback will generally be invoked when the element has zero
children, as expected, instead of a random number depending on when the
next custom element is seen.
@ju-lien
Copy link

ju-lien commented Aug 24, 2016

Sorry to come back but i was checking the SkateJS library and i saw that in the todolist example on the homepage:

...
attached(elem) {
    // Setup the initial list of items from the current children.
    elem.items = elem.children;
  },
...

And attached() seems to rely on connectedCallback().

So i think it show the need to really clarify the situation - or - provide a real solution even requestAnimationFrame() could do the trick.

@domenic
Copy link
Collaborator

domenic commented Aug 24, 2016

Yeah, this might be worth someone doing a blog post on or similar.

@matthewp
Copy link

A library could provide that hook for you, or conversely a small function could give you the same behavior:

connectedCallback() {
  onChildren(this, children => {

  });
}

@WebReflection
Copy link

WebReflection commented Oct 19, 2018

@franktopel things I'd do differently:

  1. use a WeakSet for the already parsed bit
  2. I wouldn't cache the parents. Nodes can be easily moved around, caching parents wouldn't give you much performance boost but it might hide shenanigans
  3. the setup should be instead the connectedCallback of the HTMLBaseElement class, and the class should also have a childrenAvailableCallback no-op (or actually ignore everything on comnnectedCallback if "childrenAvailableCallback" in this is false)
  4. the method still doesn't solve the empty custom elements case, which is not edge at all, so that if the document is loading but there won't ever be children, the mutation observer will leak forever and no childrenAvailableCallback will ever be called
  5. I usually clean up before invoking "the unknown" so the mutation observer disconnect should be the first thing to do, so that if there is an error in the childrenAvailableCallback callback it doesn't backfire forever.

Accordingly, this is how I'd go, or what makes sense to propose as standard.

const HTMLParsedElement = (() => {
  const DCL = 'DOMContentLoaded';
  const init = new WeakSet;
  const isParsed = el => {
    do {
      if (el.nextSibling)
        return true;
    } while (el = el.parentNode);
    return false;
  };
  const cleanUp = (el, observer, onDCL) => {
    observer.disconnect();
    el.ownerDocument.removeEventListener(DCL, onDCL);
    parsedCallback(el);
  };
  const parsedCallback = el => el.parsedCallback();
  return class HTMLParsedElement extends HTMLElement {
    connectedCallback() {
      if ('parsedCallback' in this && !init.has(this)) {
        init.add(this);
        if (document.readyState === 'complete' || isParsed(this))
          // ensure an order via a micro-task so that
          // parsedCallback is always after connectedCallback
          Promise.resolve(this).then(parsedCallback);
        else {
          // the need a DOMContentLoaded case
          const onDCL = () => cleanUp(this, observer, onDCL);
          this.ownerDocument.addEventListener(DCL, onDCL);
          // the early case still good to setup one
          const observer = new MutationObserver(changes => {
            changes.some(record => {
              if (record.addedNodes.length) {
                cleanUp(this, observer, onDCL);
                return true;
              }
            });
          });
          // we are interested in the element nextSibling so
          // lets observe its parent instead of its own nodes
          observer.observe(this.parentNode, {childList: true});
        }
      }
    }
  };
})();

Observing the parentNode still might hide shenanigans but at least is its only direct container and not the whole document so it's IMO most likely a more reliable approach.

Eventually, the observer could be configured as {childList: true, subtree: true} and the if should check if (isParsed(this)) instead of checking record.addedNodes.length.

@WebReflection
Copy link

WebReflection commented Oct 19, 2018

Right ... little variation that uses a WeakMap instead and it's also observing children for all occasions

const HTMLParsedElement = (() => {
  const DCL = 'DOMContentLoaded';
  const init = new WeakMap;
  const isParsed = el => {
    do {
      if (el.nextSibling)
        return true;
    } while (el = el.parentNode);
    return false;
  };
  const cleanUp = (el, observer, ownerDocument, onDCL) => {
    observer.disconnect();
    ownerDocument.removeEventListener(DCL, onDCL);
    init.set(el, true);
    parsedCallback(el);
  };
  const parsedCallback = el => el.parsedCallback();
  return class HTMLParsedElement extends HTMLElement {
    connectedCallback() {
      if ('parsedCallback' in this && !init.has(this)) {
        const self = this;
        const {ownerDocument} = self;
        init.set(self, false);
        if (ownerDocument.readyState === 'complete' || isParsed(self))
          Promise.resolve(self).then(parsedCallback);
        else {
          const onDCL = () => cleanUp(self, observer, ownerDocument, onDCL);
          ownerDocument.addEventListener(DCL, onDCL);
          const observer = new MutationObserver(changes => {
            if (isParsed(self)) {
              cleanUp(self, observer, ownerDocument, onDCL);
              return true;
            }
          });
          observer.observe(self.parentNode, {childList: true, subtree: true});
        }
      }
    }
    get parsed() {
      return init.get(this) === true;
    }
  };
})();

I think I'll publish this one to npm.

@franktopel
Copy link

franktopel commented Oct 19, 2018

@WebReflection

Comments on your comments:

  1. in empty custom elements a childrenAvailableCallback, as the name implies, doesn't make much sense anyway. As far as I can see you can simply extend HTMLElement instead of HTMLBaseElement in that case. At the end of the day, this approach is meant to solve the problem that arises from children being unavailable when connectedCallback triggers.

  2. Then how would you address asynchronous adding of child elements? Probably the right approach here would be to pass a cleanUp callback to childrenAvailableCallback.

What is still open with this solution is to adjust it for use in customized-built-ins that have expectations about their children.

Btw, I was also considering publishing this on npm, but probably it'll gain more traction if you do it.

@franktopel
Copy link

franktopel commented Oct 19, 2018

@WebReflection I saw you're quick: https://github.com/WebReflection/html-parsed-element

I wouldn't reject attribution, neither would @irhadkul I assume :)

@WebReflection
Copy link

well, if any of you want I can include attributions but just to be clear, this is what HyperHTMLElement does since about ever, using a timeout instead of MutationObserver for better compatibility (down to IE9) and without guarding all calls to connected/attributeChanged before the created() is invoked.

The html-parsed-element is an alternative that might become handy for those not interested in HyperHTMLElement.

I will still link to this ticket in there so again, I don't mind adding anyone in here as contributor 👋

@WebReflection
Copy link

WebReflection commented Oct 19, 2018

also @franktopel ...

this approach is meant to solve the problem that arises from children being unavailable when connectedCallback triggers.

My approach solves every issue. When parsedCallback is invoked you will have children in there, if any. If you want to listen to further mutations to children, just add your own Mutation Observer.

Then how would you address asynchronous adding of child elements?

You don't . You disconnect the observer too so that's not your intent and also you want to this.childrenAvailableCallback() once, and once only indeed.

Again, the missing bit that is essential is to know when it's safe to handle children or even inject nodes/html. Once we have that, everything else is trivial.

@franktopel
Copy link

franktopel commented Oct 19, 2018

Attribution is definitely welcome. While none of us has done anything even remotely comparable to your work, we surely contributed to the birth of html-parsed-element with the last 10 days' work.
Regarding your comment for better compatibility (down to IE9) I was surprised to find HyperHTMLElement is compatible with every mobile browser and IE11 or greater on https://github.com/WebReflection/hyperHTML-Element

@WebReflection
Copy link

WebReflection commented Oct 19, 2018

IIR you can test this page and it should work in IE9 too
https://webreflection.github.io/hyperHTML-Element/test/?es5

compatibility is probably for something not fully transpilable but I don't remember what.

Feel free to file a PR for the attribution so I'm sure it's done properly/as you expect.

@franktopel
Copy link

franktopel commented Oct 19, 2018

Btw, how does attributeChangedCallback() behave with respect to children? We have a tabs component that is controlled via a data-active-tab attribute in combination with an attributeChangedCallback case.

@WebReflection
Copy link

@franktopel the attributeChangedCallback is triggered as soon as the beginning of the node is known, AKA the opening tag.

Differently from connectedCallback or whatever children watcher we want, the attributeChangedCallback will never trigger when one attribute is known but another one isn't ,because attributeChangedCallback is consistent in triggering when all attributes on the opening tag are known, instead of randomly in the wild without any signal all nodes are known, which is what my proposal addresses.

@WebReflection
Copy link

P.S. @franktopel attributions are live

@franktopel
Copy link

franktopel commented Oct 20, 2018

@WebReflection So in that case we (our team) have the exact same problem with attributeChangedCallback as well.

the attributeChangedCallback is triggered as soon as the beginning of the node is known, AKA the opening tag. How is that ever useful at all? What would a developer ever want to do at that point in time, without being able to access the element's content?

@WebReflection
Copy link

WebReflection commented Oct 20, 2018

@franktopel it's useful for empty nodes with shadow dom, and not much else indeed.

With parsedCallback you can setup sure that attributes are there as well.

Meanwhile, if you react to attributeChangedCallback, if this.parsed is false you should queue the operations if important for the component state.

const attributeChanged = new WeakMap;
class MyEl extends HTMLParsedElement {
  parsedCallback() {
    // setup the node, you have access to all its content/attributes
    // then ...
    const changes = attributeChanged.get(this);
    if (changes) {
      attributeChanged.delete(this);
      changes.forEach(args => this.attributeChangedCallback(...args));
    }
  }

  attributeChangedCallback(...args) {
    if (!this.parsed) {
      const changes = attributeChanged.get(this) || [];
      if (changes.push(args) === 1)
        attributeChanged.set(this, changes);
      return;
    }
    // the rest of the code
  }

  connectedCallback() {
    // here you can safely add listeners
    // or set own component properties
    this.live = true;
  }

  disconnectedCallback() {
    // here you can safely remove listeners
    this.live = false;
  }
}

@franktopel
Copy link

franktopel commented Oct 20, 2018

Well, we're not using shadow DOM at all. We just need to replicate Swing controls for the web, for a huge migration project. And currently it needs to support IE 11 mainly. And it has to work for the next 10 to 15 years, without huge update efforts, like you'd have with using a framework like Angular or React. That's why the company decided to use native web components.

So you're saying the whole spec has been designed around a very limited use edge case?

@franktopel
Copy link

franktopel commented Oct 20, 2018

Also, we have been using attributeChangedCallback on a tabs component to set the initially active/visible tab, so this must again have been an issue of it working in the upgrade case, but not in the parsing case. What a mess, again!

@franktopel
Copy link

franktopel commented Oct 21, 2018

Wouldn't it be an even better approach to dispatch a ComponentContentLoaded custom event? That would solve the attributeChangedCallback problem as well, and it would make it so outside elements and components can attach a listener to that event (if they rely on it).

We could even have all components register at a central service in their constructor on creation, and have that same service emit a global ComponentsReady event as soon as all registered component instances have emitted their ComponentContentLoaded event.

@WebReflection
Copy link

You can dispatch any event you want from parsedCallback ...having hybrid callbacks and events feels inconsistent with the API , imo

alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
Fixes WICG/webcomponents#551 by ensuring that
insertions into the DOM trigger connectedCallback immediately, instead
of putting the callback reaction on the the backup element queue and
letting it get triggered at the next microtask checkpoint. This means
connectedCallback will generally be invoked when the element has zero
children, as expected, instead of a random number depending on when the
next custom element is seen.
@Offroaders123
Copy link

Offroaders123 commented Apr 9, 2022

#551 (comment)

@franktopel and @WebReflection, thank you again for making a full implementation of this, I think it will help out a ton! I love how Web Components work, and using this in addition with Constructable Stylesheets will make everything so much more seamless, now being able to parse innerHTML consistently.

@mangelozzi
Copy link

Noob suggestion here, why not just link your scripts with deferred or type="module", that way you know the DOM will be parsed and the child nodes present when your connectedCallback runs?

<script type="module" src="/static/..../component.min.js>

@dux
Copy link

dux commented Feb 9, 2023

Noob suggestion here, why not just link your scripts with deferred or type="module", that way you know the DOM will be parsed and the child nodes present when your connectedCallback runs?

That will not work for nodes that are dynamically inserted, after initial setup.

@dux
Copy link

dux commented Feb 9, 2023

Solution for this problem is pretty simple, all one has to do it wrap connectedCallback in window.requestAnimationFrame and you will always have all children available.

I am pretty sure this is faster then any other proposed solution, as setTimeout # 0 and similar.

More in my answer here
https://stackoverflow.com/questions/70949141/web-components-accessing-innerhtml-in-connectedcallback/75402874#75402874

@Sleepful
Copy link

Sleepful commented Mar 8, 2023

@dux not correct apparently, read thread from here: #809 (comment)

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

No branches or pull requests