Skip to content

Correct changing readyState and script running order#1920

Closed
sttk wants to merge 4 commits intojsdom:mainfrom
sttk:ready_state
Closed

Correct changing readyState and script running order#1920
sttk wants to merge 4 commits intojsdom:mainfrom
sttk:ready_state

Conversation

@sttk
Copy link

@sttk sttk commented Jul 20, 2017

JSDOM is different from Web browsers about changing document.readyState and the order of running scripts.

This pr is for modifying this issue.

A sample HTML:

<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8"/>
<title>script test</title>
<script>
console.log("script-0 (readyState = " + document.readyState + ")");
window.addEventListener("load", () => {
  console.log("window#load-0 (readyState = " + document.readyState + ")");
});
document.addEventListener("DOMContentLoaded", () => {
  console.log("document#DOMContentLoaded (readyState = " + document.readyState + ")");
});
function loaded() {
  console.log("body#onload (readyState = " + document.readyState + ")");
}
</script>
<body onload="loaded()">
<script>
console.log("script-1 (readyState = " + document.readyState + ")");
window.addEventListener("load", () => {
  console.log("window#load-1 (readyState = " + document.readyState + ")");
});
</script>
</body>
<script>
console.log("script-2 (readyState = " + document.readyState + ")");
window.addEventListener("load", () => {
  console.log("window#load-2 (readyState = " + document.readyState + ")");
});
</script>
</html>

The result by Chrome, Firefox, Edge, IE is as follows:

script-0 (readyState = loading)
script-1 (readyState = loading)
script-2 (readyState = loading)
document#DOMContentLoaded (readyState = interactive)
window#load-0 (readyState = complete)
body#onload (readyState = complete)
window#load-1 (readyState = complete)
window#load-2 (readyState = complete)

The result by JSDOM before modified is as follows:

script-0 (readyState = loading)
script-1 (readyState = loading)
script-2 (readyState = loading)
document#DOMContentLoaded (readyState = complete)
body#onload (readyState = complete)
window#load-0 (readyState = complete)
window#load-1 (readyState = complete)
window#load-2 (readyState = complete)

@domenic
Copy link
Member

domenic commented Jul 20, 2017

Thanks! However, this does not follow the guidelines in CONTRIBUTING.md for introducing new tests.

Also, I am not happy with the code structuring here which modifies the global event handlers implementation to add a special case. You should be able to make it work without that.

@domenic
Copy link
Member

domenic commented Jul 22, 2017

Sorry, maybe I wasn't clear. This should be tested using web-platform-tests (which we can then run against browsers), not mocha tests. And this should not impact the general event handler implementation (onxxx) at all; it should instead use the same eventing mechanism as anything else. There is nothing in the spec that corresponds to the special-case algorithm you are introducing for onload, so we shouldn't put anything in jsdom.

In general, please try to implement the spec.

@sttk
Copy link
Author

sttk commented Aug 9, 2017

This issue which should be solved by this pr consists of five sub issues:

  1. In setChild function of htmltodom.js, loading script files or evaluating inline scripts of all script elements, which are in _attach methods, are executed after parsing all HTML elements with their attributes, including event handlers.

    Therefore, onload of body is evaluated before all script element contents.

  2. In _attach method of HTMLScriptElement-impl.js, evaluating inline script is executed asynchronously.

    It should be executed immediately after parsing script element.

  3. In _attach method of HTMLScriptElement-impl.js, loading external script file is executed asynchronously, like specified defer attribute.

    It should be executed immediately after parsing script element if neither async nor defer are specified.

  4. In vm-shim.js, vm.runInContent does not deal with variables and functions declared at top level as globals, for example:

    x = 2;  // global
var y = 2; // not global
function z() {} // not global

    Therefore, even if html contains <body onload="z()">..., the function z is not executed because z is not found.

    This causes on karma.

  5. DOMContentLoaded event is occured after loading resources, such as script and css, and the value of document.readyState is changed to "completed".

    That event should be occured after parsing html and before loading resources. In addition, document.readyState should be changed to "interactive" before that event is occured.

Refs.

@sttk
Copy link
Author

sttk commented Sep 8, 2017 •
edited
Loading

I've modified the codes about the above 1,2,3,5 for this issue with new ResourceQueue.
I defered the 4 because it is the issue of acorn-globals.

--
EDIT: RequestQueue -> ResourceQueue

@domenic domenic mentioned this pull request Jan 16, 2018
Merged
domenic added a commit that referenced this pull request Jan 22, 2018
@domenic
Overhaul HTML parsing infrastructure
8c84ebf 
This changes the HTML parsing infrastructure, in particular how we interface with parse5. The original motivation for this change was increased performance; the new benchmark, which appends 65535 <tr>s, gets a speedup of ~3x from these changes.

Other user-visible changes to script evaluation might be present. In particular, the tests revealed that one document.write-related test that used to somehow pass now fails, while another one succeeds (at least on Node 8). In most cases, however, behavior should be the same.

While here, we also changed style sheets to reevaluate their rules and update styleEl.sheet or linkEl.sheet appropriately when their child text content changed.

Behind the scenes details:

- We now use a parse5 tree adapter directly for parsing, instead of using the htmlparser2 adapter layer (which had an O(n) insertion cost for new siblings)
- SAX XML parsing code has been simplified by no longer being shared with parse5/htmlparser2 parsing code.
- Nodes no longer have a reference to the "core" god-object. This was only used in a couple places, and was error prone because this reference was not available in cases such as document nodes created via the Document constructor. This removes a lot of code that threaded the object throughout everything.
- We continued to use hacky workarounds for script evaluation, during parsing and elsewhere. Perhaps one day, inspired by #1316 and #1920, we can fix these.
@domenic domenic mentioned this pull request Nov 1, 2018
Open
@sttk sttk closed this by deleting the head repository Sep 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sttk @domenic