From f2d61d3b0d73c139239c9a606f3d30893432e521 Mon Sep 17 00:00:00 2001 From: Chris Lorenzo Date: Sun, 4 Oct 2026 14:05:51 -0400 Subject: [PATCH] fix(focus): skip destroyed elements in restoreFocus restoreFocus would focus an element saved by storeFocus even after it had been destroyed. It now pops destroyed elements off the stack and focuses the most recent one that still exists, returning false if none remain. The DOM renderer never set `destroyed` on its nodes, so the check would never fire in DOM rendering mode. DOMNode now sets `destroyed` on itself and its subtree when destroyed, matching CoreNode. Co-Authored-By: Claude Opus 5.5 --- docs/primitives/createFocusStack.md | 2 +- src/core/dom-renderer/domRenderer.ts | 11 ++++ src/primitives/createFocusStack.tsx | 8 ++- tests/focusStack.test.tsx | 79 ++++++++++++++++++++++++++++ 4 files changed, 97 insertions(+), 3 deletions(-) create mode 100644 tests/focusStack.test.tsx diff --git a/docs/primitives/createFocusStack.md b/docs/primitives/createFocusStack.md index 6893008f..ec0ce230 100644 --- a/docs/primitives/createFocusStack.md +++ b/docs/primitives/createFocusStack.md @@ -61,7 +61,7 @@ Stores the currently focused element. If the element is already active, it does ### `restoreFocus(): boolean` -Restores focus to the last stored element and removes it from the stack. +Restores focus to the last stored element and removes it from the stack. Elements destroyed since they were stored are removed and skipped, so focus goes to the most recent element that still exists. - Returns `true` if focus was successfully restored, otherwise `false`. diff --git a/src/core/dom-renderer/domRenderer.ts b/src/core/dom-renderer/domRenderer.ts index 818fc6e4..4ec1adf2 100644 --- a/src/core/dom-renderer/domRenderer.ts +++ b/src/core/dom-renderer/domRenderer.ts @@ -1358,6 +1358,8 @@ export class DOMNode extends EventEmitter implements IRendererNode { preventCleanup = true; + destroyed = false; + constructor( public stage: IRendererStage, public props: IRendererNodeProps, @@ -1380,6 +1382,7 @@ export class DOMNode extends EventEmitter implements IRendererNode { } destroy(): void { + this.markDestroyed(); elMap.delete(this); const parent = this.props.parent; if (parent instanceof DOMNode) { @@ -1388,6 +1391,14 @@ export class DOMNode extends EventEmitter implements IRendererNode { this.div.parentNode?.removeChild(this.div); } + // Like CoreNode, destroying a node marks its whole subtree destroyed. + private markDestroyed() { + this.destroyed = true; + for (const child of this.children) { + child.markDestroyed(); + } + } + get parent() { return this.props.parent; } diff --git a/src/primitives/createFocusStack.tsx b/src/primitives/createFocusStack.tsx index 2937adbd..abf3e148 100644 --- a/src/primitives/createFocusStack.tsx +++ b/src/primitives/createFocusStack.tsx @@ -14,7 +14,7 @@ * * Functions: * - `storeFocus(element: ElementNode, prevElement?: ElementNode)`: Stores the provided element in the focus stack. - * - `restoreFocus()`: Restores focus to the last stored element and removes it from the stack. Returns `true` if successful, `false` otherwise. + * - `restoreFocus()`: Restores focus to the last stored element and removes it from the stack, skipping (and removing) destroyed elements. Returns `true` if successful, `false` otherwise. * - `clearFocusStack()`: Empties the focus stack. */ import * as s from 'solid-js'; @@ -41,7 +41,11 @@ export function FocusStackProvider(props: { children: s.JSX.Element}) { function restoreFocus(): boolean { let wasFocused = false; setFocusStack((stack) => { - const prevElement = stack.pop(); + let prevElement = stack.pop(); + // Skip elements destroyed since they were stored + while (prevElement?.destroyed) { + prevElement = stack.pop(); + } if (prevElement && typeof prevElement.setFocus === 'function') { prevElement.setFocus(); wasFocused = true; diff --git a/tests/focusStack.test.tsx b/tests/focusStack.test.tsx new file mode 100644 index 00000000..03c9db6d --- /dev/null +++ b/tests/focusStack.test.tsx @@ -0,0 +1,79 @@ +import * as v from 'vitest'; +import * as lng from '@solidtv/solid'; +import { createSignal, Show } from 'solid-js'; +import { FocusStackProvider, useFocusStack } from '@solidtv/solid/primitives'; +import { renderer } from './setup.js'; + +const wait = (ms = 10) => new Promise((r) => setTimeout(r, ms)); + +// Renders `kept` plus a page under ; hiding the page destroys it and +// its child `inner`. +const setup = () => { + const [showPage, setShowPage] = createSignal(true); + let stack!: ReturnType; + let kept!: lng.ElementNode; + let page!: lng.ElementNode; + let inner!: lng.ElementNode; + + const Capture = () => { + stack = useFocusStack(false); + return null; + }; + + const dispose = renderer.render(() => ( + + + + + + + + + + )); + + return { stack, kept, page, inner, setShowPage, dispose }; +}; + +v.describe('useFocusStack restoreFocus', () => { + v.test('skips destroyed elements and focuses the next one', async () => { + const { stack, kept, page, inner, setShowPage, dispose } = setup(); + await wait(); + + stack.storeFocus(kept); + stack.storeFocus(page); + stack.storeFocus(inner); + + setShowPage(false); + await wait(); + v.expect(page.destroyed).toBe(true); + v.expect(inner.destroyed).toBe(true); + + v.expect(stack.restoreFocus()).toBe(true); + await wait(); + v.expect(lng.activeElement()).toBe(kept); + + // destroyed entries were removed along with the restored one + v.expect(stack.restoreFocus()).toBe(false); + + dispose(); + }); + + v.test('returns false when every stored element is destroyed', async () => { + const { stack, kept, page, setShowPage, dispose } = setup(); + await wait(); + + kept.setFocus(); + await wait(); + stack.storeFocus(page); + + setShowPage(false); + await wait(); + + v.expect(stack.restoreFocus()).toBe(false); + await wait(); + v.expect(lng.activeElement()).toBe(kept); + + dispose(); + }); +});