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 <tom@getoutline.com>

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) =>

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: <TrashIcon />,
     },
+    {
+      name: "separator",
+    },
+    {
+      name: "commentOnImage",
+      tooltip: dictionary.comment,
+      shortcut: `${metaDisplay}+⌥+M`,
+      icon: <CommentIcon />,
+    },
   ];
 }

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;
+  };

shared/editor/components/Image.tsx

@@ -94,7 +94,11 @@ const Image = (props: Props) => {
     <div contentEditable={false} className={className} ref={ref}>
       <ImageWrapper
         isFullWidth={isFullWidth}
-        className={isSelected || dragging ? "ProseMirror-selectednode" : ""}
+        className={
+          isSelected || dragging
+            ? "image-wrapper ProseMirror-selectednode"
+            : "image-wrapper"
+        }
         style={widthStyle}
       >
         {!dragging && width > 60 && isDownloadable && (

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,

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;
   }
 

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<string, number> = {};
-
-      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 }) {

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<string, Command> {
+    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 }),
     };
   }
 

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";

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();

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";

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<string, number> = {};
+    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.
    *