diff --git a/src/editor/extension.ts b/src/editor/extension.ts index 1cbc98b..6a26250 100644 --- a/src/editor/extension.ts +++ b/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(); @@ -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( @@ -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"); } }, ); diff --git a/src/editor/selection-trust.ts b/src/editor/selection-trust.ts new file mode 100644 index 0000000..a7ecff0 --- /dev/null +++ b/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, ""); +} 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"); + }); +});