Skip to content

Pin the focus hand-back's non-last-dialog case, and fix three comments that overstate the code - #284

Merged
wormeyman merged 2 commits into
wormeyman-space-age-supportfrom
fix/281
Aug 30, 2026
Merged

Pin the focus hand-back's non-last-dialog case, and fix three comments that overstate the code#284
wormeyman merged 2 commits into
wormeyman-space-age-supportfrom
fix/281

Conversation

@wormeyman

@wormeyman wormeyman commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator

Closes #281.

Four follow-ups from #242, grouped because they are one commit's tail. Only the
first adds coverage; the rest are a wrong fixture and wording that says more
than the code does.

1. The untested case, which is the point of this PR

Dialog.close()'s comment spends a paragraph on why the hand-back is
deliberately not also conditional on this being the last dialog. Nothing
pinned that. Measured, every existing test passes under
!Dialog.anyOpen() && document.activeElement === document.body as well, so
re-adding that half would go green everywhere a person would look for it.

New test: open Import, open Export, wait for Export's field to take focus, close
Export through its own toggle, then assert one dialog is still open and the
canvas is focused.

Mutation table, measured across quick-actions.spec.ts + settings-pane-book-index.spec.ts
(17 tests), changing only Dialog.ts:126:

guard result
shipped, activeElement === document.body 17 passed
A !Dialog.anyOpen() && activeElement === document.body 1 failed - only the new test
B unconditional focus() 2 failed - the live-field test here, and settings-pane's treatment
C hand-back deleted 2 failed - the two hand-back tests here; both settings-pane tests pass

Read down the columns: each mutation is caught by a different test and A by
exactly one, which is the gap. The table is now in the spec's file header, which
is where someone looks before deleting a test that seems to always pass.

2. The fixture was wrong, and one character was not enough

settings-pane-book-index.spec.ts had item: 'blueprint_book' where the schema
pins blueprint-book. Fixing that left the spec still warning, because
definitions/blueprint also requires icons and the fixture had none:

fixture validation
blueprint_book, no icons /blueprint_book/item must be equal to constant
blueprint-book, no icons .../blueprint must have required property 'icons'
blueprint-book with icons clean - the only toast is the success one

Confirmed against the committed fixture with a throwaway probe reading both the
console and .toasts-toast, then removed. This is repo-wide, not local to
this spec - quick-actions.spec.ts's own ONE_CHEST and
book-serialize.spec.ts's book warn for the same missing-icons reason. Left
alone here as outside the issue, but worth its own pass.

3. A dropped barrier

The test at quick-actions.spec.ts:299 opened ExportDialog and went straight to
clicking Import's field. Export's focus is deferred through
G.app.ticker.addOnce, so nothing forced the ordering. If the click ever won,
the toggle would destroy a focused field and the final assertion would fail
after the 60s expect timeout - reading as "the guard is broken". Barrier
restored.

4. Three comments that say something untrue

  • The barrier comment claimed the assertion was there so the canvas would lose
    focus first. On a fresh page the focus is already on <body> - waitForEditor
    only navigates and loadBlueprint runs through page.evaluate, so nothing
    ever clicked. It is load-bearing for a different reason: the field has to hold
    the focus so that destroying it orphans it.
  • A test title still said "Closing the last dialog" when the rule is no
    longer about last.
  • The settings-pane header claimed setValue starts an async load with a gap
    the second arrow could race into. Checked: Editor.loadBlueprint has no
    await in its body and initBP() is synchronous, so the whole load and its
    Dialog.closeAll() run inside the keydown handler and no such gap exists.
  • Dialog.close()'s own comment opened with "only when this close is what
    orphaned it", and the merged Add a ToolsPanel with entity-info toggle/import string/export string/export image/undo/redo UI actions #242 subject says the same. The check cannot see
    that - it reads whether anything holds the focus right now, which has a third
    answer: nothing held it to begin with. Dragging the BP Book Index slider
    leaves <body> focused (dat.gui blurs on mousedown, its track is a <div>),
    and an InventoryDialog owns no <input> so closing one can never orphan
    anything. Both reach a true condition and both get a focused canvas, which is
    right for them. The guard is fine; the sentence was not.

That comment also quoted the clipboard listeners' condition verbatim. #279 gives
it a second half, so the copy would have gone stale with nobody opening this
file - it now says what the listeners require rather than how they spell it.
Comment only; the executable line is untouched.

Not done

The issue's optional item - widening === document.body to also accept null
and documentElement - is deliberately skipped. The e2e job is Chromium only,
where those arms are unreachable, so they would be untestable speculation. The
assumption and its failure direction (closed and silent, no worse than before
#242) are worth documenting if anyone adds a second browser.

Test plan

  • vp check . - 0 warnings, lint or type errors across 187 files
  • vp test - 221 passed
  • quick-actions.spec.ts + settings-pane-book-index.spec.ts - 17 passed
  • Mutations A, B and C each measured, Dialog.ts restored and verified byte-identical afterwards

🤖 Generated with Claude Code

https://claude.ai/code/session_01L7dF9eT5yMLqdJZ2rwytCz

wormeyman and others added 2 commits August 30, 2026 13:04
@wormeyman @claude
Pin the focus hand-back's non-last-dialog case and fix the book fixture
98953b2 
Four test-side follow-ups from #242.

The case `Dialog.close()`'s comment argues hardest for had no test: the
hand-back is deliberately not also conditional on this being the last
dialog, and every test that existed passed under
`!Dialog.anyOpen() && document.activeElement === document.body` too. The
new test opens Import, opens Export, waits for Export's field to take the
focus, closes Export through its own toggle, and expects one dialog still
open with the canvas focused. Mutation-checked three ways against both
spec files (17 tests): adding the `anyOpen()` half fails only the new
test, an unconditional `focus()` fails the live-field test here and the
settings-pane treatment, and deleting the hand-back fails the two
hand-back tests here and neither settings-pane test. The table is in the
file header.

`settings-pane-book-index.spec.ts` used `blueprint_book` where the schema
pins `blueprint-book`, so every run took the "loaded anyway" validation
path and raised a warning toast. The hyphen alone does not clear it -
`definitions/blueprint` also requires `icons`, which the fixture omitted -
so the entries carry icons now. Confirmed by reading the console and the
toast text: no validation line, and only the success toast.

Restored the `toBeFocused()` barrier that used to sit between opening
ExportDialog and clicking Import's field. Export's focus is deferred to a
ticker callback, so without it a click that won the race would fail the
final assertion on a 60s timeout and read as a broken guard.

Corrected three comments. The barrier's stated reason (the canvas losing
focus) was wrong - a fresh page has the focus on `<body>` already, and
what the barrier really does is make the close orphan something. A test
title still said "last dialog". And the settings-pane header claimed
`setValue` starts an async load that the second arrow could race;
`Editor.loadBlueprint` has no `await` in its body and `initBP()` does not
yield, so the whole load runs inside the keydown handler and there is no
such gap. The sync point stays for the reason that does hold.

Closes #281

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L7dF9eT5yMLqdJZ2rwytCz
@wormeyman
Say what the focus guard measures instead of what it intends
15a5b83 
The comment opened with "only when this close is what orphaned it", and the
merged #242 subject says the same. The check cannot see that. It reads
whether anything holds the focus right now, which is a different question
with a third answer: nothing held it to begin with.

Two reachable cases land there. Dragging the BP Book Index *slider* rather
than typing in its box leaves <body> focused, since dat.gui blurs on
mousedown and its track is a plain <div>; and an InventoryDialog owns no
<input> at all, so closing one can never orphan anything. Both reach a true
condition and both get a focused canvas, which is the right answer for them -
the guard is fine, the sentence describing it was not.

Also stops quoting the clipboard listeners' condition verbatim. Issue #279
gives it a second half, so the copy here would have gone stale with nobody
opening this file. It says what the listeners require instead.

Comment only - the executable line is untouched.
@wormeyman
wormeyman merged commit 74de74a into wormeyman-space-age-support Aug 30, 2026
9 checks passed
@wormeyman wormeyman mentioned this pull request Aug 31, 2026
Closed
@byalex33 byalex33 mentioned this pull request Sep 1, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Follow-ups from #242: one untested focus case, a wrong test fixture, and three comments that overstate what the code does

1 participant

@wormeyman