Merge pull request #2716 from microsoft/u/juliaroldi/remove-span-solu…

…tion Remove span from image
microsoft · Jun 19, 2024 · f26ce35 · f26ce35
2 parents 129c3c4 + e164299
commit f26ce35
Show file tree

Hide file tree

Showing 10 changed files with 31 additions and 82 deletions.
diff --git a/...ages/roosterjs-content-model-core/lib/coreApi/setDOMSelection/ensureImageHasSpanParent.ts b/...ages/roosterjs-content-model-core/lib/coreApi/setDOMSelection/ensureImageHasSpanParent.ts
diff --git a/packages/roosterjs-content-model-core/lib/coreApi/setDOMSelection/setDOMSelection.ts b/packages/roosterjs-content-model-core/lib/coreApi/setDOMSelection/setDOMSelection.ts
@@ -1,6 +1,5 @@
 import { addRangeToSelection } from './addRangeToSelection';
 import { areSameSelections } from '../../corePlugin/cache/areSameSelections';
-import { ensureImageHasSpanParent } from './ensureImageHasSpanParent';
 import { ensureUniqueId } from '../setEditorStyle/ensureUniqueId';
 import { findLastedCoInMergedCell } from './findLastedCoInMergedCell';
 import { findTableCellElement } from './findTableCellElement';
@@ -20,6 +19,7 @@ const TABLE_ID = 'table';
 const CARET_CSS_RULE = 'caret-color: transparent';
 const TRANSPARENT_SELECTION_CSS_RULE = 'background-color: transparent !important;';
 const SELECTION_SELECTOR = '*::selection';
+const DEFAULT_SELECTION_BORDER_COLOR = '#DB626C';
 
 /**
  * @internal
@@ -45,23 +45,21 @@ export const setDOMSelection: SetDOMSelection = (core, selection, skipSelectionC
     try {
         switch (selection?.type) {
             case 'image':
-                const image = ensureImageHasSpanParent(selection.image);
+                const image = selection.image;
+
+                core.selection.selection = selection;
 
-                core.selection.selection = {
-                    type: 'image',
-                    image,
-                };
                 const imageSelectionColor = isDarkMode
                     ? core.selection.imageSelectionBorderColorDark
                     : core.selection.imageSelectionBorderColor;
 
                 core.api.setEditorStyle(
                     core,
                     DOM_SELECTION_CSS_KEY,
-                    `outline-style:solid!important; outline-color:${imageSelectionColor}!important;display: ${
-                        core.environment.isSafari ? '-webkit-inline-flex' : 'inline-flex'
-                    };`,
-                    [`span:has(>img#${ensureUniqueId(image, IMAGE_ID)})`]
+                    `outline-style:auto!important; outline-color:${
+                        imageSelectionColor || DEFAULT_SELECTION_BORDER_COLOR
+                    }!important;`,
+                    [`#${ensureUniqueId(image, IMAGE_ID)}`]
                 );
                 core.api.setEditorStyle(
                     core,

diff --git a/packages/roosterjs-content-model-core/test/coreApi/setDOMSelection/setDOMSelectionTest.ts b/packages/roosterjs-content-model-core/test/coreApi/setDOMSelection/setDOMSelectionTest.ts
@@ -1,7 +1,7 @@
 import * as addRangeToSelection from '../../../lib/coreApi/setDOMSelection/addRangeToSelection';
-import * as ensureImageHasSpanParent from '../../../lib/coreApi/setDOMSelection/ensureImageHasSpanParent';
 import { DOMSelection, EditorCore } from 'roosterjs-content-model-types';
 import { setDOMSelection } from '../../../lib/coreApi/setDOMSelection/setDOMSelection';
+
 import {
     DEFAULT_SELECTION_BORDER_COLOR,
     DEFAULT_TABLE_CELL_SELECTION_BACKGROUND_COLOR,
@@ -314,8 +314,8 @@ describe('setDOMSelection', () => {
             expect(setEditorStyleSpy).toHaveBeenCalledWith(
                 core,
                 '_DOMSelection',
-                'outline-style:solid!important; outline-color:#DB626C!important;display: inline-flex;',
-                ['span:has(>img#image_0)']
+                'outline-style:auto!important; outline-color:#DB626C!important;',
+                ['#image_0']
             );
             expect(setEditorStyleSpy).toHaveBeenCalledWith(
                 core,
@@ -374,8 +374,8 @@ describe('setDOMSelection', () => {
             expect(setEditorStyleSpy).toHaveBeenCalledWith(
                 core,
                 '_DOMSelection',
-                'outline-style:solid!important; outline-color:red!important;display: inline-flex;',
-                ['span:has(>img#image_0)']
+                'outline-style:auto!important; outline-color:red!important;',
+                ['#image_0']
             );
             expect(setEditorStyleSpy).toHaveBeenCalledWith(
                 core,
@@ -441,8 +441,8 @@ describe('setDOMSelection', () => {
             expect(setEditorStyleSpy).toHaveBeenCalledWith(
                 coreValue,
                 '_DOMSelection',
-                'outline-style:solid!important; outline-color:DarkColorMock-red!important;display: inline-flex;',
-                ['span:has(>img#image_0)']
+                'outline-style:auto!important; outline-color:DarkColorMock-red!important;',
+                ['#image_0']
             );
             expect(setEditorStyleSpy).toHaveBeenCalledWith(
                 coreValue,
@@ -502,8 +502,8 @@ describe('setDOMSelection', () => {
             expect(setEditorStyleSpy).toHaveBeenCalledWith(
                 core,
                 '_DOMSelection',
-                'outline-style:solid!important; outline-color:#DB626C!important;display: inline-flex;',
-                ['span:has(>img#image_0)']
+                'outline-style:auto!important; outline-color:#DB626C!important;',
+                ['#image_0']
             );
             expect(setEditorStyleSpy).toHaveBeenCalledWith(
                 core,
@@ -563,8 +563,8 @@ describe('setDOMSelection', () => {
             expect(setEditorStyleSpy).toHaveBeenCalledWith(
                 core,
                 '_DOMSelection',
-                'outline-style:solid!important; outline-color:#DB626C!important;display: inline-flex;',
-                ['span:has(>img#image_0_0)']
+                'outline-style:auto!important; outline-color:#DB626C!important;',
+                ['#image_0_0']
             );
             expect(setEditorStyleSpy).toHaveBeenCalledWith(
                 core,
@@ -927,9 +927,6 @@ describe('setDOMSelection', () => {
     describe('Same selection', () => {
         beforeEach(() => {
             querySelectorAllSpy.and.returnValue([]);
-            spyOn(ensureImageHasSpanParent, 'ensureImageHasSpanParent').and.callFake(
-                image => image
-            );
         });
 
         function runTest(

diff --git a/packages/roosterjs-content-model-dom/lib/modelToDom/optimizers/removeUnnecessarySpan.ts b/packages/roosterjs-content-model-dom/lib/modelToDom/optimizers/removeUnnecessarySpan.ts
@@ -8,8 +8,7 @@ export function removeUnnecessarySpan(root: Node) {
         if (
             isNodeOfType(child, 'ELEMENT_NODE') &&
             child.tagName == 'SPAN' &&
-            child.attributes.length == 0 &&
-            !isImageSpan(child)
+            child.attributes.length == 0
         ) {
             const node = child;
             let refNode = child.nextSibling;
@@ -27,11 +26,3 @@ export function removeUnnecessarySpan(root: Node) {
         }
     }
 }
-
-const isImageSpan = (child: HTMLElement) => {
-    return (
-        isNodeOfType(child.firstChild, 'ELEMENT_NODE') &&
-        child.firstChild.tagName == 'IMG' &&
-        child.firstChild == child.lastChild
-    );
-};
diff --git a/packages/roosterjs-content-model-dom/test/modelToDom/optimizers/removeUnnecessarySpanTest.ts b/packages/roosterjs-content-model-dom/test/modelToDom/optimizers/removeUnnecessarySpanTest.ts
@@ -54,12 +54,12 @@ describe('removeUnnecessarySpan', () => {
         expect(div.innerHTML).toBe('test1<span id="a">test2</span><b>test3</b>');
     });
 
-    it('Do not remove image span', () => {
+    it('Remove image span', () => {
         const div = document.createElement('div');
-        div.innerHTML = '<span><img src="test" /></span>';
+        div.innerHTML = '<span><img src="test"></span>';
 
         removeUnnecessarySpan(div);
 
-        expect(div.innerHTML).toBe('<span><img src="test"></span>');
+        expect(div.innerHTML).toBe('<img src="test">');
     });
 });
diff --git a/packages/roosterjs-content-model-plugins/lib/imageEdit/ImageEditPlugin.ts b/packages/roosterjs-content-model-plugins/lib/imageEdit/ImageEditPlugin.ts
@@ -125,10 +125,6 @@ export class ImageEditPlugin implements ImageEditor, EditorPlugin {
         image: HTMLImageElement,
         apiOperation?: ImageEditOperation
     ) {
-        const imageSpan = image.parentElement;
-        if (!imageSpan || (imageSpan && !isElementOfType(imageSpan, 'span'))) {
-            return;
-        }
         this.imageEditInfo = getSelectedImageMetadata(editor, image);
         this.lastSrc = image.getAttribute('src');
         this.imageHTMLOptions = getHTMLImageOptions(editor, this.options, this.imageEditInfo);
@@ -142,7 +138,6 @@ export class ImageEditPlugin implements ImageEditor, EditorPlugin {
         } = createImageWrapper(
             editor,
             image,
-            imageSpan,
             this.options,
             this.imageEditInfo,
             this.imageHTMLOptions,

diff --git a/packages/roosterjs-content-model-plugins/lib/imageEdit/utils/createImageWrapper.ts b/packages/roosterjs-content-model-plugins/lib/imageEdit/utils/createImageWrapper.ts
@@ -1,6 +1,7 @@
 import { createImageCropper } from '../Cropper/createImageCropper';
 import { createImageResizer } from '../Resizer/createImageResizer';
 import { createImageRotator } from '../Rotator/createImageRotator';
+import { wrap } from 'roosterjs-content-model-dom';
 
 import type {
     IEditor,
@@ -28,7 +29,6 @@ export interface WrapperElements {
 export function createImageWrapper(
     editor: IEditor,
     image: HTMLImageElement,
-    imageSpan: HTMLSpanElement,
     options: ImageEditOptions,
     editInfo: ImageMetadataFormat,
     htmlOptions: ImageHtmlOptions,
@@ -60,6 +60,7 @@ export function createImageWrapper(
         rotators,
         croppers
     );
+    const imageSpan = wrap(doc, image, 'span');
     const shadowSpan = createShadowSpan(wrapper, imageSpan);
     return { wrapper, shadowSpan, imageClone, resizers, rotators, croppers };
 }
@@ -97,7 +98,9 @@ const createWrapper = (
             editInfo.angleRad ?? 0
         }rad); text-align: left;`
     );
-    wrapper.style.display = editor.getEnvironment().isSafari ? 'inline-block' : 'inline-flex';
+    wrapper.style.display = editor.getEnvironment().isSafari
+        ? '-webkit-inline-flex'
+        : 'inline-flex';
 
     const border = createBorder(editor, options.borderColor);
     wrapper.appendChild(imageBox);

diff --git a/packages/roosterjs-content-model-plugins/test/imageEdit/Rotator/updateRotateHandleTest.ts b/packages/roosterjs-content-model-plugins/test/imageEdit/Rotator/updateRotateHandleTest.ts
@@ -57,15 +57,7 @@ xdescribe('updateRotateHandlePosition', () => {
             bottomPercent: 0,
             angleRad: 0,
         };
-        const { wrapper } = createImageWrapper(
-            editor,
-            image,
-            imageSpan,
-            {},
-            imageInfo,
-            options,
-            'rotate'
-        );
+        const { wrapper } = createImageWrapper(editor, image, {}, imageInfo, options, 'rotate');
         const rotateCenter = wrapper.querySelector('.r_rotateC')! as HTMLElement;
         const rotateHandle = wrapper.querySelector('.r_rotateH')! as HTMLElement;
         spyOn(rotateHandle, 'getBoundingClientRect').and.returnValues(rotatePosition);

diff --git a/packages/roosterjs-content-model-plugins/test/imageEdit/utils/createImageWrapperTest.ts b/packages/roosterjs-content-model-plugins/test/imageEdit/utils/createImageWrapperTest.ts
@@ -24,7 +24,7 @@ describe('createImageWrapper', () => {
         const result = createImageWrapper(
             editor,
             image,
-            imageSpan,
+
             options,
             editInfo,
             htmlOptions,

diff --git a/packages/roosterjs-content-model-plugins/test/imageEdit/utils/updateWrapperTest.ts b/packages/roosterjs-content-model-plugins/test/imageEdit/utils/updateWrapperTest.ts
@@ -32,15 +32,12 @@ describe('updateWrapper', () => {
         isSmallImage: false,
     };
     const image = document.createElement('img');
-    const imageSpan = document.createElement('span');
-    imageSpan.appendChild(image);
-    document.body.appendChild(imageSpan);
+    document.body.appendChild(image);
 
     it('should update size', () => {
         const { wrapper, imageClone, resizers } = createImageWrapper(
             editor,
             image,
-            imageSpan,
             options,
             editInfo,
             htmlOptions,