Skip to content

fix(focus): skip destroyed elements in restoreFocus - #69

Merged
chiefcll merged 1 commit into
mainfrom
fix/focus-stack-skip-destroyed
Oct 4, 2026
Merged

chiefcll merged 1 commit into
mainfrom
fix/focus-stack-skip-destroyed

Conversation

@chiefcll

@chiefcll chiefcll commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

Problem

restoreFocus from useFocusStack would give focus to an element saved by storeFocus even after that element had been destroyed (e.g. the page it lived on was removed).

Fix

  • restoreFocus now pops destroyed elements off the stack and focuses the most recent element that still exists. If every stored element is destroyed, it returns false and leaves focus unchanged.
  • DOM renderer: element.destroyed reads the flag from the renderer node. The WebGL CoreNode sets it on a node and its whole subtree, but DOMNode never set it, so the check could never fire in DOM rendering mode (or in the jsdom tests). DOMNode now sets destroyed on itself and its subtree when destroyed, matching CoreNode.

Side effect

KeepAlive already checks .destroyed before reusing a cached child. In DOM rendering mode that check never fired before; it now does, so destroyed children get rebuilt instead of reused, the same as with WebGL.

Known gap (unchanged)

A removed element is only destroyed in the post-mutation flush. If restoreFocus runs in the same synchronous step that removes the element (e.g. inside that page's onCleanup), it isn't marked destroyed yet and still receives focus.

Tests

tests/focusStack.test.tsx:

  • A stored page and a stored child inside it are removed → restoreFocus skips both, focuses the earlier element, and the skipped entries are gone from the stack.
  • Every stored element destroyed → restoreFocus returns false and focus stays put.

Both tests fail on main and pass with this change. Full suite, tsc and lint pass.

🤖 Generated with Claude Code

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 <noreply@anthropic.com>
@chiefcll
chiefcll merged commit 3070ced into main Oct 4, 2026
1 check passed
chiefcll added a commit that referenced this pull request Oct 6, 2026
Main brought the contract tests and benchmark harness from 1.7, adapted to
1.6.4 on renderer 1.9 (#68), the per-side boundsMargin tuple on the DOM
renderer (#67), restoreFocus skipping destroyed elements (#69), the
renderer 1.10.1 requirement (#70) and version 1.6.5. Resolution, keeping
1.7's behaviour and the smallest diff against main:

- Tests: merged against the Phase 1 commits main started from (a12c778),
  so only main's rewording applied. 1.7's assertions and expected values
  win. Main's headers are kept where true on 1.7; "1.6.4 (renderer 1.9)"
  becomes "1.7 (renderer 2.0)". contract-types.tsx keeps 1.7's renderer 2
  type names (main added no Solid-side name that 1.7 lacks);
  tests/webgl/setup.ts and the late-font WebGL test stay 1.7's (renderer 2
  lets a text wait for its font). tests/focusStack.test.tsx from main
  passes on 1.7.
- bench/: main's files, plus arm B (f1c8ab0 on renderer faf4b9f), arm C on
  the linked ../renderer-v2-solid (main's installed-renderer lookup, plus
  1.7's rebuild of its dist and its revision in the record), skipC for the
  demo scripts, the A/B and B/C ratio table, default --arms A,B,C.
  Main's probe fix (renderer 1.x shader props are non-enumerable accessors)
  and the summary's per-arm Solid/renderer lines are kept; the renderer 2
  hooks install on arms B and C. bench/src/arm-v2.ts, bench/demo/* and
  bench/micro/* stay.
- docs/perf/README.md: main's text plus arm B, the per-major count hooks,
  the 120 Hz note and a section for bench/demo and bench/micro.
- package.json: version 1.6.5; @solidtv/renderer stays 1.7's (peer
  ^2.0.0-alpha.0, devDependency link:../renderer-v2-solid). pnpm install
  regenerated the lockfile (unchanged from 1.7's).
- .gitignore, eslint.config.js: union (main's recursive results ignore,
  1.7's .superpowers ignores).
- src/: #67 and #69's createFocusStack change auto-merged. #69's DOMNode
  `destroyed` field and markDestroyed() are dropped: 1.7's DOMNode already
  has a `destroyed` getter (this node or an ancestor destroyed), which
  restoreFocus reads through ElementNode.destroyed. Renderer 2.0 still
  takes one boundsMargin number (the tuple's widest edge, with a warning),
  so the tuple works on the DOM renderer only.
- MIGRATION-1.7.md: the boundsMargin tuple is a break inherited from
  renderer 2.0; the DOM renderer's `destroyed` was undefined before 1.6.5.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant