From 4d1f3ab702f500a012b3d85b71f676cd035f91f0 Mon Sep 17 00:00:00 2001 From: PathGao Date: Sat, 8 Aug 2026 10:13:34 +0800 Subject: [PATCH 1/3] feat(preview): Edit jumps to the fragment you right-clicked (#90) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "Edit" in the preview's context menu opened the editor wherever the tab was last left. It now opens it on the block, inline element or selection that was right-clicked, with those lines selected. No new preview-to-source mapping. comrak already renders with `options.render.sourcepos = true`, and `previewAnchor.ts` is already the module that reads those ranges for the tab's reading position and for split-view scroll sync; this adds two small functions there — the climb to the narrowest annotated ancestor, and the union of a selection's two ends — and reuses `parseSourceposLineRange` for the parsing. Nor a new jump. `revealHeader` already revealed and selected a source line for the outline; its line branch now delegates to `revealSourceRange`, which both callers share, so the two cannot scroll or focus differently. That function also gained the clamp `revealHeader` never had: `getLineMaxColumn` throws past the end of the model, and both callers can hand over a line from a render of a buffer that has since got shorter. Lines only, never columns. `data-sourcepos` describes the text `convert_markdown` hands comrak, and only its line numbers are contractually equal to the raw buffer's — the math mask substitutes a token of a different length, so columns after it on the line drift. The selection is the highlight the report asks for: Monaco draws it in the theme's own colour, and it clears itself on the next click or keystroke, so no decoration, CSS or timer is needed. Co-Authored-By: Claude Opus 5 --- scripts/jumpToSelectedFragment.test.ts | 273 +++++++++++++++++++++++++ src/lib/MarkdownViewer.svelte | 69 ++++++- src/lib/components/Editor.svelte | 68 +++++- src/lib/utils/previewAnchor.ts | 62 +++++- 4 files changed, 462 insertions(+), 10 deletions(-) create mode 100644 scripts/jumpToSelectedFragment.test.ts diff --git a/scripts/jumpToSelectedFragment.test.ts b/scripts/jumpToSelectedFragment.test.ts new file mode 100644 index 00000000..7748b5b6 --- /dev/null +++ b/scripts/jumpToSelectedFragment.test.ts @@ -0,0 +1,273 @@ +/** + * #90: "Edit" in the preview's context menu opens the editor ON the fragment + * that was right-clicked, and highlights it. + * + * The mapping this needs already exists. comrak runs with + * `options.render.sourcepos = true` (`markdown_options` in + * src-tauri/src/lib.rs), so every rendered element carries the source range it + * came from, and `previewAnchor.ts` is the module that reads those ranges for + * the tab's reading position and for split-view scroll sync. This feature is a + * third consumer of the same attribute, not a third mapping: it resolves an + * element to a `LineRange` with the parser those two already use, and hands it + * to the one line-to-editor jump in `Editor.svelte` — the one the outline + * already calls through `revealHeader`. + * + * Two facts about `data-sourcepos` that the tests below pin, because getting + * either wrong is silent: + * + * - INLINE nodes carry a range, not just blocks. Recorded from + * `convert_markdown` at comrak 0.54: + * + *

A paragraph with + * bold text and an + * alt + * inline image.

+ * + * which is what makes "jump to the selected image" land on the image's own + * line rather than on the whole paragraph. + * + * - Only the LINE numbers are meaningful against the buffer the user edits. + * comrak parses the output of `convert_markdown`'s preprocessing, and that + * pipeline is line-preserving, not column-preserving. Recorded from the + * same renderer, for the raw line `Math $a+b$ then ![alt](img.png) here.`: + * + *

…` is left alone — which +// is exactly the branch the "does the range survive the rewrite" test must NOT +// be allowed to take. +(globalThis as unknown as Record).window = { + __TAURI_INTERNALS__: { + convertFileSrc: (path: string, protocol: string) => `${protocol}://localhost/${path}`, + }, +}; + +const { processMarkdownHtml } = await import('../src/lib/utils/markdown.ts'); +const { findSourceLineRange, mergeSourceLineRanges } = await import( + '../src/lib/utils/previewAnchor.ts' +); + +const editorSource = readSource(new URL('../src/lib/components/Editor.svelte', import.meta.url)); +const viewerSource = readSource(new URL('../src/lib/MarkdownViewer.svelte', import.meta.url)); + +const FILE_PATH = '/documents/notes.md'; + +/** + * comrak-shaped output for + * + * 1 # Notes + * 2 + * 3 A paragraph with **bold text** and an ![alt](img.png) inline image. + * 4 + * 5 ![standalone](pic.png) + * 6 + * 7 Trailing prose + * 8 over two lines. + * + * taken from `convert_markdown`, hardbreaks and heading anchor included, then + * put through the app's own `processMarkdownHtml` — which is where the fold + * wrapper that carries no source range of its own appears. + */ +const RENDERED = processMarkdownHtml( + '

Notes

\n' + + '

A paragraph with bold text and an alt inline image.

\n' + + '

standalone

\n' + + '

Trailing prose
\nover two lines.

\n', + FILE_PATH, + new Set(), +); + +const body = parseHtml(RENDERED).body; + +/** The first descendant of `root` the shim's `querySelector` can name. */ +function pick(selector: string): ShimElement { + const found = body.querySelector(selector); + assert.ok(found, `expected the rendered preview to contain ${selector}`); + return found; +} + +/** + * The first text node under `node` — where the end of a browser selection, and + * a `MouseEvent.target` inside prose, actually lands. Text nodes have no + * `closest` of their own, which is the case the lookup has to climb out of. + */ +function firstText(node: ShimNode): ShimNode { + if (node.nodeType === NODE_TEXT) return node; + for (const child of node.childNodes) { + const found = firstText(child); + if (found.nodeType === NODE_TEXT) return found; + } + return node; +} + +/* ------------------------------------------------------------------ */ +/* what a click resolves to */ +/* ------------------------------------------------------------------ */ + +test('an image inside a paragraph resolves to the image, not to the paragraph', () => { + // The whole point of #90 for the "or an image" half of the report. The + // enclosing

spans line 3 too here, but the narrowest match is what + // keeps a multi-line paragraph from selecting all of itself. + assert.deepEqual(findSourceLineRange(pick('img[alt="alt"]')), { + startLine: 3, + endLine: 3, + }); +}); + +test('a caret inside a block resolves to the whole block', () => { + // Text nodes have no `closest`; the lookup has to climb to the element. + // A paragraph is the finest granularity available for plain prose, and it + // is the right one: the reader asked to edit this paragraph. + const paragraph = pick('p[data-sourcepos="7:1-8:15"]'); + assert.deepEqual(findSourceLineRange(firstText(paragraph)), { startLine: 7, endLine: 8 }); +}); + +test('an inline node with its own range beats the block around it', () => { + assert.deepEqual(findSourceLineRange(firstText(pick('strong'))), { startLine: 3, endLine: 3 }); +}); + +test('the fold wrapper processMarkdownHtml inserts does not hide the range', () => { + // `processMarkdownHtml` re-parents everything after a heading into a + // `.foldable-content-wrapper` it creates, and that wrapper carries no + // `data-sourcepos`. The climb has to pass straight through it — this is + // the same shape of defect #420 fixed for the restore path. + assert.ok(body.querySelector('.foldable-content-wrapper'), 'expected a fold wrapper'); + assert.deepEqual(findSourceLineRange(pick('img[alt="standalone"]')), { + startLine: 5, + endLine: 5, + }); +}); + +test('anything the app renders around the document resolves to nothing', () => { + // The front matter panel, the outline, the window chrome. The context menu + // leaves its "Edit" entry alone rather than jumping somewhere arbitrary. + assert.equal(findSourceLineRange(null), null); + assert.equal(findSourceLineRange(parseHtml('

x
').body), null); +}); + +/* ------------------------------------------------------------------ */ +/* what a selection resolves to */ +/* ------------------------------------------------------------------ */ + +test('a selection spanning several blocks covers all of them', () => { + const first = findSourceLineRange(firstText(pick('h1'))); + const last = findSourceLineRange(firstText(pick('p[data-sourcepos="7:1-8:15"]'))); + + assert.deepEqual(mergeSourceLineRanges(first, last), { startLine: 1, endLine: 8 }); +}); + +test('a backwards selection covers the same lines as a forwards one', () => { + const a = { startLine: 3, endLine: 3 }; + const b = { startLine: 7, endLine: 8 }; + + assert.deepEqual(mergeSourceLineRanges(a, b), mergeSourceLineRanges(b, a)); + assert.deepEqual(mergeSourceLineRanges(b, a), { startLine: 3, endLine: 8 }); +}); + +test('an end that resolves to nothing leaves the other end in charge', () => { + // Dragging out of the document — into the front matter panel, past the + // last block — must not throw the whole jump away. + assert.deepEqual(mergeSourceLineRanges({ startLine: 4, endLine: 6 }, null), { + startLine: 4, + endLine: 6, + }); + assert.deepEqual(mergeSourceLineRanges(null, { startLine: 4, endLine: 6 }), { + startLine: 4, + endLine: 6, + }); + assert.equal(mergeSourceLineRanges(null, null), null); +}); + +/* ------------------------------------------------------------------ */ +/* the jump itself */ +/* ------------------------------------------------------------------ */ + +test('the outline and the context menu share one line-to-editor jump', () => { + // `revealHeader` used to reveal and select the line itself. Leaving that + // copy in place while #90 grew a second one is the drift + // singleImplementationConvention.test.ts exists to catch: the two would + // scroll differently, focus differently, and only one of them would clamp. + const revealHeader = functionSource(editorSource, 'revealHeader'); + assert.match(revealHeader, /revealSourceRange\(lineNumber, lineNumber\)/); + assert.doesNotMatch(revealHeader, /setSelection\(\{[\s\S]*?startColumn: 1/); +}); + +test('the jump clamps to the buffer before asking Monaco for a column', () => { + // `getLineMaxColumn` throws past the end of the model, and the preview can + // hand over a stale line: its HTML is the render of a buffer that may have + // been replaced by a shorter one since. + const reveal = functionSource(editorSource, 'revealSourceRange'); + assert.match(reveal, /lastLine = model\.getLineCount\(\)/); + assert.match(reveal, /end = Math\.min\([^\n]*lastLine\)/); + assert.doesNotMatch(reveal, /getLineMaxColumn\((?!end\))/); +}); + +test('a jump asked for before Monaco has loaded is queued, not dropped', () => { + // `monaco-editor` is imported dynamically, so the editor exists several + // frames after the component does — and the preview calls in immediately + // after flipping into edit mode. + const reveal = functionSource(editorSource, 'revealSourceRange'); + assert.match(reveal, /if \(!editorReady \|\| !editor\) \{\s*\n\s*pendingReveal = \{ startLine, endLine \};/); + + // Spent after the view-state / anchor-line restore, so an explicit "edit + // this fragment" wins over the position the tab was left at. + const afterReady = sliceFrom(editorSource, 'editorReady = true;'); + assert.ok( + afterReady.indexOf('pendingReveal') < afterReady.indexOf('return () => {'), + 'the queued jump must be spent inside onMount, before the teardown closure', + ); +}); + +/* ------------------------------------------------------------------ */ +/* the context menu wiring */ +/* ------------------------------------------------------------------ */ + +test('the Edit entry resolves its target while the selection still exists', () => { + // Resolving inside the `onClick` would read the selection AFTER the reader + // clicked a menu item, and a click is how a selection goes away. + const handler = functionSource(viewerSource, 'handleContextMenu'); + assert.match(handler, /const editSourceTarget = getContextMenuSourceRange\(e\);/); + assert.match(handler, /t\('menu\.edit', uiLanguage\), onClick: \(\) => editSourceRange\(editSourceTarget\)/); + assert.doesNotMatch(handler, /onClick: \(\) => toggleEdit\(\)/); +}); + +test('Edit with a target never leaves edit mode', () => { + // In split view the editor is already on screen, and "edit this fragment" + // is the one thing that cannot mean "close the editor". + const edit = functionSource(viewerSource, 'editSourceRange'); + assert.match(edit, /if \(!isEditing\) await toggleEdit\(\);/); + // And a read that failed leaves the tab in reading mode — arming the jump + // anyway would fire it at whatever document is edited next. + assert.match(edit, /tabManager\.activeTab\?\.isEditing/); +}); + +test('the target is a source range and never a column', () => { + // `parseSourceposLineRange` is the only reader of the attribute, and it + // keeps line numbers only. Nothing in the preview may take the `:col` half + // and aim Monaco with it — see the header of this file for why. + const resolve = functionSource(viewerSource, 'getContextMenuSourceRange'); + assert.match(resolve, /findSourceLineRange\(/); + assert.doesNotMatch(resolve, /startColumn|endColumn/); +}); diff --git a/src/lib/MarkdownViewer.svelte b/src/lib/MarkdownViewer.svelte index 9b1a77fb..e6eafebe 100644 --- a/src/lib/MarkdownViewer.svelte +++ b/src/lib/MarkdownViewer.svelte @@ -40,13 +40,16 @@ import { routeDroppedFile, type DropPane } from './utils/fileDrop.js'; import { headingReference, preferredReferenceStyle } from './utils/headingReference.js'; import { findAnchorElement, + findSourceLineRange, getAnchorScrollTop, getPreviewOffsetForSourceLine, getSourceLineAtPreviewOffset, measureAnchorBox, + mergeSourceLineRanges, PREVIEW_ANCHOR_OFFSET, type AnchorBox, type AnchorNode, + type LineRange, type OffsetLayoutNode, } from './utils/previewAnchor.js'; import { @@ -143,6 +146,7 @@ import { createDocumentSession, type LoadMarkdownOptions } from './sessions/docu undo: () => void; redo: () => void; revealHeader: (sourceLine: number | null, text: string) => void; + revealSourceRange: (startLine: number, endLine: number) => void; triggerFind: () => void; } | null>(null); let liveMode = $state(false); @@ -1660,6 +1664,64 @@ import { createDocumentSession, type LoadMarkdownOptions } from './sessions/docu } } + /** + * A jump the editor has not been asked for yet, because it does not exist + * yet: `toggleEdit` only flips a flag, and the `Editor` it mounts — and the + * `editorPane` binding that reaches it — arrive on the next render. The + * effect below spends it the moment they do, and immediately when the + * editor is already on screen (split view). + */ + let pendingEditReveal = $state(null); + + $effect(() => { + const range = pendingEditReveal; + if (!range || !editorPane) return; + + pendingEditReveal = null; + editorPane.revealSourceRange(range.startLine, range.endLine); + }); + + /** + * Which source lines the reader means by right-clicking here (#90). + * + * The selection when there is one, so a range spanning several blocks opens + * the editor on all of them; a caret or a click with nothing selected falls + * through to whatever is under the pointer, which is how a click on an + * image lands on the image. + * + * `null` when nothing under the pointer came from the document: the front + * matter panel, the outline, the window chrome. The caller then leaves the + * "Edit" entry doing exactly what it did before. + */ + function getContextMenuSourceRange(e: MouseEvent): LineRange | null { + const selection = window.getSelection(); + const selected = + selection && selection.rangeCount > 0 && !selection.isCollapsed + ? mergeSourceLineRanges( + findSourceLineRange(selection.getRangeAt(0).startContainer), + findSourceLineRange(selection.getRangeAt(selection.rangeCount - 1).endContainer), + ) + : null; + + return selected ?? findSourceLineRange(e.target as Node | null); + } + + /** + * The preview's "Edit": open the editor on what the reader pointed at. + * + * With a range in hand this stops being a toggle. In split view the editor + * is already on screen and the old behaviour — leave edit mode — is the one + * thing "edit this fragment" cannot mean, so the toggle is skipped and only + * the jump happens. With no range (right-click outside the document) the + * entry is untouched. + */ + async function editSourceRange(range: LineRange | null) { + if (!isEditing) await toggleEdit(); + // `toggleEdit` swallows a failed read and stays in reading mode. Arming + // the jump anyway would fire it at whatever document is edited next. + if (range && tabManager.activeTab?.isEditing) pendingEditReveal = range; + } + async function saveContent(tabId?: string): Promise { const saved = await documentSession.saveContent(tabId); // Every route into here is an explicit decision — Cmd+S, the close @@ -2163,6 +2225,11 @@ import { createDocumentSession, type LoadMarkdownOptions } from './sessions/docu ]; } + // Resolved now rather than inside the "Edit" handler: by the time that + // runs the reader has clicked a menu item, and a click is how a + // selection goes away. + const editSourceTarget = getContextMenuSourceRange(e); + const mermaidDiag = (e.target as HTMLElement).closest('.mermaid-diagram'); if (mermaidDiag) { mediaItems = [ @@ -2193,7 +2260,7 @@ import { createDocumentSession, type LoadMarkdownOptions } from './sessions/docu } }, { separator: true }, { label: t('menu.openLocation', uiLanguage), onClick: openFileLocation, disabled: !currentFile }, - { label: t('menu.edit', uiLanguage), onClick: () => toggleEdit() }, + { label: t('menu.edit', uiLanguage), onClick: () => editSourceRange(editSourceTarget) }, { separator: true }, { label: t('menu.closeFile', uiLanguage), onClick: closeFile }, ], diff --git a/src/lib/components/Editor.svelte b/src/lib/components/Editor.svelte index 999df50f..44b2f4fd 100644 --- a/src/lib/components/Editor.svelte +++ b/src/lib/components/Editor.svelte @@ -764,6 +764,14 @@ editorReady = true; + // After the view-state / anchor-line restore above, deliberately: an + // explicit "edit this fragment" beats the position the tab was left at. + if (pendingReveal) { + const { startLine, endLine } = pendingReveal; + pendingReveal = null; + revealSourceRange(startLine, endLine); + } + return () => { editorReady = false; window.open = originalOpen; @@ -1671,20 +1679,64 @@ dragCaretDecoration = editor.deltaDecorations(dragCaretDecoration, []); } + /** + * A jump asked for before the editor existed. `monaco-editor` is imported + * dynamically, so `bind:this` on this component resolves — and the preview + * can call in — several frames before `editor` does; without somewhere to + * put it, a jump issued in the same turn as the switch into edit mode is + * simply dropped. `onMount` spends it once the editor is up. + */ + let pendingReveal: { startLine: number; endLine: number } | null = null; + + /** + * Put the reader on `startLine`..`endLine` of the buffer. + * + * The one line-to-editor jump in this component. The outline reaches it + * through `revealHeader`, and the preview's context-menu "Edit" calls it + * with the source range of whatever the reader had selected (#90). + * + * The selection IS the highlight #90 asks for: Monaco draws it in the + * theme's own selection colour, so it needs no decoration, no CSS and no + * timer — and it clears itself on the reader's next click or keystroke, + * which is precisely when it has stopped being useful. A decoration that + * fades on a timer would be a second highlighting mechanism doing what this + * one already does, and would leave the caret at the top of the file. + */ + export function revealSourceRange(startLine: number, endLine: number) { + if (!editorReady || !editor) { + pendingReveal = { startLine, endLine }; + return; + } + + const model = editor.getModel(); + if (!model) return; + + // Clamp before `getLineMaxColumn`, which throws on a line past the end + // of the buffer. Both callers can hand one over: the preview's HTML is + // the render of a buffer that may since have been replaced by a shorter + // one (an external change, a session restored around a file edited + // elsewhere), and the outline carries the same lines. + const lastLine = model.getLineCount(); + const start = Math.min(Math.max(1, Math.trunc(startLine)), lastLine); + const end = Math.min(Math.max(start, Math.trunc(endLine)), lastLine); + + editor.revealLineInCenterIfOutsideViewport(start, monaco.editor.ScrollType.Smooth); + editor.setSelection({ + startLineNumber: start, + startColumn: 1, + endLineNumber: end, + endColumn: model.getLineMaxColumn(end), + }); + editor.focus(); + } + export function revealHeader(sourceLine: number | null, text: string) { if (!editor) return; const model = editor.getModel(); if (!model) return; const lineNumber = sourceLine ?? 0; if (Number.isInteger(lineNumber) && lineNumber > 0) { - editor.revealLineInCenterIfOutsideViewport(lineNumber, monaco.editor.ScrollType.Smooth); - editor.setSelection({ - startLineNumber: lineNumber, - startColumn: 1, - endLineNumber: lineNumber, - endColumn: model.getLineMaxColumn(lineNumber), - }); - editor.focus(); + revealSourceRange(lineNumber, lineNumber); return; } diff --git a/src/lib/utils/previewAnchor.ts b/src/lib/utils/previewAnchor.ts index a2108e6b..d1483181 100644 --- a/src/lib/utils/previewAnchor.ts +++ b/src/lib/utils/previewAnchor.ts @@ -29,7 +29,7 @@ */ /** Inclusive source line range, as written in `data-sourcepos`. */ -type LineRange = { +export type LineRange = { startLine: number; endLine: number; }; @@ -148,6 +148,66 @@ export function parseSourceposLineRange(sourcepos: string | null | undefined): L return { startLine, endLine }; } +/** + * The subset of `Element` the lookup below reads, declared structurally for the + * same reason `AnchorNode` is: so it can be exercised against the + * render-protocol DOM shim over real `processMarkdownHtml` output. + */ +export type SourceposElement = { + getAttribute(name: string): string | null; + closest(selector: string): SourceposElement | null; +}; + +/** Any DOM node a selection or a pointer event can land on. */ +export type SourceposNode = { + readonly nodeType: number; + readonly parentElement?: SourceposElement | null; + closest?(selector: string): SourceposElement | null; +}; + +/** + * The source lines behind whatever `node` is part of: the narrowest annotated + * element at or above it. + * + * Narrowest matters. comrak stamps a range on inline nodes as well as blocks — + * ``, ``, ``, ``, `` all carry one — so an image + * inside a paragraph answers with the image's own line instead of the whole + * paragraph's. Everything the app renders around the document (the front + * matter panel, the outline, the window chrome) has no annotated ancestor and + * answers `null`. + */ +export function findSourceLineRange(node: SourceposNode | null | undefined): LineRange | null { + const element = node?.nodeType === ELEMENT_NODE ? node : node?.parentElement; + const annotated = element?.closest?.('[data-sourcepos]'); + return parseSourceposLineRange(annotated?.getAttribute('data-sourcepos')); +} + +/** + * The lines two ends of a preview selection cover together. + * + * A selection in the preview can start in one rendered block and end in + * another, and the two ends resolve independently — so "which source lines did + * the reader select" is the union of what each end resolved to, and one end on + * its own when the other landed somewhere with no source range (the front + * matter panel, the outline, a rendered diagram that lost its range). + * + * Lines, never columns, here and everywhere else this attribute is read. + * `data-sourcepos` describes the text `convert_markdown` handed comrak, not the + * buffer the user edits: only the LINE numbers of the two are contractually + * equal (`line_preserving_transforms` in src-tauri/src/lib.rs). The columns are + * not — masking `$a+b$` for the math pass substitutes a token of a different + * length, so every column after it on that line is off by the difference. + */ +export function mergeSourceLineRanges(a: LineRange | null, b: LineRange | null): LineRange | null { + if (!a) return b; + if (!b) return a; + + return { + startLine: Math.min(a.startLine, b.startLine), + endLine: Math.max(a.endLine, b.endLine), + }; +} + function elementChildren(node: AnchorNode): AnchorNode[] { const out: AnchorNode[] = []; for (const child of node.childNodes) { From 71d8ac2e748cd76fe04d84caaff0555306f475b1 Mon Sep 17 00:00:00 2001 From: PathGao Date: Sat, 8 Aug 2026 11:02:37 +0800 Subject: [PATCH 2/3] =?UTF-8?q?fix(preview):=20shift=20renderer=20line=20n?= =?UTF-8?q?umbers=20onto=20the=20buffer,=20and=20carry=20the=20selection?= =?UTF-8?q?=20into=20=E2=8C=98E?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups from testing the jump by hand. **The jump landed short.** `renderMarkdownPreview` hands comrak `getMarkdownBodyWithoutFrontMatter(raw)`, so every `data-sourcepos` counts from the first line of the BODY. The editor holds the whole file. In a document with front matter every jump was early by its height — 11 lines in `samples/stress-test.md`. Nothing in the attribute says which of the two numberings it means, and the difference is invisible in a document without front matter, which was every fixture in the suite. **The outline has had the same bug for as long as both have existed.** `Toc.svelte` reads the same attribute and passes it to `revealHeader` untouched, so clicking a heading in a document with front matter has always landed short too. It went unnoticed because `revealHeader` falls back to a text search when the line is null, and that path is correct. Both now go through `toBufferRange`. Worth noting the task-checkbox write-back is *not* affected: it counts lines in the body as well, so both sides of that comparison share one numbering. **⌘E and the toolbar now carry the selection too.** The context menu's "Edit" already opened the editor on what you had selected; the entry points people actually use did not. `toggleEdit` resolves the selection before it flips `isEditing` — reading after would ask whether the reader was in the editor rather than in the preview — and arms the jump only if the switch took, since the read can fail and leave the tab in reading mode. `editSourceRange` passes `revealSelection: false` because it has already captured its target: clicking a menu item is how a selection goes away. Tests cover the arithmetic (including CRLF, a `---` that is not front matter, and the real stress document) and, separately, that all three call sites route through the shift — pinning the arithmetic alone would pass with a call site still handing over a raw body line. Co-Authored-By: Claude Opus 5 --- scripts/jumpToSelectedFragment.test.ts | 91 +++++++++++++++++++++++++- src/lib/MarkdownViewer.svelte | 78 ++++++++++++++++++---- src/lib/utils/frontMatter.ts | 20 ++++++ 3 files changed, 175 insertions(+), 14 deletions(-) diff --git a/scripts/jumpToSelectedFragment.test.ts b/scripts/jumpToSelectedFragment.test.ts index 7748b5b6..a76b1969 100644 --- a/scripts/jumpToSelectedFragment.test.ts +++ b/scripts/jumpToSelectedFragment.test.ts @@ -257,7 +257,11 @@ test('Edit with a target never leaves edit mode', () => { // In split view the editor is already on screen, and "edit this fragment" // is the one thing that cannot mean "close the editor". const edit = functionSource(viewerSource, 'editSourceRange'); - assert.match(edit, /if \(!isEditing\) await toggleEdit\(\);/); + assert.match(edit, /if \(!isEditing\) await toggleEdit\(/, 'entered only when not already editing'); + // And it does not ask the toggle to resolve a selection of its own: this + // path already has its target, captured before the menu click that would + // have cleared it. + assert.match(edit, /toggleEdit\(\{ revealSelection: false \}\)/); // And a read that failed leaves the tab in reading mode — arming the jump // anyway would fire it at whatever document is edited next. assert.match(edit, /tabManager\.activeTab\?\.isEditing/); @@ -271,3 +275,88 @@ test('the target is a source range and never a column', () => { assert.match(resolve, /findSourceLineRange\(/); assert.doesNotMatch(resolve, /startColumn|endColumn/); }); + +// ---------------------------------------------------- the front-matter shift +// +// `renderMarkdownPreview` hands comrak `getMarkdownBodyWithoutFrontMatter(raw)`, +// so every `data-sourcepos` counts from the first line of the BODY while the +// editor holds the whole file. Nothing in the attribute says which of the two +// it means, and in a document without front matter the two are equal — which +// is every other fixture in this file, and why the shift went unseen in the +// outline for as long as both have existed. + +const { frontMatterLineOffset } = await import('../src/lib/utils/frontMatter.js'); + +test('a document without front matter needs no shift', () => { + assert.equal(frontMatterLineOffset('# Title\n\nbody\n'), 0); +}); + +test('the shift is the number of buffer lines above the body', () => { + const raw = ['---', 'title: "T"', 'tags:', ' - a', '---', '', '# Title'].join('\n') + '\n'; + // The heading is line 7 of the buffer and line 1 of the body. + assert.equal(frontMatterLineOffset(raw), 6); + assert.equal(raw.split('\n')[1 + frontMatterLineOffset(raw) - 1], '# Title'); +}); + +test('the shift counts CRLF lines the same', () => { + const raw = ['---', 'title: "T"', '---', '', '# Title'].join('\r\n') + '\r\n'; + assert.equal(frontMatterLineOffset(raw), 4); + assert.equal(raw.split('\r\n')[1 + frontMatterLineOffset(raw) - 1], '# Title'); +}); + +test('a --- that is not front matter shifts nothing', () => { + // A horizontal rule as the first line is not front matter, and treating it + // as such would push every jump down by the width of whatever followed. + const raw = ['---', '', 'Just a rule above some prose.'].join('\n') + '\n'; + assert.equal(frontMatterLineOffset(raw), 0); +}); + +test('the real stress document shifts by its front matter', () => { + // The fixture the reporter of the offset was reading: 10 lines of front + // matter plus the blank line after it, heading on buffer line 12. + const raw = readSource(new URL('../samples/stress-test.md', import.meta.url)); + assert.equal(frontMatterLineOffset(raw), 11); + assert.equal(raw.split('\n')[11], '# Markdown Reader Stress Test'); +}); + +// ---------------------------------------- the shift, and where it is applied +// +// Two consumers read `data-sourcepos` and hand the number to the editor: the +// context menu (#90) and the outline. Both need the same shift, so both go +// through `toBufferRange`. A test that only pinned the arithmetic would pass +// with either call site still handing over a raw body line. + +test('every renderer line reaching the editor goes through the shift', () => { + const edit = functionSource(viewerSource, 'editSourceRange'); + assert.match(edit, /pendingEditReveal = toBufferRange\(range\)/, 'the context menu shifts'); + + const toggle = functionSource(viewerSource, 'toggleEdit'); + assert.match(toggle, /pendingEditReveal = toBufferRange\(carried\)/, 'the carried selection shifts'); + + // The outline is wired inline in the markup rather than in a function. + assert.match( + viewerSource, + /editorPane\.revealHeader\(\s*sourceLine === null \? null : toBufferRange\(/, + 'the outline shifts', + ); +}); + +test('the shift reads the buffer, not the rendered body', () => { + // `frontMatterLineOffset(rawContent)` — measuring the render would answer 0 + // forever, since the render is what the front matter was stripped out of. + const shift = functionSource(viewerSource, 'toBufferRange'); + assert.match(shift, /frontMatterLineOffset\(rawContent\)/); +}); + +test('a selection carried into edit mode is read before the flag flips', () => { + // `isEditing` is flipped inside `toggleEdit`, so reading the selection + // after it would ask "was the reader in the editor" instead of "was the + // reader in the preview" — and leaving the editor would arm a jump for the + // next time it is entered. + const toggle = functionSource(viewerSource, 'toggleEdit'); + assert.match(toggle, /const carried = !isEditing && revealSelection \? getSelectionSourceRange\(\) : null;/); + assert.ok( + toggle.indexOf('const carried') < toggle.indexOf('tab.isEditing = true'), + 'read before the switch, not after', + ); +}); diff --git a/src/lib/MarkdownViewer.svelte b/src/lib/MarkdownViewer.svelte index e6eafebe..4e6e3028 100644 --- a/src/lib/MarkdownViewer.svelte +++ b/src/lib/MarkdownViewer.svelte @@ -54,6 +54,7 @@ import { } from './utils/previewAnchor.js'; import { addFrontMatterListItems, + frontMatterLineOffset, getMarkdownBodyWithoutFrontMatter, getFrontMatterListItems, parseFrontMatter, @@ -1606,10 +1607,23 @@ import { createDocumentSession, type LoadMarkdownOptions } from './sessions/docu } } - async function toggleEdit() { + /** + * @param revealSelection Carry whatever the reader has selected in the + * preview into the editor, so ⌘E and the toolbar button land on the passage + * being read rather than at the top of the file — the same behaviour the + * context menu's "Edit" gives, from the entry points people actually use. + * `editSourceRange` passes false because it has already resolved its target: + * the reader clicked a menu item to get there, and a click is how a + * selection goes away. + */ + async function toggleEdit({ revealSelection = true }: { revealSelection?: boolean } = {}) { const tab = tabManager.activeTab; if (!tab || tab.path === undefined) return; + // Read before the switch: `isEditing` flips below, and leaving the + // editor must not arm a jump for the next time it is entered. + const carried = !isEditing && revealSelection ? getSelectionSourceRange() : null; + if (isEditing) { // Switch back to view. // @@ -1662,6 +1676,11 @@ import { createDocumentSession, type LoadMarkdownOptions } from './sessions/docu tab.isEditing = true; } } + + // Only once the switch actually took: the read above can fail and leave + // the tab in reading mode, and a jump armed then would fire at whatever + // document is edited next. + if (carried && tab.isEditing) pendingEditReveal = toBufferRange(carried); } /** @@ -1694,16 +1713,26 @@ import { createDocumentSession, type LoadMarkdownOptions } from './sessions/docu * "Edit" entry doing exactly what it did before. */ function getContextMenuSourceRange(e: MouseEvent): LineRange | null { - const selection = window.getSelection(); - const selected = - selection && selection.rangeCount > 0 && !selection.isCollapsed - ? mergeSourceLineRanges( - findSourceLineRange(selection.getRangeAt(0).startContainer), - findSourceLineRange(selection.getRangeAt(selection.rangeCount - 1).endContainer), - ) - : null; + return getSelectionSourceRange() ?? findSourceLineRange(e.target as Node | null); + } - return selected ?? findSourceLineRange(e.target as Node | null); + /** + * The source lines the reader has selected in the preview, or null when + * nothing is selected or the selection came from outside the document. + * + * Both ends resolve independently and are merged, so a selection spanning + * several blocks answers with all of them, and one end landing somewhere + * with no range (the front matter panel, the outline) leaves the other end + * in charge. Direction-independent: `startContainer` is the range's start, + * not the point the drag began at. + */ + function getSelectionSourceRange(): LineRange | null { + const selection = window.getSelection(); + if (!selection || selection.rangeCount === 0 || selection.isCollapsed) return null; + return mergeSourceLineRanges( + findSourceLineRange(selection.getRangeAt(0).startContainer), + findSourceLineRange(selection.getRangeAt(selection.rangeCount - 1).endContainer), + ); } /** @@ -1716,10 +1745,26 @@ import { createDocumentSession, type LoadMarkdownOptions } from './sessions/docu * entry is untouched. */ async function editSourceRange(range: LineRange | null) { - if (!isEditing) await toggleEdit(); + if (!isEditing) await toggleEdit({ revealSelection: false }); // `toggleEdit` swallows a failed read and stays in reading mode. Arming // the jump anyway would fire it at whatever document is edited next. - if (range && tabManager.activeTab?.isEditing) pendingEditReveal = range; + if (range && tabManager.activeTab?.isEditing) pendingEditReveal = toBufferRange(range); + } + + /** + * A renderer line range moved onto the buffer's numbering. + * + * `data-sourcepos` counts from the first line of the BODY, because that is + * what `renderMarkdownPreview` hands comrak — front matter is stripped + * first. The editor holds the whole file. Without this every jump into a + * document with front matter lands that many lines early, and the outline + * has been doing exactly that: `Toc.svelte` reads the same attribute and + * passes it to `revealHeader` untouched. + */ + function toBufferRange(range: LineRange): LineRange { + const offset = frontMatterLineOffset(rawContent); + if (!offset) return range; + return { startLine: range.startLine + offset, endLine: range.endLine + offset }; } async function saveContent(tabId?: string): Promise { @@ -3561,7 +3606,14 @@ import { createDocumentSession, type LoadMarkdownOptions } from './sessions/docu oncopyref={(text: string, slug: string) => copyHeadingReference(text, slug)} onjump={(id: string, text: string, sourceLine: number | null) => { if (isEditing && editorPane) { - editorPane.revealHeader(sourceLine, text); + // Same renderer-to-buffer shift as the context menu: the + // outline reads `data-sourcepos` too, and has been landing + // short by the front matter's height for as long as both + // have existed. + editorPane.revealHeader( + sourceLine === null ? null : toBufferRange({ startLine: sourceLine, endLine: sourceLine }).startLine, + text, + ); } }} oncontext={(e, item) => { diff --git a/src/lib/utils/frontMatter.ts b/src/lib/utils/frontMatter.ts index 8e670439..d1eeda64 100644 --- a/src/lib/utils/frontMatter.ts +++ b/src/lib/utils/frontMatter.ts @@ -154,6 +154,26 @@ export function getMarkdownBodyWithoutFrontMatter(content: string): string { return parseFrontMatter(content).body; } +/** + * How many buffer lines sit above the body — the number to add to any line + * number that came out of the renderer to get back to the buffer. + * + * The preview renders `getMarkdownBodyWithoutFrontMatter(raw)`, so every + * `data-sourcepos` comrak emits counts from the first line of the BODY. The + * editor holds the whole file. Nothing in the attribute says which of the two + * it means, and the difference is invisible in any document without front + * matter — which is most documents, and was every test fixture. + * + * `parseFrontMatter` returns the body as a suffix of the content, so the + * offset is the newline count of everything before it. Counting `\n` alone is + * correct for CRLF too, since `\r\n` contains one. + */ +export function frontMatterLineOffset(content: string): number { + const { body } = parseFrontMatter(content); + if (body.length === content.length) return 0; + return content.slice(0, content.length - body.length).split('\n').length - 1; +} + export function parseFrontMatterEditableValue(field: FrontMatterField, value: string): unknown { const trimmed = value.trim(); switch (field.kind) { From b8dac085c4d46185e809e0b2790648f16e49a940 Mon Sep 17 00:00:00 2001 From: PathGao Date: Sat, 8 Aug 2026 11:33:06 +0800 Subject: [PATCH 3/3] =?UTF-8?q?fix(view):=20one=20meaning=20for=20?= =?UTF-8?q?=E2=8C=98E,=20and=20end=20the=20jump=20highlight=20on=20any=20m?= =?UTF-8?q?ove?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **⌘E and the preview's "Edit" are now the same move.** They were two implementations of one intent, and they disagreed: "Edit" carried the reader to the fragment they had selected, the chord did not, and in split view the chord did nothing at all. `toggleEditView` is now what ⌘E means, and the hotkey, the toolbar, the title bar and Monaco's own command all route through it — so the chord cannot mean one thing with the caret in the editor and another with it in the preview. `formatShortcutKeymap.test.ts` holds those two layers together and caught the divergence while this was being written. reading → editor, on the selected fragment editor (alone) → back to reading split + selection → jump to it; the layout does not move split, no selection→ nothing **The inert case is the considered one.** Split view already grants what ⌘E asks for — the editor is on screen — and with no selection there is no fragment to travel to, so every remaining reading of the chord is a layout change nobody requested. A mistyped ⌘E would cost the reader the preview pane and a keystroke to get it back; doing nothing costs nothing. ⌘\ opens and closes the split and stays the only way. `toggleEdit` goes back to being only a mode switch. Resolving the selection lives in `toggleEditView`, above every path that flips `isEditing`, because a selection read after the switch answers for the editor rather than for the preview the reader was looking at. **The outline's jump highlight could not be dismissed.** It is a temporary emphasis, but the only thing that removed it was a scroll event on the preview — and the jump's own smooth scroll is swallowed by `clickLock`, so unless the reader scrolled again afterwards it stayed until the document was re-rendered. A pointerdown in the preview and a keydown anywhere now end it too, both bypassing `clickLock`: that lock exists to ignore scrolling the app itself caused, and a deliberate action by the reader is never that. Both were found while testing #90 by hand. Neither is from that change — the ⌘E branch is #421's and the highlight is older — but both are in the paths it made people exercise. Co-Authored-By: Claude Opus 5 --- scripts/formatShortcutKeymap.test.ts | 2 +- scripts/jumpToSelectedFragment.test.ts | 55 +++++++++++++------- scripts/tocFollowsEditor.test.ts | 34 +++++++++++- src/lib/MarkdownViewer.svelte | 71 ++++++++++++++++++-------- src/lib/components/Toc.svelte | 44 ++++++++++++++-- 5 files changed, 160 insertions(+), 46 deletions(-) diff --git a/scripts/formatShortcutKeymap.test.ts b/scripts/formatShortcutKeymap.test.ts index 3aa23cc9..534fede1 100644 --- a/scripts/formatShortcutKeymap.test.ts +++ b/scripts/formatShortcutKeymap.test.ts @@ -214,7 +214,7 @@ test('a chord that both layers answer means the same thing in both', () => { 'file-open': 'selectFile', 'file-save': 'saveContent', 'file-close': 'closeFile', - 'view-toggle-edit': 'toggleEdit', + 'view-toggle-edit': 'toggleEditView', 'view-toggle-split': 'toggleSplitView', 'tab-undo-close': 'handleUndoCloseTab', }; diff --git a/scripts/jumpToSelectedFragment.test.ts b/scripts/jumpToSelectedFragment.test.ts index a76b1969..2cdc9e15 100644 --- a/scripts/jumpToSelectedFragment.test.ts +++ b/scripts/jumpToSelectedFragment.test.ts @@ -257,11 +257,7 @@ test('Edit with a target never leaves edit mode', () => { // In split view the editor is already on screen, and "edit this fragment" // is the one thing that cannot mean "close the editor". const edit = functionSource(viewerSource, 'editSourceRange'); - assert.match(edit, /if \(!isEditing\) await toggleEdit\(/, 'entered only when not already editing'); - // And it does not ask the toggle to resolve a selection of its own: this - // path already has its target, captured before the menu click that would - // have cleared it. - assert.match(edit, /toggleEdit\(\{ revealSelection: false \}\)/); + assert.match(edit, /if \(!isEditing\) await toggleEdit\(\)/, 'entered only when not already editing'); // And a read that failed leaves the tab in reading mode — arming the jump // anyway would fire it at whatever document is edited next. assert.match(edit, /tabManager\.activeTab\?\.isEditing/); @@ -327,11 +323,11 @@ test('the real stress document shifts by its front matter', () => { // with either call site still handing over a raw body line. test('every renderer line reaching the editor goes through the shift', () => { + // Two consumers hand a `data-sourcepos` number to the editor, and both have + // to shift it. `toggleEditView` is not a third: it resolves the range and + // passes it to `editSourceRange`, which is where the shift happens. const edit = functionSource(viewerSource, 'editSourceRange'); - assert.match(edit, /pendingEditReveal = toBufferRange\(range\)/, 'the context menu shifts'); - - const toggle = functionSource(viewerSource, 'toggleEdit'); - assert.match(toggle, /pendingEditReveal = toBufferRange\(carried\)/, 'the carried selection shifts'); + assert.match(edit, /pendingEditReveal = toBufferRange\(range\)/, 'the context menu and ⌘E shift'); // The outline is wired inline in the markup rather than in a function. assert.match( @@ -348,15 +344,38 @@ test('the shift reads the buffer, not the rendered body', () => { assert.match(shift, /frontMatterLineOffset\(rawContent\)/); }); -test('a selection carried into edit mode is read before the flag flips', () => { - // `isEditing` is flipped inside `toggleEdit`, so reading the selection - // after it would ask "was the reader in the editor" instead of "was the - // reader in the preview" — and leaving the editor would arm a jump for the - // next time it is entered. - const toggle = functionSource(viewerSource, 'toggleEdit'); - assert.match(toggle, /const carried = !isEditing && revealSelection \? getSelectionSourceRange\(\) : null;/); +test('the selection is read before anything switches mode', () => { + // `toggleEdit` and `editSourceRange` both flip `isEditing`, and a selection + // read after that would answer for the editor rather than for the preview + // the reader was looking at. + const view = functionSource(viewerSource, 'toggleEditView'); + assert.match(view, /const selected = getSelectionSourceRange\(\);/); assert.ok( - toggle.indexOf('const carried') < toggle.indexOf('tab.isEditing = true'), - 'read before the switch, not after', + view.indexOf('const selected') < view.indexOf('toggleEdit()'), + 'resolved first, then the mode changes', + ); +}); + +test('one function owns what ⌘E means, and every entry point uses it', () => { + // The hotkey, the toolbar, the title bar and Monaco's own command all route + // here, so the chord cannot mean one thing with the caret in the editor and + // another with it in the preview — which is what + // `formatShortcutKeymap.test.ts` pins from the other side. + assert.match(viewerSource, /if \(cmdOrCtrl && key === 'e'\) \{[\s\S]{0,600}?toggleEditView\(\)/); + assert.equal( + (viewerSource.match(/ontoggleEdit=\{\(\) => toggleEditView\(\)\}/g) ?? []).length, + 3, + 'all three component entry points', ); + assert.doesNotMatch(viewerSource, /ontoggleEdit=\{\(\) => toggleEdit\(\)\}/, 'none left on the raw toggle'); +}); + +test('split view with nothing selected is deliberately inert', () => { + // The editor is already on screen, so the ability ⌘E asks for is already + // granted; with no selection there is nothing to travel to either. Closing + // the preview would be a layout change nobody asked for, and a mistyped ⌘E + // would cost the reader the pane. + const view = functionSource(viewerSource, 'toggleEditView'); + assert.match(view, /if \(isSplit && !selected\) \{[\s\S]*?return;/); + assert.doesNotMatch(view, /setSplitEnabled/, 'the chord never changes the layout'); }); diff --git a/scripts/tocFollowsEditor.test.ts b/scripts/tocFollowsEditor.test.ts index 0c975f8c..fd7db286 100644 --- a/scripts/tocFollowsEditor.test.ts +++ b/scripts/tocFollowsEditor.test.ts @@ -128,7 +128,39 @@ test('the outline no longer measures the preview to decide', () => { assert.doesNotMatch(handler, /querySelector/); // What it still does: a click leaves a highlight on its target, and the // next scroll takes it off. - assert.match(handler, /activeTargetEl\.classList\.remove\('toc-target-active'\)/); + assert.match(handler, /clearTargetHighlight\(\)/); +}); + +test('the jump highlight ends on anything that means the reader moved on', () => { + // It is a temporary emphasis. Hanging it off scrolling alone made it + // permanent whenever that one event did not arrive: the jump's own smooth + // scroll is swallowed by `clickLock`, and nothing else scrolls the preview + // unless the reader does. A click or a keystroke has to end it too. + const clear = sliceBetween(tocSource, 'function clearTargetHighlight()', '\n\tfunction '); + assert.match(clear, /classList\.remove\('toc-target-active'\)/, 'one place removes the class'); + assert.match(clear, /activeTargetEl = null/); + + // And the reader's own actions must not go through the lock, which exists + // only to ignore scrolling the app itself caused. + const reader = sliceBetween(tocSource, 'function handleReaderAction()', '\n\t}'); + assert.doesNotMatch(reader, /clickLock/, 'a deliberate action is never locked out'); + assert.match(reader, /clearTargetHighlight\(\)/); + + for (const [target, event] of [ + ['el', 'pointerdown'], + ['window', 'keydown'], + ] as const) { + assert.match( + tocSource, + new RegExp(`${target}\\.addEventListener\\('${event}', handleReaderAction`), + `${event} clears it`, + ); + assert.match( + tocSource, + new RegExp(`${target}\\.removeEventListener\\('${event}', handleReaderAction\\)`), + `${event} is detached again`, + ); + } }); test('there is no preference for it, because the preview never had one', () => { diff --git a/src/lib/MarkdownViewer.svelte b/src/lib/MarkdownViewer.svelte index 4e6e3028..2276f310 100644 --- a/src/lib/MarkdownViewer.svelte +++ b/src/lib/MarkdownViewer.svelte @@ -1608,22 +1608,14 @@ import { createDocumentSession, type LoadMarkdownOptions } from './sessions/docu } /** - * @param revealSelection Carry whatever the reader has selected in the - * preview into the editor, so ⌘E and the toolbar button land on the passage - * being read rather than at the top of the file — the same behaviour the - * context menu's "Edit" gives, from the entry points people actually use. - * `editSourceRange` passes false because it has already resolved its target: - * the reader clicked a menu item to get there, and a click is how a - * selection goes away. + * Move the active tab between reading and editing. Just the mode — where + * the reader LANDS is `editSourceRange`'s business, and `toggleEditView` + * is what decides which of the two a ⌘E means. */ - async function toggleEdit({ revealSelection = true }: { revealSelection?: boolean } = {}) { + async function toggleEdit() { const tab = tabManager.activeTab; if (!tab || tab.path === undefined) return; - // Read before the switch: `isEditing` flips below, and leaving the - // editor must not arm a jump for the next time it is entered. - const carried = !isEditing && revealSelection ? getSelectionSourceRange() : null; - if (isEditing) { // Switch back to view. // @@ -1676,11 +1668,6 @@ import { createDocumentSession, type LoadMarkdownOptions } from './sessions/docu tab.isEditing = true; } } - - // Only once the switch actually took: the read above can fail and leave - // the tab in reading mode, and a jump armed then would fire at whatever - // document is edited next. - if (carried && tab.isEditing) pendingEditReveal = toBufferRange(carried); } /** @@ -1744,8 +1731,47 @@ import { createDocumentSession, type LoadMarkdownOptions } from './sessions/docu * the jump happens. With no range (right-click outside the document) the * entry is untouched. */ + /** + * What ⌘E means, from every entry point that offers it — the hotkey, the + * toolbar, the title bar, and Monaco's own command. + * + * One function so the chord cannot mean two things depending on where the + * caret happens to be (`formatShortcutKeymap.test.ts` holds the two layers + * together), and so "take me to the editor" behaves the same whether the + * reader asked for it with a key or with a menu item. + */ + async function toggleEditView() { + const selected = getSelectionSourceRange(); + + if (isEditing && !isSplit) { + // The editor is already the whole window: nowhere further to take + // the reader, so this is the toggle back out. + await toggleEdit(); + return; + } + + if (isSplit && !selected) { + // Deliberately nothing. + // + // What ⌘E is asked for is the ability to edit, and split view + // already grants it — the editor is on screen. With no selection + // there is no fragment to travel to either, so every remaining + // reading of the chord is a LAYOUT change nobody asked for: closing + // the preview on a mistyped ⌘E costs the reader the pane and a + // second keystroke to get it back, while doing nothing costs + // nothing. ⌘\ opens and closes the split, and stays the only way. + return; + } + + // Reading, or split with something selected — identical to the context + // menu's "Edit". In split view the editor is already on screen, so + // `editSourceRange` skips the toggle and only jumps, which is what + // gives the highlight there too. + await editSourceRange(selected); + } + async function editSourceRange(range: LineRange | null) { - if (!isEditing) await toggleEdit({ revealSelection: false }); + if (!isEditing) await toggleEdit(); // `toggleEdit` swallows a failed read and stays in reading mode. Arming // the jump anyway would fire it at whatever document is edited next. if (range && tabManager.activeTab?.isEditing) pendingEditReveal = toBufferRange(range); @@ -2598,7 +2624,8 @@ import { createDocumentSession, type LoadMarkdownOptions } from './sessions/docu // other pane" is not a request to write the file: whether a dirty // tab is flushed is now decided by the user's auto-save setting // alone, identically for the hotkey and the toolbar button. - if (!isSplit) toggleEdit(); + // + toggleEditView(); } if (cmdOrCtrl && e.shiftKey && !e.altKey && key === 's') { // Save As. The app menu advertised this chord for as long as the menu has @@ -3260,7 +3287,7 @@ import { createDocumentSession, type LoadMarkdownOptions } from './sessions/docu ontoggleHome={toggleHome} ononpenFileLocation={openFileLocation} ontoggleLiveMode={toggleLiveMode} - ontoggleEdit={() => toggleEdit()} + ontoggleEdit={() => toggleEditView()} ontoggleSplit={() => tabManager.activeTabId && toggleSplitView(tabManager.activeTabId)} {isEditing} ondetach={handleDetach} @@ -3300,7 +3327,7 @@ import { createDocumentSession, type LoadMarkdownOptions } from './sessions/docu ontoggleHome={toggleHome} ononpenFileLocation={openFileLocation} ontoggleLiveMode={toggleLiveMode} - ontoggleEdit={() => toggleEdit()} + ontoggleEdit={() => toggleEditView()} ontoggleEditorToolbar={() => settings.toggleEditorToolbar()} ontoggleSplit={() => tabManager.activeTabId && toggleSplitView(tabManager.activeTabId)} {isEditing} @@ -3387,7 +3414,7 @@ import { createDocumentSession, type LoadMarkdownOptions } from './sessions/docu onopen={selectFile} onclose={closeFile} onreveal={openFileLocation} - ontoggleEdit={() => toggleEdit()} + ontoggleEdit={() => toggleEditView()} ontoggleLive={toggleLiveMode} ontoggleSplit={() => tabManager.activeTabId && toggleSplitView(tabManager.activeTabId)} onhome={() => (showHome = true)} diff --git a/src/lib/components/Toc.svelte b/src/lib/components/Toc.svelte index 604c154a..48fc9102 100644 --- a/src/lib/components/Toc.svelte +++ b/src/lib/components/Toc.svelte @@ -151,13 +151,38 @@ * now send a source line instead (`activeLine`), and one rule picks the * entry from it. */ - function handleScroll() { - if (clickLock || !activeTargetEl) return; + /** + * Drop the "you jumped here" highlight. + * + * It is a temporary emphasis, so it has to end on anything that means the + * reader has moved on. Hanging it off scrolling alone made it permanent + * whenever that one event did not arrive — the jump's own smooth scroll is + * swallowed by `clickLock`, and if nothing scrolls the preview afterwards + * there is no second chance. + */ + function clearTargetHighlight() { + if (!activeTargetEl) return; activeTargetEl.classList.remove('toc-target-active'); activeTargetEl = null; } + function handleScroll() { + // The lock is here for the jump's OWN smooth scroll, which would + // otherwise clear the highlight before the reader has seen it. + if (clickLock) return; + clearTargetHighlight(); + } + + /** + * A deliberate action by the reader, which the lock must not swallow: the + * lock exists to ignore scrolling the app itself caused, and a click or a + * keystroke is never that. + */ + function handleReaderAction() { + clearTargetHighlight(); + } + /** * Keep the current entry in the MIDDLE of the outline, not merely on screen. * @@ -206,7 +231,18 @@ const el: HTMLElement | null = markdownBody; if (el) { el.addEventListener('scroll', handleScroll, { passive: true }); - return () => el.removeEventListener('scroll', handleScroll); + // `pointerdown`, not `click`: clicking a link inside the preview + // navigates, and the click never completes on the element that + // carried the highlight. + el.addEventListener('pointerdown', handleReaderAction, { passive: true }); + // On the window, because after a jump the focus can be in either + // pane or in neither — the reader typing anywhere has moved on. + window.addEventListener('keydown', handleReaderAction, { passive: true }); + return () => { + el.removeEventListener('scroll', handleScroll); + el.removeEventListener('pointerdown', handleReaderAction); + window.removeEventListener('keydown', handleReaderAction); + }; } }); @@ -226,7 +262,7 @@ if (item) onjump?.(id, item.text, sourceLineOf(el.dataset.sourcepos)); // highlight element persistently until scroll - if (activeTargetEl) activeTargetEl.classList.remove('toc-target-active'); + clearTargetHighlight(); el.classList.add('toc-target-active'); activeTargetEl = el;