Read ownerDocument off the Node prototype - #163
Conversation
`saveAndRestoreFocus` resolves the document that owns the focus by reading `ctx.target.ownerDocument`. `<form>` is [LegacyOverrideBuiltIns], so a named control such as `<input name="ownerDocument">` installs an own property on the form that shadows `Node.prototype.ownerDocument`. When the morph target is such a form, `doc` is an `HTMLInputElement`, `doc.activeElement` is `undefined`, and the function early-returns, so focus and selection restoration silently stop happening. Add an `ownerDocumentOf` helper that reads through the prototype getter, caching the descriptor once at module scope. The getter is also realm-safe: it brand-checks the internal slot rather than the realm.
61be01e to
f66723a
Compare
myabc
changed the title
ownerDocument off the Node prototype
There was a problem hiding this comment.
🟢 Approval recommended
The fix is narrowly scoped, aligns with the documented root cause, and is supported by targeted regression tests.
Pull request overview
This PR fixes a morphing failure caused by <form> elements that shadow the ownerDocument property (via [LegacyOverrideBuiltIns]), by resolving ctx.doc through the native Node.prototype.ownerDocument getter and caching that getter lazily to avoid requiring DOM constructors at import time.
Changes:
- Switch morph context document resolution from
oldNode.ownerDocumentto a newownerDocumentOf(oldNode)helper. - Add a regression test covering focus/selection restoration when a target
<form>shadowsownerDocument. - Add a test ensuring
src/idiomorph.jscan be evaluated even when DOM constructors are unavailable.
File summaries
| File | Description |
|---|---|
| test/restore-focus.js | Adds coverage for focus/selection restoration when a form shadows ownerDocument. |
| test/core.js | Verifies the library source can be loaded/evaluated without DOM constructors. |
| src/idiomorph.js | Introduces ownerDocumentOf and uses it when creating the morph context. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@botandrose @lizarusi apologies that these patches didn't make the cut for v0.8.0 (although tbf I don't think they are critical). I had Claude also take a look at #156, but sat on the findings until I could verify. Would be grateful for your feedback! |
|
@myabc Hi Alex, thank you for exploring this one! While this is indeed a bug, its such an extreme corner-case, that I think the juice on this one is just not worth the squeeze. It seems to me that someone has to essentially try to hit this, and its not easy to hit! They have to have a very unlikely setup with a form element named So I would bet this bug has probably literally never happened in the real world, and the consequences are minor anyways. Furthermore, the upcoming Safari 26.6 / 27.0 is supposed to support So I'm going to close this one. Let me know if you disagree! |
|
@botandrose I understand the trade-off between covering all edge cases vs. keeping a solution clean and easy to reason about, so 👍🏻 from me for closing these two PRs. While we have hit occasional issues with form DOM properties colliding method names at @opf, this particular edge case hasn't caused us issues.
Nice! |
|
@myabc thanks for looking into this! Yeah, very interesting findings, I agree that even though these are real bugs, you'd almost have to be trying to break it to hit these cases. Let's see if anyone hits them in the wild! |
Note
This and #164 are pre-emptive fixes to possible issues that I found when reviewing @lizarusi's #156
A form control named
ownerDocumentshadows the form's DOM property, causing morphing to fail when creating the pantry in the target document.Resolve
ctx.docthrough the nativeNodegetter. Cache it on first use so importing the module does not require a DOM.Tests cover focus and selection restoration on a shadowing form and loading without DOM constructors.