Don't comment on a stale range when the selection is in a widget

src/editor/extension.ts

@@ -34,6 +34,11 @@ import {
 } from "./comment-draft-anchor";
 import { findCriticTaskPrefixes, type CriticTaskPrefix } from "./task-prefix";
 import { canReuseCriticStateForTrailingChanges } from "./incremental";
+import {
+	isFragmentEditor,
+	isSelectionTrusted,
+	readDomSelectionFacts,
+} from "./selection-trust";
 
 export const setCommentDraftAnchor =
 	StateEffect.define<CommentDraftAnchor | null>();
@@ -184,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<HTMLElement>(
@@ -229,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);
@@ -243,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);
@@ -285,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 {
@@ -309,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(() => {
@@ -340,6 +378,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 +401,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;
 				}
@@ -403,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");
 			}
 		},
 	);

src/editor/selection-trust.ts

@@ -0,0 +1,119 @@
+/**
+ * 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";
+}
+
+/**
+ * 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,
+	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, "");
+}

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> = {}): 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");
+	});
+});