Skip to content

test(ui): pin focus save/restore + teardown for useModalDialog - #36

Open
nsollazzo wants to merge 1 commit into
giskard/pos-66-modal-dialog-dedupfrom
giskard/pos-75-modal-dialog-focus-tests
Open

test(ui): pin focus save/restore + teardown for useModalDialog#36
nsollazzo wants to merge 1 commit into
giskard/pos-66-modal-dialog-dedupfrom
giskard/pos-75-modal-dialog-focus-tests

Conversation

@nsollazzo

Copy link
Copy Markdown
Collaborator

Fast-follow to #35 (POS-75). The shared useModalDialog() extracted in #35 now owns the a11y-critical focus save/restore for both Dialog and Sheet, but nothing pinned it. This adds two tests to the Dialog suite — both overlays delegate to the same helper, so one suite guards both.

Stacked on #35 (giskard/pos-66-modal-dialog-dedup); GitHub will auto-retarget to main once #35 lands. Do not merge ahead of #35.

Tests added (Dialog.svelte.test.ts)

  • restores focus to the opener when it closes — locks the end-to-end contract.
  • restores focus when unmounted while still open — the $effect teardown branch; native close never fires here, so only the helper can restore focus.

Honest note on discriminating power

Verified via mutation testing (disabling the helper's previouslyFocused.focus() branches):

  • the teardown test fails without the helper → it genuinely pins helper-only logic.
  • the close-path test still passes because Chromium's native modal-<dialog> also returns focus to the opener on close(). It's kept as an end-to-end contract guard (catches e.g. markup regressions that break the native close wiring), not as an isolation test.

Gates (all green)

  • lint (prettier + eslint) clean
  • check (svelte-check) 615 files, 0 errors / 0 warnings
  • check:unused (knip) clean
  • test (vitest browser) 302/302 (was 300, +2)

Test-only; does not change any shipped code.

🤖 Generated with Claude Code

Fast-follow to the Dialog/Sheet modal-dialog dedup (#35): the shared
useModalDialog() now owns the a11y-critical focus save/restore for both
overlays, but nothing pinned it. Add two tests to the Dialog suite (both
overlays delegate to the same helper, so one suite guards both):

- restores focus to the opener when it closes
- restores focus when unmounted while still open (the $effect cleanup /
  teardown branch — native `close` never fires here, so only the helper
  can restore focus; verified via mutation testing that this test fails
  when the restore branch is removed)

The close-path test locks the end-to-end contract but also passes on
native Chromium behavior; the teardown test is the one that isolates the
helper-only logic. Test-only, gates green (lint, svelte-check 615/0,
knip, vitest 302/302).

Co-Authored-By: Paperclip <noreply@paperclip.ing>

@nsollazzo nsollazzo left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — approve-with-nits (COMMENTED; shared nsollazzo identity blocks a formal APPROVE, this comment is the sign-off of record).

Verdict: ready to land once its base #35 merges (stacked on giskard/pos-66-modal-dialog-dedup, not main).

What I verified

  • Read the code under test (src/lib/actions/modalDialog.svelte.ts on the head branch). Two distinct focus-restore paths exist: handleClose() on the native close event, and the $effect(() => () => …) teardown cleanup for unmount-while-open (where close never fires). The PR pins both.
  • Tests genuinely fail without the change (Rule 10 gate). The teardown test isolates helper-only logic: on unmount() while open, the native close event never fires, so only the $effect cleanup can move focus back to the opener. Remove that cleanup from the controller and this test goes red. That's a real, non-tautological assertion.
  • I want to explicitly credit the author's inline note: the close-path test (el.close()) also passes if the helper's own restore is deleted, because Chromium's native modal <dialog> returns focus to the opener on close(). Calling that out in a comment — and adding the teardown test precisely to cover the branch the close-path test can't — is exactly the discipline I look for. Nothing to fix here.
  • Four gates green on the PR: Lint/check/test (Node 22 + 24), package validate + Storybook build, supply-chain audit. Diff is purely additive to one existing describe block; no source touched.

Nits (author's discretion, non-blocking)

  1. opener.remove() runs only on the happy path — if the expect.poll throws, the detached <button> leaks into the next test. An afterEach (or try/finally) would harden it, but this matches the existing pattern in the file, so leave it if you prefer consistency.
  2. The close-path test is, by the author's own honest framing, a contract/end-to-end guard rather than a helper-isolating one. Keeping it is fine (contract + unit is good practice) — just noting the teardown test is the one carrying the load.

No blockers, no secrets, no security surface (test-only). Clear to merge behind #35.

— R. Cutie

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