Allow built-in image paste handling to be disabled (#4874).

[FlowIT-JIT]

This will allow 3rd party plugins to handle images being pasted.
Related issue: #4874

What is the purpose of this pull request?

Bug fix

Does your PR contain necessary tests?

All patches that change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

• Unit tests
• Manual tests

Did you follow the CKEditor 4 code style guide?

Your code should follow the guidelines from the CKEditor 4 code style guide which helps keep the entire codebase consistent.

• PR is consistent with the code style guide

What is the proposed changelog entry for this pull request?

* [#4874](https://github.com/ckeditor/ckeditor4/issues/4874): Allow 3rd party plugins to handle images being pasted.

What changes did you make?

Added a new configuration option to clipboard plugin which allows for the built-in image paste handling to be disabled, so that other plugins can intercept clipboard data and handle images being pasted.

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

Which issues does your PR resolve?

Closes #4874.

[Comandeer]

Hi, @FlowIT-JIT, and thanks for your contribution! There are however some issues – please see my inline comments for more info.

Additionally, it will be good to add some unit tests – to check if setting this option to false disables built-in handling of pasting images and if the paste event is no longer stopped. Could you add such tests?

[Comandeer]

This condition became too long and therefore unreadable. Could you extract its first part to some variable (e.g. isImagePastedSupported)?

Additionally editor.config.clipboard_handleImagePasting alone should be enough to check if the plugin is supposed to handle image pasting.

[FlowIT-JIT]

I just committet a change that shortens the code by extracting the first part of the condition into an isImagePasteSupported variable as suggested. I'm not too crazy about the idea having only clipboard_handleImagePasting determine whether to enable image handling, since it will allow a developer to enable the feature on unsupported browsers.

[Comandeer]

I'm not too crazy about the idea having only clipboard_handleImagePasting determine whether to enable image handling, since it will allow a developer to enable the feature on unsupported browsers.

Actually, I meant something different, but I see that my message could be ambiguous. I thought about shortening editor.config.clipboard_handleImagePasting !== false to editor.config.clipboard_handleImagePasting.

[FlowIT-JIT]

Oh, okay 😅😊 Got it!

[Comandeer]

I'd split it into two tests: the one that will test the plugin with handling image pasting and one without it. It should be more readable than putting both of these checks inside one test. Could you split the test?

[FlowIT-JIT]

I added unit tests to cover scenarios where image paste handling has been disabled using the new clipboard_handleImagePasting flag - see tests/plugins/clipboard/pasteimagedisabled.js.
Native image paste handling (when enabled) is already covered by tests/plugins/clipboard/pasteimage.js.

Do you still want manual test coverage, or should I just remove it again ?

[FlowIT-JIT]

Oh, worth mentioning; I took the liberty to move some code from tests/plugins/clipboard/pasteimage.js into tests/plugins/clipboard/_helpers/pasting.js to reduce duplicate code. I needed the exact same code for the new unit test in tests/plugins/clipboard/pasteimagedisabled.js

[Comandeer]

Do you still want manual test coverage, or should I just remove it again ?

It's always better to have an additional manual test. For me manual tests are for people and unit ones – for machines.

[FlowIT-JIT]

Alright, I'll refacter the manual test as you suggested. Thanks 😊

[FlowIT-JIT]

There. The new manual test (tests/plugins/clipboard/manual/pasteimagedisable.md) is now only about testing the new clipboard_handleImagePasting option, as support for image pasting is already covered with tests/plugins/clipboard/manual/pasteimage.md.

[FlowIT-JIT]

@Comandeer Thank you for reviewing my PR. I have made some adjustments based on your feedback. Please see my comments to your review.

[Comandeer]

Thx for the updates!

Your changes work correctly, but it seems that some additional tests will be needed, as the new config variable also disables dragging and dropping images. Could you add a manual test and unit test for this case? You can probably reuse some code from dropimage.js test. I'm sorry for not noticing it earlier.

It'd be also good to add a unit test for the original case, so checking if some custom paste handler can receive paste data after disabling image handling. Could you also add such a test?

[FlowIT-JIT]

Thx for the updates!

Your changes work correctly, but it seems that some additional tests will be needed, as the new config variable also disables dragging and dropping images. Could you add a manual test and unit test for this case? You can probably reuse some code from dropimage.js test. I'm sorry for not noticing it earlier.

It'd be also good to add a unit test for the original case, so checking if some custom paste handler can receive paste data after disabling image handling. Could you also add such a test?

Thank you for additional feedback. I should probably have noticed drag and drop support myself - sorry about that. I will look into extending test coverage as suggested.

[FlowIT-JIT]

@Comandeer

I have now added a unit test and manual test to cover drag and drop.
In addition I extended the unit tests to verify that a custom image handler plugin receives the image when pasting or dropping. I combined this check with the existing unit tests to reduce duplicate code.

Furthermore I renamed the new option from clipboard_handleImagePasting to clipboard_handleImages as it is not restricted to paste operations alone. In conjunction with this change I also renamed the unit test file from pasteimagedisabled.js to clipboardimagedisabled.js which execute tests for both paste and drag'n'drop.

Please note that I once again had to do a little refactoring to reduce duplicate code for the new unit test covering image dropping. I moved code from tests/plugins/clipboard/dropimage.js to tests/plugins/clipboard/_helpers/pasting.js, but this time had to do some minor changes to tear down logic, as outline below (code in red boxes was added):

Prior to this change the cleanup was - probably not entirely logical nor correct - performed in setUp(..) as shown below:

[Comandeer]

LGTM! I've just updated tests a bit:

• drop test should run in IE10+, so I updated check accordingly,
• we use global assert in our tests – I've updated asserts accordingly in your tests,
• I've renamed assertDropImage to assertImageDrop to achieve better coherency with assertImagePaste.

Thanks for the PR!

[FlowIT-JIT]

@Comandeer Perfect! Thank you 😊