From 9510406f8b6856eea29984ff49e7c9df5cc4bd2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=BAlia=20Roldi?= Date: Fri, 26 Apr 2024 21:21:51 -0300 Subject: [PATCH 1/7] Wip --- .../setDOMSelection/setDOMSelection.ts | 2 +- .../coreApi/setEditorStyle/setEditorStyle.ts | 3 +-- .../corePlugin/selection/SelectionPlugin.ts | 22 ++++++++++++++++++- .../selection/isSingleImageInSelection.ts | 2 +- .../setDOMSelection/setDOMSelectionTest.ts | 14 +++++++----- .../optimizers/removeUnnecessarySpan.ts | 10 ++++++++- 6 files changed, 42 insertions(+), 11 deletions(-) diff --git a/packages/roosterjs-content-model-core/lib/coreApi/setDOMSelection/setDOMSelection.ts b/packages/roosterjs-content-model-core/lib/coreApi/setDOMSelection/setDOMSelection.ts index 2d199b1889d..a5b19de3146 100644 --- a/packages/roosterjs-content-model-core/lib/coreApi/setDOMSelection/setDOMSelection.ts +++ b/packages/roosterjs-content-model-core/lib/coreApi/setDOMSelection/setDOMSelection.ts @@ -48,7 +48,7 @@ export const setDOMSelection: SetDOMSelection = (core, selection, skipSelectionC `outline-style:auto!important; outline-color:${ core.selection.imageSelectionBorderColor || DEFAULT_SELECTION_BORDER_COLOR }!important;`, - [`#${ensureUniqueId(image, IMAGE_ID)}`] + [`span:has(>img#${ensureUniqueId(image, IMAGE_ID)})`] ); core.api.setEditorStyle( core, diff --git a/packages/roosterjs-content-model-core/lib/coreApi/setEditorStyle/setEditorStyle.ts b/packages/roosterjs-content-model-core/lib/coreApi/setEditorStyle/setEditorStyle.ts index a26a905e1f4..57adfc05282 100644 --- a/packages/roosterjs-content-model-core/lib/coreApi/setEditorStyle/setEditorStyle.ts +++ b/packages/roosterjs-content-model-core/lib/coreApi/setEditorStyle/setEditorStyle.ts @@ -44,7 +44,7 @@ export const setEditorStyle: SetEditorStyle = ( subSelectors, maxRuleLength - cssRule.length - 3 // minus 3 for " {}" ); - + console.log(); selectors.forEach(selector => { sheet.insertRule(`${selector} {${cssRule}}`); }); @@ -72,6 +72,5 @@ function buildSelectors(rootSelector: string, subSelectors: string[], maxLen: nu }); result.push(stringBuilder.join(',')); - return result; } diff --git a/packages/roosterjs-content-model-core/lib/corePlugin/selection/SelectionPlugin.ts b/packages/roosterjs-content-model-core/lib/corePlugin/selection/SelectionPlugin.ts index 9dfef270c5c..9bb34b4223c 100644 --- a/packages/roosterjs-content-model-core/lib/corePlugin/selection/SelectionPlugin.ts +++ b/packages/roosterjs-content-model-core/lib/corePlugin/selection/SelectionPlugin.ts @@ -660,7 +660,9 @@ class SelectionPlugin implements PluginWithState { private trySelectSingleImage(selection: RangeSelection) { if (!selection.range.collapsed) { const image = isSingleImageInSelection(selection.range); - if (image) { + const imageSpan = image?.parentNode; + + if (image && imageSpan && ensureImageHasSpanParent(image)) { this.setDOMSelection( { type: 'image', @@ -673,6 +675,24 @@ class SelectionPlugin implements PluginWithState { } } +function ensureImageHasSpanParent(image: HTMLImageElement) { + const parent = image.parentElement; + if (!parent) return false; + if ( + isNodeOfType(parent, 'ELEMENT_NODE') && + isElementOfType(parent, 'span') && + parent.firstElementChild == image && + parent.lastElementChild == image + ) { + return true; + } + + const span = image.ownerDocument.createElement('span'); + span.appendChild(image); + parent?.appendChild(span); + return true; +} + /** * @internal * Create a new instance of SelectionPlugin. diff --git a/packages/roosterjs-content-model-core/lib/corePlugin/selection/isSingleImageInSelection.ts b/packages/roosterjs-content-model-core/lib/corePlugin/selection/isSingleImageInSelection.ts index a63d9e80f91..18df04f0ddd 100644 --- a/packages/roosterjs-content-model-core/lib/corePlugin/selection/isSingleImageInSelection.ts +++ b/packages/roosterjs-content-model-core/lib/corePlugin/selection/isSingleImageInSelection.ts @@ -5,7 +5,6 @@ import { isElementOfType, isNodeOfType } from 'roosterjs-content-model-dom'; */ export function isSingleImageInSelection(selection: Selection | Range): HTMLImageElement | null { const { startNode, endNode, startOffset, endOffset } = getProps(selection); - const max = Math.max(startOffset, endOffset); const min = Math.min(startOffset, endOffset); @@ -17,6 +16,7 @@ export function isSingleImageInSelection(selection: Selection | Range): HTMLImag } return null; } + function getProps( selection: Selection | Range ): { startNode: Node | null; endNode: Node | null; startOffset: number; endOffset: number } { diff --git a/packages/roosterjs-content-model-core/test/coreApi/setDOMSelection/setDOMSelectionTest.ts b/packages/roosterjs-content-model-core/test/coreApi/setDOMSelection/setDOMSelectionTest.ts index a2dd2548a3c..eddea35f524 100644 --- a/packages/roosterjs-content-model-core/test/coreApi/setDOMSelection/setDOMSelectionTest.ts +++ b/packages/roosterjs-content-model-core/test/coreApi/setDOMSelection/setDOMSelectionTest.ts @@ -221,9 +221,13 @@ describe('setDOMSelection', () => { describe('Image selection', () => { let mockedImage: HTMLImageElement; - beforeEach(() => { mockedImage = { + parentElement: { + ownerDocument: doc, + firstElementChild: mockedImage, + lastElementChild: mockedImage, + }, ownerDocument: doc, } as any; }); @@ -274,7 +278,7 @@ describe('setDOMSelection', () => { core, '_DOMSelection', 'outline-style:auto!important; outline-color:#DB626C!important;', - ['#image_0'] + ['span:has(>img#image_0)'] ); }); @@ -326,7 +330,7 @@ describe('setDOMSelection', () => { core, '_DOMSelection', 'outline-style:auto!important; outline-color:#DB626C!important;', - ['#image_0_0'] + ['span:has(>img#image_0_0)'] ); expect(setEditorStyleSpy).toHaveBeenCalledWith( core, @@ -385,7 +389,7 @@ describe('setDOMSelection', () => { core, '_DOMSelection', 'outline-style:auto!important; outline-color:red!important;', - ['#image_0'] + ['span:has(>img#image_0_0)'] ); expect(setEditorStyleSpy).toHaveBeenCalledWith( core, @@ -439,7 +443,7 @@ describe('setDOMSelection', () => { core, '_DOMSelection', 'outline-style:auto!important; outline-color:#DB626C!important;', - ['#image_0'] + ['span:has(>img#image_0_0)'] ); expect(setEditorStyleSpy).toHaveBeenCalledWith( core, diff --git a/packages/roosterjs-content-model-dom/lib/modelToDom/optimizers/removeUnnecessarySpan.ts b/packages/roosterjs-content-model-dom/lib/modelToDom/optimizers/removeUnnecessarySpan.ts index ee048a395bb..891846aeecc 100644 --- a/packages/roosterjs-content-model-dom/lib/modelToDom/optimizers/removeUnnecessarySpan.ts +++ b/packages/roosterjs-content-model-dom/lib/modelToDom/optimizers/removeUnnecessarySpan.ts @@ -8,7 +8,8 @@ export function removeUnnecessarySpan(root: Node) { if ( isNodeOfType(child, 'ELEMENT_NODE') && child.tagName == 'SPAN' && - child.attributes.length == 0 + child.attributes.length == 0 && + !isImageSpan(child) ) { const node = child; let refNode = child.nextSibling; @@ -26,3 +27,10 @@ export function removeUnnecessarySpan(root: Node) { } } } + +const isImageSpan = (child: HTMLElement) => { + return ( + child?.firstElementChild?.tagName == 'IMG' && + child.firstElementChild == child.lastElementChild + ); +}; From 08ec7b00dacb073987e453647f94b4f9e1853dfb Mon Sep 17 00:00:00 2001 From: "Julia Roldi (from Dev Box)" Date: Mon, 29 Apr 2024 11:41:57 -0300 Subject: [PATCH 2/7] add span --- .../coreApi/setEditorStyle/setEditorStyle.ts | 2 +- .../corePlugin/selection/SelectionPlugin.ts | 4 +- .../setDOMSelection/setDOMSelectionTest.ts | 58 +++++++++---------- .../selection/SelectionPluginTest.ts | 36 ++++++++++++ 4 files changed, 68 insertions(+), 32 deletions(-) diff --git a/packages/roosterjs-content-model-core/lib/coreApi/setEditorStyle/setEditorStyle.ts b/packages/roosterjs-content-model-core/lib/coreApi/setEditorStyle/setEditorStyle.ts index 57adfc05282..c0d6f5ee91c 100644 --- a/packages/roosterjs-content-model-core/lib/coreApi/setEditorStyle/setEditorStyle.ts +++ b/packages/roosterjs-content-model-core/lib/coreApi/setEditorStyle/setEditorStyle.ts @@ -44,7 +44,7 @@ export const setEditorStyle: SetEditorStyle = ( subSelectors, maxRuleLength - cssRule.length - 3 // minus 3 for " {}" ); - console.log(); + selectors.forEach(selector => { sheet.insertRule(`${selector} {${cssRule}}`); }); diff --git a/packages/roosterjs-content-model-core/lib/corePlugin/selection/SelectionPlugin.ts b/packages/roosterjs-content-model-core/lib/corePlugin/selection/SelectionPlugin.ts index 9bb34b4223c..4ee9262b868 100644 --- a/packages/roosterjs-content-model-core/lib/corePlugin/selection/SelectionPlugin.ts +++ b/packages/roosterjs-content-model-core/lib/corePlugin/selection/SelectionPlugin.ts @@ -677,8 +677,8 @@ class SelectionPlugin implements PluginWithState { function ensureImageHasSpanParent(image: HTMLImageElement) { const parent = image.parentElement; - if (!parent) return false; if ( + parent && isNodeOfType(parent, 'ELEMENT_NODE') && isElementOfType(parent, 'span') && parent.firstElementChild == image && @@ -690,7 +690,7 @@ function ensureImageHasSpanParent(image: HTMLImageElement) { const span = image.ownerDocument.createElement('span'); span.appendChild(image); parent?.appendChild(span); - return true; + return !!parent; } /** diff --git a/packages/roosterjs-content-model-core/test/coreApi/setDOMSelection/setDOMSelectionTest.ts b/packages/roosterjs-content-model-core/test/coreApi/setDOMSelection/setDOMSelectionTest.ts index eddea35f524..b56baceb7bc 100644 --- a/packages/roosterjs-content-model-core/test/coreApi/setDOMSelection/setDOMSelectionTest.ts +++ b/packages/roosterjs-content-model-core/test/coreApi/setDOMSelection/setDOMSelectionTest.ts @@ -282,7 +282,7 @@ describe('setDOMSelection', () => { ); }); - it('image selection with duplicated id', () => { + it('image selection with customized selection border color', () => { const mockedSelection = { type: 'image', image: mockedImage, @@ -294,12 +294,11 @@ describe('setDOMSelection', () => { collapse: collapseSpy, }; - mockedImage.id = 'image_0'; + core.selection.imageSelectionBorderColor = 'red'; + createRangeSpy.and.returnValue(mockedRange); - querySelectorAllSpy.and.callFake(selector => { - return selector == '#image_0' ? ['', ''] : ['']; - }); + querySelectorAllSpy.and.returnValue([]); hasFocusSpy.and.returnValue(false); setDOMSelection(core, mockedSelection); @@ -307,6 +306,7 @@ describe('setDOMSelection', () => { expect(core.selection).toEqual({ skipReselectOnFocus: undefined, selection: mockedSelection, + imageSelectionBorderColor: 'red', } as any); expect(triggerEventSpy).toHaveBeenCalledWith( core, @@ -321,6 +321,7 @@ describe('setDOMSelection', () => { expect(addRangeToSelectionSpy).toHaveBeenCalledWith(doc, mockedRange, undefined); expect(setEditorStyleSpy).toHaveBeenCalledTimes(5); expect(setEditorStyleSpy).toHaveBeenCalledWith(core, '_DOMSelection', null); + expect(setEditorStyleSpy).toHaveBeenCalledWith(core, '_DOMSelectionHideCursor', null); expect(setEditorStyleSpy).toHaveBeenCalledWith( core, '_DOMSelectionHideSelection', @@ -329,8 +330,8 @@ describe('setDOMSelection', () => { expect(setEditorStyleSpy).toHaveBeenCalledWith( core, '_DOMSelection', - 'outline-style:auto!important; outline-color:#DB626C!important;', - ['span:has(>img#image_0_0)'] + 'outline-style:auto!important; outline-color:red!important;', + ['span:has(>img#image_0)'] ); expect(setEditorStyleSpy).toHaveBeenCalledWith( core, @@ -340,7 +341,7 @@ describe('setDOMSelection', () => { ); }); - it('image selection with customized selection border color', () => { + it('do not select if node is out of document', () => { const mockedSelection = { type: 'image', image: mockedImage, @@ -352,7 +353,7 @@ describe('setDOMSelection', () => { collapse: collapseSpy, }; - core.selection.imageSelectionBorderColor = 'red'; + doc.contains = () => false; createRangeSpy.and.returnValue(mockedRange); @@ -364,7 +365,6 @@ describe('setDOMSelection', () => { expect(core.selection).toEqual({ skipReselectOnFocus: undefined, selection: mockedSelection, - imageSelectionBorderColor: 'red', } as any); expect(triggerEventSpy).toHaveBeenCalledWith( core, @@ -374,22 +374,18 @@ describe('setDOMSelection', () => { }, true ); - expect(selectNodeSpy).toHaveBeenCalledWith(mockedImage); - expect(collapseSpy).not.toHaveBeenCalledWith(); - expect(addRangeToSelectionSpy).toHaveBeenCalledWith(doc, mockedRange, undefined); + expect(selectNodeSpy).not.toHaveBeenCalled(); + expect(collapseSpy).not.toHaveBeenCalled(); + expect(addRangeToSelectionSpy).not.toHaveBeenCalled(); + expect(mockedImage.id).toBe('image_0'); expect(setEditorStyleSpy).toHaveBeenCalledTimes(5); expect(setEditorStyleSpy).toHaveBeenCalledWith(core, '_DOMSelection', null); expect(setEditorStyleSpy).toHaveBeenCalledWith(core, '_DOMSelectionHideCursor', null); - expect(setEditorStyleSpy).toHaveBeenCalledWith( - core, - '_DOMSelectionHideSelection', - null - ); expect(setEditorStyleSpy).toHaveBeenCalledWith( core, '_DOMSelection', - 'outline-style:auto!important; outline-color:red!important;', - ['span:has(>img#image_0_0)'] + 'outline-style:auto!important; outline-color:#DB626C!important;', + ['span:has(>img#image_0)'] ); expect(setEditorStyleSpy).toHaveBeenCalledWith( core, @@ -399,7 +395,7 @@ describe('setDOMSelection', () => { ); }); - it('do not select if node is out of document', () => { + it('image selection with duplicated id', () => { const mockedSelection = { type: 'image', image: mockedImage, @@ -411,11 +407,12 @@ describe('setDOMSelection', () => { collapse: collapseSpy, }; - doc.contains = () => false; - + mockedImage.id = 'image_0'; createRangeSpy.and.returnValue(mockedRange); - querySelectorAllSpy.and.returnValue([]); + querySelectorAllSpy.and.callFake(selector => { + return selector == '#image_0' ? ['', ''] : ['']; + }); hasFocusSpy.and.returnValue(false); setDOMSelection(core, mockedSelection); @@ -432,13 +429,16 @@ describe('setDOMSelection', () => { }, true ); - expect(selectNodeSpy).not.toHaveBeenCalled(); - expect(collapseSpy).not.toHaveBeenCalled(); - expect(addRangeToSelectionSpy).not.toHaveBeenCalled(); - expect(mockedImage.id).toBe('image_0'); + expect(selectNodeSpy).toHaveBeenCalledWith(mockedImage); + expect(collapseSpy).not.toHaveBeenCalledWith(); + expect(addRangeToSelectionSpy).toHaveBeenCalledWith(doc, mockedRange, undefined); expect(setEditorStyleSpy).toHaveBeenCalledTimes(5); expect(setEditorStyleSpy).toHaveBeenCalledWith(core, '_DOMSelection', null); - expect(setEditorStyleSpy).toHaveBeenCalledWith(core, '_DOMSelectionHideCursor', null); + expect(setEditorStyleSpy).toHaveBeenCalledWith( + core, + '_DOMSelectionHideSelection', + null + ); expect(setEditorStyleSpy).toHaveBeenCalledWith( core, '_DOMSelection', diff --git a/packages/roosterjs-content-model-core/test/corePlugin/selection/SelectionPluginTest.ts b/packages/roosterjs-content-model-core/test/corePlugin/selection/SelectionPluginTest.ts index cc1adf1e727..f373f696049 100644 --- a/packages/roosterjs-content-model-core/test/corePlugin/selection/SelectionPluginTest.ts +++ b/packages/roosterjs-content-model-core/test/corePlugin/selection/SelectionPluginTest.ts @@ -2416,6 +2416,9 @@ describe('SelectionPlugin selectionChange on image selected', () => { it('onSelectionChange on image | 4', () => { const image = document.createElement('img'); + const div = document.createElement('div'); + div.appendChild(image); + document.body.appendChild(div); spyOn(isSingleImageInSelection, 'isSingleImageInSelection').and.returnValue(image); const plugin = createSelectionPlugin({}); @@ -2448,5 +2451,38 @@ describe('SelectionPlugin selectionChange on image selected', () => { image, }); expect(getDOMSelectionSpy).toHaveBeenCalledTimes(1); + document.body.removeChild(div); + }); + + it('onSelectionChange on image with image without parents | 5', () => { + const image = document.createElement('img'); + spyOn(isSingleImageInSelection, 'isSingleImageInSelection').and.returnValue(image); + + const plugin = createSelectionPlugin({}); + const state = plugin.getState(); + const mockedOldSelection = { + type: 'image', + image: {} as any, + } as DOMSelection; + + state.selection = mockedOldSelection; + + plugin.initialize(editor); + + const onSelectionChange = addEventListenerSpy.calls.argsFor(0)[1] as Function; + const mockedNewSelection = { + type: 'range', + range: {} as any, + } as any; + + hasFocusSpy.and.returnValue(true); + isInShadowEditSpy.and.returnValue(false); + getDOMSelectionSpy.and.returnValue(mockedNewSelection); + getRangeAtSpy.and.returnValue({ startContainer: {} }); + + onSelectionChange(); + + expect(setDOMSelectionSpy).not.toHaveBeenCalled(); + expect(getDOMSelectionSpy).toHaveBeenCalledTimes(1); }); }); From dcfe23bf6204dd057f3b9d970d46c44956cc8f02 Mon Sep 17 00:00:00 2001 From: "Julia Roldi (from Dev Box)" Date: Mon, 29 Apr 2024 11:54:12 -0300 Subject: [PATCH 3/7] fix test --- .../lib/coreApi/setEditorStyle/setEditorStyle.ts | 1 + .../lib/corePlugin/selection/isSingleImageInSelection.ts | 2 +- .../modelToDom/optimizers/removeUnnecessarySpanTest.ts | 9 +++++++++ 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/packages/roosterjs-content-model-core/lib/coreApi/setEditorStyle/setEditorStyle.ts b/packages/roosterjs-content-model-core/lib/coreApi/setEditorStyle/setEditorStyle.ts index c0d6f5ee91c..a26a905e1f4 100644 --- a/packages/roosterjs-content-model-core/lib/coreApi/setEditorStyle/setEditorStyle.ts +++ b/packages/roosterjs-content-model-core/lib/coreApi/setEditorStyle/setEditorStyle.ts @@ -72,5 +72,6 @@ function buildSelectors(rootSelector: string, subSelectors: string[], maxLen: nu }); result.push(stringBuilder.join(',')); + return result; } diff --git a/packages/roosterjs-content-model-core/lib/corePlugin/selection/isSingleImageInSelection.ts b/packages/roosterjs-content-model-core/lib/corePlugin/selection/isSingleImageInSelection.ts index 18df04f0ddd..a63d9e80f91 100644 --- a/packages/roosterjs-content-model-core/lib/corePlugin/selection/isSingleImageInSelection.ts +++ b/packages/roosterjs-content-model-core/lib/corePlugin/selection/isSingleImageInSelection.ts @@ -5,6 +5,7 @@ import { isElementOfType, isNodeOfType } from 'roosterjs-content-model-dom'; */ export function isSingleImageInSelection(selection: Selection | Range): HTMLImageElement | null { const { startNode, endNode, startOffset, endOffset } = getProps(selection); + const max = Math.max(startOffset, endOffset); const min = Math.min(startOffset, endOffset); @@ -16,7 +17,6 @@ export function isSingleImageInSelection(selection: Selection | Range): HTMLImag } return null; } - function getProps( selection: Selection | Range ): { startNode: Node | null; endNode: Node | null; startOffset: number; endOffset: number } { diff --git a/packages/roosterjs-content-model-dom/test/modelToDom/optimizers/removeUnnecessarySpanTest.ts b/packages/roosterjs-content-model-dom/test/modelToDom/optimizers/removeUnnecessarySpanTest.ts index fd79446c9af..f2aab1c27e2 100644 --- a/packages/roosterjs-content-model-dom/test/modelToDom/optimizers/removeUnnecessarySpanTest.ts +++ b/packages/roosterjs-content-model-dom/test/modelToDom/optimizers/removeUnnecessarySpanTest.ts @@ -53,4 +53,13 @@ describe('removeUnnecessarySpan', () => { expect(div.innerHTML).toBe('test1test2test3'); }); + + it('Do not remove image span', () => { + const div = document.createElement('div'); + div.innerHTML = ''; + + removeUnnecessarySpan(div); + + expect(div.innerHTML).toBe(''); + }); }); From e7d09ebde82405b16b83209c67692b1477e13440 Mon Sep 17 00:00:00 2001 From: "Julia Roldi (from Dev Box)" Date: Tue, 30 Apr 2024 11:06:15 -0300 Subject: [PATCH 4/7] changes --- .../setDOMSelection/setDOMSelection.ts | 34 +++++++++++++++++-- .../corePlugin/selection/SelectionPlugin.ts | 20 +---------- .../setDOMSelection/setDOMSelectionTest.ts | 27 +++++++++++++-- 3 files changed, 57 insertions(+), 24 deletions(-) diff --git a/packages/roosterjs-content-model-core/lib/coreApi/setDOMSelection/setDOMSelection.ts b/packages/roosterjs-content-model-core/lib/coreApi/setDOMSelection/setDOMSelection.ts index a5b19de3146..158da4db215 100644 --- a/packages/roosterjs-content-model-core/lib/coreApi/setDOMSelection/setDOMSelection.ts +++ b/packages/roosterjs-content-model-core/lib/coreApi/setDOMSelection/setDOMSelection.ts @@ -2,7 +2,12 @@ import { addRangeToSelection } from './addRangeToSelection'; import { ensureUniqueId } from '../setEditorStyle/ensureUniqueId'; import { findLastedCoInMergedCell } from './findLastedCoInMergedCell'; import { findTableCellElement } from './findTableCellElement'; -import { isNodeOfType, parseTableCells, toArray } from 'roosterjs-content-model-dom'; +import { + isElementOfType, + isNodeOfType, + parseTableCells, + toArray, +} from 'roosterjs-content-model-dom'; import type { ParsedTable, SelectionChangedEvent, @@ -18,7 +23,7 @@ const TABLE_ID = 'table'; const DEFAULT_SELECTION_BORDER_COLOR = '#DB626C'; const TABLE_CSS_RULE = 'background-color:#C6C6C6!important;'; const CARET_CSS_RULE = 'caret-color: transparent'; -const TRANSPARENT_SELECTION_CSS_RULE = 'background-color: transparent !important'; +const TRANSPARENT_SELECTION_CSS_RULE = 'background-color: transparent !important;'; const SELECTION_SELECTOR = '*::selection'; /** @@ -41,6 +46,11 @@ export const setDOMSelection: SetDOMSelection = (core, selection, skipSelectionC case 'image': const image = selection.image; + const spanImage = ensureImageHasSpanParent(image); + if (!spanImage) { + return; + } + core.selection.selection = selection; core.api.setEditorStyle( core, @@ -231,3 +241,23 @@ function setRangeSelection(doc: Document, element: HTMLElement | undefined, coll addRangeToSelection(doc, range, isReverted); } } + +function ensureImageHasSpanParent(image: HTMLImageElement) { + const parent = image.parentElement; + + if ( + parent && + isNodeOfType(parent, 'ELEMENT_NODE') && + isElementOfType(parent, 'span') && + parent.firstElementChild == image && + parent.lastElementChild == image && + !parent.textContent + ) { + return parent; + } + + const span = image.ownerDocument.createElement('span'); + span.appendChild(image); + parent?.appendChild(span); + return parent; +} diff --git a/packages/roosterjs-content-model-core/lib/corePlugin/selection/SelectionPlugin.ts b/packages/roosterjs-content-model-core/lib/corePlugin/selection/SelectionPlugin.ts index 4ee9262b868..42bcefc91d9 100644 --- a/packages/roosterjs-content-model-core/lib/corePlugin/selection/SelectionPlugin.ts +++ b/packages/roosterjs-content-model-core/lib/corePlugin/selection/SelectionPlugin.ts @@ -662,7 +662,7 @@ class SelectionPlugin implements PluginWithState { const image = isSingleImageInSelection(selection.range); const imageSpan = image?.parentNode; - if (image && imageSpan && ensureImageHasSpanParent(image)) { + if (image && imageSpan) { this.setDOMSelection( { type: 'image', @@ -675,24 +675,6 @@ class SelectionPlugin implements PluginWithState { } } -function ensureImageHasSpanParent(image: HTMLImageElement) { - const parent = image.parentElement; - if ( - parent && - isNodeOfType(parent, 'ELEMENT_NODE') && - isElementOfType(parent, 'span') && - parent.firstElementChild == image && - parent.lastElementChild == image - ) { - return true; - } - - const span = image.ownerDocument.createElement('span'); - span.appendChild(image); - parent?.appendChild(span); - return !!parent; -} - /** * @internal * Create a new instance of SelectionPlugin. diff --git a/packages/roosterjs-content-model-core/test/coreApi/setDOMSelection/setDOMSelectionTest.ts b/packages/roosterjs-content-model-core/test/coreApi/setDOMSelection/setDOMSelectionTest.ts index b56baceb7bc..f1db9866d87 100644 --- a/packages/roosterjs-content-model-core/test/coreApi/setDOMSelection/setDOMSelectionTest.ts +++ b/packages/roosterjs-content-model-core/test/coreApi/setDOMSelection/setDOMSelectionTest.ts @@ -14,6 +14,8 @@ describe('setDOMSelection', () => { let doc: Document; let contentDiv: HTMLDivElement; let mockedRange = 'RANGE' as any; + let createElementSpy: jasmine.Spy; + let appendChildSpy: jasmine.Spy; beforeEach(() => { querySelectorAllSpy = jasmine.createSpy('querySelectorAll'); @@ -27,11 +29,16 @@ describe('setDOMSelection', () => { createRangeSpy = jasmine.createSpy('createRange'); setEditorStyleSpy = jasmine.createSpy('setEditorStyle'); containsSpy = jasmine.createSpy('contains').and.returnValue(true); + appendChildSpy = jasmine.createSpy('appendChild'); + createElementSpy = jasmine.createSpy('createElement').and.returnValue({ + appendChild: appendChildSpy, + }); doc = { querySelectorAll: querySelectorAllSpy, createRange: createRangeSpy, contains: containsSpy, + createElement: createElementSpy, } as any; contentDiv = { ownerDocument: doc, @@ -227,6 +234,7 @@ describe('setDOMSelection', () => { ownerDocument: doc, firstElementChild: mockedImage, lastElementChild: mockedImage, + appendChild: appendChildSpy, }, ownerDocument: doc, } as any; @@ -269,6 +277,7 @@ describe('setDOMSelection', () => { expect(mockedImage.id).toBe('image_0'); expect(setEditorStyleSpy).toHaveBeenCalledTimes(5); expect(setEditorStyleSpy).toHaveBeenCalledWith(core, '_DOMSelection', null); + expect(setEditorStyleSpy).toHaveBeenCalledWith(core, '_DOMSelectionHideCursor', null); expect(setEditorStyleSpy).toHaveBeenCalledWith( core, '_DOMSelectionHideSelection', @@ -280,6 +289,12 @@ describe('setDOMSelection', () => { 'outline-style:auto!important; outline-color:#DB626C!important;', ['span:has(>img#image_0)'] ); + expect(setEditorStyleSpy).toHaveBeenCalledWith( + core, + '_DOMSelectionHideSelection', + 'background-color: transparent !important;', + ['*::selection'] + ); }); it('image selection with customized selection border color', () => { @@ -336,7 +351,7 @@ describe('setDOMSelection', () => { expect(setEditorStyleSpy).toHaveBeenCalledWith( core, '_DOMSelectionHideSelection', - 'background-color: transparent !important', + 'background-color: transparent !important;', ['*::selection'] ); }); @@ -381,6 +396,11 @@ describe('setDOMSelection', () => { expect(setEditorStyleSpy).toHaveBeenCalledTimes(5); expect(setEditorStyleSpy).toHaveBeenCalledWith(core, '_DOMSelection', null); expect(setEditorStyleSpy).toHaveBeenCalledWith(core, '_DOMSelectionHideCursor', null); + expect(setEditorStyleSpy).toHaveBeenCalledWith( + core, + '_DOMSelectionHideSelection', + null + ); expect(setEditorStyleSpy).toHaveBeenCalledWith( core, '_DOMSelection', @@ -390,7 +410,7 @@ describe('setDOMSelection', () => { expect(setEditorStyleSpy).toHaveBeenCalledWith( core, '_DOMSelectionHideSelection', - 'background-color: transparent !important', + 'background-color: transparent !important;', ['*::selection'] ); }); @@ -434,6 +454,7 @@ describe('setDOMSelection', () => { expect(addRangeToSelectionSpy).toHaveBeenCalledWith(doc, mockedRange, undefined); expect(setEditorStyleSpy).toHaveBeenCalledTimes(5); expect(setEditorStyleSpy).toHaveBeenCalledWith(core, '_DOMSelection', null); + expect(setEditorStyleSpy).toHaveBeenCalledWith(core, '_DOMSelectionHideCursor', null); expect(setEditorStyleSpy).toHaveBeenCalledWith( core, '_DOMSelectionHideSelection', @@ -448,7 +469,7 @@ describe('setDOMSelection', () => { expect(setEditorStyleSpy).toHaveBeenCalledWith( core, '_DOMSelectionHideSelection', - 'background-color: transparent !important', + 'background-color: transparent !important;', ['*::selection'] ); }); From e2c6ea62e6a83ca63affc869d067a815b48fd63f Mon Sep 17 00:00:00 2001 From: "Julia Roldi (from Dev Box)" Date: Tue, 30 Apr 2024 11:08:48 -0300 Subject: [PATCH 5/7] remove code --- .../lib/corePlugin/selection/SelectionPlugin.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/roosterjs-content-model-core/lib/corePlugin/selection/SelectionPlugin.ts b/packages/roosterjs-content-model-core/lib/corePlugin/selection/SelectionPlugin.ts index 42bcefc91d9..34589ba71fd 100644 --- a/packages/roosterjs-content-model-core/lib/corePlugin/selection/SelectionPlugin.ts +++ b/packages/roosterjs-content-model-core/lib/corePlugin/selection/SelectionPlugin.ts @@ -660,9 +660,8 @@ class SelectionPlugin implements PluginWithState { private trySelectSingleImage(selection: RangeSelection) { if (!selection.range.collapsed) { const image = isSingleImageInSelection(selection.range); - const imageSpan = image?.parentNode; - if (image && imageSpan) { + if (image) { this.setDOMSelection( { type: 'image', From 73f236245d6fef9e62679b64665a1dd18ced519d Mon Sep 17 00:00:00 2001 From: "Julia Roldi (from Dev Box)" Date: Tue, 30 Apr 2024 11:20:27 -0300 Subject: [PATCH 6/7] fixes --- .../corePlugin/selection/SelectionPlugin.ts | 1 - .../selection/SelectionPluginTest.ts | 36 ------------------- 2 files changed, 37 deletions(-) diff --git a/packages/roosterjs-content-model-core/lib/corePlugin/selection/SelectionPlugin.ts b/packages/roosterjs-content-model-core/lib/corePlugin/selection/SelectionPlugin.ts index 34589ba71fd..9dfef270c5c 100644 --- a/packages/roosterjs-content-model-core/lib/corePlugin/selection/SelectionPlugin.ts +++ b/packages/roosterjs-content-model-core/lib/corePlugin/selection/SelectionPlugin.ts @@ -660,7 +660,6 @@ class SelectionPlugin implements PluginWithState { private trySelectSingleImage(selection: RangeSelection) { if (!selection.range.collapsed) { const image = isSingleImageInSelection(selection.range); - if (image) { this.setDOMSelection( { diff --git a/packages/roosterjs-content-model-core/test/corePlugin/selection/SelectionPluginTest.ts b/packages/roosterjs-content-model-core/test/corePlugin/selection/SelectionPluginTest.ts index f373f696049..cc1adf1e727 100644 --- a/packages/roosterjs-content-model-core/test/corePlugin/selection/SelectionPluginTest.ts +++ b/packages/roosterjs-content-model-core/test/corePlugin/selection/SelectionPluginTest.ts @@ -2416,9 +2416,6 @@ describe('SelectionPlugin selectionChange on image selected', () => { it('onSelectionChange on image | 4', () => { const image = document.createElement('img'); - const div = document.createElement('div'); - div.appendChild(image); - document.body.appendChild(div); spyOn(isSingleImageInSelection, 'isSingleImageInSelection').and.returnValue(image); const plugin = createSelectionPlugin({}); @@ -2451,38 +2448,5 @@ describe('SelectionPlugin selectionChange on image selected', () => { image, }); expect(getDOMSelectionSpy).toHaveBeenCalledTimes(1); - document.body.removeChild(div); - }); - - it('onSelectionChange on image with image without parents | 5', () => { - const image = document.createElement('img'); - spyOn(isSingleImageInSelection, 'isSingleImageInSelection').and.returnValue(image); - - const plugin = createSelectionPlugin({}); - const state = plugin.getState(); - const mockedOldSelection = { - type: 'image', - image: {} as any, - } as DOMSelection; - - state.selection = mockedOldSelection; - - plugin.initialize(editor); - - const onSelectionChange = addEventListenerSpy.calls.argsFor(0)[1] as Function; - const mockedNewSelection = { - type: 'range', - range: {} as any, - } as any; - - hasFocusSpy.and.returnValue(true); - isInShadowEditSpy.and.returnValue(false); - getDOMSelectionSpy.and.returnValue(mockedNewSelection); - getRangeAtSpy.and.returnValue({ startContainer: {} }); - - onSelectionChange(); - - expect(setDOMSelectionSpy).not.toHaveBeenCalled(); - expect(getDOMSelectionSpy).toHaveBeenCalledTimes(1); }); }); From ca86446c91045576c3062a773db11f9dc4dc7177 Mon Sep 17 00:00:00 2001 From: "Julia Roldi (from Dev Box)" Date: Tue, 30 Apr 2024 14:32:36 -0300 Subject: [PATCH 7/7] fixes --- .../setDOMSelection/setDOMSelection.ts | 28 ++++++++----------- .../optimizers/removeUnnecessarySpan.ts | 5 ++-- 2 files changed, 15 insertions(+), 18 deletions(-) diff --git a/packages/roosterjs-content-model-core/lib/coreApi/setDOMSelection/setDOMSelection.ts b/packages/roosterjs-content-model-core/lib/coreApi/setDOMSelection/setDOMSelection.ts index 158da4db215..a89fee21134 100644 --- a/packages/roosterjs-content-model-core/lib/coreApi/setDOMSelection/setDOMSelection.ts +++ b/packages/roosterjs-content-model-core/lib/coreApi/setDOMSelection/setDOMSelection.ts @@ -7,6 +7,7 @@ import { isNodeOfType, parseTableCells, toArray, + wrap, } from 'roosterjs-content-model-dom'; import type { ParsedTable, @@ -44,14 +45,12 @@ export const setDOMSelection: SetDOMSelection = (core, selection, skipSelectionC try { switch (selection?.type) { case 'image': - const image = selection.image; + const image = ensureImageHasSpanParent(selection.image); - const spanImage = ensureImageHasSpanParent(image); - if (!spanImage) { - return; - } - - core.selection.selection = selection; + core.selection.selection = { + type: 'image', + image, + }; core.api.setEditorStyle( core, DOM_SELECTION_CSS_KEY, @@ -242,22 +241,19 @@ function setRangeSelection(doc: Document, element: HTMLElement | undefined, coll } } -function ensureImageHasSpanParent(image: HTMLImageElement) { +function ensureImageHasSpanParent(image: HTMLImageElement): HTMLImageElement { const parent = image.parentElement; if ( parent && isNodeOfType(parent, 'ELEMENT_NODE') && isElementOfType(parent, 'span') && - parent.firstElementChild == image && - parent.lastElementChild == image && - !parent.textContent + parent.firstChild == image && + parent.lastChild == image ) { - return parent; + return image; } - const span = image.ownerDocument.createElement('span'); - span.appendChild(image); - parent?.appendChild(span); - return parent; + wrap(image.ownerDocument, image, 'span'); + return image; } diff --git a/packages/roosterjs-content-model-dom/lib/modelToDom/optimizers/removeUnnecessarySpan.ts b/packages/roosterjs-content-model-dom/lib/modelToDom/optimizers/removeUnnecessarySpan.ts index 891846aeecc..3b0ccb11b88 100644 --- a/packages/roosterjs-content-model-dom/lib/modelToDom/optimizers/removeUnnecessarySpan.ts +++ b/packages/roosterjs-content-model-dom/lib/modelToDom/optimizers/removeUnnecessarySpan.ts @@ -30,7 +30,8 @@ export function removeUnnecessarySpan(root: Node) { const isImageSpan = (child: HTMLElement) => { return ( - child?.firstElementChild?.tagName == 'IMG' && - child.firstElementChild == child.lastElementChild + isNodeOfType(child.firstChild, 'ELEMENT_NODE') && + child.firstChild.tagName == 'IMG' && + child.firstChild == child.lastChild ); };