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

U/juliaroldi/image editing refactor #1345

Merged
merged 25 commits into from
Oct 28, 2022
Merged

U/juliaroldi/image editing refactor #1345

Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
4518dd0
WIP
juliaroldi Oct 11, 2022
08f2eca
Merge branch 'master' of https://github.com/microsoft/roosterjs into …
juliaroldi Oct 18, 2022
e679c15
WIP
juliaroldi Oct 18, 2022
d00b413
enable flight
juliaroldi Oct 18, 2022
6a8eb94
adapt Image Edit to Image Selection
juliaroldi Oct 19, 2022
82781fd
typo
juliaroldi Oct 19, 2022
ad4f6d7
Merge branch 'master' into u/juliaroldi/image-editing-refactor
juliaroldi Oct 19, 2022
28b74d3
Merge branch 'master' of https://github.com/microsoft/roosterjs into …
juliaroldi Oct 20, 2022
e03a7df
compare files
juliaroldi Oct 21, 2022
7fe3b15
add image selection
juliaroldi Oct 21, 2022
bd0d44c
remove alignment
juliaroldi Oct 21, 2022
5de5a1c
Merge branch 'master' of https://github.com/microsoft/roosterjs into …
juliaroldi Oct 21, 2022
d990969
typo
juliaroldi Oct 21, 2022
8dc542c
if click in image, select it
juliaroldi Oct 24, 2022
04bdf52
Merge branch 'master' of https://github.com/microsoft/roosterjs into …
juliaroldi Oct 24, 2022
f7ab210
remove check
juliaroldi Oct 24, 2022
0c5e98e
setEditing
juliaroldi Oct 25, 2022
d17dad7
change to mouse up
juliaroldi Oct 26, 2022
2fc5a53
Merge branch 'master' into u/juliaroldi/image-editing-refactor
juliaroldi Oct 27, 2022
59e4814
remove comment
juliaroldi Oct 27, 2022
0021149
remove comment
juliaroldi Oct 27, 2022
974e6eb
fix copy/paste
juliaroldi Oct 27, 2022
4d1ac7e
Merge branch 'master' into u/juliaroldi/image-editing-refactor
juliaroldi Oct 27, 2022
64708a6
use queryelements
juliaroldi Oct 28, 2022
f84a800
Merge branch 'master' into u/juliaroldi/image-editing-refactor
juliaroldi Oct 28, 2022
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
2 changes: 1 addition & 1 deletion demo/scripts/controls/getToggleablePlugins.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ export default function getToggleablePlugins(initState: BuildInPluginState) {
paste: pluginList.paste ? new Paste() : null,
watermark: pluginList.watermark ? new Watermark(initState.watermarkText) : null,
imageEdit,
imageSelection: pluginList.imageSelection ? new ImageSelection() : null,
cutPasteListChain: pluginList.cutPasteListChain ? new CutPasteListChain() : null,
tableCellSelection: pluginList.tableCellSelection ? new TableCellSelection() : null,
tableResize: pluginList.tableResize ? new TableResize() : null,
Expand All @@ -56,7 +57,6 @@ export default function getToggleablePlugins(initState: BuildInPluginState) {
? createTableEditMenuProvider()
: null,
contextMenu: pluginList.contextMenu ? createContextMenuPlugin() : null,
imageSelection: pluginList.imageSelection ? new ImageSelection() : null,
};

return Object.values(plugins);
Expand Down
2 changes: 0 additions & 2 deletions demo/scripts/controls/sidePane/editorOptions/ExperimentalFeatures.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@ const FeatureNames: Partial<Record<ExperimentalFeatures, string>> = {
'Use pending style format to do formatting when selection is collapsed',
[ExperimentalFeatures.NormalizeList]:
'Normalize list to make sure it can be displayed correctly in other client',
[ExperimentalFeatures.ImageSelection]:
'Image Selection: the selected image data will be stored by editor core',
[ExperimentalFeatures.ReuseAllAncestorListElements]:
"Reuse ancestor list elements even if they don't match the types from the list item.",
};
Expand Down
5 changes: 1 addition & 4 deletions demo/scripts/controls/sidePane/editorOptions/Plugins.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,7 @@ export default class Plugins extends React.Component<PluginsProps, {}> {
'Show customized context menu for special cases'
)}
{this.renderPluginItem('tableCellSelection', 'Table Cell Selection')}
{this.renderPluginItem(
'imageSelection',
'Image Selection (requires Image Selection experimental feature)'
)}
{this.renderPluginItem('imageSelection', 'Image Selection')}
</tbody>
</table>
);
Expand Down
10 changes: 10 additions & 0 deletions packages/roosterjs-editor-core/lib/corePlugins/CopyPastePlugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,9 @@ export default class CopyPastePlugin implements PluginWithState<CopyPastePluginS

if (image) {
newRange = createRange(image);
if (isCut) {
this.deleteImage(this.editor, selection.image.id);
}
}
} else {
newRange =
Expand Down Expand Up @@ -280,4 +283,11 @@ export default class CopyPastePlugin implements PluginWithState<CopyPastePluginS
table.style.removeProperty('height');
}
}

private deleteImage(editor: IEditor, imageId: string) {
const image = editor.getDocument().querySelector('#' + imageId);
Copy link
Collaborator

Choose a reason for hiding this comment

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

use editor.queryElements() instead to avoid query image outside editor

if (image) {
editor.deleteNode(image);
}
}
}
6 changes: 1 addition & 5 deletions packages/roosterjs-editor-core/lib/editor/Editor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,11 +476,7 @@ export default class Editor implements IEditor {
table: arg1,
coordinates: arg2,
};
} else if (
this.isFeatureEnabled(ExperimentalFeatures.ImageSelection) &&
safeInstanceOf(arg1, 'HTMLImageElement') &&
typeof arg2 == 'undefined'
) {
} else if (safeInstanceOf(arg1, 'HTMLImageElement') && typeof arg2 == 'undefined') {
rangeEx = {
type: SelectionRangeTypes.ImageSelection,
ranges: [],
Expand Down
79 changes: 29 additions & 50 deletions packages/roosterjs-editor-plugins/lib/plugins/ImageEdit/ImageEdit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import {
getEntityFromElement,
getEntitySelector,
getObjectKeys,
matchesSelector,
safeInstanceOf,
toArray,
wrap,
Expand All @@ -29,8 +28,8 @@ import {
doubleCheckResize,
getSideResizeHTML,
getCornerResizeHTML,
getResizeBordersHTML,
OnShowResizeHandle,
getResizeBordersHTML,
} from './imageEditors/Resizer';
import {
ExperimentalFeatures,
Expand All @@ -42,19 +41,14 @@ import {
PluginEvent,
PluginEventType,
EntityOperation,
Entity,
Keys,
PositionType,
CreateElementData,
KnownCreateElementDataIndex,
ModeIndependentColor,
SelectionRangeTypes,
Entity,
} from 'roosterjs-editor-types';
import type { CompatibleImageEditOperation } from 'roosterjs-editor-types/lib/compatibleTypes';

const SHIFT_KEYCODE = 16;
const CTRL_KEYCODE = 17;
const ALT_KEYCODE = 18;

const DIRECTIONS = 8;
const DirectionRad = (Math.PI * 2) / DIRECTIONS;
const DirectionOrder = ['nw', 'n', 'ne', 'e', 'se', 's', 'sw', 'w'];
Expand Down Expand Up @@ -184,45 +178,28 @@ export default class ImageEdit implements EditorPlugin {
*/
onPluginEvent(e: PluginEvent) {
switch (e.eventType) {
case PluginEventType.MouseDown:
this.setEditingImage(null);
break;

case PluginEventType.MouseUp:
const target = e.rawEvent.target;
case PluginEventType.SelectionChanged:
if (
e.isClicking &&
e.rawEvent.button == 0 &&
safeInstanceOf(target, 'HTMLImageElement') &&
target.isContentEditable &&
matchesSelector(target, this.options.imageSelector)
e.selectionRangeEx &&
e.selectionRangeEx.type === SelectionRangeTypes.ImageSelection
) {
this.setEditingImage(target, ImageEditOperation.ResizeAndRotate);
this.setEditingImage(
e.selectionRangeEx.image,
ImageEditOperation.ResizeAndRotate
);
}

break;

case PluginEventType.MouseDown:
this.setEditingImage(null);
JiuqingSong marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

This works. But I think that is what I concerned about current implementation. We can go with this for now, and revisit it later.

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 think we can change this implementation, but we would have to change the select function to trigger selection change event, when no range is selected.

break;
case PluginEventType.KeyDown:
const key = e.rawEvent.which;
if (key == Keys.DELETE || key == Keys.BACKSPACE) {
// Set current editing image to null and select the image if any, and do not prevent default of the event
// so that browser will delete the selected image for us
this.setEditingImage(null, true /*selectImage*/);
} else if (key == Keys.ESCAPE && this.image) {
// Press ESC should cancel current editing operation, resume back to original edit info
this.editInfo = getEditInfoFromImage(this.image);
this.setEditingImage(null);
e.rawEvent.preventDefault();
} else if (key != SHIFT_KEYCODE && key != CTRL_KEYCODE && key != ALT_KEYCODE) {
// For other key, just unselect current image and select it. If this is an input key, the image will be replaced
this.setEditingImage(null, true /*selectImage*/);
}
this.setEditingImage(null);
break;

case PluginEventType.ContentChanged:
if (
e.source != ChangeSource.InsertEntity ||
(<Entity>e.data).type != IMAGE_EDIT_WRAPPER_ENTITY_TYPE
(e.source !== ChangeSource.Format && e.source !== ChangeSource.InsertEntity) ||
(e.source === ChangeSource.InsertEntity &&
(<Entity>e.data)?.type != IMAGE_EDIT_WRAPPER_ENTITY_TYPE)
) {
// After contentChanged event, the current image wrapper may not be valid any more, remove all of them if any
this.editor.queryElements(
Expand All @@ -232,7 +209,6 @@ export default class ImageEdit implements EditorPlugin {
}

break;

case PluginEventType.EntityOperation:
if (e.entity.type == IMAGE_EDIT_WRAPPER_ENTITY_TYPE) {
if (e.operation == EntityOperation.ReplaceTemporaryContent) {
Expand Down Expand Up @@ -288,7 +264,7 @@ export default class ImageEdit implements EditorPlugin {
typeof operationOrSelect === 'number' ? operationOrSelect : ImageEditOperation.None;
const selectImage = typeof operationOrSelect === 'number' ? false : !!operationOrSelect;

if (this.image) {
if (!image && this.image) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why need this check? What if I select another image?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To avoid a loop. In line 315, the image is being selected again, because we add the wrapper we remove the selection from the image, then we need to select the image again and we do this selection setEditingImage will be called, then to avoid select the image again we do not have the wrapper, unless we use setEditingImage is called with null.
When we trigger mouse we set editing to null, but if we click in another image the imageSelect plugin will select the image and then selection event will be trigger and the new image will be selected.

// When there is image in editing, clean up any cached objects and elements
this.clearDndHelpers();

Expand All @@ -306,7 +282,6 @@ export default class ImageEdit implements EditorPlugin {
if (selectImage) {
this.editor.select(this.image);
}

this.image = null;
this.editInfo = null;
this.lastSrc = null;
Expand All @@ -328,7 +303,7 @@ export default class ImageEdit implements EditorPlugin {
this.allowedOperations;

// Create and update editing wrapper and elements
const wrapper = this.createWrapper(operation);
this.createWrapper(operation);
this.updateWrapper();

// Init drag and drop
Expand All @@ -339,16 +314,15 @@ export default class ImageEdit implements EditorPlugin {
...this.createDndHelpers(ImageEditElementClass.CropContainer, Cropper),
];

// Put cursor next to the image
this.editor.select(wrapper, PositionType.After);
this.editor.select(this.image);
}
}

/**
* quit editing mode when editor lose focus
*/
private onBlur = () => {
this.setEditingImage(null);
this.setEditingImage(null, true);
};

/**
Expand All @@ -367,12 +341,11 @@ export default class ImageEdit implements EditorPlugin {
wrapper.style.position = 'relative';
wrapper.style.maxWidth = '100%';
// keep the same vertical align
const originalVerticalAlign = this.image.ownerDocument.defaultView
.getComputedStyle(this.image)
.getPropertyValue('vertical-align');
const originalVerticalAlign = this.getStylePropertyValue(this.image, 'vertical-align');
if (originalVerticalAlign) {
wrapper.style.verticalAlign = originalVerticalAlign;
}

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
Expand Down Expand Up @@ -418,6 +391,12 @@ export default class ImageEdit implements EditorPlugin {
return wrapper;
}

private getStylePropertyValue(element: HTMLElement, property: string): string {
return element.ownerDocument.defaultView
.getComputedStyle(this.image)
.getPropertyValue(property);
}

/**
* Get image wrapper from image
* @param image The image to get wrapper from
Expand Down
62 changes: 30 additions & 32 deletions packages/roosterjs-editor-plugins/lib/plugins/ImageEdit/imageEditors/Resizer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,7 @@ import { ImageEditElementClass } from '../types/ImageEditElementClass';
* will be picked up by ImageEdit code
*/
export interface OnShowResizeHandle {
(
elementData: CreateElementData,
x: DNDDirectionX,
y: DnDDirectionY
): void
(elementData: CreateElementData, x: DNDDirectionX, y: DnDDirectionY): void;
}

const enum HandleTypes {
Expand Down Expand Up @@ -55,17 +51,17 @@ export const Resizer: DragAndDropHandler<DragAndDropContext, ResizeInfo> = {
// first sure newHeight is right，calculate newWidth
newWidth = newHeight * ratio;
if (newWidth < options.minWidth) {
newWidth = options.minWidth;
newHeight = newWidth / ratio;
newWidth = options.minWidth;
newHeight = newWidth / ratio;
}
} else {
} else {
// first sure newWidth is right，calculate newHeight
newHeight = newWidth / ratio;
if (newHeight < options.minHeight) {
newHeight = options.minHeight;
newWidth = newHeight * ratio;
newHeight = options.minHeight;
newWidth = newHeight * ratio;
}
}
}
}

editInfo.widthPx = newWidth;
Expand Down Expand Up @@ -142,18 +138,19 @@ export function getCornerResizeHTML(

Xs.forEach(x =>
Ys.forEach(y => {
let elementData = (x == '') == (y == '')
? getResizeHandleHTML(
x,
y,
resizeBorderColor,
handlesExperimentalFeatures
? HandleTypes.CircularHandlesCorner
: HandleTypes.SquareHandles
)
: null;
let elementData =
(x == '') == (y == '')
? getResizeHandleHTML(
x,
y,
resizeBorderColor,
handlesExperimentalFeatures
? HandleTypes.CircularHandlesCorner
: HandleTypes.SquareHandles
)
: null;
if (onShowResizeHandle) {
onShowResizeHandle(elementData, x, y)
onShowResizeHandle(elementData, x, y);
Copy link
Contributor

Choose a reason for hiding this comment

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

elementData can be null, should we add a null check?

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 think this had to be node in another PR to enable strict mode. I didn't made any changes in this files, but the beautify extension add the semicolon

}
result.push(elementData);
})
Expand All @@ -179,16 +176,17 @@ export function getSideResizeHTML(
const result: CreateElementData[] = [];
Xs.forEach(x =>
Ys.forEach(y => {
let elementData = (x == '') != (y == '')
? getResizeHandleHTML(
x,
y,
resizeBorderColor,
handlesExperimentalFeatures
? HandleTypes.CircularHandlesCorner
: HandleTypes.SquareHandles
)
: null;
let elementData =
(x == '') != (y == '')
? getResizeHandleHTML(
x,
y,
resizeBorderColor,
handlesExperimentalFeatures
? HandleTypes.CircularHandlesCorner
: HandleTypes.SquareHandles
)
: null;
if (onShowResizeHandle) {
onShowResizeHandle(elementData, x, y);
}
Expand Down
7 changes: 3 additions & 4 deletions packages/roosterjs-editor-plugins/lib/plugins/ImageSelection/ImageSelection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import { createRange, safeInstanceOf } from 'roosterjs-editor-dom';

import {
EditorPlugin,
ExperimentalFeatures,
IEditor,
PluginEvent,
PluginEventType,
Expand Down Expand Up @@ -47,7 +46,7 @@ export default class ImageSelection implements EditorPlugin {
}

onPluginEvent(event: PluginEvent) {
if (this.editor && this.editor.isFeatureEnabled(ExperimentalFeatures.ImageSelection)) {
if (this.editor) {
JiuqingSong marked this conversation as resolved.
Show resolved Hide resolved
switch (event.eventType) {
case PluginEventType.EnteredShadowEdit:
case PluginEventType.LeavingShadowEdit:
Expand All @@ -57,7 +56,7 @@ export default class ImageSelection implements EditorPlugin {
}
break;

case PluginEventType.MouseDown:
case PluginEventType.MouseUp:
const target = event.rawEvent.target;
if (safeInstanceOf(target, 'HTMLImageElement')) {
if (event.rawEvent.button === mouseRightButton) {
Expand All @@ -68,7 +67,7 @@ export default class ImageSelection implements EditorPlugin {
}
}
break;
case PluginEventType.KeyDown:
case PluginEventType.KeyUp:
const key = event.rawEvent.key;
const keyDownSelection = this.editor.getSelectionRangeEx();
if (keyDownSelection.type === SelectionRangeTypes.ImageSelection) {
Expand Down
Loading