refactor(dashboard): consolidate ConfirmBtn onto useDialogFocusTrap and unify the synthetic IME claim - #7006
Conversation
…ap and give the synthetic claim one owner ConfirmBtn's Pull+Build popover in DevFleetPage hand-rolled the dialog keyboard contract that useDialogFocusTrap already owns for 8 other dialogs: Escape, the two boundary-Tab branches, the IME claim on each, and focus entry. The popover is now a child component that mounts on open, which is what the hook's mount-keyed focus effects need, and the hand-rolled listener is gone. The hook's focus-RETURN half is wrong for a trigger-anchored popover, so it gained a restoreFocus flag (default unchanged). Its capture is document.activeElement at mount, which on Safari is not a clicked button, and its restore is unconditional where an outside-click dismissal must leave focus where the click put it. ConfirmBtn keeps its own close() restore and passes restoreFocus: false. The trailing booleans became an options object so the third call shape stays readable. Four sites claimed an IME key from a React handler by reaching into claimKey(e.nativeEvent) and then remembering e.stopPropagation() themselves, because the native call cannot set React's propagation flag. claimSyntheticKey on ImeLatch owns both halves; useImeGuard().claimKey delegates to it rather than re-spelling the split, and a new ratchet rule rejects the hand-spelled pair. Closes #5542
github-actions
Bot
added
the
readiness: checking
UX Review (Fable 5) — ✅ PASSUX-level review of All evidence reviewed: the diff is a focus-trap consolidation with no new strings or layout; the screenshot confirms the popover renders unchanged; keyboard behavior (initial focus on Cancel, Tab ring, Escape-to-trigger, outside-click leaving focus at the click target) traces through the shared hook with parity to the removed hand-rolled trap. UX-Verdict: PASS Pure focus-trap re-hosting: every keyboard and dismissal behavior a user could feel is preserved verbatim, and Tab-from-outside now re-enters the popover instead of escaping it. [UX-REVIEWED] 1ad3f98 |
Design Review (Fable 5) — ✅ PASSDesign-level review of Design-Verdict: PASS Consolidates two hand-rolled keyboard contracts onto their existing shared owners, with defaults preserved, mutation-verified pins, and a ratchet closing the regression path. Suggestions
[DESIGN-REVIEWED] 1ad3f98 |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
First Principles Review (Fable 5) — ✅ PASSPremise-level review of All verification is done. The counts hold: zero hand-spelled First-Principles-Verdict: PASS Two hand-spelled duplicates of shared keyboard contracts are deleted at cause level, and a ratchet makes each unwritable again; every added surface has counted consumers. What this change shipsIntent: stop the dialog/IME keyboard contract from being maintained in per-site copies that drift — a FIX (of duplication), not an ADDITION.
WatchThe broader cause — the dialog contract spelled by hand — has counted siblings this change defers: ~15 core [FIRST-PRINCIPLES-REVIEWED] 1ad3f98 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
|
Dispositions for the two advisory concerns on Design Review - "file the deferred family-wide restore-policy follow-up so the flag doesn't quietly become the permanent answer." Accepted, and correctly aimed: the 8 default- Captured in this repo's local backlog as First Principles - "~15 core Deliberately not folded in: those surfaces need focus ENTRY and a Tab ring they have never had, which is behavior change on roughly 15 dialogs. This PR deletes duplicate implementations of a contract; adding the contract where it was never present is a different change with a different blast radius, and mixing them would make one review answer for both. One correction to the framing, not to the finding: this PR does not defer that class so much as never include it. Issue 5542 scoped itself to the seventh COPY of the trap plus the synthetic-claim seam, whereas the untrapped surfaces are the population the trap was never applied to. That is why they need their own scope rather than a line in this one. |
github-actions
Bot
added
readiness: action required
bolichen97
left a comment
bolichen97
left a comment
There was a problem hiding this comment.
Full-diff maintainer review passed: change matches its stated scope, no regressions or trust-boundary weakening found, checks green and no outstanding change requests. Approving.
github-actions
Bot
removed
the
readiness: passed
2 participants
1. What is the problem?
Two shapes that the shared keyboard seams were built to eliminate are still spelled by hand.
ConfirmBtn's popover re-implements the dialog contract.
DevFleetPage's Pull+Build confirm popover is a plainrole="dialog"two-button surface, anduseDialogFocusTrap-- used by 8 dialogs -- already owns Escape, the boundary-Tab ring, the IME claim on each, and focus entry. ConfirmBtn had its own document-capture listener for all of it. PR #5533 latch-guarded that hand-rolled trap rather than deleting it, because a direct hook call does not work here: the hook's focus effects key on component MOUNT and every other call site mounts when its dialog opens, whereas ConfirmBtn is a persistent component whose popover is conditionally rendered -- the effect would run once at page load against a null container.Four React handlers hand-spell the synthetic half of an IME claim.
ImeLatch.claimKeytakes a NATIVE event, so a ReactonKeyDownclaiming through it must reach into.nativeEventand then remembere.stopPropagation()itself, because the native call cannot set React's own propagation flag (React walks that flag when dispatching to component ancestors).ArtifactPanel,MarkdownPanel,ChatSidebar's column popover and -- not listed in the issue but the same pair --Modal's key isolation each carried that two-part spelling.2. Why this issue matters to the user
A duplicated dialog contract drifts, and it drifts silently. The ConfirmBtn trap is the seventh copy of a shape whose IME defect (#5410, #5427) had to be fixed once per copy; the next fix to the shared hook would not reach this popover. For a CJK user the specific failure is a Tab or Escape composed into a dialog that steals focus and aborts the composition.
The synthetic pair is the same risk one level down: a later edit that drops the caller-side
stopPropagation()leaves a declined key reaching an ancestor's keyboard handler, and nothing catches it -- no test asserts the second half at three of the four sites.3. How our fix solves it
ConfirmBtn -> the shared trap. The popover is now
ConfirmPopover, a component that mounts on open, so the hook's mount-keyed focus effects run at the right moment against a real container. The hand-rolled listener, its twoTabbranches, its Escape branch, itsuseDocumentImeLatchcall and thecancelRef/confirmRefboundary refs are all deleted -- the ring is derived from DOM order by the hook. What stays with the host is what is genuinely the host's: WHERE the popover sits (portal + flip geometry, outside-click and scroll dismissal) and WHAT dismissal means.The focus-restore decision the issue asked for, made explicitly. The hook's focus-RETURN half is wrong for a trigger-anchored popover, in two independent ways: its capture is
document.activeElementat mount -- which on Safari is NOT a clicked button, so the capture lands on<body>and the unmount-ordered restore runs AFTERclose()'strigger.focus()and blurs it -- and its restore is unconditional, where an outside-click dismissal must leave focus where the browser routed it (#2533). So the hook gainedrestoreFocus(default unchanged, all 8 existing dialogs keep today's behavior) and ConfirmBtn passesfalse, keeping its explicitclose()restore as the single owner. This is not a stylistic choice: taking the hook's default instead breaks three DevFleetPage tests, two of which predate this PR (see section 4).The trailing boolean flags became an options object in the same change, because the alternative spelling at the ConfirmBtn call site would have been
useDialogFocusTrap(popRef, close, true, true, false). Three call sites changed shape ({ handleEscape: false }x2,{ enabled: !paused }); five read identically.One owner for the synthetic claim.
claimSyntheticKey(e)onImeLatchdoes the native claim and stops React's flag, so the pair has one home.useImeGuard().claimKeynow delegates to it instead of carrying a second copy of the same two lines, and the four call sites becameif (!latch.claimSyntheticKey(e)) return. A new ratchet rule rejects anyclaimKey(...nativeEvent...)outside the hook, so the pair cannot come back; the existing boundary-Tab rule learned the new spelling as an equally valid claim.4. What tests we did
Targeted suites only (this host cannot take a full run); CI runs the rest.
DevFleetPage.test.tsx-- the 9 tests that pin the popover pass unchanged (portal placement, flip up/down, outside-click and Escape close, Start still fires the request, focus containment + restore after Cancel, and the three IME decline tests from fix(dashboard): ratchet boundary-Tab traps onto the shared IME latch #5533). That the pins survive untouched is the main evidence the conversion is behavior-preserving. 94 tests green.restoreFocusin the hook reddens the new Safari-shape test. (b) Dropping{ restoreFocus: false }at the ConfirmBtn call site reddens three tests, including the two pre-existing ones that assert focus returns to the trigger -- that is the measurement behind the decision in section 3. (c) Emptying the hook's focusable ring reddens the popover's Tab-containment test, proving the new path is genuinely exercised rather than passing vacuously. (d) Reverting one call site to the hand-spelled pair reddens the new ratchet rule at that exact line.useDialogFocusTrap.restoreFocus.test.tsx(3 tests): focus still enters when the return half is off; the default still restores; the host's restore survives in the Safari shape.claimSyntheticKeytests on the shared latch: a declined key consumes the native event AND stops the synthetic flag; an accepted key is left entirely alone.Modal(x3 suites),DialogKeyboardIsolation,CommandBarOverlay,SideSheet,useMenuKeyboard,ConfirmDialog,ArtifactPanelCoverage,ArtifactPanel.copyIcon,AgentImportFlow,PrivacyChapter,AgentSelector.dialog,ChatSidebar.boardColumnTagFilter,useDialogFocusTrap.imeGuard,useImeGuard.documentLatch,ImeEnterClaimRatchet-- all green.tsc --noEmitclean,eslint0 errors on every touched file with the warning count forDevFleetPage.tsxunchanged at 1 (pre-existing, verified by linting the stashed base),i18n:checkgreen withI18N_BASE_REFset the way CI sets it.aria-modal=true, portaled to<body>,data-placement=down, the shared trap put focus on Cancel, Escape dismissed the dialog, and focus returned to the trigger.5. Any other suggestions on the work
ImeLatchitself rather than auseDocumentImeLatch-adjacent wrapper, because one seam then serves both latch flavours and letsuseImeGuard().claimKeystop carrying its own copy. The ratchet rule it needs is a rejection of the old spelling, which is independent of ImeEnterClaimRatchet has no rule for boundary-Tab traps; DevFleetPage carries an unguarded seventh instance #5475's boundary-Tab vocabulary, so it lands here rather than in that lane.restoreFocusdefault keeps all 8 existing dialogs exactly as they are. If the maintainers would rather settle the Safari policy for the whole family (make the explicit trigger-anchored restore the norm and have the hook take a target ref), that is a bigger, separate change and this flag is the seam it would grow from.Closes #5542