fix(cb-v2): return DOM focus to editor on PropPanel close#381
fix(cb-v2): return DOM focus to editor on PropPanel close#381nick-inkeep merged 3 commits intomainfrom
Conversation
…eep typing) After Escape closed the descriptor PropPanel for img/video/audio, the next keystroke vanished — Radix's FocusScope unmount restored focus to a stale `previouslyFocusedElement` (gear button or detached slash-menu element) instead of the editor body. The user had to click back into the editor before typing worked, breaking the Notion-style "fill prop → Escape → continue typing" loop the placeholder feature was designed to mirror. Override Radix's default close-time focus restore via `onCloseAutoFocus` on `<PopoverContent>`, gated on self-closing-leaf descriptors so containers (Callout/Accordion) keep the trigger-restore default. Runs synchronously inside Radix's setTimeout(0) close-tick, beating the rAF-vs-setTimeout race that the existing `editor.view.focus()` in `handleOpenChange`'s rAF couldn't reliably win. The rAF still owns PM `TextSelection.near` advance; DOM focus now belongs to the new path, so the redundant `editor.view.focus()` call is dropped. Adds PLACEHOLDER-CLOSE-RETURNS-DOM-FOCUS regression test — the canary the existing PLACEHOLDER-CLOSE-ADVANCES-CARET test was rewritten to avoid because typing-after-Escape didn't work. Test types a sentinel after Escape and asserts both `document.activeElement` is inside the editor and the sentinel made it into the PM doc past the img. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 798d0f7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Claude Code is working… I'll analyze this and get back to you. |
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![inkeep-internal-ci[bot]](https://avatars.githubusercontent.com/u/123133689?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Clean, well-scoped fix. The onCloseAutoFocus override is the canonical Radix idiom for this problem (Discussion #915), the gating condition is consistent with two existing sites in the same file, and the E2E regression test directly verifies the user-visible behavior that was broken. The removal of editor.view.focus() from the rAF is safe — by the time the rAF fires, onCloseAutoFocus has already established DOM focus in the preceding setTimeout(0) tick, and PM's dispatch() doesn't need focus to advance the selection.
💭 Consider (3) 💭
💭 1) JsxComponentView.tsx · slash-command-auto-open.e2e.ts Consolidate the Radix focus-restore explanation to one canonical site
Issue: The same Radix previouslyFocusedElement timing mechanism is explained in three places: the handleOpenChange comment block (lines 670-673), the onCloseAutoFocus comment block (lines 963-976), and the test preamble (lines 436-445) — ~28 lines of commentary total for 6 lines of code.
Why: If the focus mechanism changes (e.g. a Radix upgrade alters FocusScope's unmount behavior), all three copies must be updated independently. The onCloseAutoFocus site is the right canonical location since it co-locates with the fix. The other two sites could be shortened to cross-references.
Fix: Keep the detailed 14-line comment at onCloseAutoFocus (lines 963-976). Shorten the handleOpenChange comment (lines 670-673) to something like: // DOM focus is owned by onCloseAutoFocus on <PopoverContent> below; this path only advances PM selection. Shorten the test preamble (lines 440-445) to a behavioral summary + cross-reference.
Refs:
- JsxComponentView.tsx:670-673 — handleOpenChange comment
- JsxComponentView.tsx:963-976 — onCloseAutoFocus comment (canonical)
- slash-command-auto-open.e2e.ts:436-445 — test preamble
💭 2) JsxComponentView.tsx Extract a named predicate for the self-closing-leaf guard
Issue: The condition descriptor.hasChildren && !descriptor.isSelfClosing appears at three sites in this file (lines 596, 677, 978) — evaluated in the "true → skip" sense at the first two, and as a ternary selector at the third. If any site drifts, focus/selection behavior silently breaks for one class of descriptor.
Why: The coupling between these three sites is implicit. A named boolean like const isSelfClosingLeaf = !descriptor.hasChildren || !!descriptor.isSelfClosing (or the inverse) would make the predicate a single source of truth and signal to future editors that all three sites must agree.
Fix: Extract near the other descriptor-derived booleans (around line 540-550):
const isSelfClosingLeaf = !descriptor.hasChildren || !!descriptor.isSelfClosing;Then use isSelfClosingLeaf at lines 596 (if (!isSelfClosingLeaf) return), 677 (if (!isSelfClosingLeaf) return), and 978 (isSelfClosingLeaf ? (e) => { ... } : undefined).
Refs:
💭 3) slash-command-auto-open.e2e.ts:465 Test comment slightly misleading post-fix
Issue: The comment says "Two rAFs absorb Radix's setTimeout(0) focus restore + our rAF caret-advance" but with the fix applied, e.preventDefault() in onCloseAutoFocus prevents Radix's default focus restore. The setTimeout(0) tick now runs our editor.view.focus(), not Radix's previouslyFocusedElement restore.
Why: A future developer reading the test will think Radix's default focus-restore race is still active, when in fact it's been eliminated by the fix this test guards.
Fix: Update to: // Two rAFs settle the onCloseAutoFocus setTimeout(0) tick (editor.view.focus()) + the handleOpenChange rAF (PM selection advance).
Refs:
✅ APPROVE
Summary: Textbook Radix focus-override — right API, right gating, right test. The onCloseAutoFocus + preventDefault idiom eliminates the setTimeout(0) vs rAF race at its source rather than trying to outrun it. Ship it. 🚀
Discarded (7)
| Location | Issue | Reason Discarded |
|---|---|---|
JsxComponentView.tsx:980 |
Click-outside onto non-editor element steals focus back to editor | PR author explicitly acknowledges this tradeoff and defers a document.activeElement guard — reasonable engineering judgment for a rare edge case |
slash-command-auto-open.e2e.ts:465 |
Double-rAF wait is a timing heuristic, not deterministic | LOW confidence; same pattern used in companion test (line 394) without reported issues; setTimeout(0) < rAF ordering is reliable in practice |
slash-command-auto-open.e2e.ts:432 |
Only tests img, not video/audio |
Code path is flag-driven (hasChildren/isSelfClosing), not component-name-driven; registry tests already validate flag shapes for all three |
slash-command-auto-open.e2e.ts |
File not in CI E2E subset (test:e2e) |
Pre-existing CI policy; the file had 4 tests before this PR; adding to CI subset is a separate decision |
slash-command-auto-open.e2e.ts:481 |
page.evaluate inline interfaces duplicate companion test |
Pre-existing pattern; self-contained evaluate blocks are consistent with other E2E tests |
JsxComponentView.tsx:670 |
"this path" pronoun ambiguous — could refer to handleOpenChange |
Preceding clause explicitly names onCloseAutoFocus on <PopoverContent> below as the referent; grammar is clear |
JsxComponentView.tsx:974 |
"any other racing focus calls" is vague | Minor stylistic concern; removing the phrase doesn't change understanding of the timing model |
Reviewers (5)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-tests |
5 | 0 | 1 | 0 | 0 | 0 | 3 |
pr-review-comments |
5 | 0 | 2 | 0 | 0 | 0 | 2 |
pr-review-frontend-react |
1 | 0 | 0 | 0 | 0 | 0 | 1 |
pr-review-precision |
1 | 0 | 0 | 0 | 0 | 0 | 1 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 12 | 0 | 3 | 0 | 0 | 0 | 7 |
Note: pr-review-standards returned zero findings — clean across all checks. Consider items 1 and 3 are merged from overlapping findings across pr-review-comments and pr-review-precision.
C1 — Consolidate Radix focus-restore explanation: handleOpenChange comment
shortened from 11 → 6 lines; test preamble shortened from 10 → 4 lines.
Canonical explanation lives at onCloseAutoFocus on PopoverContent; the
other two sites cross-reference it.
C2 — Extract isSelfClosingLeaf predicate near other descriptor-derived
booleans. The expression `descriptor.hasChildren && !descriptor.isSelfClosing`
appeared at 3 sites (handleBodyClick, handleOpenChange, onCloseAutoFocus
ternary). Single source of truth makes the cross-site coupling explicit;
ternary at onCloseAutoFocus also reads more naturally now ("if self-closing
leaf, override; else default" matches intent).
C3 — Update "Two rAFs" test comment: post-fix, the setTimeout(0) tick
runs OUR onCloseAutoFocus + editor.view.focus(), not Radix's restore (we
preventDefault). Comment updated to reflect that.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Addressed all 3 Consider items in C1 — Consolidate Radix focus-restore explanation. ✅ Accepted. The C2 — Extract C3 — Test comment factually misleading post-fix. ✅ Accepted. The "Two rAFs absorb..." comment is incorrect now that Re: the discarded items — agree with all 7 dismissals. The click-outside-onto-non-editor edge case in particular is documented in the PR body's Key Decisions section as an explicit tradeoff. Quality gate: |
|
Claude finished @nick-inkeep's task in 1m 48s —— View job Re-review of PR #381 complete — APPROVED. All 3 prior Consider items cleanly addressed in |
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Re-review after author addressed all 3 Consider items from the prior review. No new code logic — delta is strictly comment consolidation, predicate extraction, and comment correction.
C1 — Consolidate Radix focus-restore explanation ✅ handleOpenChange comment shortened to 6 lines with a cross-reference to onCloseAutoFocus as the focus owner. Test preamble shortened to 2 lines with a cross-reference. Canonical explanation stays at the onCloseAutoFocus site.
C2 — Extract isSelfClosingLeaf predicate ✅ Single const isSelfClosingLeaf = !descriptor.hasChildren || !!descriptor.isSelfClosing at L325, consumed at all 3 sites (handleBodyClick L602, handleOpenChange L679, onCloseAutoFocus L980). De Morgan's negation of the original inline condition is correct. The ternary flip to isSelfClosingLeaf ? handler : undefined reads more naturally than the original double-negation.
C3 — Test comment factually misleading ✅ Updated to accurately describe what the two rAFs settle post-fix (our editor.view.focus() in the setTimeout(0) tick + PM selection advance in the rAF), rather than the pre-fix Radix race.
No regressions detected in the delta. All prior Discarded items remain correctly dismissed.
✅ APPROVE
Summary: All 3 prior Consider items cleanly addressed — comment DRY-up, predicate extraction, and factual correction. The fix itself was already approved: canonical Radix onCloseAutoFocus override, correct gating, and solid E2E coverage. Ship it! 🚢
Reviewers (0)
No sub-reviewers dispatched for this re-review — the delta is limited to comment/naming improvements addressing prior feedback. The substantive code was fully reviewed in the prior run (5 reviewers, 12 findings processed).
|
CI flake — re-triggered, now green.
Pipeline now CLEAN, mergeable. |
1 participant
Summary
After the descriptor PropPanel popover closed via Escape (e.g.
/image→ fillsrc→ Escape), the next keystroke vanished. The user had to click back into the editor body before typing worked, breaking the Notion-style "fill prop → Escape → continue typing" loop the placeholder feature was designed to mirror. This PR restores DOM focus to the editor body on close for self-closing leaves (img/video/audio), via a gatedonCloseAutoFocusoverride on<PopoverContent>.Key decisions
Why
onCloseAutoFocusoverride instead ofeditor.view.focus()in the rAF.The existing
handleOpenChangerAF callback already callededitor.view.focus(), but it raced Radix's own focus-restoration mechanism. Reading@radix-ui/react-focus-scopesource confirms the unmount path runssetTimeout(focus, 0)aiming atpreviouslyFocusedElement(captured when the popover mounted — typically the gear button or a now-detached slash-menu element). In standard browsers,setTimeout(0)resolves beforerequestAnimationFrame, so Radix focused the stale element after our rAF, and keystrokes routed there.onCloseAutoFocus={(e) => { e.preventDefault(); editor.view.focus(); }}is the canonical Radix idiom (Discussion #915). It runs synchronously inside Radix's setTimeout-deferred close-tick, withpreventDefaultskipping FocusScope's own restore — eliminating the race entirely instead of trying to win it.Why the override is gated on
!hasChildren || isSelfClosing.Container components (Callout, Accordion, Tabs) have an editable content hole that pulls focus naturally on close — Radix's "return focus to trigger" default is the right behavior there. The override only applies to self-closing leaves whose post-close UX is "keep typing in the editor body."
Tradeoff knowingly accepted: click-outside onto a non-editor button.
If the user click-outside-closes the popover by clicking, say, a sidebar button, the override will steal focus back to the editor. The popover is anchored to the inline pill / gear button, so click-outside-to-non-editor is rare in practice. If reported, the fix is a
document.activeElementguard inside the override (deferred — not adding complexity for a hypothetical edge case).The redundant
editor.view.focus()inside thehandleOpenChangerAF is dropped — the rAF still owns PMTextSelection.nearadvancement; DOM focus now belongs to the new path.Verification
Automated regression coverage:
document.activeElementis inside the editor and the sentinel made it into the PM doc past the img.Manual verification recommended:
/image, fillsrc, press Escape, type — keystrokes should appear inline after the img./videoand/audio./callout, edit any prop, press Escape — focus should still return to the callout's content (Radix default, unchanged).🤖 Generated with Claude Code