Merge pull request #254 from CoreMedia/drag-image-fix

Dnd of Content Images should not be converted to imageBlock elements

Author: jens-meisner

packages/ckeditor5-coremedia-images/src/ContentImageEditingPlugin.ts

@@ -6,7 +6,7 @@ import { getOptionalPlugin, reportInitEnd, reportInitStart } from "@coremedia/ck
 import type { Logger } from "@coremedia/ckeditor5-logging";
 import { LoggerProvider } from "@coremedia/ckeditor5-logging";
 import ModelBoundSubscriptionPlugin from "./ModelBoundSubscriptionPlugin";
-import { editingDowncastXlinkHref, preventUpcastImageSrc } from "./converters";
+import { editingDowncastXlinkHref, preventUpcastImageSrc, upcastContentImageAsInline } from "./converters";
 import {
   openImageInTabCommandName,
   registerOpenImageInTabCommand,
@@ -62,6 +62,20 @@ export default class ContentImageEditingPlugin extends Plugin {
     // src attribute for the editing view asynchronously.
     // If not prevented, the src-attribute from GRS would be written to the model.
     this.editor.conversion.for("upcast").add(preventUpcastImageSrc());
+
+    // Force content images to become imageInline when dropped inside a
+    // paragraph (inline context). This prevents ImageBlockEditing from
+    // splitting the host paragraph. Runs at high priority, before
+    // ImageBlockEditing's normal-priority converter.
+    this.editor.conversion.for("upcast").add(upcastContentImageAsInline());
+
+    // When a content image is dragged and dropped between block elements,
+    // CKEditor's image upcasting may create imageBlock instead of imageInline
+    // (because the drop target is a block-level position). The post-fixer
+    // registered here converts any such imageBlock back to a paragraph
+    // containing an imageInline, which is the correct model representation
+    // for CoreMedia content images.
+    ContentImageEditingPlugin.#setupImageBlockToInlinePostFixer(this.editor);
   }
 
   static #setupXlinkHrefConversion(editor: Editor, modelAttributeName: string, dataAttributeName: string): void {
@@ -103,6 +117,86 @@ export default class ContentImageEditingPlugin extends Plugin {
       .add(editingDowncastXlinkHref(editor, modelElementName, ContentImageEditingPlugin.#logger));
   }
 
+  /**
+   * Ensures that content images dropped between block elements remain as
+   * `imageInline` in the model.
+   *
+   * When dragging a content image and dropping it between two paragraphs,
+   * CKEditor's image upcast pipeline detects a block-level drop position and
+   * creates an `imageBlock` model element instead of `imageInline`.  Because
+   * `xlink-href` was not allowed on `imageBlock`, the attribute was silently
+   * dropped and GHS captured `data-xlink-href` into `htmlImgAttributes`,
+   * making the image disappear from the editing view.
+   *
+   * This method:
+   * 1. Extends the `imageBlock` schema to allow `xlink-href` so that the
+   *    existing `attributeToAttribute` upcast converter sets the attribute
+   *    correctly (and GHS no longer captures it).
+   * 2. Registers a model post-fixer that converts every `imageBlock` carrying
+   *    a `xlink-href` attribute into a `<paragraph>` containing an
+   *    `imageInline`, which is the correct CoreMedia RichText representation.
+   *
+   * @param editor - Editor instance
+   */
+  static #setupImageBlockToInlinePostFixer(editor: Editor): void {
+    const { model } = editor;
+    const { schema } = model;
+
+    if (!schema.isRegistered("imageBlock")) {
+      return;
+    }
+
+    // Allow xlink-href on imageBlock so the upcast can set the attribute
+    // (preventing GHS from capturing data-xlink-href into htmlImgAttributes).
+    schema.extend("imageBlock", {
+      allowAttributes: [ContentImageEditingPlugin.XLINK_HREF_MODEL_ATTRIBUTE_NAME],
+    });
+
+    const xlinkHrefAttr = ContentImageEditingPlugin.XLINK_HREF_MODEL_ATTRIBUTE_NAME;
+    const imageInlineName = ContentImageEditingPlugin.IMAGE_INLINE_MODEL_ELEMENT_NAME;
+
+    model.document.registerPostFixer((writer) => {
+      let changed = false;
+
+      for (const change of model.document.differ.getChanges()) {
+        if (change.type === "insert" && change.name === "imageBlock") {
+          const item = change.position.nodeAfter;
+          if (!item || !item.is("element", "imageBlock")) {
+            continue;
+          }
+
+          const xlinkHref = item.getAttribute(xlinkHrefAttr);
+          if (!xlinkHref) {
+            continue;
+          }
+
+          // Replace the imageBlock with a paragraph containing an imageInline.
+          const paragraph = writer.createElement("paragraph");
+          const imageInline = writer.createElement(imageInlineName, {
+            [xlinkHrefAttr]: xlinkHref,
+          });
+          writer.append(imageInline, paragraph);
+          writer.insert(paragraph, item, "before");
+          writer.remove(item);
+          changed = true;
+        }
+
+        // When a content image is moved (drag-and-drop) out of a paragraph
+        // that contained only that image, the source paragraph becomes empty.
+        // CKEditor does not remove it automatically, so we clean it up here.
+        if (change.type === "remove" && change.name === "imageInline") {
+          const parent = change.position.parent;
+          if (parent.is("element", "paragraph") && parent.childCount === 0 && parent.parent !== null) {
+            writer.remove(parent);
+            changed = true;
+          }
+        }
+      }
+
+      return changed;
+    });
+  }
+
   /**
    * Register `imageInline` model elements for subscription cleanup
    * on model changes.

packages/ckeditor5-coremedia-images/src/converters.ts

@@ -78,6 +78,77 @@ export const preventUpcastImageSrc =
     );
   };
 
+/**
+ * High-priority upcast converter that creates `imageInline` for any
+ * `<img data-xlink-href="...">` when the current insertion position allows
+ * inline content (e.g. inside a paragraph).
+ *
+ * Without this, `ImageBlockEditing`'s `normal`-priority converter runs first
+ * and calls `safeInsert` for an `imageBlock`. When the drop cursor is *inside*
+ * a paragraph (between letters), `safeInsert` splits that paragraph to
+ * accommodate the block element, leaving empty paragraph fragments behind.
+ *
+ * By creating `imageInline` at high priority for inline positions, no
+ * paragraph splitting occurs. When the cursor is at a block-level position
+ * (between paragraphs) `imageInline` is not allowed there; the converter
+ * returns without consuming so that `ImageBlockEditing` can place an
+ * `imageBlock` at block level. The `imageBlock` post-fixer then converts
+ * that back to `paragraph + imageInline` without any splitting.
+ */
+export const upcastContentImageAsInline =
+  (): ((dispatcher: UpcastDispatcher) => void) =>
+  (dispatcher: UpcastDispatcher): void => {
+    dispatcher.on(
+      `element:img`,
+      (evt: EventInfo, data, conversionApi: UpcastConversionApi) => {
+        if (!data.viewItem.hasAttribute("data-xlink-href")) {
+          return;
+        }
+
+        if (!conversionApi.consumable.test(data.viewItem, { name: true })) {
+          return;
+        }
+        // Only proceed when imageInline is allowed at the current cursor
+        // position. If not (e.g. between block elements at root level), fall
+        // through to ImageBlockEditing so the post-fixer can handle it.
+
+        if (!conversionApi.schema.checkChild(data.modelCursor, "imageInline")) {
+          return;
+        }
+
+        const xlinkHref = String(data.viewItem.getAttribute("data-xlink-href"));
+        const imageInline = conversionApi.writer.createElement("imageInline", {
+          "xlink-href": xlinkHref,
+        });
+
+        if (!conversionApi.safeInsert(imageInline, data.modelCursor)) {
+          return;
+        }
+
+        // Consume the element name so that ImageBlockEditing and
+        // ImageInlineEditing (both guard with `consumable.test(viewItem,
+        // { name: true })`) skip this element — they would otherwise create
+        // a duplicate model element.
+        //
+        // Also consume `data-xlink-href` so the attributeToAttribute upcast
+        // converter does not try to set xlink-href a second time (we already
+        // set it directly in createElement above).
+        //
+        // IMPORTANT: do NOT call evt.stop() here. Other lower-priority
+        // element:img listeners include CKEditor's attributeToAttribute
+        // converters for `alt`, `width`, `height`, etc. They are guarded by
+        // their own consumable entries ({ attributes: 'alt' }), not by
+        // { name: true }, so they will still fire, find the imageInline via
+        // data.modelRange, and set the remaining attributes correctly.
+        conversionApi.consumable.consume(data.viewItem, { name: true });
+        conversionApi.consumable.consume(data.viewItem, { attributes: "data-xlink-href" });
+
+        conversionApi.updateConversionResult(imageInline, data);
+      },
+      { priority: "high" },
+    );
+  };
+
 /**
  * Conversion for `modelElementName:xlink-href` to `img:src`.
  *