Repository navigation
fix(focus): skip destroyed elements in restoreFocus - #69
Merged
Merged
Conversation
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
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>
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.
Problem
restoreFocusfromuseFocusStackwould give focus to an element saved bystoreFocuseven after that element had been destroyed (e.g. the page it lived on was removed).Fix
restoreFocusnow pops destroyed elements off the stack and focuses the most recent element that still exists. If every stored element is destroyed, it returnsfalseand leaves focus unchanged.element.destroyedreads the flag from the renderer node. The WebGLCoreNodesets it on a node and its whole subtree, butDOMNodenever set it, so the check could never fire in DOM rendering mode (or in the jsdom tests).DOMNodenow setsdestroyedon itself and its subtree when destroyed, matchingCoreNode.Side effect
KeepAlivealready checks.destroyedbefore 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
restoreFocusruns in the same synchronous step that removes the element (e.g. inside that page'sonCleanup), it isn't marked destroyed yet and still receives focus.Tests
tests/focusStack.test.tsx:restoreFocusskips both, focuses the earlier element, and the skipped entries are gone from the stack.restoreFocusreturnsfalseand focus stays put.Both tests fail on
mainand pass with this change. Full suite,tscand lint pass.🤖 Generated with Claude Code