Modify Image wrapper to insert image wrapper outside the editor #1377

juliaroldi · 2022-10-31T23:06:26Z

After this change, when in editing the image will be cloned and it's clone will be added to a wrapper outside the editor.
Every change made to cloned image will be reflected to image when the quite editing, the editing process will happen outside the editor and DOM structure of the editor will not change while editing.

…u/juliaroldi/image-wrapper

JiuqingSong · 2022-11-02T16:41:41Z

packages/roosterjs-editor-plugins/lib/plugins/ImageEdit/ImageEdit.ts

+        const clone = this.image.cloneNode();
+        const wrappedImage = wrap(clone, KnownCreateElementDataIndex.ImageEditWrapper);
+
+        this.clonedImage = wrappedImage.firstElementChild as HTMLImageElement;


Why not just use "clone" instead of "wrappedImage.firstElementChild as HTMLImageElement" here?

JiuqingSong · 2022-11-02T16:42:36Z

packages/roosterjs-editor-plugins/lib/plugins/ImageEdit/ImageEdit.ts

+
+        this.clonedImage = wrappedImage.firstElementChild as HTMLImageElement;
+
+        // Wrap the cloned image with an entity so that we can easily retrieve it later
        const { wrapper } = insertEntity(


Since the editing wrapper is not part of editor now, I don't think we still need to "insertEntity"

JiuqingSong · 2022-11-02T16:43:50Z

packages/roosterjs-editor-plugins/lib/plugins/ImageEdit/ImageEdit.ts

@@ -352,9 +373,11 @@ export default class ImageEdit implements EditorPlugin {
        this.lastSrc = this.image.getAttribute('src');

        // Set image src to original src to help show editing UI, also it will be used when regenerate image dataURL after editing
+        this.image.style.visibility = 'hidden';


So this will still modify the editor content.

Can we add this visibility rule to image selection's global CSS so that editor content is not impacted?

JiuqingSong · 2022-11-02T16:44:13Z

packages/roosterjs-editor-plugins/lib/plugins/ImageEdit/ImageEdit.ts

@@ -352,9 +373,11 @@ export default class ImageEdit implements EditorPlugin {
        this.lastSrc = this.image.getAttribute('src');

        // Set image src to original src to help show editing UI, also it will be used when regenerate image dataURL after editing
+        this.image.style.visibility = 'hidden';
        this.image.src = this.editInfo.src;


Do we still need to change original image's src?

Yes, because when we applyFormat we compare the image edited with the previous src, so to make sure apply format works as expected we also need to change this src.

JiuqingSong · 2022-11-02T16:46:27Z

packages/roosterjs-editor-plugins/lib/plugins/ImageEdit/ImageEdit.ts

-
-        return entity?.type == IMAGE_EDIT_WRAPPER_ENTITY_TYPE ? entity.wrapper : null;
+    private getImageWrapper(): HTMLElement {
+        return document.getElementById(this.imageId);


So the cloned image will have the same id with the original one, how can we guarantee this will return the cloned one?

And if there are 2 editors in the same document, it is possible two editors both have image with the same id. Better change the image id to combine editor id and image id to make it unique

And instead of querying by image id, since there will be only one image editing per editor, you can just hold the reference to the wrapper element in this plugin. So it will be easier to get it

JiuqingSong · 2022-11-02T16:48:37Z

Generally, the behavior is good. Only one issue, if editor is zoomed, the editing image is not zoomed together, so when select and edit the image, its size changes.

JiuqingSong · 2022-11-04T16:57:29Z

packages/roosterjs-editor-plugins/lib/plugins/ImageEdit/ImageEdit.ts

@@ -195,6 +190,7 @@ export default class ImageEdit implements EditorPlugin {
                        e.selectionRangeEx.image,
                        ImageEditOperation.ResizeAndRotate
                    );
+                    this.addVisibilityCssTag();


I think this call need to be inside setEditingImage() function. Because setEditingImage() is a public function and other code can also call it. When it is called we should show/hide the original image

JiuqingSong · 2022-11-04T17:00:03Z

packages/roosterjs-editor-plugins/lib/plugins/ImageEdit/ImageEdit.ts

+        this.wrapper = this.editor.getDocument().createElement('span');
+        this.wrapper.appendChild(wrappedImage);
+        this.wrapper.style.maxWidth = '100%';
+        this.wrapper.style.position = 'fixed';


Can this be added into KnownCreateElementDataIndex.ImageEditWrapper so that one single wrap() call should make all of them working?

JiuqingSong · 2022-11-04T17:05:05Z

packages/roosterjs-editor-plugins/lib/plugins/ImageEdit/ImageEdit.ts


        // Cache current src so that we can compare it after edit see if src is changed
        this.lastSrc = this.image.getAttribute('src');

        // Set image src to original src to help show editing UI, also it will be used when regenerate image dataURL after editing
-        this.image.style.visibility = 'hidden';
+        this.image.className = STYLE_IMAGE;


Actually the image should already have id. If not, you add some API in editor to ensure it. So no need to change its className

JiuqingSong · 2022-11-04T17:07:22Z

packages/roosterjs-editor-plugins/lib/plugins/ImageEdit/ImageEdit.ts

+
+    private addVisibilityCssTag() {
+        const doc = this.editor.getDocument();
+        let styleTag = doc.getElementById(STYLE_IMAGE) as HTMLStyleElement;


Why reuse the same string for style tag and image class?

Also, if there are multiple editor in the same page, there will be duplicated id for style tags then hard to distinguish.

We already have code in editor core to generate STYLE tag and add style rule. Consider reuse that code by adding some editor API or parameter to existing API. The reuse part can be in separate PR, but at least make sure the style element id is unique. You can use editor DIV id.

JiuqingSong · 2022-11-04T17:07:45Z

packages/roosterjs-editor-plugins/lib/plugins/ImageEdit/ImageEdit.ts

+        const doc = this.editor.getDocument();
+        let styleTag = doc.getElementById(STYLE_IMAGE) as HTMLStyleElement;
+        if (!styleTag) {
+            const cssRule = `.${STYLE_IMAGE} {visibility: hidden}`;


Use editor id and image id, just like how we do selection border for image

JiuqingSong · 2022-11-04T17:08:36Z

packages/roosterjs-editor-plugins/lib/plugins/ImageEdit/ImageEdit.ts

+        if (!styleTag) {
+            const cssRule = `.${STYLE_IMAGE} {visibility: hidden}`;
+            styleTag = doc.createElement('style');
+            styleTag.id = STYLE_IMAGE;


Actually I think you can hold a reference to this style element so no id is needed at all.

JiuqingSong · 2022-11-04T17:10:24Z

packages/roosterjs-editor-plugins/lib/plugins/ImageEdit/ImageEdit.ts

@@ -174,6 +168,7 @@ export default class ImageEdit implements EditorPlugin {
     * Dispose this plugin
     */
    dispose() {
+        this.removeVisibilityCssTag();


We also need to remove the editing image DOM element here, and the call to removeVisibilityCssTag() can be part of it.

JiuqingSong · 2022-11-04T17:12:03Z

packages/roosterjs-editor-plugins/lib/plugins/ImageEdit/ImageEdit.ts

    };

    /**
     * Update image edit elements to reflect current editing result
     * @param context
     */
    private updateWrapper = (context?: DragAndDropContext) => {
-        const wrapper = this.getImageWrapper();
-        if (wrapper) {
+        if (this.wrapper) {


consider do

const wrapper = this.wrapper; if (wrapper) { ...

So no need to do use "this.wrapper" everywhere to save some bytes

JiuqingSong · 2022-11-04T22:47:26Z

packages/roosterjs-editor-core/lib/coreApi/selectImage.ts

@@ -49,7 +53,7 @@ export const selectImage: SelectImage = (core: EditorCore, image: HTMLImageEleme
 const select = (core: EditorCore, image: HTMLImageElement) => {
    removeImportantStyleRule(image, ['border', 'margin']);
    const borderCSS = buildBorderCSS(core, image.id);
-    addSelectionStyle(core, borderCSS, STYLE_ID);
+    setGlobalCssStyles(core.contentDiv.ownerDocument, borderCSS, STYLE_ID + core.contentDiv.id);


Use removeGlobalCssStyle() in line 67.
Same for selectTable()

JiuqingSong · 2022-11-04T22:53:36Z

packages/roosterjs-editor-plugins/lib/plugins/ImageEdit/ImageEdit.ts

+        const wrappedImage = wrap(this.clonedImage, KnownCreateElementDataIndex.ImageEditWrapper);
+        this.wrapper = wrap(wrappedImage, KnownCreateElementDataIndex.ImageEditContainer);


Actually, I mean merge these two into one. Because wrappedImage is only used here. So we can do
this.wrapper = wrap(this.clonedImage, KnownCreateElementDataIndex.ImageEditWrapper);

Then you can modify the definition of ImageEditWrapper to make it has two layers of wrapper.

JiuqingSong · 2022-11-04T22:54:33Z

packages/roosterjs-editor-plugins/lib/plugins/ImageEdit/ImageEdit.ts

        }

-        wrapper.style.display = Browser.isSafari ? 'inline-block' : 'inline-flex';
+        this.wrapper.style.display = Browser.isSafari ? 'inline-block' : 'inline-flex';

        // Cache current src so that we can compare it after edit see if src is changed
        this.lastSrc = this.image.getAttribute('src');

        // Set image src to original src to help show editing UI, also it will be used when regenerate image dataURL after editing
        this.image.src = this.editInfo.src;


I still have concern here. If you need to compare, can you compare with the cloned image?

My primary goal here is not to do any change to editor content when edit.

…u/juliaroldi/image-wrapper

JiuqingSong · 2022-11-07T21:39:40Z

packages/roosterjs-editor-dom/lib/style/removeGlobalCssStyle.ts

+export default function removeGlobalCssStyle(doc: Document, styleId: string) {
+    const styleTag = doc.getElementById(styleId) as HTMLStyleElement;
+    if (styleTag) {
+        doc.head.removeChild(styleTag);


Use styleTag.parentNode?.removeChild(styleTag) instead

Although accoding to our code, style tag should always be under HEAD. But if we changed that behavior, there will be no build time error or test case to remind us change here. So better make it more robust

juliaroldi added 3 commits October 31, 2022 20:05

WIP

86467d6

new update wrapper

c8b601f

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

7438c17

…u/juliaroldi/image-wrapper

juliaroldi changed the title ~~WIP~~ Modify Image wrapper to insert image wrapper outside the editor Nov 1, 2022

juliaroldi marked this pull request as ready for review November 1, 2022 17:49

juliaroldi requested review from JiuqingSong, BryanValverdeU, Andres-CT98 and ianeli1 and removed request for JiuqingSong November 1, 2022 17:49

JiuqingSong reviewed Nov 2, 2022

View reviewed changes

juliaroldi and others added 3 commits November 3, 2022 17:28

refactor image edit

3aa2834

Merge branch 'master' into u/juliaroldi/image-wrapper

43da778

refactor

4f00d16

JiuqingSong reviewed Nov 4, 2022

View reviewed changes

refactor

6b9b089

juliaroldi mentioned this pull request Nov 4, 2022

Fix image zoom scale while editing #1383

Merged

JiuqingSong reviewed Nov 4, 2022

View reviewed changes

juliaroldi added 3 commits November 7, 2022 16:40

use remove global styles

333b6de

use clone to applay changes

68f24b4

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

98f41cc

…u/juliaroldi/image-wrapper

juliaroldi force-pushed the u/juliaroldi/image-wrapper branch from 7a7562b to 98f41cc Compare November 7, 2022 21:03

JiuqingSong reviewed Nov 7, 2022

View reviewed changes

JiuqingSong approved these changes Nov 7, 2022

View reviewed changes

refactor

5d6971d

juliaroldi merged commit 3630fb1 into master Nov 7, 2022

JiuqingSong deleted the u/juliaroldi/image-wrapper branch September 8, 2023 22:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify Image wrapper to insert image wrapper outside the editor #1377

Modify Image wrapper to insert image wrapper outside the editor #1377

juliaroldi commented Oct 31, 2022 •

edited

Loading

JiuqingSong Nov 2, 2022

JiuqingSong Nov 2, 2022

JiuqingSong Nov 2, 2022

JiuqingSong Nov 2, 2022

juliaroldi Nov 3, 2022

JiuqingSong Nov 2, 2022

JiuqingSong Nov 2, 2022

JiuqingSong commented Nov 2, 2022

JiuqingSong Nov 4, 2022

JiuqingSong Nov 4, 2022

JiuqingSong Nov 4, 2022

JiuqingSong Nov 4, 2022

JiuqingSong Nov 4, 2022

JiuqingSong Nov 4, 2022

JiuqingSong Nov 4, 2022

JiuqingSong Nov 4, 2022

JiuqingSong Nov 4, 2022

JiuqingSong Nov 4, 2022

JiuqingSong Nov 4, 2022

JiuqingSong Nov 7, 2022

		const wrappedImage = wrap(this.clonedImage, KnownCreateElementDataIndex.ImageEditWrapper);
		this.wrapper = wrap(wrappedImage, KnownCreateElementDataIndex.ImageEditContainer);

Modify Image wrapper to insert image wrapper outside the editor #1377

Modify Image wrapper to insert image wrapper outside the editor #1377

Conversation

juliaroldi commented Oct 31, 2022 • 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

JiuqingSong commented Nov 2, 2022

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 Oct 31, 2022 •

edited

Loading