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.

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

Clipboard plugin's handling of images should be configurable - will break other plugins by default #4874

Closed
FlowIT-JIT opened this issue Sep 8, 2021 · 3 comments · Fixed by #4896
Assignees
Labels
plugin:clipboard The plugin which probably causes the issue. regression This issue is a regression. status:confirmed An issue confirmed by the development team. type:bug A bug.
Milestone

Comments

@FlowIT-JIT
Copy link
Contributor

FlowIT-JIT commented Sep 8, 2021

Type of report

Bug

Provide detailed reproduction steps (if any)

In june 2021 the built-in clipboard plugin was extended to offer native support for pasting images as base64.
But since there is already multiple plugins out there offering similar functionality (some with additional features), pasting an image will now result in two images being inserted.

Code causing the problem starts here:

// Convert image file (if present) to base64 string for modern browsers except IE<10, as it does not support

While native support for pasting images is wonderful, the current implementation seems to break support for custom image paste plugins as there is no way to prevent the clipboard event from inserting the image too, without stopping the event chain. At least not exposed as public API. One can hack it using CKEDITOR.plugins.clipboard.isCustomDataTypesSupported = false;

Furthermore, base64 images can be huge and it may severely hurt performance, especially if using the change event to continuously validate data or push changes to application state - although this use case is discouraged:
https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR_editor.html#event-change

If it is important not to get the change event fired too often, you should compare the previous and the current editor content inside the event listener. It is not recommended to do that on every change event.

In addition, the clipboard event even stops the event chain once the image has been processed, preventing other plugins registered later from receiving the image data.

Why would you even use a custom image plugin to handle image pasting?
To automatically upload images immediately. To temporarily store images in blob storage rather than using base64, which greatly increases performance when working with large documents (we use this). To optimize/modify images before inserting them. To support pasting of multiple images at once (we use this). There could be many reasons.

A small sample has been put together which lets the user switch between CKEditor 4.16.1 and the current Major, to test image pasting in both of them.

image

Download: https://codemagic.dk/CKEditorDemo.zip

  1. Open CKEditor demo/index.html (can run locally)
  2. Make sure CKEditor version is set to "Old CKEditor"
  3. Paste an image into the editor - everything is fine, we get exatly one image.

Now test with the current major.

  1. Open CKEditor demo/index.html (can run locally)
  2. Make sure CKEditor version is set to "New CKEditor (improved)"
  3. Paste an image into the editor - BUG: image is inserted twice !

Expected result

Clipboard plugin should allow for other plugins to handle image pasting

Actual result

Clipboard plugin causes duplicate images

Other details

  • Browser: Chrome 93.0.4577.63 (Official Build) (x86_64)
  • OS: macOS 10.13.6 (17G14042)
  • CKEditor version: 4.16.1 and 4.17.x
  • Installed CKEditor plugins: clipboard, misc.
@FlowIT-JIT FlowIT-JIT added the type:bug A bug. label Sep 8, 2021
@Comandeer Comandeer added plugin:clipboard The plugin which probably causes the issue. status:confirmed An issue confirmed by the development team. size:S labels Sep 14, 2021
@Comandeer
Copy link
Member

I can confirm the issue.

I can think of two possible solutions:

  • adding config variable for switching image pasting on/off,
  • moving the image pasting logic to separate plugin.

Probably adding config variable would be less invasive solution.

@jacekbogdanski jacekbogdanski added the regression This issue is a regression. label Sep 14, 2021
FlowIT-JIT added a commit to FlowIT-JIT/ckeditor4 that referenced this issue Sep 20, 2021
This will allow 3rd party plugins to handle images being pasted.
@FlowIT-JIT
Copy link
Contributor Author

FlowIT-JIT commented Sep 20, 2021

Thanks for confirming @Comandeer 👍😊

I took the liberty to take a go at it. I agree with the suggested approach; adding an option to control the behaviour.

Pull request: #4896

The new option is called "handleImagePasting" (boolean).
We can certainly talk about the ideal name for the new option, but I think "handleImagePasting" makes sense. It indicates that the plugin is responsible for handling image pasting. I considered both "enableImagePasting" and "disableImagePasting", but I don't quite think it communicates that the plugin now "owns" that domain and prevents other plugins from receiving the data. But I'm not really religious about it - feel free to rename it 😊

Example

CKEDITOR.replace( 'editor', {
	// Allow 3rd party plugins to handle image pasting
	clipboard_handleImagePasting: false,
	extraPlugins: "customImagePasteHandlerPlugin"
} );

Comandeer pushed a commit to FlowIT-JIT/ckeditor4 that referenced this issue Sep 26, 2021
This will allow 3rd party plugins to handle images being pasted.
@CKEditorBot
Copy link
Collaborator

Closed in #4896

@CKEditorBot CKEditorBot added this to the 4.17.1 milestone Sep 26, 2021
@jacekbogdanski jacekbogdanski modified the milestones: 4.17.1, 4.17.0 Oct 12, 2021
NexediGitlab pushed a commit to Nexedi/erp5 that referenced this issue Jun 4, 2024
When pasting an image in CKEditor, the image was displayed twice.
This behaviour is due to the new paste-as-base64 feature implemented in CKE,
which conflict with existing plugins implementing the same feature.

To solve it, add `clipboard_handleImages` option to the CKE configuration object,
which disables its own image-pasting feature.

See ckeditor/ckeditor4#4874 for the corresponding issue on CKEditor’s repository.

/cc @jerome @Romain @tomo
/reviewed-by @jerome
/reviewed-on https://lab.nexedi.com/nexedi/erp5/-/merge_requests/1951
NexediGitlab pushed a commit to Nexedi/erp5 that referenced this issue Jul 11, 2024
When pasting an image in CKEditor, the image was displayed twice.
This behaviour is due to the new paste-as-base64 feature implemented in CKE,
which conflict with existing plugins implementing the same feature.

To solve it, add `clipboard_handleImages` option to the CKE configuration object,
which disables its own image-pasting feature.

See ckeditor/ckeditor4#4874 for the corresponding issue on CKEditor’s repository.

/cc @jerome @Romain @tomo
/reviewed-by @jerome
/reviewed-on https://lab.nexedi.com/nexedi/erp5/-/merge_requests/1951
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin:clipboard The plugin which probably causes the issue. regression This issue is a regression. status:confirmed An issue confirmed by the development team. type:bug A bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants