From 06ec6fdfbb15581a137f5d62f7ff37fcc3c6a2ee Mon Sep 17 00:00:00 2001 From: Apoorv Mishra Date: Tue, 28 Oct 2025 11:34:40 +0530 Subject: [PATCH] Enable commenting on images (#10474) * feat: enable commenting on image nodes * chore: make anchorPlugin a top level plugin * fix: className * fix: review * fix: tsc * fix: checks * Tweak menu order to match --------- Co-authored-by: Tom Moor --- app/editor/index.tsx | 61 ++++++++++++++++--- app/editor/menus/image.tsx | 11 ++++ shared/editor/commands/comment.ts | 65 +++++++++++++++++++++ shared/editor/components/Image.tsx | 6 +- shared/editor/components/Styles.ts | 8 +-- shared/editor/marks/Comment.ts | 31 ++-------- shared/editor/nodes/Heading.ts | 67 +++------------------ shared/editor/nodes/Image.tsx | 12 ++++ shared/editor/nodes/SimpleImage.tsx | 2 +- shared/editor/plugins/anchorPlugin.ts | 51 ++++++++++++++++ shared/editor/styles/EditorStyleHelper.ts | 6 ++ shared/utils/ProsemirrorHelper.ts | 71 +++++++++++++++++++++++ 12 files changed, 289 insertions(+), 102 deletions(-) create mode 100644 shared/editor/commands/comment.ts create mode 100644 shared/editor/plugins/anchorPlugin.ts diff --git a/app/editor/index.tsx b/app/editor/index.tsx index aa7d4f3b0b..66c7c13833 100644 --- a/app/editor/index.tsx +++ b/app/editor/index.tsx @@ -55,12 +55,13 @@ import { NodeViewRenderer } from "./components/NodeViewRenderer"; import WithTheme from "./components/WithTheme"; import isNull from "lodash/isNull"; -import { map } from "lodash"; +import { isArray, map } from "lodash"; import { LightboxImage, LightboxImageFactory, } from "@shared/editor/lib/Lightbox"; import Lightbox from "~/components/Lightbox"; +import { anchorPlugin } from "@shared/editor/plugins/anchorPlugin"; export type Props = { /** An optional identifier for the editor context. It is used to persist local settings */ @@ -406,6 +407,7 @@ export class Editor extends React.PureComponent< plugins: [ ...this.keymaps, ...this.plugins, + anchorPlugin(), dropCursor({ color: this.props.theme.cursor, }), @@ -668,19 +670,36 @@ export class Editor extends React.PureComponent< public removeComment = (commentId: string) => { const { state, dispatch } = this.view; const tr = state.tr; + let markRemoved = false; state.doc.descendants((node, pos) => { - if (!node.isInline) { - return; + if (markRemoved) { + return false; } - const mark = node.marks.find( (m) => m.type === state.schema.marks.comment && m.attrs.id === commentId ); if (mark) { tr.removeMark(pos, pos + node.nodeSize, mark); + markRemoved = true; + return; } + + if (isArray(node.attrs?.marks)) { + const existingMarks = node.attrs.marks; + const updatedMarks = existingMarks.filter( + (mark: any) => mark.attrs.id !== commentId + ); + const attrs = { + ...node.attrs, + marks: updatedMarks, + }; + tr.setNodeMarkup(pos, undefined, attrs); + markRemoved = true; + } + + return; }); dispatch(tr); @@ -689,7 +708,7 @@ export class Editor extends React.PureComponent< /** * Update all marks related to a specific comment in the document. * - * @param commentId The id of the comment to remove + * @param commentId The id of the comment to update * @param attrs The attributes to update */ public updateComment = ( @@ -698,10 +717,11 @@ export class Editor extends React.PureComponent< ) => { const { state, dispatch } = this.view; const tr = state.tr; + let markUpdated = false; state.doc.descendants((node, pos) => { - if (!node.isInline) { - return; + if (markUpdated) { + return false; } const mark = node.marks.find( @@ -715,9 +735,27 @@ export class Editor extends React.PureComponent< ...mark.attrs, ...attrs, }); - tr.removeMark(from, to, mark).addMark(from, to, newMark); + markUpdated = true; + return; } + + if (isArray(node.attrs?.marks)) { + const existingMarks = node.attrs.marks; + const updatedMarks = existingMarks.map((mark: any) => + mark.type === "comment" && mark.attrs.id === commentId + ? { ...mark, attrs: { ...mark.attrs, ...attrs } } + : mark + ); + const newAttrs = { + ...node.attrs, + marks: updatedMarks, + }; + tr.setNodeMarkup(pos, undefined, newAttrs); + markUpdated = true; + } + + return; }); dispatch(tr); @@ -842,10 +880,15 @@ const EditorContainer = styled(Styles)<{ ${(props) => props.focusedCommentId && css` - #comment-${props.focusedCommentId} { + span#comment-${props.focusedCommentId} { background: ${transparentize(0.5, props.theme.brand.marine)}; border-bottom: 2px solid ${props.theme.commentMarkBackground}; } + a#comment-${props.focusedCommentId} + ~ span.component-image + div.image-wrapper { + outline: ${props.theme.commentMarkBackground} solid 2px; + } `} ${(props) => diff --git a/app/editor/menus/image.tsx b/app/editor/menus/image.tsx index 72c40793d5..f98dc2ea67 100644 --- a/app/editor/menus/image.tsx +++ b/app/editor/menus/image.tsx @@ -6,11 +6,13 @@ import { AlignImageRightIcon, AlignImageCenterIcon, AlignFullWidthIcon, + CommentIcon, } from "outline-icons"; import { EditorState } from "prosemirror-state"; import { isNodeActive } from "@shared/editor/queries/isNodeActive"; import { MenuItem } from "@shared/editor/types"; import { Dictionary } from "~/hooks/useDictionary"; +import { metaDisplay } from "@shared/utils/keyboard"; export default function imageMenuItems( state: EditorState, @@ -97,5 +99,14 @@ export default function imageMenuItems( tooltip: dictionary.deleteImage, icon: , }, + { + name: "separator", + }, + { + name: "commentOnImage", + tooltip: dictionary.comment, + shortcut: `${metaDisplay}+⌥+M`, + icon: , + }, ]; } diff --git a/shared/editor/commands/comment.ts b/shared/editor/commands/comment.ts new file mode 100644 index 0000000000..38babf76a4 --- /dev/null +++ b/shared/editor/commands/comment.ts @@ -0,0 +1,65 @@ +import { Attrs } from "prosemirror-model"; +import { Command, NodeSelection, TextSelection } from "prosemirror-state"; +import { isMarkActive } from "../queries/isMarkActive"; +import { chainTransactions } from "../lib/chainTransactions"; +import { addMark } from "./addMark"; +import { collapseSelection } from "./collapseSelection"; +import { chainCommands } from "prosemirror-commands"; + +export const addComment = (attrs: Attrs): Command => + chainCommands(addCommentTextSelection(attrs), addCommentNodeSelection(attrs)); + +const addCommentNodeSelection = + (attrs: Attrs): Command => + (state, dispatch) => { + if (!(state.selection instanceof NodeSelection)) { + return false; + } + const { selection } = state; + const existingMarks = selection.node.attrs.marks ?? []; + const newMark = { + type: "comment", + attrs: { + id: crypto.randomUUID(), + userId: attrs.userId, + draft: true, + }, + }; + const newAttrs = { + ...selection.node.attrs, + marks: [...existingMarks, newMark], + }; + dispatch?.(state.tr.setNodeMarkup(selection.from, undefined, newAttrs)); + return true; + }; + +const addCommentTextSelection = + (attrs: Attrs): Command => + (state, dispatch) => { + if (!(state.selection instanceof TextSelection)) { + return false; + } + + if ( + isMarkActive( + state.schema.marks.comment, + { + resolved: false, + }, + { exact: true } + )(state) + ) { + return false; + } + + chainTransactions( + addMark(state.schema.marks.comment, { + id: crypto.randomUUID(), + userId: attrs.userId, + draft: true, + }), + collapseSelection() + )(state, dispatch); + + return true; + }; diff --git a/shared/editor/components/Image.tsx b/shared/editor/components/Image.tsx index 8b3c5e6e58..d6bb8af0dc 100644 --- a/shared/editor/components/Image.tsx +++ b/shared/editor/components/Image.tsx @@ -94,7 +94,11 @@ const Image = (props: Props) => {
{!dragging && width > 60 && isDownloadable && ( diff --git a/shared/editor/components/Styles.ts b/shared/editor/components/Styles.ts index dd375585f9..2246f68baa 100644 --- a/shared/editor/components/Styles.ts +++ b/shared/editor/components/Styles.ts @@ -733,7 +733,7 @@ img.ProseMirror-separator { } } -.heading-name { +.${EditorStyleHelper.headingPositionAnchor}, .${EditorStyleHelper.imagePositionAnchor} { color: ${props.theme.text}; pointer-events: none; display: block; @@ -746,11 +746,11 @@ img.ProseMirror-separator { } } -.heading-name:first-child, +.${EditorStyleHelper.headingPositionAnchor}:first-child, // Edge case where multiplayer cursor is between start of cell and heading -.heading-name:first-child + .ProseMirror-yjs-cursor, +.${EditorStyleHelper.headingPositionAnchor}:first-child + .ProseMirror-yjs-cursor, // Edge case where table grips are between start of cell and heading -.heading-name:first-child + [role=button] + [role=button] { +.${EditorStyleHelper.headingPositionAnchor}:first-child + [role=button] + [role=button] { & + h1, & + h2, & + h3, diff --git a/shared/editor/marks/Comment.ts b/shared/editor/marks/Comment.ts index 8b578efc0c..4c3a6f77ab 100644 --- a/shared/editor/marks/Comment.ts +++ b/shared/editor/marks/Comment.ts @@ -1,12 +1,12 @@ import { toggleMark } from "prosemirror-commands"; -import { MarkSpec, MarkType, Schema, Mark as PMMark } from "prosemirror-model"; +import { MarkSpec, MarkType, Mark as PMMark } from "prosemirror-model"; import { Command, Plugin } from "prosemirror-state"; -import { addMark } from "../commands/addMark"; import { collapseSelection } from "../commands/collapseSelection"; import { chainTransactions } from "../lib/chainTransactions"; import { isMarkActive } from "../queries/isMarkActive"; import { EditorStyleHelper } from "../styles/EditorStyleHelper"; import Mark from "./Mark"; +import { addComment } from "../commands/comment"; export default class Comment extends Mark { get name() { @@ -94,32 +94,9 @@ export default class Comment extends Mark { : {}; } - commands({ type }: { type: MarkType; schema: Schema }) { + commands() { return this.options.onCreateCommentMark - ? (): Command => (state, dispatch) => { - if ( - isMarkActive( - state.schema.marks.comment, - { - resolved: false, - }, - { exact: true } - )(state) - ) { - return false; - } - - chainTransactions( - addMark(type, { - id: crypto.randomUUID(), - userId: this.options.userId, - draft: true, - }), - collapseSelection() - )(state, dispatch); - - return true; - } + ? (): Command => addComment({ userId: this.options.userId }) : undefined; } diff --git a/shared/editor/nodes/Heading.ts b/shared/editor/nodes/Heading.ts index 1a610e4c9c..f0dae91cbd 100644 --- a/shared/editor/nodes/Heading.ts +++ b/shared/editor/nodes/Heading.ts @@ -14,14 +14,13 @@ import Storage from "../../utils/Storage"; import backspaceToParagraph from "../commands/backspaceToParagraph"; import splitHeading from "../commands/splitHeading"; import toggleBlockType from "../commands/toggleBlockType"; -import headingToSlug, { headingToPersistenceKey } from "../lib/headingToSlug"; +import { headingToPersistenceKey } from "../lib/headingToSlug"; import { MarkdownSerializerState } from "../lib/markdown/serializer"; import { findCollapsedNodes } from "../queries/findCollapsedNodes"; import Node from "./Node"; +import { EditorStyleHelper } from "../styles/EditorStyleHelper"; export default class Heading extends Node { - className = "heading-name"; - get name() { return "heading"; } @@ -181,7 +180,10 @@ export default class Heading extends Node { (event.currentTarget.parentNode?.parentNode ?.previousSibling as HTMLElement); - if (!anchor || !anchor.className.includes(this.className)) { + if ( + !anchor || + !anchor.className.includes(EditorStyleHelper.headingPositionAnchor) + ) { throw new Error("Did not find anchor as previous sibling of heading"); } const hash = `#${anchor.id}`; @@ -219,61 +221,6 @@ export default class Heading extends Node { } get plugins() { - const getAnchors = (doc: ProsemirrorNode) => { - const decorations: Decoration[] = []; - const previouslySeen: Record = {}; - - doc.descendants((node, pos) => { - if (node.type.name !== this.name) { - return; - } - - // calculate the optimal id - const slug = headingToSlug(node); - let id = slug; - - // check if we've already used it, and if so how many times? - // Make the new id based on that number ensuring that we have - // unique ID's even when headings are identical - if (previouslySeen[slug] > 0) { - id = headingToSlug(node, previouslySeen[slug]); - } - - // record that we've seen this slug for the next loop - previouslySeen[slug] = - previouslySeen[slug] !== undefined ? previouslySeen[slug] + 1 : 1; - - decorations.push( - Decoration.widget( - pos, - () => { - const anchor = document.createElement("a"); - anchor.id = id; - anchor.className = this.className; - return anchor; - }, - { - side: -1, - key: id, - } - ) - ); - }); - - return DecorationSet.create(doc, decorations); - }; - - const plugin: Plugin = new Plugin({ - state: { - init: (config, state) => getAnchors(state.doc), - apply: (tr, oldState) => - tr.docChanged ? getAnchors(tr.doc) : oldState, - }, - props: { - decorations: (state) => plugin.getState(state), - }, - }); - const foldPlugin: Plugin = new Plugin({ props: { decorations: (state) => { @@ -290,7 +237,7 @@ export default class Heading extends Node { }, }); - return [foldPlugin, plugin]; + return [foldPlugin]; } inputRules({ type }: { type: NodeType }) { diff --git a/shared/editor/nodes/Image.tsx b/shared/editor/nodes/Image.tsx index 7b2346cbeb..994e1c465c 100644 --- a/shared/editor/nodes/Image.tsx +++ b/shared/editor/nodes/Image.tsx @@ -16,6 +16,7 @@ import { EditorStyleHelper } from "../styles/EditorStyleHelper"; import { ComponentProps } from "../types"; import SimpleImage from "./SimpleImage"; import { LightboxImageFactory } from "../lib/Lightbox"; +import { addComment } from "../commands/comment"; const imageSizeRegex = /\s=(\d+)?x(\d+)?$/; @@ -114,6 +115,9 @@ export default class Image extends SimpleImage { default: null, validate: "string|null", }, + marks: { + default: undefined, + }, }, content: "text*", marks: "", @@ -412,6 +416,12 @@ export default class Image extends SimpleImage { }; } + keys(): Record { + return { + "Mod-Alt-m": addComment({ userId: this.options.userId }), + }; + } + commands({ type }: { type: NodeType }) { return { ...super.commands({ type }), @@ -503,6 +513,8 @@ export default class Image extends SimpleImage { dispatch?.(tr.setSelection(new NodeSelection($pos))); return true; }, + commentOnImage: (): Command => + addComment({ userId: this.options.userId }), }; } diff --git a/shared/editor/nodes/SimpleImage.tsx b/shared/editor/nodes/SimpleImage.tsx index c1a6dff56b..b6abe422ea 100644 --- a/shared/editor/nodes/SimpleImage.tsx +++ b/shared/editor/nodes/SimpleImage.tsx @@ -17,7 +17,7 @@ import Node from "./Node"; import { LightboxImageFactory } from "../lib/Lightbox"; export default class SimpleImage extends Node { - options: Options; + options: Options & { userId?: string }; get name() { return "image"; diff --git a/shared/editor/plugins/anchorPlugin.ts b/shared/editor/plugins/anchorPlugin.ts new file mode 100644 index 0000000000..1b244c5761 --- /dev/null +++ b/shared/editor/plugins/anchorPlugin.ts @@ -0,0 +1,51 @@ +import { NodeAnchor, ProsemirrorHelper } from "@shared/utils/ProsemirrorHelper"; +import { EditorState, Plugin } from "prosemirror-state"; +import { Decoration, DecorationSet } from "prosemirror-view"; + +export class AnchorPlugin extends Plugin { + constructor() { + super({ + state: { + init: (_, state: EditorState) => ({ + decorations: this.createDecorations(state), + }), + apply: (tr, pluginState, oldState, newState) => { + // Only recompute if doc changed + if (tr.docChanged) { + return { decorations: this.createDecorations(newState) }; + } + return pluginState; + }, + }, + props: { + decorations: (state) => { + const pluginState = this.getState(state); + return pluginState ? pluginState.decorations : null; + }, + }, + }); + } + + private createAnchorDecoration(anchor: NodeAnchor) { + return Decoration.widget( + anchor.pos, + () => { + const anchorElement = document.createElement("a"); + anchorElement.id = anchor.id; + anchorElement.className = anchor.className; + return anchorElement; + }, + { side: -1, key: anchor.id } + ); + } + + private createDecorations(state: EditorState) { + const anchors = ProsemirrorHelper.getAnchors(state.doc); + return DecorationSet.create( + state.doc, + anchors.map(this.createAnchorDecoration) + ); + } +} + +export const anchorPlugin = () => new AnchorPlugin(); diff --git a/shared/editor/styles/EditorStyleHelper.ts b/shared/editor/styles/EditorStyleHelper.ts index e0d357a321..62a12cece8 100644 --- a/shared/editor/styles/EditorStyleHelper.ts +++ b/shared/editor/styles/EditorStyleHelper.ts @@ -12,6 +12,12 @@ export class EditorStyleHelper { static readonly imageCaption = "caption"; + static readonly imagePositionAnchor = "image-position-anchor"; + + // Headings + + static readonly headingPositionAnchor = "heading-position-anchor"; + // Comments static readonly comment = "comment-marker"; diff --git a/shared/utils/ProsemirrorHelper.ts b/shared/utils/ProsemirrorHelper.ts index 8cea23ad04..f8ef9a3a49 100644 --- a/shared/utils/ProsemirrorHelper.ts +++ b/shared/utils/ProsemirrorHelper.ts @@ -6,6 +6,7 @@ import { TextHelper } from "./TextHelper"; import env from "../env"; import { findChildren } from "@shared/editor/queries/findChildren"; import { isLightboxNode } from "@shared/editor/lib/Lightbox"; +import { EditorStyleHelper } from "@shared/editor/styles/EditorStyleHelper"; export type Heading = { /* The heading in plain text */ @@ -25,6 +26,8 @@ export type CommentMark = { text: string; }; +export type NodeAnchor = { pos: number; id: string; className: string }; + export type Task = { /* The text of the task */ text: string; @@ -187,12 +190,80 @@ export class ProsemirrorHelper { } }); + (node.attrs.marks ?? []).forEach((mark: any) => { + if (mark.type === "comment") { + comments.push({ + ...mark.attrs, + // For image nodes, we don't have any text content, so we set it to an empty string + text: "", + } as CommentMark); + } + }); + return true; }); return comments; } + private static getAnchorsForHeadingNodes(doc: Node): NodeAnchor[] { + const previouslySeen: Record = {}; + const anchors: NodeAnchor[] = []; + doc.descendants((node, pos) => { + if (node.type.name !== "heading") { + return; + } + + // calculate the optimal id + const slug = headingToSlug(node); + let id = slug; + + // check if we've already used it, and if so how many times? + // Make the new id based on that number ensuring that we have + // unique ID's even when headings are identical + if (previouslySeen[slug] > 0) { + id = headingToSlug(node, previouslySeen[slug]); + } + + // record that we've seen this slug for the next loop + previouslySeen[slug] = + previouslySeen[slug] !== undefined ? previouslySeen[slug] + 1 : 1; + + anchors.push({ + pos, + id, + className: EditorStyleHelper.headingPositionAnchor, + }); + }); + return anchors; + } + + private static getAnchorsForImageNodes(doc: Node): NodeAnchor[] { + const anchors: NodeAnchor[] = []; + doc.descendants((node, pos) => { + if (Array.isArray(node.attrs?.marks)) { + node.attrs.marks.forEach((mark: any) => { + if (mark?.type === "comment" && mark?.attrs?.id) { + anchors.push({ + pos, + id: `comment-${mark.attrs.id}`, + className: EditorStyleHelper.imagePositionAnchor, + }); + } + }); + } + }); + + return anchors; + } + + static getAnchors(doc: Node): NodeAnchor[] { + return [ + ...ProsemirrorHelper.getAnchorsForHeadingNodes(doc), + ...ProsemirrorHelper.getAnchorsForImageNodes(doc), + ]; + } + /** * Builds the consolidated anchor text for the given comment-id. *