Fix <style> and <script> parsing to wait for their close tag

lib/jsdom/browser/htmltodom.js

@@ -6,6 +6,26 @@ const attributes = require("../living/attributes");
 const DocumentType = require("../living/generated/DocumentType");
 const JSDOMParse5Adapter = require("./parse5-adapter-parsing");
 
+// Horrible monkey-patch to implement https://github.com/inikulin/parse5/issues/237
+const OpenElementStack = require("parse5/lib/parser/open_element_stack");
+const originalPop = OpenElementStack.prototype.pop;
+OpenElementStack.prototype.pop = function (...args) {
+  const before = this.items[this.stackTop];
+  originalPop.apply(this, args);
+  if (before._poppedOffStackOfOpenElements) {
+    before._poppedOffStackOfOpenElements();
+  }
+};
+
+const originalPush = OpenElementStack.prototype.push;
+OpenElementStack.prototype.push = function (...args) {
+  originalPush.apply(this, args);
+  const after = this.items[this.stackTop];
+  if (after._pushedOnStackOfOpenElements) {
+    after._pushedOnStackOfOpenElements();
+  }
+};
+
 module.exports = class HTMLToDOM {
   constructor(parsingMode) {
     this.parser = parsingMode === "xml" ? sax : parse5;
@@ -43,7 +63,6 @@ module.exports = class HTMLToDOM {
       this.parser.parse(html, options);
     }
 
-    adapter.finalize();
     return contextNode;
   }
 

lib/jsdom/browser/parse5-adapter-parsing.js

@@ -10,24 +10,6 @@ const serializationAdapter = require("./parse5-adapter-serialization");
 module.exports = class JSDOMParse5Adapter {
   constructor(documentImpl) {
     this._documentImpl = documentImpl;
-
-    // When text gets inserted into a <script> element, we track that element here. Then, when any other mutation
-    // occurs, we know the <script> element's close tag has been encountered, so we should run the script first.
-    // This is a hack around not doing proper per-spec script evaluation semantics; we should eventually be able to
-    // clear it up.
-    this._lastScriptElement = null;
-  }
-
-  finalize() {
-    this._potentiallyRunLastScriptElement();
-  }
-
-  _potentiallyRunLastScriptElement() {
-    const element = this._lastScriptElement;
-    if (element) {
-      this._lastScriptElement = null;
-      element._eval();
-    }
   }
 
   createDocument() {
@@ -43,36 +25,30 @@ module.exports = class JSDOMParse5Adapter {
   }
 
   createElement(localName, namespace, attrs) {
-    this._potentiallyRunLastScriptElement();
-
     const element = this._documentImpl._createElementWithCorrectElementInterface(localName, namespace);
     element._namespaceURI = namespace;
     this.adoptAttributes(element, attrs);
 
+    if ("_parserInserted" in element) {
+      element._parserInserted = true;
+    }
+
     return element;
   }
 
   createCommentNode(data) {
-    this._potentiallyRunLastScriptElement();
-
     return Comment.createImpl([], { data, ownerDocument: this._documentImpl });
   }
 
   appendChild(parentNode, newNode) {
-    this._potentiallyRunLastScriptElement();
-
     parentNode.appendChild(newNode);
   }
 
   insertBefore(parentNode, newNode, referenceNode) {
-    this._potentiallyRunLastScriptElement();
-
     parentNode.insertBefore(newNode, referenceNode);
   }
 
   setTemplateContent(templateElement, contentFragment) {
-    this._potentiallyRunLastScriptElement();
-
     templateElement._templateContents = contentFragment;
   }
 
@@ -98,18 +74,10 @@ module.exports = class JSDOMParse5Adapter {
   }
 
   detachNode(node) {
-    this._potentiallyRunLastScriptElement();
-
     node.remove();
   }
 
   insertText(parentNode, text) {
-    if (parentNode._eval) {
-      this._lastScriptElement = parentNode;
-    } else {
-      this._potentiallyRunLastScriptElement();
-    }
-
     const { lastChild } = parentNode;
     if (lastChild && lastChild.nodeType === nodeTypes.TEXT_NODE) {
       lastChild.data += text;
@@ -121,12 +89,6 @@ module.exports = class JSDOMParse5Adapter {
   }
 
   insertTextBefore(parentNode, text, referenceNode) {
-    if (parentNode._eval) {
-      this._lastScriptElement = parentNode;
-    } else {
-      this._potentiallyRunLastScriptElement();
-    }
-
     const { previousSibling } = referenceNode;
     if (previousSibling && previousSibling.nodeType === nodeTypes.TEXT_NODE) {
       previousSibling.data += text;

lib/jsdom/living/nodes/Document-impl.js

@@ -423,7 +423,11 @@ class DocumentImpl extends NodeImpl {
       while (child) {
         const node = child;
         child = child.nextSibling;
+
+        node._isMovingDueToDocumentWrite = true; // hack for script execution
         parent.insertBefore(node, previous.nextSibling);
+        node._isMovingDueToDocumentWrite = false;
+
         previous = node;
       }
     } else if (this.readyState === "loading") {

lib/jsdom/living/nodes/HTMLScriptElement-impl.js

@@ -7,6 +7,8 @@ const { reflectURLAttribute } = require("../../utils");
 const resourceLoader = require("../../browser/resource-loader");
 const reportException = require("../helpers/runtime-script-errors");
 const { domSymbolTree } = require("../helpers/internal-constants");
+const { asciiLowercase } = require("../helpers/strings");
+const { childTextContent } = require("../helpers/text");
 const nodeTypes = require("../node-type");
 
 const jsMIMETypes = new Set([
@@ -29,9 +31,22 @@ const jsMIMETypes = new Set([
 ]);
 
 class HTMLScriptElementImpl extends HTMLElementImpl {
+  constructor(args, privateData) {
+    super(args, privateData);
+    this._alreadyStarted = false;
+    this._parserInserted = false; // set by the parser
+  }
+
   _attach() {
     super._attach();
-    this._eval();
+
+
+    // In our current terribly-hacky document.write() implementation, we parse in a div them move elements into the main
+    // document. Thus _eval() will bail early when it gets in _poppedOffStackOfOpenElements(), since we're not attached
+    // then. Instead, we'll let it eval here.
+    if (!this._parserInserted || this._isMovingDueToDocumentWrite) {
+      this._eval();
+    }
   }
 
   _attrModified(name, value, oldValue) {
@@ -47,41 +62,65 @@ class HTMLScriptElementImpl extends HTMLElementImpl {
     }
   }
 
-  // Also used during parsing as a hack around proper script evaluation.
-  //
-  // Note: because of how hacky this all is, this is generally called twice during initial parsing cases: 1) When the
-  // element is inserted into the document (when its start tag is encountered), when there's no text content and no
-  // src="" attribute. 2a) Once the src="" attribute is encountered, if applicable. 2b) Once the end tag is encountered
-  // and there is interesting child text content, if applicable. Note that both 2a) and 2b) can be true in strange
-  // cases (see e.g. the html/semantics/scripting-1/the-script-element/execution-timing/115.html WPT).
+  _poppedOffStackOfOpenElements() {
+    // This seems to roughly correspond to
+    // https://html.spec.whatwg.org/multipage/parsing.html#parsing-main-incdata:prepare-a-script, although we certainly
+    // don't implement the full semantics.
+    this._eval();
+  }
+
+  // Vaguely similar to https://html.spec.whatwg.org/multipage/scripting.html#prepare-a-script, but we have a long way
+  // to go before it's aligned.
   _eval() {
-    if (!this._attached || this._startedEval) {
+    if (this._alreadyStarted) {
       return;
     }
 
-    if (this.src) {
-      this._startedEval = true;
+    // TODO: this text check doesn't seem completely the same as the spec, which e.g. will try to execute scripts with
+    // child element nodes. Spec bug? https://github.com/whatwg/html/issues/3419
+    if (!this.hasAttribute("src") && this.text.length === 0) {
+      return;
+    }
+
+    if (!this._attached) {
+      return;
+    }
+
+    const scriptBlocksTypeString = this._getTypeString();
+    const type = getType(scriptBlocksTypeString);
+
+    if (type !== "classic") {
+      // TODO: implement modules, and then change the check to `type === null`.
+      return;
+    }
+
+    this._alreadyStarted = true;
+
+    // Equivalent to the spec's "scripting is disabled" check.
+    if (!this._ownerDocument._defaultView || this._ownerDocument._defaultView._runScripts !== "dangerously") {
+      return;
+    }
+
+    // TODO: implement nomodule here, **but only after we support modules**.
+
+    // At this point we completely depart from the spec.
+
+    if (this.hasAttribute("src")) {
       resourceLoader.load(
         this,
         this.src,
         { defaultEncoding: whatwgEncoding.labelToName(this.getAttribute("charset")) || this._ownerDocument._encoding },
         this._innerEval
       );
-    } else if (this.text.trim().length > 0) {
-      this._startedEval = true;
+    } else {
       resourceLoader.enqueue(this, this._ownerDocument.URL, this._innerEval)(null, this.text);
     }
   }
 
   _innerEval(text, filename) {
-    const typeString = this._getTypeString();
-
-    if (this._ownerDocument._defaultView && this._ownerDocument._defaultView._runScripts === "dangerously" &&
-        jsMIMETypes.has(typeString.toLowerCase())) {
-      this._ownerDocument._writeAfterElement = this;
-      processJavaScript(this, text, filename);
-      delete this._ownerDocument._writeAfterElement;
-    }
+    this._ownerDocument._writeAfterElement = this;
+    processJavaScript(this, text, filename);
+    delete this._ownerDocument._writeAfterElement;
   }
 
   _getTypeString() {
@@ -112,13 +151,7 @@ class HTMLScriptElementImpl extends HTMLElementImpl {
   }
 
   get text() {
-    let text = "";
-    for (const child of domSymbolTree.childrenIterator(this)) {
-      if (child.nodeType === nodeTypes.TEXT_NODE) {
-        text += child.nodeValue;
-      }
-    }
-    return text;
+    return childTextContent(this);
   }
 
   set text(text) {
@@ -163,6 +196,17 @@ function processJavaScript(element, code, filename) {
   }
 }
 
+function getType(typeString) {
+  const lowercased = asciiLowercase(typeString);
+  if (jsMIMETypes.has(lowercased)) {
+    return "classic";
+  }
+  if (lowercased === "module") {
+    return "module";
+  }
+  return null;
+}
+
 module.exports = {
   implementation: HTMLScriptElementImpl
 };

lib/jsdom/living/nodes/HTMLStyleElement-impl.js

@@ -10,21 +10,41 @@ class HTMLStyleElementImpl extends HTMLElementImpl {
     super(args, privateData);
 
     this.sheet = null;
+    this._isOnStackOfOpenElements = false;
+    this._closeTagHasBeenSeen = false;
   }
 
   _attach() {
     super._attach();
-    this._updateAStyleBlock();
+    if (!this._isOnStackOfOpenElements) {
+      this._updateAStyleBlock();
+    }
   }
 
   _detach() {
     super._detach();
-    this._updateAStyleBlock();
+    if (!this._isOnStackOfOpenElements) {
+      this._updateAStyleBlock();
+    }
   }
 
   _childTextContentChangeSteps() {
-    this._updateAStyleBlock();
     super._childTextContentChangeSteps();
+
+    // This guard is not required by the spec, but should be unobservable (since you can't run script during the middle
+    // of parsing a <style> element) and saves a bunch of unnecessary work.
+    if (!this._isOnStackOfOpenElements) {
+      this._updateAStyleBlock();
+    }
+  }
+
+  _poppedOffStackOfOpenElements() {
+    this._isOnStackOfOpenElements = false;
+    this._updateAStyleBlock();
+  }
+
+  _pushedOnStackOfOpenElements() {
+    this._isOnStackOfOpenElements = true;
   }
 
   _updateAStyleBlock() {

package.json

@@ -30,7 +30,7 @@
     "html-encoding-sniffer": "^1.0.2",
     "left-pad": "^1.2.0",
     "nwmatcher": "^1.4.3",
-    "parse5": "^4.0.0",
+    "parse5": "4.0.0",
     "pn": "^1.1.0",
     "request": "^2.83.0",
     "request-promise-native": "^1.0.5",

test/api/jsdom-errors.js

@@ -0,0 +1,36 @@
+"use strict";
+const { assert } = require("chai");
+const { describe, it } = require("mocha-sugar-free");
+
+const { JSDOM, VirtualConsole } = require("../..");
+
+describe("API: virtual console jsdomErrors", () => {
+  // Note that web-platform-tests do not log CSS parsing errors, so this has to be an API test.
+  it("should not omit invalid stylesheet errors due to spaces (GH-2123)", () => {
+    const virtualConsole = new VirtualConsole();
+
+    const errors = [];
+    virtualConsole.on("jsdomError", e => {
+      errors.push(e);
+    });
+
+    // eslint-disable-next-line no-new
+    new JSDOM(`
+      <html>
+        <head></head>
+        <body>
+          <style>
+          .cool-class {
+              font-family: "Helvetica Neue", Helvetica, Arial, sans-serif;
+          }
+          </style>
+          <p class="cool-class">
+          Hello!
+          </p>
+        </body>
+      </html>
+      `, { virtualConsole });
+
+    assert.isEmpty(errors);
+  });
+});