Mostly-editorial cleanups to script execution #5154
Conversation
| @@ -59001,18 +58985,16 @@ o............A....e | |||
|
|
|||
|
|
|||
| <dt id="script-processing-style-delayed">If the element does not have a <code | |||
| data-x="attr-script-src">src</code> attribute, and the element has been flagged as | |||
| data-x="attr-script-src">src</code> attribute, and the element is | |||
| <span>"parser-inserted"</span>, and either the parser that created the <code>script</code> is | |||
| an <span>XML parser</span> or it's an <span>HTML parser</span> whose <span>script nesting | |||
There was a problem hiding this comment.
I wonder if we could just check the parser document's type? Is there any esoteric situation in which a XML parser could spit out a HTML document, or vice versa?
There was a problem hiding this comment.
About and either the parser that created the <code>script</code> is an <span>XML parser</span> or it's an <span>HTML parser</span> whose <span>script nesting level</span> is not greater than one?
Random thoughts: I suspect this is because script nesting level is not defined for XML parsers.
Conceptually script nesting level would be always 0 or 1 for XML parsers (due to lack of document.write()), so if we define script nesting level of XML parsers (perhaps merging https://html.spec.whatwg.org/multipage/xhtml.html#scriptTagXML and https://html.spec.whatwg.org/multipage/parsing.html#scriptEndTag), then we can remove this parser type check (just checking script nesting level <= 1).
This partial merging of script-related XML and HTML parsers might be also beneficial for merging implementation of HTMLParserScriptRunner and XMLParserScriptRunner in Blink. @nyaxt
The downside would be complicating XML parser unnecessarily a little bit.
There was a problem hiding this comment.
Anyway this should be done separately.
domenic
force-pushed
the
domenic/editorial-script-exec-cleanup
branch
2 times, most recently
from
755d74f to
803fddd
Compare
domenic
added
clarification
`script element's node document` is re-evaluated after script evaluation, and thus if a script is moved to another Document during its own evaluation, `currentScript` of the *new* document could be updated. After web-platform-tests#5154, `currentScript` is always set/reset on the same Document, which is expected, and tested so here.
In the spec before whatwg/html#5154, `script element's node document` is re-evaluated after script evaluation, and thus if a script is moved to another Document during its own evaluation, `currentScript` of the *new* document could be updated. After web-platform-tests#5154, `currentScript` is always set/reset on the same Document, which is expected, and tested so here.
One normative fix: by saving the script element's node document before evaluation, this properly changes that document's currentScript back after evaluation. As previously written, a script that moved during evaluation would cause its new node document's currentScript to update. Tests: web-platform-tests/wpt#20775 Editorial cleanups: * Use a named argument, scriptElement * Use the saved document variable throughout * Cleanup source formatting * Remove <!-- SCRIPT EXEC --> comments
Previously this was referenced in an implicit way, as "the parser that created the element". This makes it an explicit associated value, from which "parser-inserted" is derived.
This makes it easier to add to and rearrange them.
Previously this was referred to imprecisely as "the node document of the script element at the time the prepare a script algorithm started".
This adds a pointer to #2137, which remains somewhat contested, to the relevant parts of the prepare and execute algorithms for scripts.
domenic
force-pushed
the
domenic/editorial-script-exec-cleanup
branch
from
803fddd to
0f588ac
Compare
domenic
changed the title
domenic
removed
the
editorial
| resulting from <span data-x="prepare a script">preparing</span> the element. This is set | ||
| asynchronously after the classic script or module graph is fetched. Once it is set, either to a | ||
| <span data-x="concept-script">script</span> in the case of success or to null in the case of | ||
| failure, the fetching algorithms will note that <dfn>the script is ready</dfn>, which can trigger | ||
| other actions. <!-- similar text in various places --> The user agent must <span>delay the load | ||
| event</span> of the element's <span>node document</span> until <span>the script is | ||
| ready</span>.</p> | ||
| event</span> of the element's <span>node document</span> until <span>the script is ready</span>.</p> |
There was a problem hiding this comment.
I think this Document (of which load event should be delayed) is script's preparation-time document,
which is consistent with that parser-blocking script continue blocking script's preparation-time document.
https://html.spec.whatwg.org/multipage/scripting.html#pending-parsing-blocking-script
Not sure about the browsers' current behavior though (already tested somewhere?)
There was a problem hiding this comment.
Hmm. Let's track this as a new issue.
|
Changes looks good to me. |
|
Could you summarize the changes? That would make it easier to review. Do we really need three separate document pointers from |
|
I think the commit messages summarize the changes pretty well; are there any you are concerned with that I could detail further? We do need three separate document pointers, because there are at least three points at which the document is consulted: parsing time, fetching time, and evaluation time. The test cases at web-platform-tests/wpt#20775 illustrate cases when they are different. I believe some of the pointers might be able to be nulled out after evaluation. |
|
Thanks, I guess this is more explicit than the status quo, but I'm still unsure we covered all bases. It would also be good to do memory cleanup if possible, okay as a follow-up. And a follow-up on defining the HTML parser's document? In particular testing when you move nodes around while parsing would be good to cover there. |
In the spec before whatwg/html#5154, `script element's node document` is re-evaluated after script evaluation, and thus if a script is moved to another Document during its own evaluation, `currentScript` of the *new* document could be updated. After #5154, `currentScript` is always set/reset on the same Document, which is expected, and tested so here.
…t is moved during evaluation, a=testonly Automatic update from web-platform-tests Test `document.currentScript` when script is moved during evaluation In the spec before whatwg/html#5154, `script element's node document` is re-evaluated after script evaluation, and thus if a script is moved to another Document during its own evaluation, `currentScript` of the *new* document could be updated. After #5154, `currentScript` is always set/reset on the same Document, which is expected, and tested so here. -- wpt-commits: 4725b2375b96c0d9ee046b68af755611dba81950 wpt-pr: 20775
This pulls out the only-editorial portions of #2673, and also does a bit more cleanup, including adding the explicit slot @annevk requested. I will rebase #2673 on top of this so that we can focus only on normative changes.
Note that the parser <-> Document association is itself not super-rigorous, but I didn't want to shave that yak too, so we'll have to live with that for now :).
/parsing.html ( diff )
/scripting.html ( diff )
/webappapis.html ( diff )
/xhtml.html ( diff )