Port Image Edit Operations #2670

juliaroldi · 2024-05-29T18:27:17Z

This change aims to Port Image edit operations from Image Edit plugin to Content Model Plugin. This feature is responsible for resize, rotate, crop, flip, resize by percentage and rotate by degree. This also removes the old Image Edit Plugin from demo site. The changes related to edit images with mouse selection will be done in a second PR.

Progress:

Feature	Status
Resize handles	Done
Rotate handle	Done
Crop handles	Done
Flip	Done
Resize by percentage	Done
Rotate by degree	Done
Unit test	Done

…u/juliaroldi/port-image

…t/roosterjs into u/juliaroldi/port-image

…om/microsoft/roosterjs into u/juliaroldi/image-edit-operations

JiuqingSong · 2024-06-03T18:36:25Z

packages/roosterjs-content-model-types/lib/parameter/ImageEditor.ts


    /**
     * Start to crop selected image
     */
-    cropImage(): void;
+    cropImage(image: HTMLImageElement): void;


Instead of passing in the image, can we just use current selected image?

We can, but I was following the pattern of ImageEditor interface

JiuqingSong · 2024-06-03T18:36:55Z

packages/roosterjs-content-model-types/lib/parameter/ImageEditor.ts

+    /**
+     * Resize and rotate an image
+     */
+    | 'resizeAndRotate';


where is this used?

This is used when we right click an image and add the wrapper, can be removed for now.

JiuqingSong · 2024-06-03T18:53:36Z

packages/roosterjs-content-model-core/lib/coreApi/setDOMSelection/setDOMSelection.ts

@@ -56,7 +55,9 @@ export const setDOMSelection: SetDOMSelection = (core, selection, skipSelectionC
                core.api.setEditorStyle(
                    core,
                    DOM_SELECTION_CSS_KEY,
-                    `outline-style:auto!important; outline-color:${imageSelectionColor}!important;`,
+                    `outline-style:auto!important; outline-color:${imageSelectionColor}!important;display: ${


It seems #2676 is caused by the outline-style here. Can we just fix it as well?

JiuqingSong · 2024-06-03T18:54:09Z

packages/roosterjs-content-model-core/lib/coreApi/setDOMSelection/setDOMSelection.ts

@@ -56,7 +55,9 @@ export const setDOMSelection: SetDOMSelection = (core, selection, skipSelectionC
                core.api.setEditorStyle(
                    core,
                    DOM_SELECTION_CSS_KEY,
-                    `outline-style:auto!important; outline-color:${imageSelectionColor}!important;`,
+                    `outline-style:auto!important; outline-color:${imageSelectionColor}!important;display: ${
+                        core.environment.isSafari ? 'inline-block' : 'inline-flex'


Curious, do we still need this check today?

We still need. :(

JiuqingSong · 2024-06-03T18:54:25Z

packages/roosterjs-content-model-dom/lib/domUtils/ensureImageHasSpanParent.ts

+    entryPoint?: string
+): HTMLImageElement {
+    const parent = image.parentElement;
+    // console.log(parent, entryPoint);


remove this

JiuqingSong · 2024-06-03T19:09:01Z

packages/roosterjs-content-model-plugins/lib/imageEdit/utils/getContentModelImage.ts

+/**
+ * @internal
+ */
+export function getContentModelImage(editor: IEditor): ContentModelImage | null {


where is this used? This will give you a cloned object but not the original image object. Is that ok?

This is used to get the image dataset

JiuqingSong · 2024-06-03T19:09:40Z

packages/roosterjs-content-model-dom/lib/modelApi/metadata/updateImageMetadata.ts

+ * @internal
+ * Definition of ImageMetadataFormat
+ */
+export const ImageMetadataFormatDefinition = createObjectDefinition<Required<ImageMetadataFormat>>({


Where is this used externally?

JiuqingSong · 2024-06-03T19:11:50Z

packages/roosterjs-content-model-plugins/lib/imageEdit/ImageEditPlugin.ts

+        }
+    }
+
+    public rotateImage(image: HTMLImageElement, angleRad: number) {


add comment for public methods

JiuqingSong · 2024-06-03T19:14:01Z

packages/roosterjs-content-model-plugins/lib/imageEdit/ImageEditPlugin.ts

+        ) {
+            editor.formatContentModel(
+                (model, _) => {
+                    const selectedSegmentsAndParagraphs = getSelectedSegmentsAndParagraphs(


So here we edit image based on current selection, then why we need to pass in image object in other editing functions?

We pass the image element to add and remove the wrapper element. Here we edit the selected content model image

JiuqingSong · 2024-06-03T19:15:05Z

packages/roosterjs-content-model-plugins/lib/imageEdit/ImageEditPlugin.ts

+    }
+
+    public canRegenerateImage(image: HTMLImageElement): boolean {
+        return canRegenerateImage(image) || canRegenerateImage(this.selectedImage);


This will be very confusing. It checks either the passed in image or current selection. We should make the function behavior predictable, only check one image

JiuqingSong · 2024-06-04T19:31:12Z

packages/roosterjs-content-model-plugins/lib/imageEdit/utils/getContentModelImage.ts

    const selectedSegments = getSelectedSegments(model, false /*includeFormatHolder*/);
    if (selectedSegments.length == 1 && selectedSegments[0].segmentType == 'Image') {
-        return selectedSegments[0];
+        return selectedSegments[0] as ContentModelImage;


This is fine here, but if we do this kind of type cast in other place, it will be dangerous.

Actually, if you only want dataset, just get it directly from the connect model, and return the dataset instead.

Disconnected model actually need a clone, which is not as good perf as connected one.

function getSelectedImageDataset() { let result; editor.formatContentModel(model => { const image = getSelectedImage(model); result = getImageMetadata(image); return false; }); return result;

Something like this.

JiuqingSong · 2024-06-04T19:34:29Z

packages/roosterjs-content-model-plugins/lib/imageEdit/ImageEditPlugin.ts

+                    const paragraph = selectedSegmentsAndParagraphs[0][1];
+
+                    if (paragraph && segment.segmentType == 'Image') {
+                        mutateSegment(paragraph, segment, image => {


This is nice.

And I think we only need to return true for formatContentModel when image is really changed, right?

Actually you can move this down into the "if" block below.

JiuqingSong · 2024-06-04T19:56:44Z

packages/roosterjs-content-model-core/lib/coreApi/setDOMSelection/setDOMSelection.ts

-                    `outline-style:auto!important; outline-color:${imageSelectionColor}!important;display: ${
-                        core.environment.isSafari ? 'inline-block' : 'inline-flex'
+                    `outline-style:solid!important; outline-color:${imageSelectionColor}!important;display: ${
+                        core.environment.isSafari ? '-webkit-inline-flex' : 'inline-flex'


juliaroldi added 30 commits April 5, 2024 11:20

WIP

52f30e4

fix conflicts

5bb6b15

conflict

36f592c

WIP

ec80d60

Merge branch 'master' of https://github.com/microsoft/roosterjs into …

4aa4828

…u/juliaroldi/port-image

WIP

ec97315

WIP

531985b

croppers

b91e403

porting

7a06999

WIPP

091f2db

fix conflicts

c11771f

WIP

13dcca1

Merge branch 'master' of https://github.com/microsoft/roosterjs into …

816b769

…u/juliaroldi/port-image

WIP

cc260bc

conflicts

b7a0ca8

fixes

a1586a5

WIP

89d51b1

Merge branch 'master' of https://github.com/microsoft/roosterjs into …

63125da

…u/juliaroldi/port-image

fix build

21599f9

remove function

cca526e

wip: clean/refactor

7b5cd18

wip: clean

75bea9e

clean

554e3da

wip

207657d

Merge branch 'u/juliaroldi/image-span' of https://github.com/microsof…

3806813

…t/roosterjs into u/juliaroldi/port-image

WIP

3221722

Merge branch 'u/juliaroldi/image-span' of https://github.com/microsof…

8a50224

…t/roosterjs into u/juliaroldi/port-image

wip

abe2b18

Merge branch 'u/juliaroldi/image-span' of https://github.com/microsof…

5c15d3e

…t/roosterjs into u/juliaroldi/port-image

port image

f91b48b

remove code

5e1be98

juliaroldi marked this pull request as ready for review May 30, 2024 13:58

Merge branch 'master' into u/juliaroldi/image-edit-operations

85198f2

juliaroldi requested review from JiuqingSong, Andres-CT98, BryanValverdeU, flyingbee2012, ianeli1 and vinguyen12 and removed request for Andres-CT98 May 30, 2024 13:58

juliaroldi added 3 commits June 3, 2024 14:18

remove editor

8b5961c

Merge branch 'u/juliaroldi/image-edit-operations' of https://github.c…

357c7c7

…om/microsoft/roosterjs into u/juliaroldi/image-edit-operations

fix test

53b044b

JiuqingSong reviewed Jun 3, 2024

View reviewed changes

juliaroldi added 7 commits June 3, 2024 18:42

WIP

4c5963a

fixes

f86541c

fix test

3c465c6

status

27792a0

test

9a9abae

fix conflicts

baff339

add mutate block

4c8d7eb

JiuqingSong reviewed Jun 4, 2024

View reviewed changes

juliaroldi and others added 2 commits June 5, 2024 12:04

fixes texts

26d6e90

Merge branch 'master' into u/juliaroldi/image-edit-operations

9dd412a

JiuqingSong approved these changes Jun 5, 2024

View reviewed changes

juliaroldi merged commit b4fc6d6 into master Jun 5, 2024
7 checks passed

JiuqingSong mentioned this pull request Jun 7, 2024

'imageSelectionBorderColor' config doesn't take effect in Safari #2676

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port Image Edit Operations #2670

Port Image Edit Operations #2670

juliaroldi commented May 29, 2024 •

edited

Loading

JiuqingSong Jun 3, 2024

juliaroldi Jun 3, 2024

JiuqingSong Jun 3, 2024

juliaroldi Jun 3, 2024

JiuqingSong Jun 3, 2024

juliaroldi Jun 3, 2024

JiuqingSong Jun 3, 2024

juliaroldi Jun 3, 2024

JiuqingSong Jun 3, 2024

JiuqingSong Jun 3, 2024

juliaroldi Jun 3, 2024

JiuqingSong Jun 3, 2024

JiuqingSong Jun 3, 2024

JiuqingSong Jun 3, 2024

juliaroldi Jun 3, 2024

JiuqingSong Jun 3, 2024

JiuqingSong Jun 4, 2024

JiuqingSong Jun 4, 2024

JiuqingSong Jun 4, 2024

JiuqingSong Jun 4, 2024

Port Image Edit Operations #2670

Port Image Edit Operations #2670

Conversation

juliaroldi commented May 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliaroldi commented May 29, 2024 •

edited

Loading