Skip to content

Commit 4963674

Browse files
cscheidclaude
cscheid
and
claude
authored
Fix rich-text editor Mod-Enter commit dropping selected content (#377)
* fix(q2-preview): Mod-Enter commit no longer drops selected content in rich editor In the rich-text (tiptap) block editor, committing with `Mod-Enter` while text was selected deleted the selected text. Most visible path: click a block → select-all → bold → commit → the block was written to disk EMPTY (a tight list item `- banana` became `-`; a paragraph was deleted entirely). Root cause: tiptap's HardBreak extension binds `Mod-Enter` → setHardBreak() — the same key RichTextEditor used to commit, but via a DOM keydown listener that ran AFTER ProseMirror's keymap plugins. So `Mod-Enter` (1) fired setHardBreak, replacing the current selection with a hard break (over a full selection: the text is deleted, leaving `paragraph[hardBreak]`), then (2) the DOM handler's preventDefault fired too late and committed the emptied doc. With a collapsed caret it only appended a harmless trailing hard break — which is why plain text edits looked fine but always carried a stray break. Not specific to `Plain`/list items and not introduced by the Plain-gate change (bd-7pxub583): it reproduces identically on a `Para`, in the shared, type-agnostic RichTextEditor path. Fix (A)+(B): - (B) editorConfig.ts: a shared `buildRichTextExtensions()` disables StarterKit's built-in HardBreak and re-adds one bound to `Shift-Enter` ONLY, so `Mod-Enter` never inserts a break. Adds `@tiptap/extension-hard-break` as a direct dep. - (A) RichTextEditor.tsx: Escape / `Mod-Enter` / `Enter` move into a high-priority tiptap keymap (`q2CommitKeymap`) that runs inside ProseMirror's keymap and wins deterministically; the racy DOM keydown listener is removed (the focusout-commit is kept). Preserved: Escape cancels, plain Enter is swallowed (no split), `Shift-Enter` is a hard break, dirty/stale guards. Tests: - richtext/editorConfig.test.ts (fast jsdom): real `Mod-Enter` over a full selection leaves the text intact; no trailing hardBreak at a collapsed caret; `Shift-Enter` still inserts one. Verified red with the fix reverted. - hub-client/e2e/q2-preview-richtext-bold-content-loss.spec.ts (Playwright, real keyboard): bolding a tight-list item and a paragraph now round-trip to `**text**` on disk with no content loss. Was red before the fix. preview-renderer: tsc clean, 487 unit + 517 integration green. hub-client build + 229 unit tests green. Strand: bd-hafs0qho Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(changelog): note the Mod-Enter rich-text content-loss fix (550aaeb) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent fc512c1 commit 4963674

8 files changed

Lines changed: 563 additions & 42 deletions

File tree

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

Lines changed: 171 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,171 @@
1+
# Rich-text editor: `Mod-Enter` commit drops selected content (HardBreak collision)
2+
3+
**Strand:** bd-hafs0qho (discovered-from bd-7pxub583; related to bd-sjb4pzx8)
4+
**Status:** IN PROGRESS — user approved (A)+(B) on 2026-07-06
5+
6+
## Decision locked with user (2026-07-06)
7+
8+
Do **both (A) and (B)**:
9+
- **(A)** Move the `Mod-Enter` commit into tiptap's keymap so it wins the race
10+
and suppresses `setHardBreak`; retire the DOM `addEventListener` commit path.
11+
- **(B)** Disable HardBreak's `Mod-Enter` binding (keep `Shift-Enter`).
12+
13+
Rationale (user): a hard break on `Mod-Enter` is never wanted here — the rich
14+
editor is **not** intended for extensive multi-block editing; the preview's
15+
plain-text editor and hub-client's source editor cover those cases.
16+
**Date:** 2026-07-06
17+
18+
## Summary
19+
20+
In the `format: q2-preview` rich-text (tiptap) block editor, committing with
21+
`Mod-Enter` while text is **selected** deletes the selected text. The most
22+
visible path: click a block → select-all (`Mod-a`) → bold (`Mod-b`) → commit
23+
(`Mod-Enter`) → the block is written to disk **empty** (a tight list item
24+
`- banana` becomes `-`; a paragraph is deleted entirely).
25+
26+
It is **not** specific to `Plain` / list items and **not** introduced by
27+
bd-7pxub583 — it reproduces identically on a `Para`, in the shared,
28+
type-agnostic RichTextEditor path, and predates the Plain-gate change.
29+
30+
## Reliable reproduction (done)
31+
32+
`hub-client/e2e/q2-preview-richtext-bold-content-loss.spec.ts` (Playwright,
33+
real CDP keyboard events — the only faithful vehicle; jsdom and MCP-browser
34+
synthetic events were confounded). Two cases: a tight bullet-list item and a
35+
paragraph. Each opens the rich editor, does `Mod-a` / `Mod-b` / `Mod-Enter`
36+
(via Playwright `ControlOrMeta`, which maps to ProseMirror's `Mod`), and asserts
37+
the committed qmd contains the **bolded** text. **Currently RED** — the file is
38+
written with the item emptied:
39+
40+
```
41+
## List
42+
43+
* apple
44+
* ← "banana" gone
45+
* cherry
46+
```
47+
48+
> Cross-platform note: use `ControlOrMeta` in the spec. Plain `Control` on macOS
49+
> does NOT trigger ProseMirror's `Mod-b` (which is Meta on mac), so bold never
50+
> fires and the bug is masked — the reason two earlier runs falsely "passed".
51+
52+
## Root cause (confirmed)
53+
54+
tiptap's **HardBreak extension binds `Mod-Enter` → `setHardBreak()`** (verified
55+
in `@tiptap/extension-hard-break/dist/index.js`; it binds both `Mod-Enter` and
56+
`Shift-Enter`). RichTextEditor *also* uses `Mod-Enter` for commit — but via a
57+
**DOM `addEventListener('keydown', …)`** on `editor.view.dom` that is registered
58+
*after* ProseMirror's keymap plugin. So on `Mod-Enter`:
59+
60+
1. ProseMirror/tiptap's keymap runs first → `setHardBreak()` **replaces the
61+
current selection** with a hardBreak node. After select-all + bold the whole
62+
text is selected → the text is **deleted**, leaving `paragraph[hardBreak]`.
63+
2. RichTextEditor's DOM handler runs next → `e.preventDefault()` (too late to
64+
undo step 1) → `commit()` → serializes `paragraph[hardBreak]` → `docToMarkdown`
65+
correctly yields `""` → an empty block is committed.
66+
67+
Corroborating evidence:
68+
- Instrumented commit (captured earlier) showed the committed doc was exactly
69+
`{paragraph:[{hardBreak}]}`, md `""`.
70+
- The **serializer** and **Rust `apply_node_edit`** are both verified correct in
71+
isolation, and **pure tiptap `selectAll()+toggleBold()+serialize`** (no
72+
`Mod-Enter`) is clean — the damage only appears when `Mod-Enter` is pressed.
73+
- Simple text edits "worked" but the committed doc always carried a **trailing**
74+
hardBreak (`[text, hardBreak]`) — the same `Mod-Enter`→hardBreak insertion, but
75+
harmless at a collapsed cursor because it appends rather than replaces, and a
76+
trailing hardBreak serializes to nothing.
77+
78+
So the defect is general: **`Mod-Enter` inserts a hardBreak on every commit**; it
79+
is silent at a collapsed caret and destructive over a selection.
80+
81+
## Fix options (to discuss)
82+
83+
The goal: `Mod-Enter` must commit **without** inserting a hardBreak, deterministically.
84+
85+
- **(A) Move commit into tiptap's keymap (recommended).** Add a small tiptap
86+
extension (or `editor`-level `addKeyboardShortcuts`) binding `Mod-Enter` to a
87+
handler that fires the commit and returns `true`. Because it lives in
88+
ProseMirror's keymap with higher priority than HardBreak, it wins the race and
89+
suppresses `setHardBreak`. This also retires the fragile DOM `addEventListener`
90+
commit path (the root of the race). Escape/plain-Enter handling can move the
91+
same way for consistency.
92+
- **(B) Disable HardBreak's `Mod-Enter` binding.** Configure the HardBreak
93+
extension (via StarterKit) so only `Shift-Enter` inserts a hard break; then
94+
`Mod-Enter` falls through to the existing DOM commit handler with nothing to
95+
race. Smaller change, but leaves the DOM-listener commit path in place.
96+
- **(C) Change the commit key.** Least desirable — `Mod-Enter` is the expected
97+
"confirm" gesture; keep it.
98+
99+
Leaning **(A)**, optionally plus **(B)** as belt-and-suspenders (a hard break on
100+
`Mod-Enter` is never wanted in this single-block editor). Keep `Shift-Enter` as
101+
the intentional hard-break key (the editor already treats `Shift+Enter` as a
102+
hard break; plain `Enter` is swallowed).
103+
104+
Either way, also confirm the **trailing-hardBreak-on-plain-commit** disappears
105+
(no more `[text, hardBreak]` docs), so commits stop carrying a stray break.
106+
107+
## Plan (TDD)
108+
109+
### Phase 1 — Reproduction (DONE)
110+
- [x] Playwright e2e repro (`q2-preview-richtext-bold-content-loss.spec.ts`),
111+
red today; covers list-item **and** paragraph (shared-path proof).
112+
113+
### Phase 2 — Root cause (DONE)
114+
- [x] Identified the `Mod-Enter` ↔ HardBreak `Mod-Enter` collision + DOM-listener
115+
race; corroborated by the captured `paragraph[hardBreak]` commit doc.
116+
117+
### Phase 3 — Tests first
118+
- [x] Keep the e2e repro as the end-to-end regression gate (RED before the fix).
119+
- [x] **Fast editor-config unit test** (`richtext/editorConfig.test.ts`, jsdom):
120+
builds the editor via the shared `buildRichTextExtensions()`, select-all,
121+
dispatches a real `Mod-Enter` keydown, asserts the doc is NOT mutated to a
122+
hardBreak (text intact). Also: collapsed-caret `Mod-Enter` leaves no
123+
trailing hardBreak; `Shift-Enter` still inserts one. **Confirmed RED** with
124+
the fix reverted (default HardBreak), green with it.
125+
126+
### Phase 4 — Implement the fix (A)+(B)
127+
- [x] **(B)** `richtext/editorConfig.ts`: shared static extension config;
128+
disables StarterKit's built-in HardBreak and re-adds one bound to
129+
`Shift-Enter` only. Added `@tiptap/extension-hard-break` as a direct dep.
130+
- [x] **(A)** `RichTextEditor.tsx`: Esc/`Mod-Enter`/`Enter` moved into a
131+
high-priority tiptap keymap extension (`q2CommitKeymap`) via a handlers
132+
ref; the racy DOM `keydown` listener is removed (focusout-commit kept).
133+
- [x] Preserved behavior: Escape cancels; plain Enter swallowed; `Shift-Enter`
134+
is a hard break; dirty guard / stale-target guards unchanged.
135+
- [x] tsc clean; full preview-renderer suites green (487 unit incl. the 3 new,
136+
517 integration); the editor-mount integration tests (p3-4-inline-breadcrumb,
137+
RichTextEditor.caret) still pass.
138+
139+
### Phase 5 — Verify
140+
- [x] **e2e repro GREEN** (was red). Both cases now assert the *bolded* text is
141+
present on disk: `**banana**` (list item) and `**Hello world paragraph.**`
142+
(paragraph). This is the faithful end-to-end check — real hub + real
143+
browser + real `Mod`-keyboard, asserting the Automerge/disk content. It
144+
supersedes the earlier MCP-browser manual check (which was confounded by
145+
synthetic-event hardBreak injection).
146+
- [x] hub-client build (VITE_E2E) green; hub-client unit tests green (229).
147+
- [ ] `cargo xtask verify` (hub leg) — run before push.
148+
- [ ] hub-client changelog entry (two-commit workflow) since `hub-client/` is
149+
touched (the e2e spec) and the fix is user-facing (data-loss fix in the
150+
bundled editor). To add as part of the commit.
151+
152+
## Status: implementation complete + verified; ready to commit
153+
154+
Working tree = the fix + tests + plan only (build-artifact churn reverted).
155+
Awaiting go-ahead to commit + open a PR (will run `cargo xtask verify` and add
156+
the changelog entry as part of that).
157+
158+
## Files likely in scope
159+
160+
- `ts-packages/preview-renderer/src/q2-preview/richtext/RichTextEditor.tsx` —
161+
the `useEditor` extension config + the keydown/commit handling (the fix).
162+
- Possibly a new tiny extension module for the `Mod-Enter` keymap.
163+
- `hub-client/e2e/q2-preview-richtext-bold-content-loss.spec.ts` — the repro
164+
(already written; may add assertions).
165+
- A new fast unit/integration test under `ts-packages/preview-renderer/…/richtext/`.
166+
167+
## Non-goals
168+
169+
- Structural list editing (Enter-to-split, Tab-to-nest) — separate (bd-sjb4pzx8
170+
Phase 1c).
171+
- Any change to the serializer or the Rust commit path — both verified correct.

‎hub-client/changelog.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,10 @@ be in reverse chronological order (latest first).
1515
1616
-->
1717

18+
### 2026-07-06
19+
20+
- [`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.
21+
1822
### 2026-07-01
1923

2024
- [`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‎

Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,152 @@
1+
/**
2+
* bd-hafs0qho — reproduction: a rich-text BOLD commit on a tight-list item can
3+
* DROP the item's content.
4+
*
5+
* Observed manually in `q2 preview --allow-edit`: clicking a tight bullet-list
6+
* item opens the tiptap rich editor; select-all + bold + commit wrote an EMPTY
7+
* item to disk (`- banana` → `-`). Instrumentation showed the committed
8+
* ProseMirror doc had become `paragraph[hardBreak]` (text gone). The serializer
9+
* and the Rust commit path are both verified correct in isolation, and the pure
10+
* tiptap `selectAll()+toggleBold()+serialize` is ALSO clean — so the fault is in
11+
* the real browser event/commit flow, which only a real-keyboard e2e can drive
12+
* faithfully (synthetic DOM injection in jsdom/MCP-browser was confounded).
13+
*
14+
* This spec drives REAL keyboard events (Control+A, Control+B, Control+Enter)
15+
* through the rich editor and asserts the item KEEPS its content. It FAILS while
16+
* the bug is present (content lost) and passes once fixed.
17+
*
18+
* Harness mirrors q2-preview-inline-edit.spec.ts, with two changes:
19+
* 1. `richText: true` in the seeded preferences (default-on in prod, but the
20+
* e2e preference object otherwise omits it → reads as off), so the rich
21+
* editor opens instead of the textarea.
22+
* 2. Activation waits for the ProseMirror surface (`.ProseMirror`), not a
23+
* textarea, and asserts it is the rich editor.
24+
*/
25+
26+
import { test, expect, type Page, type FrameLocator } from '@playwright/test';
27+
import type {} from './helpers/testHooks';
28+
import {
29+
bootstrapProjectSet,
30+
createProjectOnServer,
31+
seedProjectInBrowser,
32+
getServerUrl,
33+
} from './helpers/projectFactory';
34+
import { waitForPreviewRender } from './helpers/previewExtraction';
35+
36+
async function openFileWithRichText(
37+
page: Page,
38+
serverUrl: string,
39+
docId: string,
40+
filename: string,
41+
): Promise<FrameLocator> {
42+
// richText ON + nesting cursor ON (the product default): clicking a tight
43+
// list item resolves the INNER Plain (rich editor), not the whole <ul>
44+
// (which, being a BulletList, would fall back to the textarea).
45+
await page.addInitScript(() => {
46+
localStorage.setItem(
47+
'quarto-hub:preferences',
48+
JSON.stringify({
49+
version: 1,
50+
scrollSyncEnabled: true,
51+
errorOverlayCollapsed: true,
52+
colorScheme: 'auto',
53+
unlockNestingCursor: true,
54+
richText: true,
55+
}),
56+
);
57+
});
58+
await bootstrapProjectSet(page, serverUrl);
59+
const localId = await seedProjectInBrowser(page, docId, serverUrl);
60+
await page.goto(`/#/p/${localId}/file/${filename}`);
61+
await waitForPreviewRender(page, { kind: 'q2-preview', timeout: 30000 });
62+
const iframe = page.frameLocator('iframe[src*="q2-preview.html"]');
63+
await iframe.locator('[data-block-pool-id]').first().waitFor({ timeout: 15_000 });
64+
return iframe;
65+
}
66+
67+
async function assertAutomerge(
68+
page: Page,
69+
filename: string,
70+
{ contains = [], lacks = [] }: { contains?: string[]; lacks?: string[] },
71+
): Promise<void> {
72+
await expect(async () => {
73+
const text = await page.evaluate(async f => {
74+
await window.__quartoTestReady;
75+
return window.__quartoTest!.wasmRenderer.getFileContent(f) as string | null;
76+
}, filename);
77+
expect(text).not.toBeNull();
78+
for (const s of contains) expect(text).toContain(s);
79+
for (const s of lacks) expect(text).not.toContain(s);
80+
}).toPass({ timeout: 10000 });
81+
}
82+
83+
test.describe('bd-hafs0qho — rich-text bold commit content loss', () => {
84+
test.setTimeout(120000);
85+
86+
test.beforeEach(async ({ page }, testInfo) => {
87+
if (testInfo.workerIndex > 0) await page.waitForTimeout(1000);
88+
});
89+
90+
test('bolding a tight bullet-list item preserves its content', async ({ page }) => {
91+
const serverUrl = getServerUrl();
92+
const QMD =
93+
'---\nformat: q2-preview\n---\n\n## List\n\n- apple\n- banana\n- cherry\n';
94+
const docId = await createProjectOnServer(serverUrl, [
95+
{ path: '_quarto.yml', content: 'project:\n type: default\n', contentType: 'text' },
96+
{ path: 'lists.qmd', content: QMD, contentType: 'text' },
97+
]);
98+
99+
const iframe = await openFileWithRichText(page, serverUrl, docId, 'lists.qmd');
100+
await expect(iframe.locator('text=banana')).toBeVisible();
101+
102+
// The tight-list <li> borrows the leading Plain's pool-id; clicking it
103+
// targets the Plain. Open the rich editor (ProseMirror), not a textarea.
104+
const li = iframe.locator('li[data-block-pool-id]', { hasText: 'banana' }).first();
105+
await li.click();
106+
const pm = iframe.locator('.ProseMirror');
107+
await pm.waitFor({ timeout: 5000 });
108+
// Guard: this must be the rich editor, not the textarea fallback.
109+
await expect(iframe.locator('textarea')).toHaveCount(0);
110+
await expect(pm).toContainText('banana');
111+
112+
// Real keyboard: select all, bold, commit.
113+
await pm.press('ControlOrMeta+a');
114+
await pm.press('ControlOrMeta+b');
115+
await pm.press('ControlOrMeta+Enter');
116+
117+
// The item must still contain its text, now bolded. The reported bug
118+
// dropped it, yielding an empty bullet.
119+
await assertAutomerge(page, 'lists.qmd', {
120+
contains: ['**banana**', 'apple', 'cherry'],
121+
});
122+
});
123+
124+
test('bolding a paragraph preserves its content (shared-path control)', async ({ page }) => {
125+
// Control case on a Para (rich-editable before bd-7pxub583). If this
126+
// ALSO loses content, it confirms the bug is in the shared, type-agnostic
127+
// RichTextEditor path — not specific to Plain / list items.
128+
const serverUrl = getServerUrl();
129+
const QMD =
130+
'---\nformat: q2-preview\n---\n\n## Para\n\nHello world paragraph.\n';
131+
const docId = await createProjectOnServer(serverUrl, [
132+
{ path: '_quarto.yml', content: 'project:\n type: default\n', contentType: 'text' },
133+
{ path: 'para.qmd', content: QMD, contentType: 'text' },
134+
]);
135+
136+
const iframe = await openFileWithRichText(page, serverUrl, docId, 'para.qmd');
137+
await expect(iframe.locator('text=Hello world paragraph.')).toBeVisible();
138+
139+
await iframe.locator('p[data-block-pool-id]', { hasText: 'Hello world' }).first().click();
140+
const pm = iframe.locator('.ProseMirror');
141+
await pm.waitFor({ timeout: 5000 });
142+
await expect(iframe.locator('textarea')).toHaveCount(0);
143+
144+
await pm.press('ControlOrMeta+a');
145+
await pm.press('ControlOrMeta+b');
146+
await pm.press('ControlOrMeta+Enter');
147+
148+
await assertAutomerge(page, 'para.qmd', {
149+
contains: ['**Hello world paragraph.**'],
150+
});
151+
});
152+
});

‎package-lock.json‎

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎ts-packages/preview-renderer/package.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@
6868
"@quarto/quarto-automerge-schema": "*",
6969
"@revealjs/react": "0.2.0",
7070
"@tiptap/core": "^3.27.1",
71+
"@tiptap/extension-hard-break": "^3.27.1",
7172
"@tiptap/extension-subscript": "^3.27.1",
7273
"@tiptap/extension-superscript": "^3.27.1",
7374
"@tiptap/pm": "^3.27.1",

0 commit comments

Comments
 (0)