ReviewOS

stacks/very-happy-dom

document.head and document.body go stale when documentElement.innerHTML is replaced

#1595
Closed glennmichael123 opened this 8 days ago ·
8 days ago

Summary

VirtualDocument.head and VirtualDocument.body are plain fields assigned once in the constructor. Assigning documentElement.innerHTML replaces the document element's children, but goes through VirtualElement's innerHTML setter, which knows nothing about those fields — so they keep pointing at the original <head> and <body>, which are now detached.

Everything then operates on an orphaned tree. It fails silently: no error, just writes that go nowhere and reads that come back empty.

This is the path BrowserPage.content takes, so it affects the Browser API's main setup step.

Reproduction

import { Window } from 'very-happy-dom'

const window = new Window()
const document = window.document

document.documentElement.innerHTML = '<head><title>Page title</title></head><body><h1>Hi</h1></body>'

console.log(document.body.isConnected) // false
console.log(document.body === document.querySelector('body')) // false
console.log(document.body.innerHTML) // ""
console.log(document.querySelector('body').innerHTML) // "<h1>Hi</h1>"

// document.title reads through the stale head, so it disagrees with the DOM
console.log(document.title) // ""
console.log(document.querySelector('title').textContent) // "Page title"

// Injecting a stylesheet into the head silently goes nowhere
const style = document.createElement('style')
style.textContent = 'p { color: red }'
document.head.appendChild(style)

console.log(document.querySelector('style')) // null

Same through the Browser API, since BrowserPage.content assigns documentElement.innerHTML:

import { Browser } from 'very-happy-dom'

const page = new Browser().newPage()
page.content = '<body><h1>Hi</h1></body>'

const document = page.mainFrame.document
console.log(document.body.isConnected) // false
console.log(document.body === document.querySelector('body')) // false

Confirmed against main at b570adc with Bun 1.4.3.

Scope

Only the documentElement.innerHTML path is affected. The paths that reassign the fields explicitly are fine:

PathResult
document.body.innerHTML = …correct — body is not replaced
JSDOM full-document parsecorrect
document.write()correct
documentElement.innerHTML = …stale
page.content = …stale

Current implementation

// src/nodes/VirtualDocument.ts
head: VirtualElement | null = null
body: VirtualElement | null = null

Assigned in the constructor, and reassigned in the parse path (this.head = childEl / this.body = childEl) which is why document.write and the JSDOM constructor stay correct. Nothing re-syncs them when documentElement's children are replaced from VirtualElement.

get title() reads this.head?.querySelector('title'), which is why the title symptom follows from the same cause.

Expected behavior

document.head and document.body should always be the current first <head>/<body> child of documentElement, however those children were produced:

  • document.body === document.querySelector('body')
  • document.body.isConnected === true
  • document.head.appendChild(el) puts el in the live document
  • document.title agrees with document.querySelector('title').textContent

Resolving them from documentElement rather than caching would give one source of truth. A few sites currently assign to head/body directly (the constructor and the parse path) and would need reworking to suit.

Worth deciding at the same time: document.querySelector('#x') currently finds elements that are only in the detached tree, because the id index is keyed by owner document rather than connectivity. That is arguably a second bug, and it is what masks this one in tests that go through getElementById/#id lookups after setting content.

8 days ago

Fixed on main in 99cf5b2. Leaving this open for you to close, since there is one behaviour change worth signing off on.

What changed

head and body are now accessors over documentElement's children rather than cached fields, making the document element the single source of truth instead of requiring every mutation path to keep a cache honest.

Re-running the reproduction:

document.body.isConnected                    → true   (was false)
document.body === document.querySelector('body') → true   (was false)
document.body.innerHTML                      → "<h1>Hi</h1>"  (was "")
document.title                               → "Page title"   (was "")
document.querySelector('style')              → the style element (was null)

The page.content variant is fixed by the same change.

The setters are kept, because the parse paths in jsdom/JSDOM.ts and the DOMParser in window/Window.ts assign head/body after swapping documentElement wholesale. Assigning the element that is already in place is a no-op, so those sites now go through a setter that agrees with the getter instead of writing a field that could disagree with the tree. The manual sync loop in the document's own parse path became redundant and is removed.

The behaviour change to confirm

Markup with no <body> now reports document.body === null, which is what a browser does:

document.documentElement.innerHTML = '<h1>Hi</h1>'
document.body // null — was a stale, detached <body>

Code that appended to it was already losing those writes silently; it now throws instead. That is the intent — fail loudly rather than quietly — but it is the one way this is not purely additive.

Paths verified unchanged

document.body.innerHTML = …, the JSDOM full-document constructor, and document.write() all behaved correctly before and still do — they were never affected, since they either do not replace body or reassign the references explicitly.

Tests

12 tests in test/document-api.test.ts; 9 fail against the previous implementation. The 3 that pass either way pin behaviour that had to keep working: a fresh document's head/body, the ordinary body.innerHTML path, and the no-op setter. Full suite is 1376 passing, with lint, typecheck and build clean.

Still open from the report

The second point I raised — document.querySelector('#x') matching elements that are only in a detached tree, because the id index is keyed by owner document rather than connectivity — is not addressed here. It is a separate bug and it is what masked this one in tests that look elements up by id after setting content. Happy to file it separately if you want it tracked.

8 days ago

Fixed in 99cf5b2 (head and body resolve from documentElement instead of being cached, so they survive innerHTML replacement). Released in v0.1.11, so this is now available to consumers rather than only on main. The scope notes I left earlier on this issue still stand — read those for what was deliberately left out.

Sign in to comment on this issue.