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

Run insertion steps after DocumentFragment insertion completes #576

Closed
wants to merge 1 commit into from

Conversation

annevk
Copy link
Member

@annevk annevk commented Feb 20, 2018

Fixes whatwg/html#1127 and fixes #575.


Preview | Diff

@annevk annevk added the needs tests Moving the issue forward requires someone to write tests label Feb 20, 2018
@annevk annevk requested a review from nox February 21, 2018 10:11
@annevk
Copy link
Member Author

annevk commented Feb 21, 2018

@smaug---- would appreciate review from you as well (note that you can click on Diff in OP to see the changes rendered in HTML).

@annevk
Copy link
Member Author

annevk commented Feb 21, 2018

@cdumez you might want to review this as well since WebKit has slightly different behavior here. In particular see #575 for various demos I still need to turn into tests.

@smaug----
Copy link
Collaborator

Looks reasonable to me.
Would be good to have a test for the case where there are two script elements in document fragment, and when the first one runs, it removes the second one from document. Per spec the second one should not run, but don't know what browsers do.

@annevk
Copy link
Member Author

annevk commented Feb 21, 2018

@smaug---- see #575 which tests various scenarios along those lines. Unsurprisingly, browsers differ.

@bzbarsky
Copy link

This seems like a pretty big behavior change, if all we want to fix is "don't run scripts before the fragment insertion is done". That is, we would need to audit and test all the things in the spec that do insertion steps to make sure that this is OK, and browsers would need to add a bunch of complexity and runtime overhead to various nodes to make this work.

I'm not a huge fan of this last; the runtime overhead of insertion is already bad enough. We do need some sort of delaying mechanism like this specifically for script execution (though possibly not all of "prepare a script"; Gecko does most of "prepare a script" immediately on insertion but defers the "immediately execute the script block" to a safe point, for example), but not necessarily across the board.

@annevk
Copy link
Member Author

annevk commented Feb 26, 2018

Hmm yeah, you are correct. When I was reading the Firefox code I was led to believe this is in fact the model, but if we inserted a DocumentFragment object consisting of a script element followed by an iframe element this PR would break it as the iframe hasn't had a chance to create a browsing context yet.

So yeah, we need something more akin to that "script queue" concept and define when to empty it.

cc @domenic

@annevk
Copy link
Member Author

annevk commented Feb 26, 2018

Other things to test:

  • style.sheet and script

Bonus:

  • Blur in a removed subtree (not entirely related to this, but in Fx it uses the same script runner thingy)

@annevk
Copy link
Member Author

annevk commented Feb 26, 2018

Except in Edge (and maybe Safari given how it executes scripts while inserting them rather than after they are all inserted), style.sheet initialization is queued similar to scripts. I.e., the following logs null in the Live DOM Viewer:

<script>
 var s1 = document.createElement("style"); s1.textContent = "body { background:lime }";
 var s2 = document.createElement("script"); s2.textContent = "w(s1.sheet)";
 var df = document.createDocumentFragment(); df.appendChild(s2); df.appendChild(s1);
 document.head.appendChild(df);
</script>

@annevk annevk removed the request for review from nox March 29, 2018 11:21
@annevk annevk added do not merge yet Pull request must not be merged per rationale in comment and removed do not merge labels May 28, 2018
@nox
Copy link
Member

nox commented Feb 4, 2019

Except in Edge (and maybe Safari given how it executes scripts while inserting them rather than after they are all inserted)

I confirm that Safari also logs null.

@nox
Copy link
Member

nox commented Feb 12, 2019

Except in Edge (and maybe Safari given how it executes scripts while inserting them rather than after they are all inserted), style.sheet initialization is queued similar to scripts.

That's wrong, turns out Chrome already created the stylesheet by the time the script is executed, and the only reason Safari logs null is because the style element isn't even yet in the document tree because it inserts the fragment's children one by one. If you change the sample to use a div instead, Safari logs an existing style sheet.

<script>
 var s1 = document.createElement("style"); s1.textContent = "body { background:lime }";
 var s2 = document.createElement("script"); s2.textContent = "w(s1.sheet)";
 var df = document.createElement("div"); df.appendChild(s2); df.appendChild(s1);
 document.head.appendChild(df);
</script>

@annevk
Copy link
Member Author

annevk commented Feb 26, 2019

Superseded by #732.

@annevk annevk closed this Feb 26, 2019
@annevk annevk deleted the annevk/run-scripts-post-insert branch February 26, 2019 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge yet Pull request must not be merged per rationale in comment needs tests Moving the issue forward requires someone to write tests
4 participants