From e996a26892cc4febd50b97ea19e14f56b907df7c Mon Sep 17 00:00:00 2001 From: AstroHan Date: Thu, 3 Sep 2026 10:53:53 +0800 Subject: [PATCH] fix(ui): stop the restored draft caret from taking focus MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `caretToContentEnd` places a collapsed selection at the end of the composer so a draft rewritten while the editor was blurred does not leave the caret at offset 0. A selection inside a `contenteditable` is never only a caret: the browser focuses the element to carry it — measured in the shipping runtime, from a focused button exactly as from `body` — and sequential focus navigation then resumes from the selection rather than from the top of the document. So the restore claimed focus nobody directed at it, on two paths. A cold start restores the draft tens of milliseconds in, with no `focus()` call to explain it: focus drifts `body` → 消息输入框 and the walk to the skip link costs 9 presses against a 10-press budget. Activating a session row swaps the draft the same way, and the composer takes focus out from under the row the user just activated. Hold the caret whenever the editor is not focused and land it on the editor's next real focus, which is the first moment the offset is the only thing being decided. A pointer press places the caret itself, so it drops the claim rather than being overruled by it. Same measurement with the fix: focus stays on `body`, 3 presses, and the session row keeps focus. A held caret is suspended across the Skill-chip redraw rather than left armed. `redrawSkillTokens` drives `insertToken` through the document selection, and its first range focuses the editor — which would otherwise fire the focus lander onto the range the redraw is holding, collapsing it to the end so the chip landed at the end and its source text stayed in the draft. The redraw collects the caret itself on success, so the claim is settled there and handed back untouched on a pass that redrew nothing. That removes the reason #4577 wrapped the skip-link walk in a re-parking `toPass` retry, so the retry goes with it and the walk is a one-shot park and budgeted walk again. Installing the replacement and removing the fence are one change. Refs #4577 Generated-by: Claude Code --- .../e2e/accessibility-coverage.spec.ts | 64 +++-- .../composer-draft-caret-focus.test.tsx | 265 ++++++++++++++++++ packages/ui/src/composer.tsx | 63 ++++- 3 files changed, 369 insertions(+), 23 deletions(-) create mode 100644 packages/ui/src/__tests__/composer-draft-caret-focus.test.tsx diff --git a/apps/desktop/e2e/accessibility-coverage.spec.ts b/apps/desktop/e2e/accessibility-coverage.spec.ts index 41ed8e69ac..8d55ab9c85 100644 --- a/apps/desktop/e2e/accessibility-coverage.spec.ts +++ b/apps/desktop/e2e/accessibility-coverage.spec.ts @@ -19,7 +19,7 @@ import { FAKE_HOLD_OPEN_PROMPT } from '@maka/runtime/test-only/fake-backend'; import type { CDPSession, Locator, Page } from '@playwright/test'; -import { awaitSendReady, expect, test, COMPOSER_INPUT } from './fixtures'; +import { awaitSendReady, ensureSidebarExpanded, expect, test, COMPOSER_INPUT } from './fixtures'; import { auditAxTree } from '../../../scripts/ax-tree-audit.mjs'; import { groupedNav } from '../src/renderer/settings/settings-nav'; @@ -52,27 +52,13 @@ async function tabTo(page: Page, target: Locator, label: string, limit = 30): Pr ).toBe(true); } -/** - * Walk to the skip link from the document start, taking the start back if a - * cold start moves it. - * - * Parking focus on `body` is not a one-shot the renderer respects: the composer - * restores its draft caret with `getSelection().addRange(...)`, and a range set - * inside a `contenteditable` focuses it — so once per cold start, tens of - * milliseconds after the park and with no `focus()` call to fence on, focus - * lands in the composer. A walk that starts there has to run out the tab ring - * and wrap around, which is over budget. The restore fires once, so re-park and - * walk again rather than widening the budget — the budget is the assertion. - */ async function enterMainFromSkipLink(page: Page): Promise { + await page.evaluate(() => { + document.body.tabIndex = -1; + document.body.focus(); + }); const skipLink = page.getByRole('link', { name: '跳到主要内容' }); - await expect(async () => { - await page.evaluate(() => { - document.body.tabIndex = -1; - document.body.focus(); - }); - await tabTo(page, skipLink, 'skip link', 10); - }).toPass({ timeout: 30_000 }); + await tabTo(page, skipLink, 'skip link', 10); await page.keyboard.press('Enter'); await expect(page.getByRole('main')).toBeFocused(); await page.evaluate(() => document.body.removeAttribute('tabindex')); @@ -322,3 +308,41 @@ test('composer and workbar entry points expose named actionable controls', async } } }); + +/** + * Restoring a draft is not a reason to move focus. The composer places the + * restored caret with a selection, and a selection inside a `contenteditable` + * focuses it whatever held focus before — so before the caret was held back, + * activating a session row took focus out from under the keyboard user who + * activated it. Asserted in a real browser because that is where the focus + * side effect lives; the unit harness models it and cannot observe it. + */ +test('activating a session row with an unsent draft keeps focus on the row', async ({ + window: page, +}) => { + const composer = page.locator(COMPOSER_INPUT); + const prompt = 'session for the draft focus contract'; + await composer.fill(prompt); + await awaitSendReady(page); + await composer.press('Enter'); + await expect(page.getByText(`Fake backend received: ${prompt}`)).toBeVisible({ + timeout: 30_000, + }); + + await composer.click(); + // Plain text, no Skill token: this contract is about the caret restore, and a + // token redraw writes the same selection for a reason of its own. + await page.keyboard.insertText('an unsent draft'); + await expect(composer).toHaveText('an unsent draft'); + + await ensureSidebarExpanded(page); + const sidebar = page.getByRole('navigation', { name: '任务列表' }); + await sidebar.getByRole('button', { name: '新任务', exact: true }).click(); + await expect(composer).toHaveText(''); + + const sessionRow = sidebar.locator('[data-session-id]').first(); + await sessionRow.click(); + await expect(composer).toHaveText('an unsent draft'); + await expect(composer).not.toBeFocused(); + await expect(sessionRow.locator(':focus')).toHaveCount(1); +}); diff --git a/packages/ui/src/__tests__/composer-draft-caret-focus.test.tsx b/packages/ui/src/__tests__/composer-draft-caret-focus.test.tsx new file mode 100644 index 0000000000..b9c76193a5 --- /dev/null +++ b/packages/ui/src/__tests__/composer-draft-caret-focus.test.tsx @@ -0,0 +1,265 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/** + * Who owns the composer's caret, and who owns focus. + * + * A restored draft owes the caret the end of its content, or the next keystroke + * prepends to it. But the only way to place a caret is a selection, and a + * selection inside a `contenteditable` focuses that element — whoever held focus + * before — and moves the point sequential focus navigation resumes from. So the + * restore claimed focus nobody directed at it: on a cold start, past the skip + * link with no `focus()` call to explain it, so Tab from the document start + * began in the composer; and on a session swap, out from under the sidebar row + * the user had just activated. Both are pinned here. + * + * The caret is therefore owed rather than placed whenever the editor is not + * focused, and lands on its next real focus — the first moment the offset is the + * only thing being decided. A pointer press places the caret itself and drops + * the claim. + * + * linkedom carries no selection, no focus and no `Range` motion, so the harness + * models what the composer uses: `createRange` records where a caret was aimed, + * `getSelection` records the live selection and reproduces the focus a selection + * takes, and `focus()` sets `document.activeElement` and dispatches the `focusin` + * a browser would. It also lowercases `contentEditable` on the way into the DOM, because linkedom stores + * attribute names verbatim where HTML folds them — without that the composer's + * own `[contenteditable="true"]` lookup misses its editor here and nowhere else. + */ + +import assert from 'node:assert/strict'; +import { afterEach, test } from 'node:test'; +import { act } from 'react'; +import { createRoot } from 'react-dom/client'; +import { parseHTML } from 'linkedom'; +import { Composer } from '../composer.js'; +import { LocaleProvider } from '../locale-context.js'; + +const originalGlobals = { + document: globalThis.document, + matchMedia: globalThis.matchMedia, + requestAnimationFrame: globalThis.requestAnimationFrame, + cancelAnimationFrame: globalThis.cancelAnimationFrame, + window: globalThis.window, +}; +const originalActEnvironment = (globalThis as typeof globalThis & { + IS_REACT_ACT_ENVIRONMENT?: boolean; +}).IS_REACT_ACT_ENVIRONMENT; +const mountedRoots: ReturnType[] = []; +const restoreDom: (() => void)[] = []; + +afterEach(async () => { + for (const root of mountedRoots.splice(0)) await act(() => root.unmount()); + for (const restore of restoreDom.splice(0)) restore(); + Object.assign(globalThis, { + ...originalGlobals, + IS_REACT_ACT_ENVIRONMENT: originalActEnvironment, + }); +}); + +function computedStyle(): CSSStyleDeclaration { + return { + direction: 'ltr', + writingMode: 'horizontal-tb', + getPropertyValue: () => '', + } as unknown as CSSStyleDeclaration; +} + +/** Where a caret was aimed: `selectNodeContents` then `collapse` and no more. */ +interface AimedRange { + container: Node | null; + offset: number; + collapsed: boolean; +} + +function harness() { + const { document, window } = parseHTML('
'); + window.getComputedStyle = () => computedStyle(); + const setAttribute = window.Element.prototype.setAttribute; + window.Element.prototype.setAttribute = function normalized(name: string, value: string) { + return setAttribute.call(this, name === 'contentEditable' ? 'contenteditable' : name, value); + }; + restoreDom.push(() => { + window.Element.prototype.setAttribute = setAttribute; + }); + document.createRange = () => { + const range: AimedRange & { + selectNodeContents(node: Node): void; + collapse(toStart: boolean): void; + } = { + container: null, + offset: 0, + collapsed: false, + selectNodeContents(node) { + range.container = node; + range.offset = node.childNodes.length; + }, + collapse(toStart) { + range.offset = toStart ? 0 : range.offset; + range.collapsed = true; + }, + }; + return range as unknown as Range; + }; + let active: Element | null = null; + const selected: AimedRange[] = []; + const selection = { + get anchorNode(): Node | null { + return selected.at(-1)?.container ?? null; + }, + removeAllRanges() { + selected.length = 0; + }, + addRange(range: Range) { + const aimed = range as unknown as AimedRange; + selected.push(aimed); + // The whole point: a selection inside a `contenteditable` focuses it, + // whoever held focus before. Without this the harness would let a caret + // placed on a blurred editor look free. + const container = aimed.container as Element & { closest?: Element['closest'] }; + active = container?.closest?.('[contenteditable="true"]') ?? active; + }, + }; + document.getSelection = () => selection as unknown as Selection; + window.getSelection = () => selection as unknown as Selection; + Object.defineProperty(document, 'activeElement', { configurable: true, get: () => active }); + Object.assign(globalThis, { + document, + window, + matchMedia: () => ({ matches: false, addEventListener() {}, removeEventListener() {} }), + requestAnimationFrame: () => 1, + cancelAnimationFrame() {}, + IS_REACT_ACT_ENVIRONMENT: true, + }); + const container = document.querySelector('#root'); + assert.ok(container); + const root = createRoot(container); + mountedRoots.push(root); + return { + /** The live selection: what `removeAllRanges` cleared and `addRange` added. */ + selected: selected as readonly AimedRange[], + editable() { + const editable = document.querySelector('[contenteditable="true"]'); + assert.ok(editable, 'the composer rendered no editable node'); + return editable as unknown as HTMLElement; + }, + /** A focusable outside the composer — the sidebar row that swaps the draft. */ + outside() { + const existing = document.querySelector('#outside'); + if (existing) return existing as unknown as HTMLElement; + const button = document.createElement('button'); + button.id = 'outside'; + document.documentElement.appendChild(button); + return button as unknown as HTMLElement; + }, + focused: () => active, + /** Focus an element the way a browser does: activate it, then announce it. */ + async focus(element: HTMLElement) { + active = element as unknown as Element; + await act(() => { + element.dispatchEvent(new window.Event('focusin', { bubbles: true })); + }); + }, + async pointerDown(element: HTMLElement) { + await act(() => { + element.dispatchEvent(new window.Event('pointerdown', { bubbles: true })); + }); + }, + async render(props: Parameters[0]) { + await act(() => { + root.render( + + + , + ); + }); + }, + }; +} + +const base = { + onSend: () => undefined, + onStop: () => undefined, +}; + +/** A host that hands the named session back an unsent draft, as a cold start does. */ +function withDraft(key: string, draft: string) { + return { + ...base, + draftPersistence: { + read: (draftKey: string | undefined) => (draftKey === key ? draft : ''), + write: () => undefined, + }, + }; +} + +/** The end of the content, which is where every restored draft owes its caret. */ +function assertCaretAtEnd(selected: readonly AimedRange[], editable: HTMLElement): void { + const caret = selected.at(-1); + assert.ok(caret, 'the composer placed no caret'); + assert.equal(caret.collapsed, true, 'the caret must be a collapsed selection'); + assert.equal(caret.container, editable); + assert.equal(caret.offset, editable.childNodes.length); +} + +test('a draft restored while nothing holds focus places no selection', async () => { + const dom = harness(); + await dom.render({ ...withDraft('session-a', 'restored draft'), draftKey: 'session-a' }); + assert.equal(dom.editable().textContent, 'restored draft'); + assert.equal( + dom.selected.length, + 0, + 'the restored caret took a selection inside the contenteditable, which focuses it and moves ' + + 'the point Tab resumes from off the top of the document', + ); + assert.equal(dom.focused(), null, 'the restored caret focused the composer'); +}); + +test('the owed caret lands at the end of the draft on the next focus', async () => { + const dom = harness(); + await dom.render({ ...withDraft('session-a', 'restored draft'), draftKey: 'session-a' }); + await dom.focus(dom.editable()); + assertCaretAtEnd(dom.selected, dom.editable()); +}); + +test('a pointer press into the composer drops the owed caret', async () => { + const dom = harness(); + await dom.render({ ...withDraft('session-a', 'restored draft'), draftKey: 'session-a' }); + await dom.pointerDown(dom.editable()); + await dom.focus(dom.editable()); + assert.equal( + dom.selected.length, + 0, + 'a click places the caret where it lands; the owed caret must not overrule it', + ); +}); + +test('a session swap leaves focus on the row that caused it', async () => { + const dom = harness(); + const props = withDraft('session-b', 'other draft'); + await dom.render({ ...props, draftKey: 'session-a' }); + await dom.focus(dom.outside()); + await dom.render({ ...props, draftKey: 'session-b' }); + assert.equal(dom.editable().textContent, 'other draft'); + assert.equal( + dom.focused(), + dom.outside(), + 'the restored caret took focus out from under the row the user activated', + ); +}); diff --git a/packages/ui/src/composer.tsx b/packages/ui/src/composer.tsx index b17310da8a..fe19b9ea86 100644 --- a/packages/ui/src/composer.tsx +++ b/packages/ui/src/composer.tsx @@ -554,6 +554,8 @@ export const Composer = forwardRef< * identity so neither hook re-runs an effect when the draft changes. */ const caretToEndRef = useRef(false); + /** A caret-to-end owed to an editor that was not focused when it came due. */ + const caretPendingRef = useRef(false); const redrawPendingRef = useRef(false); const textPortRef = useRef(null); if (!textPortRef.current) { @@ -578,10 +580,27 @@ export const Composer = forwardRef< * say) then landed the caret at offset 0, so typing prepended to the restored * draft. Collapse to the end here when the editor holds no selection of its * own, which is what the retired `focusTextInputAtEnd` did unconditionally. + * + * Only on a focused editor, though. A selection inside a `contenteditable` is + * never only a caret: the browser focuses the element to carry it, whatever + * held focus before — measured in the shipping runtime, a selection placed + * here takes focus from a focused button exactly as it takes it from `body` — + * and sequential focus navigation then resumes from the selection rather than + * from the top of the document. So a restored draft claimed focus nobody + * directed at it: on a cold start, tens of milliseconds in, past the skip link + * and with no `focus()` call to explain it; and on a session swap, out from + * under the sidebar row the user had just activated. Hold the caret while the + * editor is not focused and land it on the editor's next real focus, which is + * the first moment the offset is the only thing being decided. */ function caretToContentEnd() { const editable = editableNode(); if (!editable) return; + if (document.activeElement !== editable) { + caretPendingRef.current = true; + return; + } + caretPendingRef.current = false; const selection = document.getSelection(); const range = document.createRange(); range.selectNodeContents(editable); @@ -596,6 +615,30 @@ export const Composer = forwardRef< if (!editable || (selection?.anchorNode && editable.contains(selection.anchorNode))) return; caretToContentEnd(); } + /** + * Settle a held caret when focus reaches the editor for real. On the component + * root, like the other native listeners here: `focusin` and `pointerdown` + * bubble, and a disabled composer renders no editable to look up at mount. + * + * A pointer press places the caret itself and is the more specific intent, so + * it drops the claim rather than being overruled by it. + */ + useEffect(() => { + const root = inputRootRef.current; + if (!root) return undefined; + const land = () => { + if (caretPendingRef.current) caretToContentEnd(); + }; + const drop = () => { + caretPendingRef.current = false; + }; + root.addEventListener('focusin', land); + root.addEventListener('pointerdown', drop); + return () => { + root.removeEventListener('focusin', land); + root.removeEventListener('pointerdown', drop); + }; + }, []); /** * The ＋ menu's Skills entry opens the same `/` menu the keyboard opens: it * types the trigger for the user. There is no second Skill surface to keep in @@ -727,13 +770,27 @@ export const Composer = forwardRef< * The redraw gets the same treatment for the same reason, and can land a * render later than the write that owed it: `insertToken` parks the selection * after the last chip it wrote, so the caret has to be collected again. + * + * A held caret is suspended across the redraw rather than left armed. The + * redraw drives `insertToken` through the document selection, and its first + * range focuses the editor — which would otherwise fire the focus lander onto + * the very range the redraw is holding, collapsing it to the end so the chip + * landed at the end and its source text stayed in the draft. The redraw ends + * by collecting the caret itself, so on success the claim is settled; on a + * pass that redrew nothing it is handed back untouched. */ useEffect(() => { let restoreCaret = caretToEndRef.current; caretToEndRef.current = false; - if (redrawPendingRef.current && redrawSkillTokens()) { - redrawPendingRef.current = false; - restoreCaret = true; + if (redrawPendingRef.current) { + const heldCaret = caretPendingRef.current; + caretPendingRef.current = false; + const redrew = redrawSkillTokens(); + caretPendingRef.current = redrew ? false : heldCaret; + if (redrew) { + redrawPendingRef.current = false; + restoreCaret = true; + } } if (restoreCaret) caretToContentEnd(); });