Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop editing when content changed #2931

Merged
merged 5 commits into from
Jan 28, 2025
Merged

Stop editing when content changed #2931

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4578669
remove data-istrue
juliaroldi Jan 28, 2025
685ff40
use content change
juliaroldi Jan 28, 2025
2b011f2
Merge branch 'master' into u/juliaroldi/undo-images
juliaroldi Jan 28, 2025
27c11ff
refactor
juliaroldi Jan 28, 2025
f969062
Merge branch 'u/juliaroldi/undo-images' of https://github.com/microso…
juliaroldi Jan 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions packages/roosterjs-content-model-plugins/lib/imageEdit/ImageEditPlugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ const MouseRightButton = 2;
const DRAG_ID = '_dragging';
const IMAGE_EDIT_CLASS = 'imageEdit';
const IMAGE_EDIT_CLASS_CARET = 'imageEditCaretColor';
const IMAGE_EDIT_DATASET = 'data-is-editing="true"';

/**
* ImageEdit plugin handles the following image editing features:
Expand Down Expand Up @@ -177,6 +178,9 @@ export class ImageEditPlugin implements ImageEditor, EditorPlugin {
case 'extractContentWithDom':
this.removeImageEditing(event.clonedRoot);
break;
case 'beforeSetContent':
this.beforeSetContentHandler(this.editor, event.newContent);
break;
}
}

Expand Down Expand Up @@ -279,6 +283,11 @@ export class ImageEditPlugin implements ImageEditor, EditorPlugin {
}
}

private beforeSetContentHandler(editor: IEditor, newContent: string) {
newContent.replace(IMAGE_EDIT_DATASET, '');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you sure this can work since you didn't use the return value?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of changing the source HTML, I prefer to do it in DOM tree, when handling ContentChangedEvent and check change source is setContent

editor?.setEditorStyle(IMAGE_EDIT_CLASS_CARET, null);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"beforeSetContent" is a legacy event from v8. Since there is no API "setContent" in v9, I prefer not use this event. Can we do this in ContentChangedEvent? We just need to make sure do not reset cursor style if images is still selected.

Actually we should do that in SelectionPlugin since this problem can still exist if ImageEditPlugin is not added

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just testing in OWA right now and remove the dataset in new content worked, but the remove caret styled persisted, I'm testing using ContentChangedEvent right now. I think we should remove the caret style in ImageEdit, since we add this style when we add the image edit wrapper.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the style set from core API setDOMSelection?

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

Line 61 in 11c9e3e

core.api.setEditorStyle(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We add the style here
editor.setEditorStyle(IMAGE_EDIT_CLASS_CARET, caret-color: transparent;);

}

/**
* EXPOSED FOR TESTING PURPOSE ONLY
*/
Expand Down
39 changes: 39 additions & 0 deletions packages/roosterjs-content-model-plugins/test/imageEdit/ImageEditPluginTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -670,6 +670,45 @@ describe('ImageEditPlugin', () => {
expect(event.clonedRoot).toEqual(expectedClonedRoot);
plugin.dispose();
});

it('beforeSetContent - should remove isEditing', () => {
const plugin = new ImageEditPlugin();
plugin.initialize(editor);
const clonedRoot = document.createElement('div');
const image = document.createElement('img');
clonedRoot.appendChild(image);
image.dataset['editingInfo'] = JSON.stringify({
src: 'test',
});
image.dataset['isEditing'] = 'true';
const event = {
eventType: 'beforeSetContent',
newContent: JSON.stringify(clonedRoot),
} as any;
plugin.onPluginEvent(event);
const isEditing = event.newContent.includes('data-is-editing="true"');
expect(isEditing).toBeFalse();
plugin.dispose();
});

it('beforeSetContent - should editor caret style', () => {
const plugin = new ImageEditPlugin();
plugin.initialize(editor);
const clonedRoot = document.createElement('div');
const image = document.createElement('img');
clonedRoot.appendChild(image);
image.dataset['editingInfo'] = JSON.stringify({
src: 'test',
});
image.dataset['isEditing'] = 'true';
const event = {
eventType: 'beforeSetContent',
newContent: JSON.stringify(clonedRoot),
} as any;
plugin.onPluginEvent(event);
expect(editor.setEditorStyle).toHaveBeenCalledWith('imageEditCaretColor', null);
plugin.dispose();
});
});

class TestPlugin extends ImageEditPlugin {
Expand Down
Loading