read ownerDocument off the Node prototype - #163
Draft
myabc wants to merge 1 commit into
Draft
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Drive-by from a non-maintainer — take it or leave it, and I'm happy to close it if you'd rather fix this differently.
The bug
saveAndRestoreFocusreadsctx.target.ownerDocumentto find the document that owns the focus. That read is shadowable.<form>is the one element in HTML marked[LegacyOverrideBuiltIns]in the WebIDL: its named-property getter (which exposes controls byname/id) is installed ahead of the prototype chain rather than behind it. So<input name="ownerDocument">inside a form makesform.ownerDocumentreturn thatHTMLInputElement, not theDocument. This is the same class of bug that8fba6e9hardened the duck-typedis.*helpers against —<input name="nodeType">shadowingNode.prototype.nodeType.When the morph target is such a form,
docis anHTMLInputElement,doc.activeElementisundefined,is.inputElement(undefined)is false, andsaveAndRestoreFocusearly-returns. Focus and selection restoration silently stop happening — no throw, no warning.Where it came from
74fc41b(in #156) changed this from the globaldocumenttoctx.target.ownerDocument. The intent is right — focus lives in the target's document, which may not be ours, and that's what makes the cross-realm iframe test pass. The property just happens to be shadowable, and the global it replaced was not.A/B, container-move scenario with
moveBeforedisabled so the native move can't preserve focus on its own:8fba6e9is the commit immediately before74fc41b, so the regression is that commit and nothing older.The fix
Read
ownerDocumentthroughNode.prototype's getter instead of off the instance. The descriptor is cached once at module scope, so there's nogetOwnPropertyDescriptorcall per morph.The getter is also realm-safe — it brand-checks the internal slot, not the realm the node was created in — so it keeps working for nodes from an iframe document, which is what
74fc41bwas after in the first place. I verified that directly against a node from an iframe realm, not just in the same-realm case.Regression test
test/restore-focus.jsgets one test modelled on the existing IDed-container move tests. It disablesmoveBeforeon the parents first — without that the native move preserves focus by itself and the test passes unfixed, which would make it worthless.It genuinely catches the bug:
83246cb:214 passed, 1 failed—AssertionError: expected false to equal true214 passed, 0 failedVerification
Coverage stays at 100 %.
dist/is untouched — this doesn't need a rebuild.Note
There are a few more
document.activeElementreads (morphNode,ignoreAttribute,ignoreValueOfActiveElement,createActiveElementAndParents) that still resolve against the host document and are therefore inert cross-realm, plus a related shadowing hazard infindIdElements. Those are a separate change and I'd rather not pile them onto this one. Happy to open a follow-up if this direction looks right to you.