Rewrite ID caching for correctness and maybe speed

lib/jsdom/living/helpers/by-id-cache.js

@@ -0,0 +1,68 @@
+"use strict";
+const { domSymbolTree } = require("./internal-constants.js");
+const NODE_TYPE = require("../node-type.js");
+
+// For use in implementing getElementById(). Notably it ensures that when you get the result, you will always get the
+// first in tree order, not the most- or least-recently-inserted. Somewhat modeled after
+// https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/tree_ordered_map.h.
+
+module.exports = class ByIdCache {
+  constructor(root) {
+    this._root = root;
+
+    // Keys are IDs (strings).
+    // Values are `{ element, count }` tuples, where `element` can be `null` indicating we need to recompute.
+    // `count` tracks the count of times `add()` was called for this ID. We need to track it because it lets us
+    // determine what to do when calling `delete()`: a count of 0 means we can remove the entry, whereas any other
+    // count means we need to set `element` to `null` for future recomputation.
+    this._map = new Map();
+  }
+
+  add(id, element) {
+    const value = this._map.get(id);
+    if (!value) {
+      this._map.set(id, { element, count: 1 });
+    } else {
+      value.element = null;
+      ++value.count;
+    }
+  }
+
+  delete(id, element) {
+    const value = this._map.get(id);
+    if (!value) {
+      return;
+    }
+
+    if (value.count === 1 && value.element === element) {
+      this._map.delete(id);
+    } else {
+      if (value.element === element) {
+        value.element = null;
+      }
+      --value.count;
+    }
+  }
+
+  get(id) {
+    const value = this._map.get(id);
+    if (!value) {
+      return null;
+    }
+
+    if (value.element) {
+      return value.element;
+    }
+
+    for (const descendant of domSymbolTree.treeIterator(this._root)) {
+      if (descendant.nodeType === NODE_TYPE.ELEMENT_NODE && descendant.getAttributeNS(null, "id") === id) {
+        value.element = descendant;
+        return descendant;
+      }
+    }
+
+    // If we didn't find any elements for this key, we can remove it.
+    this._map.delete(id);
+    return null;
+  }
+};

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

@@ -8,6 +8,7 @@ const NODE_TYPE = require("../node-type");
 const { mixin, memoizeQuery } = require("../../utils");
 const { firstChildWithLocalName, firstChildWithLocalNames, firstDescendantWithLocalName } =
   require("../helpers/traversal");
+const ByIdCache = require("../helpers/by-id-cache");
 const whatwgURL = require("whatwg-url");
 const StyleSheetList = require("../generated/StyleSheetList.js");
 const { domSymbolTree } = require("../helpers/internal-constants");
@@ -138,7 +139,7 @@ class DocumentImpl extends NodeImpl {
 
     this._defaultView = privateData.options.defaultView || null;
     this._global = privateData.options.global;
-    this._ids = Object.create(null);
+    this._byIdCache = new ByIdCache(this);
     this._attached = true;
     this._currentScript = null;
     this._pageShowingFlag = false;
@@ -348,23 +349,9 @@ class DocumentImpl extends NodeImpl {
     this.write(...args, "\n");
   }
 
-  // This is implemented separately for Document (which has a _ids cache) and DocumentFragment (which does not).
+  // This is implemented separately for Document (which has a cache) and DocumentFragment (which does not).
   getElementById(id) {
-    if (!this._ids[id]) {
-      return null;
-    }
-
-    // Let's find the first element with where it's root is the document.
-    const matchElement = this._ids[id].find(candidate => {
-      let root = candidate;
-      while (domSymbolTree.parent(root)) {
-        root = domSymbolTree.parent(root);
-      }
-
-      return root === this;
-    });
-
-    return matchElement || null;
+    return this._byIdCache.get(id);
   }
 
   get referrer() {

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

@@ -20,7 +20,7 @@ class DocumentFragmentImpl extends NodeImpl {
     this.nodeType = NODE_TYPE.DOCUMENT_FRAGMENT_NODE;
   }
 
-  // This is implemented separately for Document (which has a _ids cache) and DocumentFragment (which does not).
+  // This is implemented separately for Document (which has a cache) and DocumentFragment (which does not).
   getElementById(id) {
     if (id === "") {
       return null;

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

@@ -1,6 +1,7 @@
 "use strict";
 const { addNwsapi } = require("../helpers/selectors");
 const { HTML_NS } = require("../helpers/namespaces");
+const { nodeRoot } = require("../helpers/node");
 const { mixin, memoizeQuery } = require("../../utils");
 const idlUtils = require("../generated/utils");
 const NodeImpl = require("./Node-impl").implementation;
@@ -27,32 +28,6 @@ const Text = require("../generated/Text");
 const { isValidHostElementName } = require("../helpers/shadow-dom");
 const { isValidCustomElementName, lookupCEDefinition } = require("../helpers/custom-elements");
 
-function attachId(id, elm, doc) {
-  if (id && elm && doc) {
-    if (!doc._ids[id]) {
-      doc._ids[id] = [];
-    }
-    doc._ids[id].push(elm);
-  }
-}
-
-function detachId(id, elm, doc) {
-  if (id && elm && doc) {
-    if (doc._ids && doc._ids[id]) {
-      const elms = doc._ids[id];
-      for (let i = 0; i < elms.length; i++) {
-        if (elms[i] === elm) {
-          elms.splice(i, 1);
-          --i;
-        }
-      }
-      if (elms.length === 0) {
-        delete doc._ids[id];
-      }
-    }
-  }
-}
-
 class ElementImpl extends NodeImpl {
   constructor(globalObject, args, privateData) {
     super(globalObject, args, privateData);
@@ -87,8 +62,8 @@ class ElementImpl extends NodeImpl {
     namedPropertiesWindow.nodeAttachedToDocument(this);
 
     const id = this.getAttributeNS(null, "id");
-    if (id) {
-      attachId(id, this, this._ownerDocument);
+    if (id && nodeRoot(this) === this._ownerDocument) {
+      this._ownerDocument._byIdCache.add(id, this);
     }
 
     // If the element is initially in an HTML document but is later
@@ -105,18 +80,17 @@ class ElementImpl extends NodeImpl {
 
     const id = this.getAttributeNS(null, "id");
     if (id) {
-      detachId(id, this, this._ownerDocument);
+      this._ownerDocument._byIdCache.delete(id, this);
     }
   }
 
   _attrModified(name, value, oldValue) {
     this._modified();
     namedPropertiesWindow.elementAttributeModified(this, name, value, oldValue);
 
-    if (name === "id" && this._attached) {
-      const doc = this._ownerDocument;
-      detachId(oldValue, this, doc);
-      attachId(value, this, doc);
+    if (name === "id" && this._attached && nodeRoot(this) === this._ownerDocument) {
+      this._ownerDocument._byIdCache.delete(oldValue, this);
+      this._ownerDocument._byIdCache.add(value, this);
     }
 
     // update classList

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

@@ -18,7 +18,7 @@ class SVGSVGElementImpl extends SVGGraphicsElementImpl {
   }
 
   getElementById(elementId) {
-    // TODO: optimize with _ids caching trick; see Document class.
+    // TODO: optimize with ByIdCache; see Document class.
     for (const node of domSymbolTree.treeIterator(this)) {
       if (node.nodeType === ELEMENT_NODE && node.getAttributeNS(null, "id") === elementId) {
         return node;

test/web-platform-tests/to-run.yaml

@@ -450,7 +450,6 @@ Document-URL.html: [fail, Unknown]
 Document-characterSet-normalization-1.html: [timeout, Some encodings are not supported - see the whatwg-encoding module]
 Document-characterSet-normalization-2.html: [timeout, Some encodings are not supported - see the whatwg-encoding module]
 Document-createEvent.https.html: [fail, We don't support every event interface yet]
-Document-getElementById.html: [fail, We cache IDs in insertion order]
 Element-closest.html: [fail, :has is not supported (by all major browsers as well)]
 Element-firstElementChild-entity-xhtml.xhtml: [fail, Unknown]
 Element-firstElementChild-entity.svg: [fail, Unknown]