Fix rich-text editor Mod-Enter commit dropping selected content

claude-notes/plans/2026-07-06-richtext-mod-enter-hardbreak-content-loss.md

@@ -0,0 +1,171 @@
+# Rich-text editor: `Mod-Enter` commit drops selected content (HardBreak collision)
+
+**Strand:** bd-hafs0qho (discovered-from bd-7pxub583; related to bd-sjb4pzx8)
+**Status:** IN PROGRESS — user approved (A)+(B) on 2026-07-06
+
+## Decision locked with user (2026-07-06)
+
+Do **both (A) and (B)**:
+- **(A)** Move the `Mod-Enter` commit into tiptap's keymap so it wins the race
+  and suppresses `setHardBreak`; retire the DOM `addEventListener` commit path.
+- **(B)** Disable HardBreak's `Mod-Enter` binding (keep `Shift-Enter`).
+
+Rationale (user): a hard break on `Mod-Enter` is never wanted here — the rich
+editor is **not** intended for extensive multi-block editing; the preview's
+plain-text editor and hub-client's source editor cover those cases.
+**Date:** 2026-07-06
+
+## Summary
+
+In the `format: q2-preview` rich-text (tiptap) block editor, committing with
+`Mod-Enter` while text is **selected** deletes the selected text. The most
+visible path: click a block → select-all (`Mod-a`) → bold (`Mod-b`) → commit
+(`Mod-Enter`) → the block is written to disk **empty** (a tight list item
+`- banana` becomes `-`; a paragraph is deleted entirely).
+
+It is **not** specific to `Plain` / list items and **not** introduced by
+bd-7pxub583 — it reproduces identically on a `Para`, in the shared,
+type-agnostic RichTextEditor path, and predates the Plain-gate change.
+
+## Reliable reproduction (done)
+
+`hub-client/e2e/q2-preview-richtext-bold-content-loss.spec.ts` (Playwright,
+real CDP keyboard events — the only faithful vehicle; jsdom and MCP-browser
+synthetic events were confounded). Two cases: a tight bullet-list item and a
+paragraph. Each opens the rich editor, does `Mod-a` / `Mod-b` / `Mod-Enter`
+(via Playwright `ControlOrMeta`, which maps to ProseMirror's `Mod`), and asserts
+the committed qmd contains the **bolded** text. **Currently RED** — the file is
+written with the item emptied:
+
+```
+## List
+
+* apple
+*            ← "banana" gone
+* cherry
+```
+
+> Cross-platform note: use `ControlOrMeta` in the spec. Plain `Control` on macOS
+> does NOT trigger ProseMirror's `Mod-b` (which is Meta on mac), so bold never
+> fires and the bug is masked — the reason two earlier runs falsely "passed".
+
+## Root cause (confirmed)
+
+tiptap's **HardBreak extension binds `Mod-Enter` → `setHardBreak()`** (verified
+in `@tiptap/extension-hard-break/dist/index.js`; it binds both `Mod-Enter` and
+`Shift-Enter`). RichTextEditor *also* uses `Mod-Enter` for commit — but via a
+**DOM `addEventListener('keydown', …)`** on `editor.view.dom` that is registered
+*after* ProseMirror's keymap plugin. So on `Mod-Enter`:
+
+1. ProseMirror/tiptap's keymap runs first → `setHardBreak()` **replaces the
+   current selection** with a hardBreak node. After select-all + bold the whole
+   text is selected → the text is **deleted**, leaving `paragraph[hardBreak]`.
+2. RichTextEditor's DOM handler runs next → `e.preventDefault()` (too late to
+   undo step 1) → `commit()` → serializes `paragraph[hardBreak]` → `docToMarkdown`
+   correctly yields `""` → an empty block is committed.
+
+Corroborating evidence:
+- Instrumented commit (captured earlier) showed the committed doc was exactly
+  `{paragraph:[{hardBreak}]}`, md `""`.
+- The **serializer** and **Rust `apply_node_edit`** are both verified correct in
+  isolation, and **pure tiptap `selectAll()+toggleBold()+serialize`** (no
+  `Mod-Enter`) is clean — the damage only appears when `Mod-Enter` is pressed.
+- Simple text edits "worked" but the committed doc always carried a **trailing**
+  hardBreak (`[text, hardBreak]`) — the same `Mod-Enter`→hardBreak insertion, but
+  harmless at a collapsed cursor because it appends rather than replaces, and a
+  trailing hardBreak serializes to nothing.
+
+So the defect is general: **`Mod-Enter` inserts a hardBreak on every commit**; it
+is silent at a collapsed caret and destructive over a selection.
+
+## Fix options (to discuss)
+
+The goal: `Mod-Enter` must commit **without** inserting a hardBreak, deterministically.
+
+- **(A) Move commit into tiptap's keymap (recommended).** Add a small tiptap
+  extension (or `editor`-level `addKeyboardShortcuts`) binding `Mod-Enter` to a
+  handler that fires the commit and returns `true`. Because it lives in
+  ProseMirror's keymap with higher priority than HardBreak, it wins the race and
+  suppresses `setHardBreak`. This also retires the fragile DOM `addEventListener`
+  commit path (the root of the race). Escape/plain-Enter handling can move the
+  same way for consistency.
+- **(B) Disable HardBreak's `Mod-Enter` binding.** Configure the HardBreak
+  extension (via StarterKit) so only `Shift-Enter` inserts a hard break; then
+  `Mod-Enter` falls through to the existing DOM commit handler with nothing to
+  race. Smaller change, but leaves the DOM-listener commit path in place.
+- **(C) Change the commit key.** Least desirable — `Mod-Enter` is the expected
+  "confirm" gesture; keep it.
+
+Leaning **(A)**, optionally plus **(B)** as belt-and-suspenders (a hard break on
+`Mod-Enter` is never wanted in this single-block editor). Keep `Shift-Enter` as
+the intentional hard-break key (the editor already treats `Shift+Enter` as a
+hard break; plain `Enter` is swallowed).
+
+Either way, also confirm the **trailing-hardBreak-on-plain-commit** disappears
+(no more `[text, hardBreak]` docs), so commits stop carrying a stray break.
+
+## Plan (TDD)
+
+### Phase 1 — Reproduction (DONE)
+- [x] Playwright e2e repro (`q2-preview-richtext-bold-content-loss.spec.ts`),
+      red today; covers list-item **and** paragraph (shared-path proof).
+
+### Phase 2 — Root cause (DONE)
+- [x] Identified the `Mod-Enter` ↔ HardBreak `Mod-Enter` collision + DOM-listener
+      race; corroborated by the captured `paragraph[hardBreak]` commit doc.
+
+### Phase 3 — Tests first
+- [x] Keep the e2e repro as the end-to-end regression gate (RED before the fix).
+- [x] **Fast editor-config unit test** (`richtext/editorConfig.test.ts`, jsdom):
+      builds the editor via the shared `buildRichTextExtensions()`, select-all,
+      dispatches a real `Mod-Enter` keydown, asserts the doc is NOT mutated to a
+      hardBreak (text intact). Also: collapsed-caret `Mod-Enter` leaves no
+      trailing hardBreak; `Shift-Enter` still inserts one. **Confirmed RED** with
+      the fix reverted (default HardBreak), green with it.
+
+### Phase 4 — Implement the fix (A)+(B)
+- [x] **(B)** `richtext/editorConfig.ts`: shared static extension config;
+      disables StarterKit's built-in HardBreak and re-adds one bound to
+      `Shift-Enter` only. Added `@tiptap/extension-hard-break` as a direct dep.
+- [x] **(A)** `RichTextEditor.tsx`: Esc/`Mod-Enter`/`Enter` moved into a
+      high-priority tiptap keymap extension (`q2CommitKeymap`) via a handlers
+      ref; the racy DOM `keydown` listener is removed (focusout-commit kept).
+- [x] Preserved behavior: Escape cancels; plain Enter swallowed; `Shift-Enter`
+      is a hard break; dirty guard / stale-target guards unchanged.
+- [x] tsc clean; full preview-renderer suites green (487 unit incl. the 3 new,
+      517 integration); the editor-mount integration tests (p3-4-inline-breadcrumb,
+      RichTextEditor.caret) still pass.
+
+### Phase 5 — Verify
+- [x] **e2e repro GREEN** (was red). Both cases now assert the *bolded* text is
+      present on disk: `**banana**` (list item) and `**Hello world paragraph.**`
+      (paragraph). This is the faithful end-to-end check — real hub + real
+      browser + real `Mod`-keyboard, asserting the Automerge/disk content. It
+      supersedes the earlier MCP-browser manual check (which was confounded by
+      synthetic-event hardBreak injection).
+- [x] hub-client build (VITE_E2E) green; hub-client unit tests green (229).
+- [ ] `cargo xtask verify` (hub leg) — run before push.
+- [ ] hub-client changelog entry (two-commit workflow) since `hub-client/` is
+      touched (the e2e spec) and the fix is user-facing (data-loss fix in the
+      bundled editor). To add as part of the commit.
+
+## Status: implementation complete + verified; ready to commit
+
+Working tree = the fix + tests + plan only (build-artifact churn reverted).
+Awaiting go-ahead to commit + open a PR (will run `cargo xtask verify` and add
+the changelog entry as part of that).
+
+## Files likely in scope
+
+- `ts-packages/preview-renderer/src/q2-preview/richtext/RichTextEditor.tsx` —
+  the `useEditor` extension config + the keydown/commit handling (the fix).
+- Possibly a new tiny extension module for the `Mod-Enter` keymap.
+- `hub-client/e2e/q2-preview-richtext-bold-content-loss.spec.ts` — the repro
+  (already written; may add assertions).
+- A new fast unit/integration test under `ts-packages/preview-renderer/…/richtext/`.
+
+## Non-goals
+
+- Structural list editing (Enter-to-split, Tab-to-nest) — separate (bd-sjb4pzx8
+  Phase 1c).
+- Any change to the serializer or the Rust commit path — both verified correct.

hub-client/changelog.md

@@ -15,6 +15,10 @@ be in reverse chronological order (latest first).
 
 -->
 
+### 2026-07-06
+
+- [`550aaeb8`](https://github.com/quarto-dev/q2/commits/550aaeb8): Fixed a rich-text editor bug where committing with Cmd/Ctrl+Enter while text was selected (for example, select-all then bold) could delete the block's content.
+
 ### 2026-07-01
 
 - [`0b13dbcb`](https://github.com/quarto-dev/q2/commits/0b13dbcb): The preview's code-execution controls are now a single status line — executor status, "showing executed output", and the Run/Re-run and Clear-results buttons share one bar instead of stacking as two.

hub-client/e2e/q2-preview-richtext-bold-content-loss.spec.ts

@@ -0,0 +1,152 @@
+/**
+ * bd-hafs0qho — reproduction: a rich-text BOLD commit on a tight-list item can
+ * DROP the item's content.
+ *
+ * Observed manually in `q2 preview --allow-edit`: clicking a tight bullet-list
+ * item opens the tiptap rich editor; select-all + bold + commit wrote an EMPTY
+ * item to disk (`- banana` → `-`). Instrumentation showed the committed
+ * ProseMirror doc had become `paragraph[hardBreak]` (text gone). The serializer
+ * and the Rust commit path are both verified correct in isolation, and the pure
+ * tiptap `selectAll()+toggleBold()+serialize` is ALSO clean — so the fault is in
+ * the real browser event/commit flow, which only a real-keyboard e2e can drive
+ * faithfully (synthetic DOM injection in jsdom/MCP-browser was confounded).
+ *
+ * This spec drives REAL keyboard events (Control+A, Control+B, Control+Enter)
+ * through the rich editor and asserts the item KEEPS its content. It FAILS while
+ * the bug is present (content lost) and passes once fixed.
+ *
+ * Harness mirrors q2-preview-inline-edit.spec.ts, with two changes:
+ *   1. `richText: true` in the seeded preferences (default-on in prod, but the
+ *      e2e preference object otherwise omits it → reads as off), so the rich
+ *      editor opens instead of the textarea.
+ *   2. Activation waits for the ProseMirror surface (`.ProseMirror`), not a
+ *      textarea, and asserts it is the rich editor.
+ */
+
+import { test, expect, type Page, type FrameLocator } from '@playwright/test';
+import type {} from './helpers/testHooks';
+import {
+    bootstrapProjectSet,
+    createProjectOnServer,
+    seedProjectInBrowser,
+    getServerUrl,
+} from './helpers/projectFactory';
+import { waitForPreviewRender } from './helpers/previewExtraction';
+
+async function openFileWithRichText(
+    page: Page,
+    serverUrl: string,
+    docId: string,
+    filename: string,
+): Promise<FrameLocator> {
+    // richText ON + nesting cursor ON (the product default): clicking a tight
+    // list item resolves the INNER Plain (rich editor), not the whole <ul>
+    // (which, being a BulletList, would fall back to the textarea).
+    await page.addInitScript(() => {
+        localStorage.setItem(
+            'quarto-hub:preferences',
+            JSON.stringify({
+                version: 1,
+                scrollSyncEnabled: true,
+                errorOverlayCollapsed: true,
+                colorScheme: 'auto',
+                unlockNestingCursor: true,
+                richText: true,
+            }),
+        );
+    });
+    await bootstrapProjectSet(page, serverUrl);
+    const localId = await seedProjectInBrowser(page, docId, serverUrl);
+    await page.goto(`/#/p/${localId}/file/${filename}`);
+    await waitForPreviewRender(page, { kind: 'q2-preview', timeout: 30000 });
+    const iframe = page.frameLocator('iframe[src*="q2-preview.html"]');
+    await iframe.locator('[data-block-pool-id]').first().waitFor({ timeout: 15_000 });
+    return iframe;
+}
+
+async function assertAutomerge(
+    page: Page,
+    filename: string,
+    { contains = [], lacks = [] }: { contains?: string[]; lacks?: string[] },
+): Promise<void> {
+    await expect(async () => {
+        const text = await page.evaluate(async f => {
+            await window.__quartoTestReady;
+            return window.__quartoTest!.wasmRenderer.getFileContent(f) as string | null;
+        }, filename);
+        expect(text).not.toBeNull();
+        for (const s of contains) expect(text).toContain(s);
+        for (const s of lacks) expect(text).not.toContain(s);
+    }).toPass({ timeout: 10000 });
+}
+
+test.describe('bd-hafs0qho — rich-text bold commit content loss', () => {
+    test.setTimeout(120000);
+
+    test.beforeEach(async ({ page }, testInfo) => {
+        if (testInfo.workerIndex > 0) await page.waitForTimeout(1000);
+    });
+
+    test('bolding a tight bullet-list item preserves its content', async ({ page }) => {
+        const serverUrl = getServerUrl();
+        const QMD =
+            '---\nformat: q2-preview\n---\n\n## List\n\n- apple\n- banana\n- cherry\n';
+        const docId = await createProjectOnServer(serverUrl, [
+            { path: '_quarto.yml', content: 'project:\n  type: default\n', contentType: 'text' },
+            { path: 'lists.qmd', content: QMD, contentType: 'text' },
+        ]);
+
+        const iframe = await openFileWithRichText(page, serverUrl, docId, 'lists.qmd');
+        await expect(iframe.locator('text=banana')).toBeVisible();
+
+        // The tight-list <li> borrows the leading Plain's pool-id; clicking it
+        // targets the Plain. Open the rich editor (ProseMirror), not a textarea.
+        const li = iframe.locator('li[data-block-pool-id]', { hasText: 'banana' }).first();
+        await li.click();
+        const pm = iframe.locator('.ProseMirror');
+        await pm.waitFor({ timeout: 5000 });
+        // Guard: this must be the rich editor, not the textarea fallback.
+        await expect(iframe.locator('textarea')).toHaveCount(0);
+        await expect(pm).toContainText('banana');
+
+        // Real keyboard: select all, bold, commit.
+        await pm.press('ControlOrMeta+a');
+        await pm.press('ControlOrMeta+b');
+        await pm.press('ControlOrMeta+Enter');
+
+        // The item must still contain its text, now bolded. The reported bug
+        // dropped it, yielding an empty bullet.
+        await assertAutomerge(page, 'lists.qmd', {
+            contains: ['**banana**', 'apple', 'cherry'],
+        });
+    });
+
+    test('bolding a paragraph preserves its content (shared-path control)', async ({ page }) => {
+        // Control case on a Para (rich-editable before bd-7pxub583). If this
+        // ALSO loses content, it confirms the bug is in the shared, type-agnostic
+        // RichTextEditor path — not specific to Plain / list items.
+        const serverUrl = getServerUrl();
+        const QMD =
+            '---\nformat: q2-preview\n---\n\n## Para\n\nHello world paragraph.\n';
+        const docId = await createProjectOnServer(serverUrl, [
+            { path: '_quarto.yml', content: 'project:\n  type: default\n', contentType: 'text' },
+            { path: 'para.qmd', content: QMD, contentType: 'text' },
+        ]);
+
+        const iframe = await openFileWithRichText(page, serverUrl, docId, 'para.qmd');
+        await expect(iframe.locator('text=Hello world paragraph.')).toBeVisible();
+
+        await iframe.locator('p[data-block-pool-id]', { hasText: 'Hello world' }).first().click();
+        const pm = iframe.locator('.ProseMirror');
+        await pm.waitFor({ timeout: 5000 });
+        await expect(iframe.locator('textarea')).toHaveCount(0);
+
+        await pm.press('ControlOrMeta+a');
+        await pm.press('ControlOrMeta+b');
+        await pm.press('ControlOrMeta+Enter');
+
+        await assertAutomerge(page, 'para.qmd', {
+            contains: ['**Hello world paragraph.**'],
+        });
+    });
+});

ts-packages/preview-renderer/package.json

@@ -68,6 +68,7 @@
     "@quarto/quarto-automerge-schema": "*",
     "@revealjs/react": "0.2.0",
     "@tiptap/core": "^3.27.1",
+    "@tiptap/extension-hard-break": "^3.27.1",
     "@tiptap/extension-subscript": "^3.27.1",
     "@tiptap/extension-superscript": "^3.27.1",
     "@tiptap/pm": "^3.27.1",