Merge pastefromwordimage plugin to pastefromword #1214

msamsel · 2017-11-23T12:55:10Z

What is the purpose of this pull request?

Other, please explain
Code refactoring - Merge pastefromwordimage plugin to pastefromword

Does your PR contain necessary tests?

There remain all old tests. No new feature or bug fixes was added.

This PR contains

Unit tests
Manual tests

What changes did you make?

move paste from word image plugin into paste from word
move tests related to this feature
add configuration option to disable pasting images from word

This is a subtask of #662

mlewand · 2017-11-23T13:06:08Z

plugins/pastefromword/plugin.js

+			// Paste From Word Image:
+			// RTF clipboard is required for embeding images.
+			if ( CKEDITOR.plugins.clipboard.isCustomDataTypesSupported && configInlineImages ) {
+				editor.filter.allow( 'img[src]' );


Actually in this case we don't want to force adding img to the allowed content list.

Instead we can do a bit more clever thing, and detect whether images are allowed. If not there's also no point in doing this logic. For example:

editor.filter.check( 'img[src]' )

mlewand

Maybe it's late and I'm missing something obvious, but the solutino does not work for me.

Sample case:

Open pastewordwithimage manual test.
Open any docx that has images in Microsoft Word.
Select all & copy.
Go back to the manual test.
Focus editor.
Empty the contents.
Paste.

Expected

Images being inlined.

Actual

Images with file:// protocol are pasted.

Additional notes

I can see two main reasons to that:

https://github.com/ckeditor/ckeditor-dev/blob/5b876ee/plugins/pastefromword/plugin.js#L67 - doesn't seem to work when called getNative=true. If you call it just as data.dataTransfer.getData( 'text/rtf' ) : null, it seems to work fine.
https://github.com/ckeditor/ckeditor-dev/blob/5b876ee/plugins/pastefromword/plugin.js#L117 - editor.filter.check( 'img[src]' ) is executed too early. Not all plugins are registered at that time, meaning not every ACF rule is already signed in. We should check it later, or simply each time at a runtime.

And I'm wondering if that's the case, why no unit test is failing?

mlewand · 2017-11-24T00:35:24Z

plugins/pastefromword/filter/default.js

@@ -38,7 +38,7 @@
 	 * @private
 	 * @member CKEDITOR.plugins
 	 */
-	CKEDITOR.plugins.pastefromword = {};
+	CKEDITOR.plugins.pastefromword = CKEDITOR.plugins.pastefromword || {};


I don't see a reason for this reinitializaiton.

This file is meant to be loaded once, thus it's ok to start with an empty object.

Edit: now I know, see my comment in plugin.js file. 🙂

mlewand · 2017-11-24T00:39:21Z

plugins/pastefromword/plugin.js

@@ -128,6 +135,117 @@

 		return !isLoaded;
 	}
+
+	function pasteFromWordImageListener( evt ) {


You did a good encapsulation of this logic, however this logic should go to the filter/default.js file.

And now I guess I know why you wanted to reuse the namespace 🙂.

There are several points to make this logic file a separate one:

It reduces the editor size, as it's loaded only on demand, when PFW is truly used.

It allows to customize it, and provide a custom implementation, as long as the interface is the same.

With that in mind, image inlining is an implementation detail and should be placed in the filter file.

mlewand · 2017-11-24T00:52:42Z

plugins/pastefromword/plugin.js

+/**
+ * Flag decides wheather emebeding images pasted with Word content is enabled or not.
+ *
+ * **Note:** Please be aware that emebeding images requires apropriate clipboard access in users' browser.


Good idea about this notice. However we could make it a bit more precise, saying that it requires Clipboard API available only in modern browsers.

We could link to CKEDITOR.plugins.clipboard.isCustomDataTypesSupported property, as this is the information that determines that.

mlewand · 2017-11-24T00:55:51Z

plugins/pastefromword/plugin.js

@@ -161,6 +279,16 @@
 * @member CKEDITOR.config
 */

+/**
+ * Flag decides wheather emebeding images pasted with Word content is enabled or not.


If you check other places in our API docs we usually put an example how the config could be changed to a behavior opposite to the default.

Please add a code listing showing how this var could be set to false.

This markup doesn't render well:

Also, you have provided a link to isCustomDataTypesSupported property, that's great - but you did that without any context, just put some words what it's all about.

mlewand · 2017-11-24T01:04:19Z

tests/plugins/pastefromword/generated/PFW_image.js

@@ -0,0 +1,39 @@
+/* bender-tags: clipboard,pastefromword */


Please, rename file to lowercased pfw_image, actually... pfw prefix doesn't have much use here. After all it's a directory for pfw so only its tests are here. 🙂 So how about just image?

mlewand · 2017-11-24T01:05:30Z

tests/plugins/pastefromword/generated/PFW_image.js

@@ -0,0 +1,39 @@
+/* bender-tags: clipboard,pastefromword */
+/* bender-ckeditor-plugins: pastefromword,ajax,basicstyles,bidi,font,link,toolbar,colorbutton,image */
+/* bender-ckeditor-plugins: list,liststyle,sourcearea,format,justify,table,tableresize,tabletools,indent,indentblock,div,dialog */


I think many of this plugins could be safely removed. Could we do that?

I reduced it, but few of them still remain.

… tests.

msamsel · 2017-11-24T14:59:08Z

I wasn't able to easily embed entire logic inside filter. It cause too many test errors. I'll extract it along with other stuff to refactoring ticket.

mlewand

Generally the code works well. There are just a few improvements we should make to the structure, and we're good to go.

mlewand · 2017-11-24T20:28:46Z

plugins/pastefromword/plugin.js

@@ -161,6 +279,16 @@
 * @member CKEDITOR.config
 */

+/**
+ * Flag decides wheather emebeding images pasted with Word content is enabled or not.


This markup doesn't render well:

mlewand · 2017-11-24T20:29:50Z

plugins/pastefromword/plugin.js

@@ -161,6 +279,16 @@
 * @member CKEDITOR.config
 */

+/**
+ * Flag decides wheather emebeding images pasted with Word content is enabled or not.


Also, you have provided a link to isCustomDataTypesSupported property, that's great - but you did that without any context, just put some words what it's all about.

mlewand · 2017-11-24T20:36:06Z

plugins/pastefromword/plugin.js

+				editor.on( 'afterPasteFromWord', pasteFromWordImageListener );
+			}
+
+			function pasteFromWordImageListener( evt ) {


Could we simplify it to imagePastingListener?

mlewand · 2017-11-24T21:08:59Z

plugins/pastefromword/filter/default.js

+		 * @returns {String} return.hex Hexadecimal string of an image embedded in `rtfContent`.
+		 * @returns {String} return.type String represent type of image, allowed values: 'image/png', 'image/jpeg'.
+		 */
+		extractImagesFromRtf: function( rtfContent ) {


I would like to avoid putting this logic at the very beginning of PFW filter file.

This part is not as important to expose it here. Instead I would suggest enclosing it in a dedicated namespace, like CKEDITOR.plugins.pastefromword.images and it could contain:

extractImagesFromRtf actually renamed to extractFromRtf.

extractImgTagsFromHtml actually renamed to extractTagsFromHtml.

Then move it a bit to the bottom of a file.

mlewand

Sorry pressed a wrong review result.

… config.pasteFromWord_inlineImages docs.

mlewand

LGTM

Merge pastefromwordimage plugin to pastefromword

mlewand reviewed Nov 23, 2017

View reviewed changes

Mateusz Samsel added 8 commits November 23, 2017 14:41

Merging PFW Image logic to PFW.

5107dbc

Moving helpers tests.

38a56bc

Moving PFW Image unit test.

d826abd

Fix failing unit tests.

edbdbb9

Moving manual test from PFW image to PFW.

06f7db7

Cleanning up PFW Image references, docs update.

aefcfd9

Adding configuration option which can disable pasting images from word.

a4bde6b

Hot-fix to check if img tags are allowed

5b876ee

msamsel force-pushed the t/662 branch from a0fac79 to db08d48 Compare November 23, 2017 13:42

msamsel force-pushed the t/662-3210 branch from 6fa04aa to 5b876ee Compare November 23, 2017 13:42

mlewand suggested changes Nov 24, 2017

View reviewed changes

Mateusz Samsel added 2 commits November 24, 2017 14:21

Review fixes: move logic to filter, update docs, rename tests, update…

194aaca

… tests.

Resolve failing tests problem.

1662c59

msamsel requested a review from mlewand November 24, 2017 14:59

msamsel mentioned this pull request Nov 24, 2017

Refactoring pasting images form word #1225

Open

Docs adjustments.

e4e4fda

mlewand approved these changes Nov 24, 2017

View reviewed changes

mlewand self-requested a review November 24, 2017 21:48

mlewand suggested changes Nov 24, 2017

View reviewed changes

mlewand mentioned this pull request Nov 24, 2017

Review corrections to Paste From Word Image merge task #1227

Merged

2 tasks

mlewand added 4 commits November 27, 2017 09:41

Renamed pasteFromWordImageListener function and added corrections for…

d5ca12e

… config.pasteFromWord_inlineImages docs.

Docs: minor API doc rewording.

9a156a9

Extracted image-related logic into a common PFW namespace.

67abe02

Docs: corrected API docs.

06ba63d

msamsel requested a review from mlewand November 27, 2017 08:45

mlewand approved these changes Nov 27, 2017

View reviewed changes

mlewand merged commit 5ee8d24 into t/662 Nov 27, 2017

mlewand added a commit that referenced this pull request Nov 27, 2017

Merge pull request #1214 from ckeditor/t/662-3210

8aba1ec

Merge pastefromwordimage plugin to pastefromword

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge pastefromwordimage plugin to pastefromword #1214

Merge pastefromwordimage plugin to pastefromword #1214

msamsel commented Nov 23, 2017 •

edited by mlewand

Loading

mlewand Nov 23, 2017

mlewand left a comment

mlewand Nov 24, 2017

mlewand Nov 24, 2017

mlewand Nov 24, 2017

mlewand Nov 24, 2017

mlewand Nov 24, 2017

mlewand Nov 24, 2017

mlewand Nov 24, 2017

mlewand Nov 24, 2017

msamsel Nov 24, 2017

msamsel commented Nov 24, 2017

mlewand left a comment

mlewand Nov 24, 2017

mlewand Nov 24, 2017

mlewand Nov 24, 2017

mlewand Nov 24, 2017

mlewand left a comment

mlewand left a comment

Merge pastefromwordimage plugin to pastefromword #1214

Merge pastefromwordimage plugin to pastefromword #1214

Conversation

msamsel commented Nov 23, 2017 • edited by mlewand Loading

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

Choose a reason for hiding this comment

mlewand left a comment

Choose a reason for hiding this comment

Expected

Actual

Additional notes

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

msamsel commented Nov 24, 2017

mlewand left a 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

mlewand left a comment

Choose a reason for hiding this comment

mlewand left a comment

Choose a reason for hiding this comment

msamsel commented Nov 23, 2017 •

edited by mlewand

Loading