From 367e8228748670a790b93c8d94cad7804d4a9bc9 Mon Sep 17 00:00:00 2001 From: Matthew Valancy Date: Tue, 27 Oct 2026 14:11:21 -0700 Subject: [PATCH] Preserve chosen focus when initial dialog autofocus is delayed --- ...ed-autofocus-must-preserve-chosen-focus.md | 45 +++++++ .agents/issues/INDEX.md | 1 + .../src/components/QuickEditSurface.test.tsx | 117 ++++++++++++++++++ packages/web/src/hooks/useModalA11y.ts | 3 + 4 files changed, 166 insertions(+) create mode 100644 .agents/inbox/2026-10-27-delayed-autofocus-must-preserve-chosen-focus.md create mode 100644 packages/web/src/components/QuickEditSurface.test.tsx diff --git a/.agents/inbox/2026-10-27-delayed-autofocus-must-preserve-chosen-focus.md b/.agents/inbox/2026-10-27-delayed-autofocus-must-preserve-chosen-focus.md new file mode 100644 index 00000000..1545c75e --- /dev/null +++ b/.agents/inbox/2026-10-27-delayed-autofocus-must-preserve-chosen-focus.md @@ -0,0 +1,45 @@ +# Delayed autofocus must preserve focus already chosen by the user + +On 2026-10-27, CAPTCHA PR #195 [CI 40579557476](https://github.com/GraphDone/GraphDone-Core/actions/runs/40579557476) +failed `LARGE-01` because the status-note Add button remained disabled. Its +retained trace shows the note input focused at 746363ms, then the note text +appended to the title at 746402ms. The note input stayed empty. Token PR #197 +[CI 40580018905](https://github.com/GraphDone/GraphDone-Core/actions/runs/40580018905) +also retained a `TEAM-01` disabled-Add failure before the overall new-user +process reached its 900-second limit. Its incomplete JSON report establishes +no passing cases. Related incident: [#200](https://github.com/GraphDone/GraphDone-Core/issues/200). + +## Cause and corrective rule + +`useModalA11y` defers initial focus until an animation frame so a portaled +dialog can mount. That callback unconditionally selected the first form field, +even when someone had already selected the status-note input. Delayed initial +focus must yield to a field already focused in the dialog and to deliberate +focus changes outside it. It may move focus when the opening trigger is still +focused or focus has fallen back to the body. A detached dialog must not take +focus. Existing keyboard trapping, cancellation and close behavior remain. + +## Regression and evidence + +Using Node 22.18.0/npm 10.8.2 in the owned `core-editor-focus-20261027` +worktree, `npm test --workspace @graphdone/web -- +src/components/QuickEditSurface.test.tsx` ran the actual portaled surface and +node editor with a controlled animation-frame queue. Against dev +`97dc8849db8f257ffa88ab790c4452a8022eeaa6`, both chosen-focus cases failed +and four initial-focus/keyboard/cleanup controls passed. With the hook guard, +all six passed. The note case asserts the title stays intact and the only +commit is note metadata. No sleeps, retries, longer timeouts or focus-wait +workarounds were added to the application flow. + +Initial lint rejected the DOM `FrameRequestCallback` type under the existing +no-undef configuration; the fixture uses an explicit local callback type. +Final browser, build, types, lint, full web-unit and harness evidence belongs +in the PR discussion. Retain the failed CI and negative-control results. + +## Limits + +The deterministic test controls frame order in jsdom; actual persona browser +workflows separately cover persistence and desktop/phone interaction. This +does not add background inertness, change the existing Tab trap contract, +prove hosted parity, or turn interrupted CI evidence into a pass. Private +traces can contain authentication state and must not be copied into the repo. diff --git a/.agents/issues/INDEX.md b/.agents/issues/INDEX.md index fafada95..5476627d 100644 --- a/.agents/issues/INDEX.md +++ b/.agents/issues/INDEX.md @@ -2,6 +2,7 @@ | Symptom | Canonical lesson and regression | | --- | --- | +| Status-note text lands in the title and Add remains disabled after quick edit opens | [Delayed autofocus must preserve chosen focus](../inbox/2026-10-27-delayed-autofocus-must-preserve-chosen-focus.md); `QuickEditSurface.test.tsx` controls the pending frame | | Minimap zoom or overview inspection is overwritten while a newly loaded graph settles | [Explicit camera input must stop automatic framing](../inbox/2026-10-27-explicit-camera-input-must-stop-auto-fit.md); timer-controlled `semantic-lod.spec.ts` negative control | | Dense-graph browser case exhausts its timeout after slow API fixture creation; cleanup errors disappear | [Bound fixture setup separately and fail on cleanup errors](../inbox/2026-10-27-cold-lod-fixtures-need-separate-budgets.md); `tests/e2e/graph/semantic-lod.spec.ts` | | GitHub connector can read a repo but PR creation returns `Resource not accessible by integration` | [Client-specific write permissions](../inbox/2026-10-27-github-connector-write-permissions.md); verify existing authorized access without changing credentials or policy | diff --git a/packages/web/src/components/QuickEditSurface.test.tsx b/packages/web/src/components/QuickEditSurface.test.tsx new file mode 100644 index 00000000..96efa045 --- /dev/null +++ b/packages/web/src/components/QuickEditSurface.test.tsx @@ -0,0 +1,117 @@ +import { act, cleanup, fireEvent, render, screen } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { QuickEditSurface } from './QuickEditSurface'; +import { NodeQuickEdit } from './NodeQuickEdit'; + +type PaintCallback = (timestamp: number) => void; +let frames: Map; +let frameId = 0; + +beforeEach(() => { + frames = new Map(); + vi.stubGlobal('requestAnimationFrame', (callback: PaintCallback) => { + const id = ++frameId; + frames.set(id, callback); + return id; + }); + vi.stubGlobal('cancelAnimationFrame', (id: number) => frames.delete(id)); + vi.stubGlobal('ResizeObserver', class { + observe() {} + disconnect() {} + }); + vi.stubGlobal('matchMedia', vi.fn(() => ({ matches: false }))); + vi.spyOn(HTMLElement.prototype, 'getClientRects').mockReturnValue([ + { x: 0, y: 0, width: 100, height: 40, top: 0, left: 0, right: 100, bottom: 40 }, + ] as unknown as DOMRectList); +}); + +afterEach(() => { + cleanup(); + vi.restoreAllMocks(); + vi.unstubAllGlobals(); +}); + +function paint() { + const pending = [...frames.values()]; + frames.clear(); + act(() => pending.forEach(callback => callback(16))); +} + +function editor() { + render(); + const trigger = screen.getByRole('button', { name: 'Open editor' }); + trigger.focus(); + const onCommit = vi.fn(); + const view = render( + + + + ); + return { ...view, trigger, onCommit }; +} + +describe('quick editor initial focus', () => { + it('keeps a chosen status-note field focused when deferred autofocus runs', () => { + const { onCommit } = editor(); + const note = screen.getByTestId('quick-note-input'); + note.focus(); + paint(); + expect(note).toHaveFocus(); + fireEvent.change(document.activeElement!, { target: { value: 'Ready for review' } }); + expect(screen.getByTestId('quick-title')).toHaveValue('Original title'); + expect(screen.getByTestId('quick-note-add')).toBeEnabled(); + fireEvent.click(screen.getByTestId('quick-note-add')); + expect(onCommit).toHaveBeenCalledTimes(1); + expect(onCommit.mock.calls[0][0].update).not.toHaveProperty('title'); + expect(onCommit.mock.calls[0][0].update.metadata).toContain('Ready for review'); + }); + + it('focuses the first field when focus is still on the opening trigger', () => { + editor(); + paint(); + expect(screen.getByTestId('quick-title')).toHaveFocus(); + }); + + it('focuses the first field when focus has fallen back to the body', () => { + const { trigger } = editor(); + trigger.blur(); + paint(); + expect(screen.getByTestId('quick-title')).toHaveFocus(); + }); + + it('preserves focus deliberately moved outside the editor', () => { + editor(); + render(); + const action = screen.getByRole('button', { name: 'Another action' }); + action.focus(); + paint(); + expect(action).toHaveFocus(); + }); + + it('keeps keyboard focus cycling through the editor', () => { + editor(); + paint(); + const surface = screen.getByTestId('quick-edit-surface'); + const first = screen.getByRole('button', { name: 'Close quick edit' }); + const fields = surface.querySelectorAll('button:not([disabled]),input,textarea'); + const last = fields[fields.length - 1]; + first.focus(); + fireEvent.keyDown(first, { key: 'Tab', shiftKey: true }); + expect(last).toHaveFocus(); + fireEvent.keyDown(last, { key: 'Tab' }); + expect(first).toHaveFocus(); + }); + + it('cancels pending autofocus when the editor unmounts', () => { + const { unmount, trigger } = editor(); + expect(frames.size).toBe(1); + unmount(); + expect(frames.size).toBe(0); + paint(); + expect(trigger).toHaveFocus(); + }); +}); diff --git a/packages/web/src/hooks/useModalA11y.ts b/packages/web/src/hooks/useModalA11y.ts index 37ed9797..eb1b66e0 100644 --- a/packages/web/src/hooks/useModalA11y.ts +++ b/packages/web/src/hooks/useModalA11y.ts @@ -86,6 +86,9 @@ export function useModalA11y(ref: RefObject, opts: ModalA11yOptions // Defer past paint so portaled fields/buttons exist before we grab focus, // and prefer the first real form field over an icon-only Close button. raf = requestAnimationFrame(() => { + const active = document.activeElement; + if (!el.isConnected || el.contains(active) || + (active && active !== document.body && active !== previouslyFocused)) return; const items = focusables(); const target = items.find(isField) || items[0]; if (target) {