useTypingObserver: capture the window reference for cleanup#78772
useTypingObserver: capture the window reference for cleanup#78772xavier-lc wants to merge 8 commits into
useTypingObserver: capture the window reference for cleanup#78772Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @xavier.lozano.carreras@a8c.com. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
@xavier-lc is there any way how to reproduce the bug in "real life"? The PR description describes only a manual scenario where I set
How can this happen? The |
|
|
||
| return () => { | ||
| node.ownerDocument.defaultView.clearTimeout( timerId ); | ||
| node.ownerDocument.defaultView?.clearTimeout( timerId ); |
There was a problem hiding this comment.
I think the very best way how to fix this is to store the reference to document/window when mounting, and then using the same object when cleaning up:
useRefEffect( ( node ) => {
const win = node.ownerDocument.defaultView;
win.addEventListener();
return () => { win.removeEventListener(); };
} );This also guarantees that we're removing the listener from the object where we actually added it, not from some different one.
Some notes:
- we probably need to fix only the cleanup and nothing else. For example, inside event handlers the window object is always there. On an inactive/loading/unloading frame, events like
focusorselectionchangewouldn't probably happen in the first place. - to call
setTimeoutandclearTimeout, we don't really need the iframe'swindowobject. We can just call it on the top-level document.
There was a problem hiding this comment.
Thanks for the feedback, I've applied the changes.
| return <div ref={ ref } data-testid="target" />; | ||
| } | ||
|
|
||
| describe( 'useTypingObserver', () => { |
There was a problem hiding this comment.
I think this unit test is not very helpful, it tests a very internal edge case and even that edge case is being mocked. I recommend to remove it.
42fc8ff to
c653873
Compare
useTypingObserver: capture the window reference for cleanup
When the block editor is hosted inside an iframe, React can fire the useRefEffect cleanup after the iframe document has been detached from its window. At that point document.defaultView === null, so the unguarded `node.ownerDocument.defaultView.clearTimeout( timerId )` inside the cleanup throws, which also aborts the removeEventListener calls that follow it (leaking those listeners on the unmounting node). Optional-chain the defaultView access in the cleanup, plus the matching setTimeout in stopTypingOnNonTextField and getSelection in stopTypingOnSelectionUncollapse, so the hook never assumes the iframe's window is still attached.
Mounts the hook in the isTyping state so the cleanup branch that touches `defaultView` is reached, overrides `ownerDocument.defaultView` to null to simulate the iframe being detached from its window, then unmounts. Asserts both that the cleanup does not throw and that the `selectionchange`, `focus`, and `keydown` listeners are still removed in that scenario — the leak path the original throw was masking.
The test mocked an internal edge case (manually nulling `ownerDocument.defaultView`) that doesn't correspond to a reachable real-world state, so it asserted against a synthetic condition rather than observable behavior.
The cleanup function called `node.ownerDocument.defaultView.clearTimeout()`. When the editor canvas iframe is detached from its window before React runs the ref cleanup, `defaultView` is `null` and the access throws, aborting the rest of the cleanup and leaking the `focus`, `keydown`, and `selectionchange` listeners. The typing timer never needed the iframe's window in the first place, so set and clear it on the top-level `setTimeout`/`clearTimeout` instead. The timer id stays valid regardless of the frame's state, and the cleanup no longer touches `defaultView`.
216e520 to
9406512
Compare
What?
Capture the iframe window once, while the node is still attached, and reuse that reference for the rest of
useTypingObserver's effect.Why?
When the block editor is hosted inside an iframe (the default in 15.x), React can fire the
useRefEffectcleanup after the iframe document has been detached from its window. At that pointdocument.defaultView === null, so the unguarded property access throws:The throw aborts the rest of the cleanup — the
removeEventListenercalls that followclearTimeoutnever run, leakingfocus,keydown, andselectionchangelisteners on the unmounting node.The matching
setTimeoutandgetSelectionaccesses are guarded for symmetry: the invariant the hook now enforces is we never assume the iframe's window is still attached, regardless of which callback runs.How?
defaultViewis a live getter that becomesnullwhen the frame detaches. We read it once at mount (always safe) and reuse the captured reference in the handlers and cleanup, so nothing re-readsnode.ownerDocument.defaultViewat teardown time.Testing Instructions
No behavior change in normal use.
Screenshots or screencast
N/A — no UI change.
Use of AI Tools
This PR was authored with assistance from Claude Code (Anthropic).