From fa1d083c7c7b85678ec97dff2455f5500f30ac07 Mon Sep 17 00:00:00 2001 From: Peter Gaultney Date: Mon, 17 Aug 2026 17:52:05 -0400 Subject: [PATCH 1/2] fix: don't comment on a stale range when the selection is in a widget Obsidian renders tables, and other embedded blocks, as CodeMirror widgets marked `contenteditable="false"`. A selection made inside one is a real document selection that produces no CodeMirror transaction, because CodeMirror's own state never changes. `state.selection` therefore keeps describing the *previous* selection and still reports itself non-empty. The inline comment button trusted that range twice over. It stayed visible, because placement is only recomputed from a ViewUpdate and no update arrives, and its click handler read the same stale range. So selecting text in a table and clicking the button wrote a comment around whatever had been selected before - typically the first characters of the document, since that is where the cursor starts. The mark existed but pointed nowhere near the table, which is why the selection did not highlight and the sidebar reported no comments on the note. `selectionTrust` compares CodeMirror's idea of the selection against the document's: an uneditable ancestor at either end, or text that disagrees once whitespace is squeezed, means the range cannot be trusted. Containment in `contentDOM` cannot answer this on its own, because widgets live inside the content element too. The button now withdraws instead of going stale, and the click handler declines a range it cannot trust. The keyboard command and context-menu paths go through `editor.getSelection()`, which already reports empty in a widget, so they insert at the cursor rather than at a distant range and needed no change. --- src/editor/extension.ts | 24 ++++- src/editor/selection-trust.ts | 103 ++++++++++++++++++++++ tests/unit/editor/selection-trust.test.ts | 80 +++++++++++++++++ 3 files changed, 206 insertions(+), 1 deletion(-) create mode 100644 src/editor/selection-trust.ts create mode 100644 tests/unit/editor/selection-trust.test.ts diff --git a/src/editor/extension.ts b/src/editor/extension.ts index 1cbc98b..759b8e6 100644 --- a/src/editor/extension.ts +++ b/src/editor/extension.ts @@ -34,6 +34,10 @@ import { } from "./comment-draft-anchor"; import { findCriticTaskPrefixes, type CriticTaskPrefix } from "./task-prefix"; import { canReuseCriticStateForTrailingChanges } from "./incremental"; +import { + isSelectionTrusted, + readDomSelectionFacts, +} from "./selection-trust"; export const setCommentDraftAnchor = StateEffect.define(); @@ -340,6 +344,11 @@ export function createReviewEditorExtension( event.stopPropagation(); const selection = this.view.state.selection.main; if (selection.empty) return; + // A selection inside an uneditable widget leaves this range describing the + // previous one, so an untrusted range would comment on text the user has + // since selected away from. + if (!this.selectionIsTrusted()) return; + controller.startCommentDraft( readPath(this.view.state), selection.from, @@ -358,13 +367,26 @@ export function createReviewEditorExtension( }); } + /** Whether the state selection still describes the on-screen selection. */ + private selectionIsTrusted(): boolean { + const selection = this.view.state.selection.main; + return isSelectionTrusted( + readDomSelectionFacts( + this.view.dom.ownerDocument.getSelection(), + this.view.contentDOM, + this.view.state.sliceDoc(selection.from, selection.to), + ), + ); + } + private measureCommentButton(): CommentButtonPlacement | null { const selection = this.view.state.selection.main; const fieldValue = this.view.state.field(criticField); if ( !controller.shouldShowInlineActions() || !fieldValue.livePreview || - selection.empty + selection.empty || + !this.selectionIsTrusted() ) { return null; } diff --git a/src/editor/selection-trust.ts b/src/editor/selection-trust.ts new file mode 100644 index 0000000..5c7bfa4 --- /dev/null +++ b/src/editor/selection-trust.ts @@ -0,0 +1,103 @@ +/** + * Whether CodeMirror's selection still describes what the user has selected on screen. + * + * Obsidian renders tables, and other embedded blocks, as widgets marked + * `contenteditable="false"`. A selection made inside one is a real DOM selection that produces no + * CodeMirror transaction at all, because CodeMirror's own state never changes. Its + * `state.selection` therefore keeps describing the *previous* selection, and reports itself + * non-empty - so a range that looks perfectly valid actually points somewhere the user is no + * longer looking. + * + * Anything reading `state.selection` to place an edit has to ask this first. Containment in + * `contentDOM` cannot answer it: widgets live inside the content element too. + */ +export type SelectionTrust = + | "trusted" + | "no-dom-selection" + | "outside-editor" + | "uneditable-widget" + | "text-mismatch"; + +export interface DomSelectionFacts { + /** The document selection's text, or null when there is no selection at all. */ + domText: string | null; + /** Whether both ends of the DOM selection are inside the editor's content element. */ + insideContent: boolean; + /** + * The `contenteditable` value of the nearest such ancestor at each end of the DOM selection. + * `"false"` marks a widget CodeMirror does not accept text selection from. + */ + anchorEditable: string | null; + focusEditable: string | null; + /** What CodeMirror believes is selected. */ + stateText: string; +} + +export function selectionTrust(facts: DomSelectionFacts): SelectionTrust { + if (facts.domText === null) return "no-dom-selection"; + + if (!facts.insideContent) return "outside-editor"; + + if (facts.anchorEditable === "false" || facts.focusEditable === "false") + return "uneditable-widget"; + + // Whitespace differs freely between rendered DOM and source text, so compare only the + // non-whitespace content. A widget that reported no `contenteditable` at all still fails here. + if (squeeze(facts.domText) !== squeeze(facts.stateText)) + return "text-mismatch"; + + return "trusted"; +} + +export function isSelectionTrusted(facts: DomSelectionFacts): boolean { + return selectionTrust(facts) === "trusted"; +} + +/** Read the facts `selectionTrust` needs from a live document selection. */ +export function readDomSelectionFacts( + selection: Selection | null, + contentDOM: HTMLElement, + stateText: string, +): DomSelectionFacts { + if (!selection || selection.rangeCount === 0) + return { + domText: null, + insideContent: false, + anchorEditable: null, + focusEditable: null, + stateText, + }; + + const range = selection.getRangeAt(0); + return { + domText: selection.toString(), + insideContent: + contains(contentDOM, range.startContainer) && + contains(contentDOM, range.endContainer), + anchorEditable: editableOf(range.startContainer), + focusEditable: editableOf(range.endContainer), + stateText, + }; +} + +function elementOf(node: Node): Element | null { + return node.nodeType === Node.ELEMENT_NODE + ? (node as Element) + : node.parentElement; +} + +function contains(contentDOM: HTMLElement, node: Node): boolean { + const element = elementOf(node); + return element ? contentDOM.contains(element) : false; +} + +function editableOf(node: Node): string | null { + return ( + elementOf(node)?.closest("[contenteditable]")?.getAttribute("contenteditable") ?? + null + ); +} + +function squeeze(value: string): string { + return value.replace(/\s+/g, ""); +} diff --git a/tests/unit/editor/selection-trust.test.ts b/tests/unit/editor/selection-trust.test.ts new file mode 100644 index 0000000..9a46aa3 --- /dev/null +++ b/tests/unit/editor/selection-trust.test.ts @@ -0,0 +1,80 @@ +import { describe, expect, it } from "@jest/globals"; +import { + isSelectionTrusted, + selectionTrust, + type DomSelectionFacts, +} from "src/editor/selection-trust"; + +function facts(overrides: Partial = {}): DomSelectionFacts { + return { + domText: "Some prose", + insideContent: true, + anchorEditable: "true", + focusEditable: "true", + stateText: "Some prose", + ...overrides, + }; +} + +describe("selection trust", () => { + it("trusts a selection the editor and the document agree on", () => { + expect(selectionTrust(facts())).toBe("trusted"); + expect(isSelectionTrusted(facts())).toBe(true); + }); + + it("distrusts a selection inside an uneditable widget", () => { + // Obsidian renders tables as contenteditable="false" widgets. Selecting in one produces no + // CodeMirror transaction, so state.selection still describes the previous selection - the + // case that wrote a comment into the first characters of a document. + expect( + selectionTrust( + facts({ + domText: "1.8.26", + anchorEditable: "false", + focusEditable: "false", + stateText: "Some prose", + }), + ), + ).toBe("uneditable-widget"); + }); + + it("distrusts a selection with one end in a widget", () => { + expect( + selectionTrust(facts({ focusEditable: "false", domText: "Some prose" })), + ).toBe("uneditable-widget"); + }); + + it("distrusts a stale range even without a contenteditable signal", () => { + // The text comparison is the backstop: a widget that reports no contenteditable at all + // still cannot make the two texts agree. + expect( + selectionTrust( + facts({ + domText: "1.8.26", + anchorEditable: null, + focusEditable: null, + stateText: "Some prose", + }), + ), + ).toBe("text-mismatch"); + }); + + it("ignores whitespace differences between rendered and source text", () => { + // A rendered selection legitimately collapses and re-wraps whitespace. + expect( + selectionTrust( + facts({ domText: "two words\n", stateText: "two words" }), + ), + ).toBe("trusted"); + }); + + it("distrusts a selection outside the editor content", () => { + expect(selectionTrust(facts({ insideContent: false }))).toBe( + "outside-editor", + ); + }); + + it("distrusts the absence of a document selection", () => { + expect(selectionTrust(facts({ domText: null }))).toBe("no-dom-selection"); + }); +}); From b334d7b1472f89576fffdf5a15f93db47ee206ee Mon Sep 17 00:00:00 2001 From: Peter Gaultney Date: Mon, 17 Aug 2026 18:34:36 -0400 Subject: [PATCH 2/2] fix: keep the comment button out of per-fragment editors Editing a table cell gets its own CodeMirror view, mounted inside the host editor, whose whole document is that cell's text. Editor extensions are installed in it too, so the floating comment button appeared a second time *inside the table*, and the state field built decorations against offsets that are the cell's rather than the note's - on text Obsidian's own renderer has already marked up. A fragment editor now gets no button and reports itself as not-live-preview, which is how a view plugin tells the state field to build nothing. The check is nesting (an editor whose ancestor is an editor) rather than a table-specific class, so callouts and embeds are covered by the same test. It is recomputed per use rather than cached in the constructor: Obsidian attaches the fragment editor's DOM into the host after the view plugin is built, so a constructor-time check sees an unparented element and reports false. The button is therefore created on first placement instead of at construction. --- src/editor/extension.ts | 57 +++++++++++++++++++++++++++++------ src/editor/selection-trust.ts | 16 ++++++++++ 2 files changed, 63 insertions(+), 10 deletions(-) diff --git a/src/editor/extension.ts b/src/editor/extension.ts index 759b8e6..6a26250 100644 --- a/src/editor/extension.ts +++ b/src/editor/extension.ts @@ -35,6 +35,7 @@ import { import { findCriticTaskPrefixes, type CriticTaskPrefix } from "./task-prefix"; import { canReuseCriticStateForTrailingChanges } from "./incremental"; import { + isFragmentEditor, isSelectionTrusted, readDomSelectionFacts, } from "./selection-trust"; @@ -188,12 +189,22 @@ export function createReviewEditorExtension( const reviewViewPlugin = ViewPlugin.fromClass( class { - private commentButton: HTMLButtonElement; + private commentButton: HTMLButtonElement | null = null; private sourceViewEl: HTMLElement | null = null; private sourceViewObserver: MutationObserver | null = null; private livePreviewPollId: number | null = null; private lastDomSignal: boolean | null = null; private destroyed = false; + /** + * A per-fragment editor (a table cell, an embed) carries no UI of its own. + * + * Recomputed rather than cached in the constructor: Obsidian attaches the fragment + * editor's DOM into the host editor after the view plugin is built, so a + * constructor-time check sees an unparented element and reports false. + */ + private get fragment(): boolean { + return isFragmentEditor(this.view.dom); + } private handleClick = (event: MouseEvent): void => { const target = event.target as HTMLElement | null; const anchor = target?.closest( @@ -233,8 +244,6 @@ export function createReviewEditorExtension( }; constructor(private view: EditorView) { - this.commentButton = this.createCommentButton(); - this.view.dom.appendChild(this.commentButton); this.view.dom.addEventListener("click", this.handleClick); this.view.dom.addEventListener("pointerover", this.handlePointerOver); this.view.dom.addEventListener("pointerout", this.handlePointerOut); @@ -247,6 +256,25 @@ export function createReviewEditorExtension( this.scheduleCommentButtonUpdate(); } + /** + * The floating button, created on first use so the fragment check runs after Obsidian + * has attached this editor. A fragment editor never gets one: the host editor already + * shows a button for the same note, and a second one appears inside the table. + */ + private ensureCommentButton(): HTMLButtonElement | null { + if (this.fragment) { + this.commentButton?.remove(); + this.commentButton = null; + return null; + } + if (!this.commentButton) { + this.commentButton = this.createCommentButton(); + this.view.dom.appendChild(this.commentButton); + } + + return this.commentButton; + } + update(update: ViewUpdate): void { this.observeSourceView(); const draftAnchor = update.state.field(commentDraftAnchorField); @@ -289,7 +317,7 @@ export function createReviewEditorExtension( this.view.dom.removeEventListener("pointerover", this.handlePointerOver); this.view.dom.removeEventListener("pointerout", this.handlePointerOut); controller.hideThreadPreview(); - this.commentButton.remove(); + this.commentButton?.remove(); } private observeSourceView(): void { @@ -313,9 +341,15 @@ export function createReviewEditorExtension( private syncDomLivePreview(): void { const sourceView = this.view.dom.closest(".markdown-source-view"); - const domSignal = sourceView - ? sourceView.classList.contains("is-live-preview") - : null; + // A fragment editor's document is the fragment, not the note, and Obsidian renders + // that markup itself - decorating here double-renders the mark against offsets that + // are not the note's. Reporting "not live preview" is how a view plugin tells the + // state field to build nothing. + const domSignal = this.fragment + ? false + : sourceView + ? sourceView.classList.contains("is-live-preview") + : null; if (domSignal === this.lastDomSignal) return; this.lastDomSignal = domSignal; queueMicrotask(() => { @@ -425,15 +459,18 @@ export function createReviewEditorExtension( private applyCommentButtonPlacement( placement: CommentButtonPlacement | null, ): void { + const button = this.ensureCommentButton(); + if (!button) return; + if (!placement) { - this.commentButton.classList.remove("is-visible"); + button.classList.remove("is-visible"); return; } - this.commentButton.setCssStyles({ + button.setCssStyles({ left: `${placement.left}px`, top: `${placement.top}px`, }); - this.commentButton.classList.add("is-visible"); + button.classList.add("is-visible"); } }, ); diff --git a/src/editor/selection-trust.ts b/src/editor/selection-trust.ts index 5c7bfa4..a7ecff0 100644 --- a/src/editor/selection-trust.ts +++ b/src/editor/selection-trust.ts @@ -53,6 +53,22 @@ export function isSelectionTrusted(facts: DomSelectionFacts): boolean { return selectionTrust(facts) === "trusted"; } +/** + * Whether this editor is one Obsidian mounted inside another to edit a fragment. + * + * Editing a table cell, and other embedded blocks, gets its own CodeMirror view whose whole + * document is that fragment - `"1.8.26"` for one cell. Editor extensions are installed in it too, + * so anything that assumes one view per note gets a second instance working on a different + * document: a duplicate floating button inside the table, and decorations computed against text + * Obsidian's own renderer has already marked up. + * + * Nesting is the test rather than a table-specific class, so callouts and embeds are covered by + * the same check. + */ +export function isFragmentEditor(editorDOM: HTMLElement): boolean { + return !!editorDOM.parentElement?.closest(".cm-editor"); +} + /** Read the facts `selectionTrust` needs from a live document selection. */ export function readDomSelectionFacts( selection: Selection | null,