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

Hook for SVG and HTML script element insertion #575

Closed
annevk opened this issue Feb 19, 2018 · 11 comments
Closed

Hook for SVG and HTML script element insertion #575

annevk opened this issue Feb 19, 2018 · 11 comments
Labels
needs tests Moving the issue forward requires someone to write tests web reality

Comments

@annevk
Copy link
Member

annevk commented Feb 19, 2018

Given http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4098 we need to do something special for script elements, in particular when inserted from a DocumentFragment node. It's my understanding this only affects script elements, but I haven't verified.

whatwg/html#1127 has an idea as to how to address this.

@annevk annevk added web reality needs tests Moving the issue forward requires someone to write tests labels Feb 19, 2018
@annevk
Copy link
Member Author

annevk commented Feb 19, 2018

Maybe the way to go here is to always notify post-DocumentFragment node insert for everyone. That seems to be what https://searchfox.org/mozilla-central/source/dom/base/nsINode.cpp#2404 is doing anyway.

@annevk
Copy link
Member Author

annevk commented Feb 19, 2018

Oh joy, if I change things to

<!DOCTYPE html>
<script>
 var s1 = document.createElement("script"); s1.textContent = "w(1 + ' ' + document.head.childElementCount); document.head.removeChild(s2)"
 var s2 = document.createElement("script"); s2.textContent = "w(2 + ' ' + document.head.childElementCount + ' ' + s2.parentNode)"
 var df = document.createDocumentFragment(); df.appendChild(s1); df.appendChild(s2)
 document.head.appendChild(df)
</script>

s2 will not execute in Chrome and Edge, but does execute in Firefox. Safari will throw an error for s1 because in Safari s2 is not in the tree yet when s1 executes.

So maybe another thing that's needed here is that script elements without parent do not execute?

@annevk
Copy link
Member Author

annevk commented Feb 20, 2018

Per https://html.spec.whatwg.org/#prepare-a-script browsers need to do a "connected" check, which means Firefox has a bug. (I want to check though if that check is correct or if it needs to be browsing-context connected.)

Another concern here is how this slightly post-insert notification interacts with slot changes. Basically, at what point do the shadow tree changes happen relative to notifying external consumers (which can execute script)?

@annevk
Copy link
Member Author

annevk commented Feb 20, 2018

With the following example I still get a slotchange event. I think that means that kind of tracking should happen before script execution, similar to queuing the mutation record.

<div id=host></div>
<script>
const host = document.getElementById("host"),
      shadow = host.attachShadow({ mode: "closed" }),
      slot = shadow.appendChild(document.createElement("slot")),
      df = document.createDocumentFragment(),
      s1 = df.appendChild(document.createElement("script")),
      s2 = df.appendChild(document.createElement("script"));

slot.addEventListener("slotchange", e => console.log(e));
s1.textContent = "df.appendChild(s1); df.appendChild(s2);";
s2.textContent = "console.log('hmm');";
host.appendChild(df);
</script>

@annevk
Copy link
Member Author

annevk commented Feb 20, 2018

I also tested custom elements:

<script>
customElements.define("ce-1", class CE1 extends HTMLElement {
  constructor() {
    super();
  }
  connectedCallback() {
    console.log("ce-1 connected");
  }
});

const df = document.createDocumentFragment(),
      s = df.appendChild(document.createElement("script")),
      ce1 = df.appendChild(document.createElement("ce-1"));
s.textContent = "df.appendChild(ce1);";
document.head.appendChild(df);
</script>

This logs "ce-1 connected" in Chrome and Firefox. It doesn't log anything in Safari. I'm inclined to side with Chrome and Firefox here.

@autisticgeniushk
Copy link

autisticgeniushk commented Apr 9, 2018

<div id=host></div>
<script>
const host = document.getElementById("host"),
      shadow = host.attachShadow({ mode: "closed" }),
      slot = shadow.appendChild(document.createElement("slot")),
      df = document.createDocumentFragment(),
      s1 = df.appendChild(document.createElement("script")),
      s2 = df.appendChild(document.createElement("script"));

slot.addEventListener("slotchange", e => console.log(e));
s1.textContent = "df.appendChild(s1); df.appendChild(s2);";
s2.textContent = "console.log('hmm');";
host.appendChild(df);
</script>

<script>
customElements.define("ce-1", class CE1 extends HTMLElement {
  constructor() {
    super();
  }
  connectedCallback() {
    console.log("ce-1 connected");
  }
});

const df = document.createDocumentFragment(),
      s = df.appendChild(document.createElement("script")),
      ce1 = df.appendChild(document.createElement("ce-1"));
s.textContent = "df.appendChild(ce1);";
document.head.appendChild(df);
</script>

@nox
Copy link
Member

nox commented Feb 1, 2019

Another snippet that exposes differences between Firefox and Safari:

<body>
<select id="select"><option selected>1</option></select>
<script>
var select = document.getElementById("select");

var script = document.createElement("script");
script.textContent = "console.log(select.selectedIndex);";

var option = document.createElement("option");
option.textContent = "2";
option.selected = true;

select.append(script, option);
</script>

Changing selectedness of options is defined as part of inserting steps, AFAIK. This prints 0 in Safari and 1 in Firefox.

@nox
Copy link
Member

nox commented Feb 4, 2019

Oh joy, if I change things to

<!DOCTYPE html>
<script>
 var s1 = document.createElement("script"); s1.textContent = "w(1 + ' ' + document.head.childElementCount); document.head.removeChild(s2)"
 var s2 = document.createElement("script"); s2.textContent = "w(2 + ' ' + document.head.childElementCount + ' ' + s2.parentNode)"
 var df = document.createDocumentFragment(); df.appendChild(s1); df.appendChild(s2)
 document.head.appendChild(df)
</script>

s2 will not execute in Chrome and Edge, but does execute in Firefox. Safari will throw an error for s1 because in Safari s2 is not in the tree yet when s1 executes.

So maybe another thing that's needed here is that script elements without parent do not execute?

Safari follows the current spec entirely for this one snippet, here is what the Live DOM Viewer says:

log: 1 1
error: NotFoundError: The object can not be found here. on line 1
log: 2 2 [object HTMLHeadElement]

Which means it first inserts s1 and runs it, which raises a NotFoundError because s2 is not a child of head, then it inserts s2 and runs it.

I'm not really sure what to do with this information, in which way do we want the spec to go? Do we want to specify less differences between inserting a DocumentFragment and a plain old node?

@nox
Copy link
Member

nox commented Feb 4, 2019

This logs "ce-1 connected" in Chrome and Firefox. It doesn't log anything in Safari. I'm inclined to side with Chrome and Firefox here.

I guess that answers my question: we want children being inserted as part of a DocumentFragment node to behave more as if they were inserted as part of a plain old node.

annevk pushed a commit that referenced this issue Jun 5, 2024
For any given insert operation, these steps run for each inserted node synchronously after all node insertions are complete. This closes #732.

The goal here is to separate the following:

1. Script-observable but not-script-executing insertion side effects
    - This includes synchronously applying styles to the document, etc...
2. Script-executing insertion side effects
    - This includes creating an iframe's document and synchronously firing its load event

For any given call to insert, the above model allows us to keep all of (1) running synchronously after each node's insertion (as part of its insertion steps), while pushing all script-executing (or DOM tree-modifying or frame tree-modifying etc.) side effects to the new set of post-connection steps, which run synchronously during insertion, but _after all_ nodes finish their insertion.

This essentially makes insertions "atomic" from the perspective of script, since script will not run until a given batch of DOM insertions are complete. This two-stage approach aligns the spec with a model most similar to Blink & Gecko's implementation, and fixes #808. This PR also helps out with whatwg/html#1127 and #575 (per #732 (comment)).

To accomplish, this we audited all insertion side effects on the web platform in https://docs.google.com/document/d/1Fu_pgSBziVIBG4MLjorpfkLTpPD6-XI3dTVrx4CZoqY/edit#heading=h.q06t2gg4vpw, and catalogued whether they have script-observable side-effects, whether they invoke script, whether we have tests for them, and how each implementation handles them. This gave us a list of present "insertion steps" that should move to the "post-connection steps", because they invoke script and therefore prevent insertions from being "atomic". This PR is powerless without counterpart changes to HTML, which will actually _use_ the post-connection steps for all current insertion steps that invoke script or modify the frame tree. The follow-up HTML work is tracked here:

- whatwg/html#10188
- whatwg/html#10241
vinhill pushed a commit to vinhill/dom that referenced this issue Jun 20, 2024
For any given insert operation, these steps run for each inserted node synchronously after all node insertions are complete. This closes whatwg#732.

The goal here is to separate the following:

1. Script-observable but not-script-executing insertion side effects
    - This includes synchronously applying styles to the document, etc...
2. Script-executing insertion side effects
    - This includes creating an iframe's document and synchronously firing its load event

For any given call to insert, the above model allows us to keep all of (1) running synchronously after each node's insertion (as part of its insertion steps), while pushing all script-executing (or DOM tree-modifying or frame tree-modifying etc.) side effects to the new set of post-connection steps, which run synchronously during insertion, but _after all_ nodes finish their insertion.

This essentially makes insertions "atomic" from the perspective of script, since script will not run until a given batch of DOM insertions are complete. This two-stage approach aligns the spec with a model most similar to Blink & Gecko's implementation, and fixes whatwg#808. This PR also helps out with whatwg/html#1127 and whatwg#575 (per whatwg#732 (comment)).

To accomplish, this we audited all insertion side effects on the web platform in https://docs.google.com/document/d/1Fu_pgSBziVIBG4MLjorpfkLTpPD6-XI3dTVrx4CZoqY/edit#heading=h.q06t2gg4vpw, and catalogued whether they have script-observable side-effects, whether they invoke script, whether we have tests for them, and how each implementation handles them. This gave us a list of present "insertion steps" that should move to the "post-connection steps", because they invoke script and therefore prevent insertions from being "atomic". This PR is powerless without counterpart changes to HTML, which will actually _use_ the post-connection steps for all current insertion steps that invoke script or modify the frame tree. The follow-up HTML work is tracked here:

- whatwg/html#10188
- whatwg/html#10241
@domfarolino
Copy link
Member

@annevk I get the sense that we can close this issue now that #1261 and whatwg/html#10188 have landed. Do you agree?

@annevk
Copy link
Member Author

annevk commented Sep 4, 2024

Yeah, nice!

@annevk annevk closed this as completed Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs tests Moving the issue forward requires someone to write tests web reality
Development

Successfully merging a pull request may close this issue.

5 participants
@nox @annevk @domfarolino @autisticgeniushk and others