[reactive-element] asynchrony between shadowRoot creation and initial update can be problematic

[sorvell]

Which package(s) are affected?

Lit Core (lit / lit-html / lit-element / reactive-element)

Description

ReactveElement calls createRenderRoot synchronously in connectedCallback which creates an empty shadowRoot by default. The initial update typically renders DOM into this shadowRoot is performed asynchronously, by default at microtask timing.

This creates a period of time which is less than a paint by default, in which an element's children will not render since they are inside an empty shadowRoot. In practice this is almost never problematic, but it is the precipitating issue for #4786. A focused element can become unfocused.

In addition, Lit allows updates to be scheduled at user defined timing by implementing scheduleUpdate. If this is done such that a paint can occur between updates, the empty shadowRoot can cause a detectable layout change (shown here with a ResizeObserver).

Reproduction

https://lit.dev/playground/#gist=3ecd7930758e2355dd3089e726e00d6d

Workaround

Forcing the initial update to be synchronous avoids this behavior:

connectedCallback() {
  super.connectedCallback();    
  if (!this.hasUpdated) {
    this.performUpdate();
  }
}

Is this a regression?

No or unsure. This never worked, or I haven't tried before.

Affected versions

Lit 3.x

Browser/OS/Node environment

All

[sorvell]

Note thatcreateRenderRoot is also called in performUpdate to address the case that performUpdate is called before the element is connected.

Given this, this issue could be addressed simply by removing the call in connectedCallback in connectedCallback. However, there is a guarantee that the renderRoot exists before calling hostConnected on controllers.

Alternatively, the initial update could be made synchronous. We've previously considered this but decided against it to avoid introducing a special timing behavior for initial rendering.

[justinfagnani]

Alternatively, the initial update could be made synchronous. We've previously considered this but decided against it to avoid introducing a special timing behavior for initial rendering.

We don't do this because of the cascading initialization problem. If a child sync renders, then the parent sets properties, the child will have to render again. Then the grandparent sets properties and the parent and child have to render again...

[justinfagnani]

I think we need something like WICG/webcomponents#1055 in order to get interoperable sync rendering without the cascade problem.

[sorvell]

We don't do this because of the cascading initialization problem. If a child sync renders, then the parent sets properties, the child will have to render again.

The proposal was for sync initial update and unless I'm mistaken I think we could safely do this without introducing a cascade because initial template updating happens before the node is appended. I definitely think this needs careful consideration, but it does seem like something we could safely consider.

[justinfagnani]

If the child also sync renders, or if the parent async render, you get the cascade problem, right?

[justinfagnani]

I opened an issue on the scheduling API for near-sync (nanotask, CE react) tasks. I think ideally we have a way to move to sync, batched rendering across the board.

[sorvell]

If the child also sync renders, or if the parent async render, you get the cascade problem, right?

Lit sets everything before connecting so I don't see how this would be a problem with Lit. For integration with other libraries, it wouldn't be a cascade, but it could cause 1x extra update depending on ordering...

// set then append
const foo = document.createElement('x-foo');
foo.prop1 = 5;
foo.prop2 = 6;
document.body.append(foo);
// updates once: now async, proposed sync.

// v. append *then* set (Lit itself does not do this)
const foo = document.createElement('x-foo');
document.body.append(foo);
foo.prop1 = 5;
foo.prop2 = 6;
// now: updates once async.
// proposed: updates 2x, once sync, once async.

[sorvell]

Just to follow through on the other idea here which is to avoid synchronously creating the renderRoot. As noted previously, this is tricky because of the expectation that renderRoot exists before controllers' hostConnected is called. It might be ok to call hostConnected async after creating renderRoot in the update cycle. This isn't super pretty but would be something like...

connectedCallback() {
    // current
    // Create renderRoot before controllers `hostConnected`
    //(this as Mutable<typeof this, 'renderRoot'>).renderRoot ??=
    //  this.createRenderRoot();
    this.enableUpdating(true);
    // new: gate on renderRoot existing
    if (this.renderRoot !== undefined) {
      this.__controllers?.forEach((c) => c.hostConnected?.());
    }
  }

disconnectedCallback() {
  // new: gate on renderRoot existing
  if (this.renderRoot !== undefined) {
    this.__controllers?.forEach((c) => c.hostDisconnected?.());
  }
}

performUpdate() {
  if (!this.hasUpdated) {
    // current...
    // this.renderRoot ??= this.createRenderRoot();
    // new...
    if (this.renderRoot === undefined) {
      this.renderRoot = this.createRenderRoot();
      if (this.isConnected) {
        this.__controllers?.forEach((c) => c.hostConnected?.());
      }
    }
  }
  // ...
}

[justinfagnani]

I wonder about which is a bigger change: not having a render root when controllers are connected, or delaying controller connectedness?

I could see controller that add event listeners to the host failing to catch events if we delay controller connectedness.

[sorvell]

I could see controller that add event listeners to the host failing to catch events if we delay controller connectedness.

That's a good point. Context does this but the processing is all in controllers so I think the delay would be fine there, but there's definitely a brittleness and it would be a problem if an element fired an event in connectedCallback that a controller on an ancestor expected to hear.

I do think this is an important issue to fix.

We could just avoid creating the renderRoot or try to be fancy and make it a getter (which would at least localize the damage). I don't think that helps much since sometimes people would just try to access shadowRoot instead of renderRoot.

Sync initial render seems cleaner if also slightly risky. Do you acknowledge that any behavior perf change there would only be for non-typical lit rendering use?

[justinfagnani]

What if we queue the fix for this up for Lit 4.0?

[sorvell]

What if we queue the fix for this up for Lit 4.0?

#4786 strikes me as really in need of a fix. The fact that our exact rendering technique has a negative interaction with something as fundamental as focus is a pretty big foot gun. Let's discuss at next eng meeting.

[sorvell]

Note, this behavior occurs when an un-slotted element is focused and subsequently slotted. This means this can occur after initial render as a result of slot assignment.