fix(focus): skip destroyed elements in restoreFocus

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`.
 

src/core/dom-renderer/domRenderer.ts

@@ -35,7 +35,7 @@
 } from './domRendererUtils.js';
 
 // Feature detection for legacy brousers
 const _styleRef: any =
   typeof document !== 'undefined' ? document.documentElement?.style || {} : {};
 
 const supportsObjectFit: boolean = 'objectFit' in _styleRef;
@@ -109,7 +109,7 @@
     for (const prop in task.propsEnd) {
       const start = task.propsStart[prop]!;
       const end = task.propsEnd[prop]!;
       (task.node.props as any)[prop] = interpolateProp(prop, start, end, t);
     }
 
     updateNodeStyles(task.node);
@@ -134,7 +134,7 @@
 
   constructor(
     public node: DOMNode,
     props: Partial<lng.INodeAnimateProps<any>>,
     rawSettings: Partial<lng.AnimationSettings>,
   ) {
     this.settings = {
@@ -152,7 +152,7 @@
 
     for (const [prop, value] of Object.entries(props)) {
       if (value != null && typeof value === 'number') {
         this.propsStart[prop] = (node.props as any)[prop];
         this.propsEnd[prop] = value;
       }
     }
@@ -214,7 +214,7 @@
 
 function animate(
   this: DOMNode,
   props: Partial<lng.INodeAnimateProps<any>>,
   settings: Partial<lng.AnimationSettings>,
 ): lng.IAnimationController {
   return new AnimationController(this, props, settings);
@@ -1358,6 +1358,8 @@
 
   preventCleanup = true;
 
+  destroyed = false;
+
   constructor(
     public stage: IRendererStage,
     public props: IRendererNodeProps,
@@ -1380,6 +1382,7 @@
   }
 
   destroy(): void {
+    this.markDestroyed();
     elMap.delete(this);
     const parent = this.props.parent;
     if (parent instanceof DOMNode) {
@@ -1388,6 +1391,14 @@
     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;
   }

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;

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 <Show>; hiding the page destroys it and
+// its child `inner`.
+const setup = () => {
+  const [showPage, setShowPage] = createSignal(true);
+  let stack!: ReturnType<typeof useFocusStack>;
+  let kept!: lng.ElementNode;
+  let page!: lng.ElementNode;
+  let inner!: lng.ElementNode;
+
+  const Capture = () => {
+    stack = useFocusStack(false);
+    return null;
+  };
+
+  const dispose = renderer.render(() => (
+    <FocusStackProvider>
+      <Capture />
+      <view ref={kept} width={100} height={100} />
+      <Show when={showPage()}>
+        <view ref={page} width={100} height={100}>
+          <view ref={inner} width={50} height={50} />
+        </view>
+      </Show>
+    </FocusStackProvider>
+  ));
+
+  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();
+  });
+});