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

This replaces our previous hacky <script>-only close tag detection, which only worked in non-empty <script> cases, with a more general hacky version that works for all elements, including empty elements, by monkeypatching parse5. (Eventually inikulin/parse5#237 should give us a better solution, but for now we lock our parse5 version and monkeypatch it.)

In particular, this allows us to not parse stylesheets before their close tag is encountered, which fixes #2123 and fixes #2131.

lib/jsdom/browser/htmltodom.js

@@ -6,6 +6,17 @@ 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.call(this, ...args);
+  if (before._poppedOffStackOfOpenElements) {
+    before._poppedOffStackOfOpenElements();
+  }
+};
+
 module.exports = class HTMLToDOM {
   constructor(parsingMode) {
     this.parser = parsingMode === "xml" ? sax : parse5;
@@ -43,7 +54,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/HTMLScriptElement-impl.js

@@ -29,6 +29,13 @@ const jsMIMETypes = new Set([
 ]);
 
 class HTMLScriptElementImpl extends HTMLElementImpl {
+  constructor(args, privateData) {
+    super(args, privateData);
+    this._startedEval = false;
+    this._closeTagHasBeenSeen = false;
+    this._parserInserted = false; // set by the parser
+  }
+
   _attach() {
     super._attach();
     this._eval();
@@ -47,15 +54,13 @@ 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._closeTagHasBeenSeen = true;
+    this._eval();
+  }
+
   _eval() {
-    if (!this._attached || this._startedEval) {
+    if ((!this._closeTagHasBeenSeen && this._parserInserted) || !this._attached || this._startedEval) {
       return;
     }
 

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

@@ -10,6 +10,8 @@ class HTMLStyleElementImpl extends HTMLElementImpl {
     super(args, privateData);
 
     this.sheet = null;
+    this._closeTagHasBeenSeen = false;
+    this._parserInserted = false; // updated by the parser
   }
 
   _attach() {
@@ -27,7 +29,16 @@ class HTMLStyleElementImpl extends HTMLElementImpl {
     super._childTextContentChangeSteps();
   }
 
+  _poppedOffStackOfOpenElements() {
+    this._closeTagHasBeenSeen = true;
+    this._updateAStyleBlock();
+  }
+
   _updateAStyleBlock() {
+    if (this._parserInserted && !this._closeTagHasBeenSeen) {
+      return;
+    }
+
     if (this.sheet) {
       removeStylesheet(this.sheet, this);
     }

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);
+  });
+});

test/index.js

@@ -8,6 +8,7 @@ require("./api/fragment.js");
 require("./api/from-file.js");
 require("./api/from-outside.js");
 require("./api/from-url.js");
+require("./api/jsdom-errors.js");
 require("./api/methods.js");
 require("./api/options.js");
 require("./api/options-run-scripts.js");

test/web-platform-tests/to-upstream/html/semantics/document-metadata/the-style-element/non-parser-style.html

@@ -0,0 +1,27 @@
+<!DOCTYPE html>
+<meta charset="utf-8">
+<title>Non-parser-inserted style elements should still work</title>
+<link rel="author" title="Domenic Denicola" href="mailto:d@domenic.me">
+<link rel="help" href="https://html.spec.whatwg.org/multipage/semantics.html#update-a-style-block">
+<link rel="help" href="https://drafts.csswg.org/cssom/#the-linkstyle-interface">
+
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+
+<p>Test</p>
+
+<script>
+"use strict";
+
+const el = document.createElement("style");
+el.textContent = "p { color: rgb(1, 2, 3); }";
+document.head.appendChild(el);
+
+assert_not_equals(el.sheet, null);
+assert_true(el.sheet instanceof CSSStyleSheet);
+assert_array_equals(document.styleSheets, [el.sheet]);
+
+assert_equals(getComputedStyle(document.querySelector("p")).color, "rgb(1, 2, 3)");
+
+done();
+</script>

test/web-platform-tests/to-upstream/html/semantics/document-metadata/the-style-element/with-spaces.html

@@ -0,0 +1,27 @@
+<!DOCTYPE html>
+<meta charset="utf-8">
+<title>Spaces inside style sheet properties should work fine</title>
+<link rel="author" title="Domenic Denicola" href="mailto:d@domenic.me">
+<!-- Regression test for https://github.com/tmpvar/jsdom/issues/2123 -->
+
+<style>
+.cool-class {
+  font-family: "Helvetica Neue", Helvetica, Arial, sans-serif;
+}
+</style>
+
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+
+<p class="cool-class">
+  Hello!
+</p>
+
+<script>
+"use strict";
+
+const el = document.querySelector(".cool-class");
+assert_equals(window.getComputedStyle(el).fontFamily, `"Helvetica Neue", Helvetica, Arial, sans-serif`);
+
+done();
+</script>