Unexpected upgrade timing when appending an element and script together

[sorvell]

Expected behavior
A fragment is appended to the document containing an upgradeable custom element candidate followed by a script. The script should be able to act on the element in its upgraded state.

Actual behavior
The element is not upgraded when the script runs.

Example
See http://jsbin.com/xesitet/edit?html,console,output.

Discussion
It seems like this behavior is probably per current spec and Safari Tech Preview 17 and Chrome both exhibit the issue. However, the behavior was unexpected when we encountered it. It was discovered when trying to adapt the HTMLImports polyfill to be compatible with Safari Tech Preview 17's implementation of custom elements. The polyfill appends fragments of imported dom that can contain custom elements and scripts together. After puzzling out the behavior for awhile we have a workaround that avoids the issue, but it was certainly very tricky to reason about.

Proposed Fix
Require the custom elements reaction stack be flushed before running script.

[rniwa]

This is because the custom element is enqueued to be upgraded but we end up executing the script in the middle of executing appendChild (we run prepare a script step whenever the script element is connected and a node or document fragment is inserted into the script element) which is earlier than when those reactions are invoked (at the end of appendChild before returning the control back to scripts).

If we're interested in just this particular case resolved, then we can add a special step to invoke custom elements callbacks in the script element's processing model, or check it in to insert.

If we're trying to solve this problem in more broader sense, and invoke enqueued callbacks before executing any script, then that's a lot more profound change.

[rniwa]

@bzbarsky @annevk @domenic @travisleithead : What do you all think about this?

[bzbarsky]

Paging @smaug----

[domenic]

@bzbarsky @annevk @domenic @travisleithead : What do you all think about this?

This is a really interesting issue. I guess my initial thought is that we kind of want to run custom element reactions "as often as possible", right? So maybe it is OK to start running them when we prepare a script. I am a little uneasy though as I wish we had a guiding principle that supported such a change, instead of just "let's sprinkle run-CE-reactions as needed".

I wonder if there are other cases where this kind of thing would come up? At first I thought innerHTML might be one, but no, scripts don't execute as a result of fragment parsing.

If we're trying to solve this problem in more broader sense, and invoke enqueued callbacks before executing any script, then that's a lot more profound change.

I think I agree, but I want to be sure. After all, it would be a very simple and nice principle to say "whenever we run script, we must have already run CE reactions and emptied the CE reactions stack"

Can you think of any other scenarios today where we start running script, and in which the CE reactions stack could be nonempty?

[rniwa]

Can you think of any other scenarios today where we start running script, and in which the CE reactions stack could be nonempty?

Dispatching any event would.

[domenic]

Dispatching any event would.

Can you give a code example of how that would arise? I cannot think of a situation in which events would be dispatched but the CE reactions stack would be non-empty.

I guess maybe mutation events :-/.

[rniwa]

!? beforeunload, blur, keydown, etc...

[domenic]

I really do not think I could write a test case which causes those to trigger when the CE reactions stack is nonempty. Maybe it is just Friday-brain though. If you would be able to do so, I'd be extremely grateful, but otherwise I can try again Monday.

[smaug----]

script element has already special case for microtask handling (during parsing at least), so having similar special case for CE doesn't feel too bad.
(it adds more special casing to the platform, and that makes maintenance harder, but given that we're doing it in certain cases already for microtask, it shouldn't be that bad, I think)

[annevk]

This might not be well-defined currently, but I think the way this works currently is that we first append each element, and then iterate over the inserted elements again to execute any scripts. That is, the script can observe later inserted elements. Emptying the stack before executing the script seems like a reasonable thing to do.

This should be easy to fix as part of fixing whatwg/html#1127.

[rniwa]

FWIW, Apple is mildly in favor of clearing/executing custom element reactions before executing scripts.

[annevk]

@rniwa can you construct an example for #606 (comment)?

Anyway, I tend to agree that if script can execute that emptying the stack is reasonable since you have to deal with side effects at that point anyway.

[domenic]

Note that this would cause things to be a bit weird in the definition of [CEReactions]. Currently the queue pushed in step 1 is the same as the queue popped in step 3. But with this change sometimes the stack would become empty during step 2, so popping in step 3 would not work.

In general, we don't have any precedent for emptying the stack; only for pushing/popping as a pair.

All of the above basically sums up to this being inelegant in spec land, which isn't necessarily a good reason to reject the idea, but it's worth thinking about.

[rniwa]

Yeah, I totally understand the implication of this spec change. I agree it's very messy but the spec complexity isn't a reason to ignore developer ergonomics as you say.

[annevk]

Here's another interesting case (you can use https://software.hixie.ch/utilities/js/live-dom-viewer/ to run it):

<script>
customElements.define("h-i", class HI extends HTMLElement {
  connectedCallback() { w("HI is connected") }
});
df = document.createDocumentFragment();
s1 = df.appendChild(document.createElement("script")); 
s1.textContent = "w('s1 executed');";
df.appendChild(document.createElement("h-i"));
s2 = df.appendChild(document.createElement("script")); 
s2.textContent = "w('s2 executed');";
document.head.appendChild(df);
</script>

Currently the order is s1, s2, HI. I guess with the proposed change it'd be HI, s1, s2? Or if we introduce a "script queue" concept as per whatwg/dom#576 could we reuse that for custom element reactions and make it s1, HI, s2?

And what is preferred for this scenario?

[annevk]

The outcome of discussion at the F2F is that we're not going to change this. The problem is that we cannot make it the same as the parser, which also empties microtasks before executing a script element.

Executing it before any JavaScript would expose extensions the user has installed, which is an non-starter.

We hope that #710 helps solve this to some extent.