From 33523fb3b3c002472cad7ad5fdfe7892e56980d1 Mon Sep 17 00:00:00 2001 From: Bartosz Tomczyk Date: Mon, 5 Oct 2026 20:00:27 +0200 Subject: [PATCH 1/5] fix(content-script): start again on a document that document.open() rewrites The e2e test "ckeditor4 typing accepts a prediction mid-line" failed on master in Firefox. The prediction menu was visible, but Tab did not accept the prediction. Tab moved the focus out of the editor. Root cause: CKEditor 4 writes its editing frame with document.open(). It does this at start and on each setData(). document.open() erases all event listeners of the document and its window, and it replaces the root element. The iframe, its window and the Document object stay. - The MAIN-world bridge adds its listeners again, but it sets itself to "disabled". It waits for the content script to send "enabled" again. The content script did not send it again, because it did not know about the rewrite. Thus the bridge ignored the write request, and Tab did not accept the prediction. - In Chrome, the content script also lost all its listeners and its root observer. After setData(), no prediction showed. The failure was flaky in Firefox because the order of the content script start and the CKEditor frame write changes with CPU load. Under CPU load, 7 of 120 local CKEditor 4 runs failed. Fix: the content script observes the Document node. A MutationObserver on the Document stays after document.open() (measured in Chrome). When the root element changes, the content script destroys its instance and starts a new one on the new document. The new instance adds its listeners again and sends "enabled" to the bridge again. A late config answer does not enable the destroyed instance. Tests: - e2e: "CKEditor 4 setData rewrites its frame with document.open, and typing still accepts a prediction". It fails in Chrome without the fix (Tab is not accepted) and passes 10 of 10 runs with the fix. - unit: a rewritten document starts a new instance that sends "enabled" to the host bridge again. Co-Authored-By: Claude Opus 5.5 --- .../chrome/content-script/content_script.ts | 17 ++++++- tests/content_script.behavior.test.ts | 44 ++++++++++++++++ tests/e2e/coverage-matrix.json | 16 ++++++ tests/e2e/full.e2e.test.ts | 50 ++++++++++++++++++- 4 files changed, 125 insertions(+), 2 deletions(-) diff --git a/src/adapters/chrome/content-script/content_script.ts b/src/adapters/chrome/content-script/content_script.ts index f2b29986e..df125b1e4 100644 --- a/src/adapters/chrome/content-script/content_script.ts +++ b/src/adapters/chrome/content-script/content_script.ts @@ -48,6 +48,17 @@ class FluentTyper { this.runtimeController.handleEarlyTabAcceptRequest(event.data.entryId); } }; + // document.open() (CKEditor 4 writes its editing frame so, also on setData) erases + // every listener of this script and replaces the root element. Only an observer of + // the document itself sees that: then a new instance starts on the new document. + private readonly root = document.documentElement; + private readonly documentRewriteObserver = new MutationObserver(() => { + if (!document.documentElement || document.documentElement === this.root) return; + logger.info("Document rewritten; restarting content script"); + this.destroy(); + window.FluentTyper = new FluentTyper(); + }); + private destroyed = false; constructor() { logger.info("Initializing content script", { @@ -88,6 +99,7 @@ class FluentTyper { }); chrome.runtime.onMessage.addListener(this.boundMessageHandler); + this.documentRewriteObserver.observe(document, { childList: true }); this.getConfig(); } @@ -137,6 +149,8 @@ class FluentTyper { destroy(): void { logger.info("Destroying content script instance"); + this.destroyed = true; + this.documentRewriteObserver.disconnect(); this.hostChangeWatcher.stop(); this.runtimeController.disable(); window.removeEventListener("message", this.boundEarlyTabAcceptHandler); @@ -154,7 +168,8 @@ class FluentTyper { }; chrome.runtime.sendMessage(msg, (response: unknown) => { checkLastError(); - this.messageHandler(response as Message); + // A late answer must not enable a destroyed instance again. + if (!this.destroyed) this.messageHandler(response as Message); }); } } diff --git a/tests/content_script.behavior.test.ts b/tests/content_script.behavior.test.ts index dbc29d8a8..7a2ea6de7 100644 --- a/tests/content_script.behavior.test.ts +++ b/tests/content_script.behavior.test.ts @@ -18,6 +18,10 @@ import { EARLY_TAB_ACCEPT_MESSAGE_TYPE, EARLY_TAB_ACCEPT_REQUEST_EVENT, } from "../src/adapters/chrome/content-script/suggestions/EarlyTabAcceptBridgeProtocol"; +import { + HOST_EDITOR_ENABLED_ATTR, + HOST_EDITOR_ENABLED_EVENT, +} from "../src/adapters/chrome/content-script/suggestions/HostEditorBridgeProtocol"; type SuggestionLike = { queryAndAttachHelper: jest.Mock; @@ -898,6 +902,46 @@ describe("content_script behavior", () => { expect(domObserver.setNode).not.toHaveBeenCalled(); }); + test("a rewritten document (document.open) starts a new instance that enables the host bridge again", async () => { + const { fluentTyper, sendMessage } = await loadContentScript(); + const lateConfig = sendMessage.mock.calls[0][1] as (response: unknown) => void; + fluentTyper.setConfig(defaultConfig()); + expect(fluentTyper.enabled).toBe(true); + + // document.open() erases every listener and replaces the root element. + const oldRoot = document.documentElement; + const newRoot = document.createElement("html"); + newRoot.append(document.createElement("head"), document.createElement("body")); + document.replaceChild(newRoot, oldRoot); + try { + await Promise.resolve(); + const restarted = (window as Window & { FluentTyper?: LoadedContentScript["fluentTyper"] }) + .FluentTyper!; + behaviorHarness.fluentTyperInstances.push(restarted); + expect(restarted).not.toBe(fluentTyper); + expect(sendMessage.mock.calls.at(-1)?.[0]).toEqual( + expect.objectContaining({ command: CMD_CONTENT_SCRIPT_GET_CONFIG }), + ); + + // A late config answer to the old instance does not enable it again. + const handled = jest.spyOn(fluentTyper, "messageHandler"); + lateConfig({ command: CMD_BACKGROUND_PAGE_SET_CONFIG, context: defaultConfig() }); + expect(handled).not.toHaveBeenCalled(); + + // The main-world bridge lost its state: the new instance sends "enabled" again. + const bridgeStates: (string | null)[] = []; + const record = () => + bridgeStates.push(document.documentElement.getAttribute(HOST_EDITOR_ENABLED_ATTR)); + document.addEventListener(HOST_EDITOR_ENABLED_EVENT, record); + restarted.setConfig(defaultConfig()); + document.removeEventListener(HOST_EDITOR_ENABLED_EVENT, record); + expect(bridgeStates).toEqual(["true"]); + } finally { + for (const instance of behaviorHarness.fluentTyperInstances) instance.destroy(); + document.replaceChild(oldRoot, newRoot); + } + }); + test("watchdog does nothing when host and observed node are unchanged", async () => { const { fluentTyper, domObserverInstances } = await loadContentScript(); const domObserver = domObserverInstances[0]; diff --git a/tests/e2e/coverage-matrix.json b/tests/e2e/coverage-matrix.json index 4f626e4b9..cd032f65c 100644 --- a/tests/e2e/coverage-matrix.json +++ b/tests/e2e/coverage-matrix.json @@ -4703,6 +4703,22 @@ } ] }, + { + "id": "rewritten_document_restart", + "description": "When a page rewrites a document with document.open() (CKEditor 4 writes its editing frame so, also on setData), every listener in it is erased and its root element is replaced. FluentTyper then starts a new content script instance on the new document, which sends the enabled state to the main-world bridge again; a late config answer does not enable the old instance. Typing in the rewritten CKEditor 4 frame accepts a prediction with Tab.", + "coverage": [ + { + "layer": "unit", + "file": "tests/content_script.behavior.test.ts", + "test": "a rewritten document (document.open) starts a new instance that enables the host bridge again" + }, + { + "layer": "e2e-full", + "file": "tests/e2e/full.e2e.test.ts", + "test": "CKEditor 4 setData rewrites its frame with document.open, and typing still accepts a prediction" + } + ] + }, { "id": "rich_editor_snippet_expansion", "description": "In real Draft.js, Trix, CKEditor 4, Froala, Summernote, Tiptap and RoosterJS editors and in bundled Quill 1 and Quill 2, a text expansion replaces its shortcut in the second paragraph of the editor's own model, keeps bold and links, and native Undo restores the shortcut in one step. A bundled Quill 2's Undo can also remove the typing of the last second.", diff --git a/tests/e2e/full.e2e.test.ts b/tests/e2e/full.e2e.test.ts index d96fbe561..61195648c 100644 --- a/tests/e2e/full.e2e.test.ts +++ b/tests/e2e/full.e2e.test.ts @@ -40,7 +40,12 @@ import { SUPPORTED_PREDICTION_LANGUAGE_KEYS } from "../../src/core/domain/lang"; import { grammarRuleSelectionToOverrides } from "../../src/core/domain/grammar/GrammarRuleSettings"; import { DEFAULT_CURRENT_GRAMMAR_RULES } from "../../src/core/domain/grammar/ruleCatalog"; import type { BackgroundContext } from "./e2e-helpers"; -import { EXTRA_LIST_ITEM, EXTRA_PARAGRAPH, SEED_TEXT } from "./fixtures/review-editors/shared"; +import { + EXTRA_LIST_ITEM, + EXTRA_PARAGRAPH, + SEED_HTML, + SEED_TEXT, +} from "./fixtures/review-editors/shared"; import { BROWSER_TYPE, clickFirstVisibleSuggestion, @@ -10766,6 +10771,49 @@ describeE2E(`Extension E2E Test [${BROWSER_TYPE}]`, () => { suiteTimeout(30000, 50000), ); + test( + "CKEditor 4 setData rewrites its frame with document.open, and typing still accepts a prediction", + async () => { + const first = await openTypingEditor("ckeditor4"); + // FluentTyper runs in the editing frame before the rewrite. + await placeCaretAfter(first.surface, first.editable, "dog."); + await page.keyboard.type(" w"); + await wPrediction(first.surface, "CKEditor 4 prediction before setData"); + // setData writes the frame again: document.open() erases every listener in it + // and replaces its root element. The iframe and its window stay. + await page.evaluate( + (html) => + new Promise((resolve) => { + type Editor = { setData(data: string, options: { callback(): void }): void }; + const ck = (window as unknown as { CKEDITOR: { instances: Record } }) + .CKEDITOR; + ck.instances["test-review-ckeditor4"].setData(html, { callback: resolve }); + }), + SEED_HTML, + ); + // Puppeteer can report the rewritten frame as a new frame. + const { surface, editable, model } = await reviewEditorFixture("ckeditor4"); + expect((await model()).text).toBe(SEED_TEXT); + await placeCaretAfter(surface, editable, "dog."); + await page.keyboard.type(" w"); + const prediction = await wPrediction(surface, "CKEditor 4 prediction after setData"); + await page.keyboard.press("Tab"); + await waitUntil( + "CKEditor 4 accepts the prediction after setData", + async () => + normalizeSuggestionText((await model()).text) === + normalizeSuggestionText(`${SEED_TEXT} ${prediction}`), + { timeoutMs: SUGGESTION_TIMEOUT_MS }, + ).catch(async (cause) => { + throw new Error(`ckeditor4 model: ${JSON.stringify({ prediction, ...(await model()) })}`, { + cause, + }); + }); + expect((await model()).runs).toEqual({ bold: ["teh"], links: ["teh"] }); + }, + suiteTimeout(30000, 50000), + ); + test( "RoosterJS typing acceptance after a click snapshot is one Rooster undo step, and Redo restores it", async () => { From d63f00984ecb97f8ad2b55806337d17aa98a2769 Mon Sep 17 00:00:00 2001 From: Bartosz Tomczyk Date: Mon, 5 Oct 2026 20:00:27 +0200 Subject: [PATCH 2/5] ci: probe CKEditor 4 typing under CPU load (temporary) Run the CKEditor 4 typing tests 20 times in each job under CPU load in Firefox (6 jobs) and Chrome (1 job). Remove this workflow before merge. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/probe-ckeditor4.yml | 37 +++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 .github/workflows/probe-ckeditor4.yml diff --git a/.github/workflows/probe-ckeditor4.yml b/.github/workflows/probe-ckeditor4.yml new file mode 100644 index 000000000..b3dc72ea4 --- /dev/null +++ b/.github/workflows/probe-ckeditor4.yml @@ -0,0 +1,37 @@ +# Temporary: run the CKEditor 4 typing tests many times under CPU load. Removed before merge. +name: Probe CKEditor 4 typing + +on: + pull_request: + branches: [master] + +permissions: + contents: read + +jobs: + stressed: + name: ${{ matrix.browser }} CKEditor 4 typing under CPU load (copy ${{ matrix.copy }}) + runs-on: ubuntu-latest + timeout-minutes: 30 + strategy: + fail-fast: false + matrix: + browser: [firefox] + copy: [1, 2, 3, 4, 5, 6] + include: + - browser: chrome + copy: 1 + steps: + - uses: actions/checkout@v4 + - uses: oven-sh/setup-bun@v2 + with: + bun-version-file: package.json + - run: bun install --frozen-lockfile + - if: matrix.browser == 'firefox' + run: bunx puppeteer browsers install firefox + - run: | + for c in $(seq $(nproc)); do (while :; do :; done) & done + status=0 + bun run test:e2e:full --platform=${{ matrix.browser }} --test-name-pattern="ckeditor4 typing accepts a prediction|CKEditor 4 setData rewrites" --rerun-each 20 > run.log 2>&1 || status=$? + grep -E "^error: ck|^\(fail\)|^ *[0-9]+ (pass|fail)$" run.log || true + exit $status From 3db7e6e5ecad109079690829ef8e4de9261246fd Mon Sep 17 00:00:00 2001 From: Bartosz Tomczyk Date: Mon, 5 Oct 2026 20:20:39 +0200 Subject: [PATCH 3/5] ci: remove the temporary CKEditor 4 probe The probe passed: 600 of 600 Firefox runs and 100 of 100 Chrome runs under CPU load. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/probe-ckeditor4.yml | 37 --------------------------- 1 file changed, 37 deletions(-) delete mode 100644 .github/workflows/probe-ckeditor4.yml diff --git a/.github/workflows/probe-ckeditor4.yml b/.github/workflows/probe-ckeditor4.yml deleted file mode 100644 index b3dc72ea4..000000000 --- a/.github/workflows/probe-ckeditor4.yml +++ /dev/null @@ -1,37 +0,0 @@ -# Temporary: run the CKEditor 4 typing tests many times under CPU load. Removed before merge. -name: Probe CKEditor 4 typing - -on: - pull_request: - branches: [master] - -permissions: - contents: read - -jobs: - stressed: - name: ${{ matrix.browser }} CKEditor 4 typing under CPU load (copy ${{ matrix.copy }}) - runs-on: ubuntu-latest - timeout-minutes: 30 - strategy: - fail-fast: false - matrix: - browser: [firefox] - copy: [1, 2, 3, 4, 5, 6] - include: - - browser: chrome - copy: 1 - steps: - - uses: actions/checkout@v4 - - uses: oven-sh/setup-bun@v2 - with: - bun-version-file: package.json - - run: bun install --frozen-lockfile - - if: matrix.browser == 'firefox' - run: bunx puppeteer browsers install firefox - - run: | - for c in $(seq $(nproc)); do (while :; do :; done) & done - status=0 - bun run test:e2e:full --platform=${{ matrix.browser }} --test-name-pattern="ckeditor4 typing accepts a prediction|CKEditor 4 setData rewrites" --rerun-each 20 > run.log 2>&1 || status=$? - grep -E "^error: ck|^\(fail\)|^ *[0-9]+ (pass|fail)$" run.log || true - exit $status From 72a706183d34aecd9719b3523cda00467ae77783 Mon Sep 17 00:00:00 2001 From: Bartosz Tomczyk Date: Mon, 5 Oct 2026 21:29:28 +0200 Subject: [PATCH 4/5] fix(content-script): do not restart when the body appears under the observed root The e2e test "ckeditor4 typing expands a snippet in one native undo step" failed at random in Chrome. The menu showed "Best regards", but Tab did not expand the shortcut. Tab moved the focus out of the editor. Root cause: in Chrome, the content script in the CKEditor 4 editing frame can start while CKEditor still writes the frame. Then the document is "loading" and has no body. The runtime observes the root element in place of the body. Later, HostChangeWatcher.watchDog() compared the observed node with document.body. The nodes were not the same, so it restarted the runtime. The restart hid the open menu. When the restart came between the menu and the Tab key, the Tab key went to the browser. Fix: when the observed node is still in the document and contains the current body, the watchdog observes the body and does not restart. A replaced root (a node that is not in the document) still causes a restart. Evidence: 4 parallel Chrome instances under full CPU load ran the snippet test 120 times. Before the fix, 9 of 120 runs failed. With the fix, 120 of 120 runs passed. All CKEditor 4 tests passed 288 of 288 runs under the same load. Tests: a new unit test in tests/content_script.behavior.test.ts fails without the fix. The coverage matrix has a new behavior, late_body_without_restart. Co-Authored-By: Claude Opus 5.5 --- .../chrome/content-script/HostChangeWatcher.ts | 10 +++++++++- tests/content_script.behavior.test.ts | 14 ++++++++++++++ tests/e2e/coverage-matrix.json | 16 ++++++++++++++++ 3 files changed, 39 insertions(+), 1 deletion(-) diff --git a/src/adapters/chrome/content-script/HostChangeWatcher.ts b/src/adapters/chrome/content-script/HostChangeWatcher.ts index 527325061..6a6a44e5e 100644 --- a/src/adapters/chrome/content-script/HostChangeWatcher.ts +++ b/src/adapters/chrome/content-script/HostChangeWatcher.ts @@ -68,7 +68,15 @@ export class HostChangeWatcher { } const currentNode = document.body || document.documentElement; - if (this.dependencies.getObservedNode() === currentNode) { + const observedNode = this.dependencies.getObservedNode(); + if (observedNode === currentNode) { + return; + } + // A script that started while the page was loading, before the body (CKEditor 4 + // writes its editing frame so), observes the root. The root still holds the body: + // a restart would only drop the open suggestions. + if (observedNode.isConnected && observedNode.contains(currentNode)) { + this.dependencies.setObservedNode(currentNode); return; } diff --git a/tests/content_script.behavior.test.ts b/tests/content_script.behavior.test.ts index 7a2ea6de7..341f9de6f 100644 --- a/tests/content_script.behavior.test.ts +++ b/tests/content_script.behavior.test.ts @@ -886,6 +886,20 @@ describe("content_script behavior", () => { expect(sendMessage).toHaveBeenCalled(); }); + test("watchdog observes a body that appeared under the observed root without a restart", async () => { + const { fluentTyper, domObserverInstances } = await loadContentScript(); + const domObserver = domObserverInstances[0]; + const restartSpy = jest.spyOn(fluentTyper, "restart"); + fluentTyper.enabled = true; + // The script started while the page was loading: there was no body, so it observes the root. + domObserver.getNode.mockReturnValue(document.documentElement); + + fluentTyper.watchDog(); + + expect(restartSpy).not.toHaveBeenCalled(); + expect(domObserver.setNode).toHaveBeenCalledWith(document.body); + }); + test("watchdog prefers a host change over a node change and skips the DOM restart", async () => { const { fluentTyper, domObserverInstances } = await loadContentScript(); const domObserver = domObserverInstances[0]; diff --git a/tests/e2e/coverage-matrix.json b/tests/e2e/coverage-matrix.json index cd032f65c..68ff77c78 100644 --- a/tests/e2e/coverage-matrix.json +++ b/tests/e2e/coverage-matrix.json @@ -4719,6 +4719,22 @@ } ] }, + { + "id": "late_body_without_restart", + "description": "A content script that starts while its document is still loading, before the body (CKEditor 4 writes its editing frame so), observes the root element. When the body appears under that root, the watchdog observes the body and does not restart the runtime, so a visible menu stays and Tab still accepts from it.", + "coverage": [ + { + "layer": "unit", + "file": "tests/content_script.behavior.test.ts", + "test": "watchdog observes a body that appeared under the observed root without a restart" + }, + { + "layer": "e2e-full", + "file": "tests/e2e/full.e2e.test.ts", + "test": "%s typing expands a snippet in one native undo step" + } + ] + }, { "id": "rich_editor_snippet_expansion", "description": "In real Draft.js, Trix, CKEditor 4, Froala, Summernote, Tiptap and RoosterJS editors and in bundled Quill 1 and Quill 2, a text expansion replaces its shortcut in the second paragraph of the editor's own model, keeps bold and links, and native Undo restores the shortcut in one step. A bundled Quill 2's Undo can also remove the typing of the last second.", From cbfa6756af31423234ea6f79b19976eff0d59586 Mon Sep 17 00:00:00 2001 From: Bartosz Tomczyk Date: Mon, 5 Oct 2026 21:47:30 +0200 Subject: [PATCH 5/5] fix(suggestions): keep the suggestion that the user chose with an arrow key A prediction answer that came after an arrow key moved the menu selection back to the first row. Then Tab accepted a word that the user did not choose. This occurs when the answer for the typed text is late: the user presses ArrowDown on the old menu, and the new answer resets the selection. Reproduction: type "th" in an input. The menu shows "the", "that", "this". Press ArrowDown to "that". A new answer for the same text comes. Before this fix, Tab inserted "the ". Fix: the keyboard handler records the suggestion that the user moved to (entry.chosenSuggestion). A new answer keeps that suggestion highlighted when the answer contains it. If the answer does not contain it, the first row is highlighted. An edit or a cleared menu clears the choice. Tests: - e2e: "A late prediction answer keeps the suggestion that the user chose with an arrow key". It sends a new answer after ArrowDown. It failed without the fix ("the " in place of "that ") and passes 5 of 5 runs with the fix. - unit: the keyboard handler records the choice; the session keeps it for a late answer, drops it when the answer does not contain it, and clears it on input. - The coverage matrix has a new behavior, late_answer_keeps_arrow_choice. Co-Authored-By: Claude Opus 5.5 --- .../suggestions/SuggestionEntrySession.ts | 10 ++++- .../suggestions/SuggestionKeyboardHandler.ts | 1 + .../suggestions/SuggestionManagerRuntime.ts | 1 + .../content-script/suggestions/types.ts | 2 + tests/SuggestionEntrySession.test.ts | 35 ++++++++++++++++ tests/SuggestionKeyboardHandler.test.ts | 14 +++++++ tests/e2e/coverage-matrix.json | 21 ++++++++++ tests/e2e/full.e2e.test.ts | 42 +++++++++++++++++++ tests/suggestionTestUtils.ts | 1 + 9 files changed, 126 insertions(+), 1 deletion(-) diff --git a/src/adapters/chrome/content-script/suggestions/SuggestionEntrySession.ts b/src/adapters/chrome/content-script/suggestions/SuggestionEntrySession.ts index 7b34702cb..4a0961a6a 100644 --- a/src/adapters/chrome/content-script/suggestions/SuggestionEntrySession.ts +++ b/src/adapters/chrome/content-script/suggestions/SuggestionEntrySession.ts @@ -304,6 +304,8 @@ export class SuggestionEntrySession { this.deferredInput = event; return; } + // An edit makes a new choice necessary. + this.entry.chosenSuggestion = null; if (!this.refreshInteraction()) { if (this.entry.isComposing) this.handleSuppressedInput(); return; @@ -404,6 +406,7 @@ export class SuggestionEntrySession { this.snippetSuggestions.clear(); this.snippetShortcuts = undefined; this.entry.selectedIndex = 0; + this.entry.chosenSuggestion = null; this.entry.visibleSuggestionBeforeCursorText = null; this.entry.visibleSuggestionFullText = null; this.entry.inlineSuggestion = null; @@ -442,7 +445,12 @@ export class SuggestionEntrySession { this.snippetSuggestions = new Set( this.entry.suggestions.filter((_, index) => context.snippetShortcuts?.[index]), ); - this.entry.selectedIndex = 0; + // An answer that comes after an arrow key (it was late) keeps the user's choice. + const chosen = this.entry.chosenSuggestion + ? this.entry.suggestions.indexOf(this.entry.chosenSuggestion) + : -1; + if (chosen < 0) this.entry.chosenSuggestion = null; + this.entry.selectedIndex = Math.max(0, chosen); this.entry.menuHeader = this.options.showSuggestionFooter && context.lang && SUPPORTED_LANGUAGES[context.lang] ? suggestionLanguageLabel(SUPPORTED_LANGUAGES[context.lang]) diff --git a/src/adapters/chrome/content-script/suggestions/SuggestionKeyboardHandler.ts b/src/adapters/chrome/content-script/suggestions/SuggestionKeyboardHandler.ts index 4fd10a5ef..6fd1b6ebc 100644 --- a/src/adapters/chrome/content-script/suggestions/SuggestionKeyboardHandler.ts +++ b/src/adapters/chrome/content-script/suggestions/SuggestionKeyboardHandler.ts @@ -154,6 +154,7 @@ export class SuggestionKeyboardHandler { if (next < rows) { entry.selectedIndex = next; } + entry.chosenSuggestion = next < rows ? entry.suggestions[next] : null; this.options.updateSelectionHighlight(entry); } diff --git a/src/adapters/chrome/content-script/suggestions/SuggestionManagerRuntime.ts b/src/adapters/chrome/content-script/suggestions/SuggestionManagerRuntime.ts index a28f37163..7145db3ba 100644 --- a/src/adapters/chrome/content-script/suggestions/SuggestionManagerRuntime.ts +++ b/src/adapters/chrome/content-script/suggestions/SuggestionManagerRuntime.ts @@ -572,6 +572,7 @@ export class SuggestionManagerRuntime { requestId: 0, suggestions: [], selectedIndex: 0, + chosenSuggestion: null, menuHeader: null, latestMentionText: "", latestMentionStart: 0, diff --git a/src/adapters/chrome/content-script/suggestions/types.ts b/src/adapters/chrome/content-script/suggestions/types.ts index 9ea6428b2..27cbda4b2 100644 --- a/src/adapters/chrome/content-script/suggestions/types.ts +++ b/src/adapters/chrome/content-script/suggestions/types.ts @@ -134,6 +134,8 @@ export interface SuggestionEntry { requestId: number; suggestions: string[]; selectedIndex: number; + /** The suggestion the user moved to with an arrow key after the last edit. A later answer keeps it highlighted. */ + chosenSuggestion: string | null; menuHeader: string | null; latestMentionText: string; latestMentionStart: number; diff --git a/tests/SuggestionEntrySession.test.ts b/tests/SuggestionEntrySession.test.ts index 1dc07c469..5f524b07b 100644 --- a/tests/SuggestionEntrySession.test.ts +++ b/tests/SuggestionEntrySession.test.ts @@ -1161,6 +1161,41 @@ test("session ignores stale responses and renders fresh menu responses", () => { expect(recordSuggestionShown).toHaveBeenCalledWith({ suggestionCount: 1, language: "en_US" }); }); +test("a late answer keeps the suggestion that the user chose with an arrow key", () => { + const renderMenu = jest.fn(); + const entry = createSuggestionEntry({ requestId: 2, latestMentionText: "th" }); + const input = entry.elem as HTMLInputElement; + input.value = "th"; + input.selectionStart = 2; + input.selectionEnd = 2; + const session = makeSession({ entry, renderMenu }); + const answer = (predictions: string[]) => + session.handlePredictionResponse( + partialResponse({ requestId: 2, suggestionId: 1, predictions }), + ); + + answer(["the", "that", "this"]); + // ArrowDown: the keyboard handler records the choice. + entry.selectedIndex = 1; + entry.chosenSuggestion = "that"; + // The answer for the typed text comes after the arrow key, in another order. + answer(["this", "the", "that"]); + expect(entry.selectedIndex).toBe(2); + expect(renderMenu).toHaveBeenLastCalledWith(expect.objectContaining({ selectedIndex: 2 })); + + // An answer without the chosen suggestion highlights the first row again. + answer(["they", "them"]); + expect(entry.selectedIndex).toBe(0); + expect(entry.chosenSuggestion).toBeNull(); + + // Typing makes a new choice necessary. + entry.chosenSuggestion = "them"; + input.value = "the"; + input.selectionStart = input.selectionEnd = 3; + session.handleInput(new Event("input")); + expect(entry.chosenSuggestion).toBeNull(); +}); + test("session does not fulfill pending inline accept when the ghost render is vetoed", () => { const textEditService = { acceptSuggestion: jest.fn(() => null), diff --git a/tests/SuggestionKeyboardHandler.test.ts b/tests/SuggestionKeyboardHandler.test.ts index e8bede067..0d05dafbc 100644 --- a/tests/SuggestionKeyboardHandler.test.ts +++ b/tests/SuggestionKeyboardHandler.test.ts @@ -32,6 +32,20 @@ describe("SuggestionKeyboardHandler", () => { expect(updateSelectionHighlight).toHaveBeenCalledWith(entry); }); + test("arrow keys record the chosen suggestion for a later answer", () => { + const handler = createHandler({ + inlineSuggestionEnabled: false, + consumeKeyboardEvent: jest.fn((event: KeyboardEvent) => event.preventDefault()), + isMenuVisible: jest.fn(() => true), + }); + const entry = createSuggestionEntry({ suggestions: ["one", "two"], selectedIndex: 0 }); + + handler.handle(entry, createEvent("ArrowDown")); + expect(entry.chosenSuggestion).toBe("two"); + handler.handle(entry, createEvent("ArrowDown")); + expect(entry.chosenSuggestion).toBe("one"); + }); + test("moves selection the other way when the menu lists suggestions bottom-up", () => { const handler = createHandler({ inlineSuggestionEnabled: false, diff --git a/tests/e2e/coverage-matrix.json b/tests/e2e/coverage-matrix.json index 68ff77c78..a6d2d325d 100644 --- a/tests/e2e/coverage-matrix.json +++ b/tests/e2e/coverage-matrix.json @@ -4746,6 +4746,27 @@ } ] }, + { + "id": "late_answer_keeps_arrow_choice", + "description": "When the user moves the menu selection with an arrow key and a prediction answer comes after that (it was late), the menu keeps the chosen suggestion highlighted and Tab accepts it. When the new answer does not contain the chosen suggestion, the first row is highlighted. An edit clears the choice.", + "coverage": [ + { + "layer": "unit", + "file": "tests/SuggestionKeyboardHandler.test.ts", + "test": "arrow keys record the chosen suggestion for a later answer" + }, + { + "layer": "unit", + "file": "tests/SuggestionEntrySession.test.ts", + "test": "a late answer keeps the suggestion that the user chose with an arrow key" + }, + { + "layer": "e2e-full", + "file": "tests/e2e/full.e2e.test.ts", + "test": "A late prediction answer keeps the suggestion that the user chose with an arrow key" + } + ] + }, { "id": "stale_predictions_after_caret_move", "description": "Predictions are for one caret place. When the page moves the caret without a text change (for example after focus), a prediction response for the old place is not shown, and a visible contenteditable menu is dismissed, so Space or Tab cannot write a stale word at the new place.", diff --git a/tests/e2e/full.e2e.test.ts b/tests/e2e/full.e2e.test.ts index 61195648c..d91ca69c5 100644 --- a/tests/e2e/full.e2e.test.ts +++ b/tests/e2e/full.e2e.test.ts @@ -5868,6 +5868,48 @@ describeE2E(`Extension E2E Test [${BROWSER_TYPE}]`, () => { suiteTimeout(25000, 45000), ); + test( + "A late prediction answer keeps the suggestion that the user chose with an arrow key", + async () => { + const selector = "#test-input"; + + await openEnglishField(selector); + await typeInInput(page, selector, "th"); + const suggestions = await waitForVisibleSuggestionTexts(page, suiteTimeout(5000, 9000)); + expect(suggestions.length).toBeGreaterThan(1); + const chosen = suggestions[1]!; + await highlightSuggestion(page, chosen); + // A new answer renders new rows: mark the rows of this one. + const rows = () => + page.evaluate(() => + Array.from(document.querySelectorAll('[id^="ft-menu-"]')) + .filter((menu) => getComputedStyle(menu).display !== "none") + .flatMap((menu) => + Array.from((menu.shadowRoot ?? menu).querySelectorAll("li[data-index]")), + ) + .map((row) => { + const marked = row.hasAttribute("data-test-shown"); + row.setAttribute("data-test-shown", ""); + return marked; + }), + ); + await rows(); + // An answer that comes after the arrow key, as an answer for typing that was late. + await worker.evaluate(async () => { + const tabs = await chrome.tabs.query({ active: true, lastFocusedWindow: true }); + const tab = tabs.find((candidate) => /^https?:/.test(candidate.url ?? "")) ?? tabs[0]; + await chrome.tabs.sendMessage(tab!.id!, { command: "CMD_TRIGGER_FT_ACTIVE_TAB" }); + }); + await waitUntil("a new prediction answer", async () => { + const marked = await rows(); + return marked.length > 0 && !marked.some(Boolean); + }); + await page.keyboard.press("Tab"); + await waitForInputContentEqual(page, selector, chosen); + }, + suiteTimeout(25000, 45000), + ); + test( "Grammar Rule Engine reverts latest auto-fix via Cmd/Ctrl+Z in #test-input", async () => { diff --git a/tests/suggestionTestUtils.ts b/tests/suggestionTestUtils.ts index fe0707be4..f7d7a3d77 100644 --- a/tests/suggestionTestUtils.ts +++ b/tests/suggestionTestUtils.ts @@ -33,6 +33,7 @@ export function createSuggestionEntry( requestId: 0, suggestions: [], selectedIndex: 0, + chosenSuggestion: null, menuHeader: null, latestMentionText: "", latestMentionStart: 0,