Test whether parse document check is done in #prepare-a-script

[hiroshige-g]

This PR adds empty-src tests, which moves <script src=""> elements
between Documents before #prepare-a-script.
If the parse document check is done in #prepara-a-script,

<script>'s error event is not fired, and thus we can test whether the parse document check is done.

[domenic]

Nice stuff. But, can you help me understand it a bit better? Probably we should expand the comment in

wpt/html/semantics/scripting-1/the-script-element/moving-between-documents/resources/moving-between-documents-helper.js

Lines 36 to 81 in 959a0d0

	// For a script, there are four associated Documents that can
	// potentially different:
	//
	// [1] script's parser document
	// https://html.spec.whatwg.org/C/#parser-document
	//
	// [2] script's preparation-time document
	// https://html.spec.whatwg.org/C/#preparation-time-document
	// == script's node document at the beginning of #prepare-a-script
	//
	// [3] script's node document at the beginning of
	// #execute-the-script-block
	//
	// This helper is for tests where [1]/[2]/[3] are different.
	// In the spec, scripts are not executed only if [1]/[2]/[3] are all the same
	// (or [1] is null and [2]==[3]).
	//
	// A check for [1]==[2] is in #prepare-a-script and
	// a check for [1]==[3] is in #execute-the-script-block,
	// but these are under debate: https://github.com/whatwg/html/issues/2137
	//
	// A check for [2]==[3] is in #execute-the-script-block, which is added by
	// https://github.com/whatwg/html/pull/2673
	// timing:
	// "before-prepare":
	// A <script> is moved during parsing before #prepare-a-script.
	// [1] != [2] == [3]
	//
	// "after-prepare":
	// A <script> is moved after parsing/#prepare-a-script but
	// before #execute-the-script-block.
	// [1] == [2] != [3]
	//
	// To move such scripts, #has-a-style-sheet-that-is-blocking-scripts
	// is utilized to block inline scripts after #prepare-a-script.
	// Note: this is a corner case in the spec which might be removed
	// from the spec in the future, e.g.
	// https://github.com/whatwg/html/issues/1349
	// https://github.com/chrishtr/rendering/blob/master/stylesheet-loading-proposal.md
	//
	// "parsing but moved back"
	// A <script> is moved before #prepare-a-script, but moved back again
	// to the original Document after #prepare-a-script.
	// [1] == [3] != [2]

.

In particular, that comment makes it seem like every combination is captured currently. So what do these tests add?

[domfarolino]

In particular, that comment makes it seem like every combination is captured currently. So what do these tests add?

We have tests that assert if a document is moved "before-prepare", then it fails to run. However, imagine an implementation that:

• Does not implement the #prepare-a-script step 12 check for parser document == node document
• But does implement step 2 and 3 of #execute-a-script-block

This implementation will accidentally pass all (or some, I haven't checked) of the "before-prepare" tests because the script won't run, but it's because of a different check than we're testing. So in order to see if the algorithm progresses past #prepare-a-script step 2 inappropriately, the tests added here exercise this line.

[domfarolino]

Are you just removing this because it's already guarded against in generate.py?

[domfarolino]

I randomly came across whatwg/html#2137 the other day, from some discussion with @hiroshige-g I was pointed to this PR. I saw it was sitting for a bit so I decided to take a crack at some of the remaining bits.

The changes I've made:

• Addressed @domenic's review
• Added documentation
• Fixed some documentation (would like Hiroshige to confirm all comments are correct)
• Re-ordered result and inlineOrExternal in generate.py, because I think this is much more clear (i.e., inlineOrExternal simply creates a script, and result potentially modifies it)
• Removed window.scriptErrorEventFired, since it didn't ever appear to be used?
• I found it a little confusing that we exposed scriptOnError in the JS helper as well as tScriptErrorEvent, but only used the former
  • So I stopped exposing the latter
  • I also introduced scriptOnLoad similar to scriptOnError, and exposed it, instead of tScriptLoadEvent. This isn't strictly necessary, but I think the uniformity makes it a lot clearer
• Added a TODO to remove the "parsing but moved back" tests, which I think we can remove. See my proposal

[domenic]

The changes from 5/6 to 4/6 passing in Blink/WebKit make me a bit worried that the refactoring broke something. But, maybe it just made things better? Gecko still passes everything, after all. Have you looked in to why that change happened?

[domenic]

It sounds like, if the Chromium/spec change lands, we can remove "but these are under debate" (or change it into "these used to be under debate; see link for background"). We can do that in either this PR or in a followup, depending on how the timing works out.

[domfarolino]

Have you looked in to why that change happened?

Thanks for the heads up! Just did. So basically before my changed, Chrome / Safari failed tests that executed script inappropriately. We also have a test that should fail whenever the onload handler for is invoked, because it should never be invoked. Before my change, the onload handler looked like this. So when the onload handler is invoked, it just an unreached_func, but does not invoke it (that was an error predating this PR).

My refactoring creates the variable scriptOnLoad, which is meant to be invoked in the onload handler for any tests that inappropriately load and execute. However, I forgot to actually use that variable in the onload handlers, so the extra test you saw failing, was failing like it should, but for the wrong reason (ReferenceError trying to reference tScriptLoadEvent which I deleted). My latest commit actually uses scriptOnLoad correctly in the onload handler, so the extra test is failing as it should, but now for the right reason, thanks to your catch.

I've also confirmed that with my Chromium fix, Chromium still passes the tests.

[domenic]

Awesome! This LGTM; will let you decide whether to wait for @hiroshige-g's review or not.