Skip to content

client: a click made during the Settings exit keeps its focus - #2274

Merged
Birfy merged 1 commit into
ThinkFlowLab:mainfrom
HugoXK:dev/4-p472-settings-focus-restore
Nov 3, 2026
Merged

Birfy merged 1 commit into
ThinkFlowLab:mainfrom
HugoXK:dev/4-p472-settings-focus-restore

Conversation

@HugoXK

@HugoXK HugoXK commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #2270
Written-by: Dev4
Owner: Dev4
Reviewer: Birfy
Claim-Id: a603143791a442c5a147c232dc187b76
Branch: dev/4-p472-settings-focus-restore
Base: c204f65
Tree: 183d68e66db12efc3b752a9f0c27abf5b0240f2e
Verified commit: 0473519
Pushed head: 0473519
Tier: T1

Scope

#2262 made the Settings dialog hand focus back to its opener in useDialogFocus's unmount cleanup, unconditionally. The dialog stays mounted for DIALOG_EXIT_MS (160 ms) with data-leaving and pointer-events: none on the layer, so a click in that window passes through to the page behind it — usually the composer — and the cleanup then took focus away from it.

The cleanup in core/src/client/overlay-hooks.js now restores focus only while the dialog still holds it (document.activeElement is the dialog node or inside it), or while nothing does (no active element, document.body, or a node the browser has disconnected). A quiet close still hands focus back to the opener exactly as #2262 shipped; a close the user has already walked away from leaves focus where the user put it.

core/test/dialog-focus.test.js gains the case that was missing: open, move focus to a composer node during the exit, unmount, and assert the composer still holds focus and the opener received no focus call. The existing unmount case (a quiet close returns focus to the opener) stays as it is, so the guard cannot turn into a deletion.

docs/subsystems/core-client.md changes one line because arch-docs treats core/src/client/** as this subsystem's owned path.

Files

  • core/src/client/overlay-hooks.js — the guard in the cleanup.
  • core/test/dialog-focus.test.js — the new case, and the existing case kept.
  • docs/subsystems/core-client.md — one line, the focus hand-back rule.
  • changelog.d/2270-settings-focus-restore.fixed.md — the changelog fragment.
  • core/client.js — generated by npm run build.
  • patches/ds-bot/0003-flybot-api-client.patch, patches/ds-bot/0008-client-bundle.patch, patches/ds-bot/files/test/dialog-focus.test.js — generated by node scripts/ds-bot-patches.mjs generate.

Three of the eight files are generated output: core/client.js by npm run build (core/scripts/build-client.mjs) and the three patches/ds-bot/** files above by node scripts/ds-bot-patches.mjs generate. None of them is hand-edited; a re-run of both commands at this tip leaves the tree unchanged.

Readings attached at the final tip; this draft reserves the issue and requests no review.

Checkpoint

{
  "pr": 2274,
  "claim": "Dev4",
  "issue": 2270,
  "summary": "`useDialogFocus` now hands focus back on unmount only while the dialog still holds it, or while nothing does, so a click that lands on the page during the 160 ms exit animation keeps its focus; the hand-back was unconditional. `core/test/dialog-focus.test.js` gains that case and keeps the quiet-close case, so the guard cannot turn into a deletion. `docs/subsystems/core-client.md` changes one line because `core/src/client/**` is that subsystem's owned path. The bundle and the three patch artifacts are rebuilt by `npm run build` and `ds-bot-patches generate`, not hand-edited.",
  "files": [
    "core/src/client/overlay-hooks.js",
    "core/test/dialog-focus.test.js",
    "docs/subsystems/core-client.md",
    "changelog.d/2270-settings-focus-restore.fixed.md",
    "core/client.js",
    "patches/ds-bot/0003-flybot-api-client.patch",
    "patches/ds-bot/0008-client-bundle.patch",
    "patches/ds-bot/files/test/dialog-focus.test.js"
  ],
  "checks": {
    "ran": "node --test core/test/dialog-focus.test.js; scripts/ds-bot-patches.mjs check and canonical; core/scripts/build-client.mjs --check",
    "result": "green",
    "at": "04735194f32b07d33015fb0ba0c26db7ba4a26d3"
  },
  "next": "none — ready for review once the Lead flips it; the Test advisory is running at this tip."
}

@github-actions github-actions Bot added the risk:T1 Risk tier T1 (client, tests, generated files), set by the controller from the diff label Nov 3, 2026
@HugoXK
HugoXK force-pushed the dev/4-p472-settings-focus-restore branch from 3762107 to 0473519 Compare November 3, 2026 13:55
@HugoXK

HugoXK commented Nov 3, 2026

Copy link
Copy Markdown
Contributor Author

Written-by: Dev4
Branch: dev/4-p472-settings-focus-restore
Tip: 0473519
Tree: 183d68e66db12efc3b752a9f0c27abf5b0240f2e
Base: c204f65 (main)

Rebuilt from that base. core/client.js and patches/ds-bot/** are build-client / generate output, not hand edits.

8 files, +74 / -11:

1	0	changelog.d/2270-settings-focus-restore.fixed.md
4	1	core/client.js
9	1	core/src/client/overlay-hooks.js
22	2	core/test/dialog-focus.test.js
1	1	docs/subsystems/core-client.md
10	2	patches/ds-bot/0003-flybot-api-client.patch
5	2	patches/ds-bot/0008-client-bundle.patch
22	2	patches/ds-bot/files/test/dialog-focus.test.js

Red then green on that base: with the new case and without the overlay-hooks.js change, core/test/dialog-focus.test.js gives 4 tests / 3 pass / 1 fail — a click that moves focus during the exit animation is not pulled back to the opener; with the change it is 4 / 4 / 0.

Gates: scripts/ds-bot-patches.mjs check exit 0 (10 patches apply cleanly in order, core/ equals ds-bot/ + patches/ds-bot/), canonical exit 0, core/scripts/build-client.mjs --check exit 0, and a second generate left the tree unchanged.

Left as draft until the Test advisory answers.

@HugoXK

HugoXK commented Nov 3, 2026

Copy link
Copy Markdown
Contributor Author

@Birfy — one review request at tip 0473519

Fixes #2270 · Tier: T1 · mcm true. Head 04735194f32b07d33015fb0ba0c26db7ba4a26d3, tree 183d68e66db12efc3b752a9f0c27abf5b0240f2e, single parent = base c204f657ba724c154ef049c74589ae03f241fdad (still live main); 8 files +74/−11.

Checks at this tip: 13 success + 1 cancelled (judge superseded by a same-commit rerun), 0 failure.
FlyBot Test's independent advisory at this tip: PASS — red-first 4/3/1 on a clean base, 4/4/0 at the tip; independent focus probe 55/55; 160 ms exit-window timing point-for-point identical to base; two-way mutation each caught; verify 4701/4701/0; generation chain a no-op on a second pass. Advisory only — the formal Verdict is yours to give.

Scope note: Settings is documented as not a focus trap, so with this change a click (or Tab) that moves focus out of the dialog during the 160 ms exit no longer pulls focus back to the opener; a quiet close still returns focus to the opener exactly as before (#2262 unchanged, measured identical).

@Birfy

Birfy commented Nov 3, 2026

Copy link
Copy Markdown
Collaborator

Verdict: PASS at 0473519
Written-by: Reviewer
Attestation: 0473519 role=Reviewer

Ran at this tip:

$ node --test core/test/dialog-focus.test.js
# tests 4  # pass 4  # fail 0
$ node core/scripts/build-client.mjs --check
exit 0

Red first, one swap and one rerun at the same tip: with core/src/client/overlay-hooks.js taken from the base blob d6c19d0bb5f488df9b41191c111e5307a0b7051f (c204f657, live main) and the tip test file unchanged, the same command gives 4 tests / 3 pass / 1 fail — a click that moves focus during the exit animation is not pulled back to the opener. Restored to the tip source it is 4 / 4 / 0, so the new case answers this change and the quiet-close case (opening moves focus into the dialog and closing gives it back to the still-connected opener) stays green on both arms: the guard did not turn into a deletion of #2262.

Checks at 04735194, read 2026-11-03T14:34Z: 16 check runs, 13 success, 0 failure, one judge cancelled (a same-commit rerun superseded it). Required set all success — tests-ok, ubuntu-latest · Node 24 · DSH 0.2.0-rc.2, plugins (tree), plugins (social), plugins (timeline), plugins (workflow), package, merge-gate, enqueue-guard.

What I read at the tip: 8 files, +74/−11. core/src/client/overlay-hooks.js narrows the unmount hand-back to three states — the dialog still holds focus (active === ref.current or contains), nothing holds it (null, body, or a disconnected node), or the opener already holds it — so a click that landed on the page during the 160 ms DIALOG_EXIT_MS exit keeps its focus. core/test/dialog-focus.test.js teaches its fake document to move activeElement on focus() and adds the missing case; the earlier case is kept as it was. docs/subsystems/core-client.md moves the one line arch-docs owns. changelog.d/2270-settings-focus-restore.fixed.md is one line, present and well formed. core/client.js and patches/ds-bot/{0003,0008,files/test/dialog-focus.test.js} carry the same delta; build-client --check exit 0 at the tip says the bundle is what src/client builds, not a hand edit.

Gate state for the enqueue step, so the next node does not re-derive it: head repo is HugoXK/FlyBot, a fork, so the guard's agent-pr / Fixes # / Owner: / Reviewer: body checks are exempt there; the body's Reviewer: Birfy names the one shared identity, which is refused on a repository branch (#1228) and is not read on a fork. The ## Checkpoint block is present and its files are all in this diff. No comment after the tip asks not to merge. Head is frozen once queued — enqueue at 04735194f32b07d33015fb0ba0c26db7ba4a26d3 and do not move it.

Not run — the full npm run verify, the smoke test (no index.js or src/host/ change), and the pack/typecheck/actionlint half of the package job; CI's tests-ok at this tip covers the suites, and this review covers the one file the change touches.

@github-actions github-actions Bot added the pass label Nov 3, 2026
@Birfy
Birfy added this pull request to the merge queue Nov 3, 2026
Merged via the queue into ThinkFlowLab:main with commit 4c58ae3 Nov 3, 2026
15 of 16 checks passed
@Birfy
Birfy deleted the dev/4-p472-settings-focus-restore branch November 3, 2026 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pass risk:T1 Risk tier T1 (client, tests, generated files), set by the controller from the diff

Projects

None yet

Development

Successfully merging this pull request may close these issues.

client: closing Settings pulls focus back 160 ms later, over a click the user made

2 participants