Paste replacement #976

msamsel · 2017-09-28T11:54:55Z

What is the purpose of this pull request?

New feature

Does your PR contain necessary tests?

yes

This PR contains

Unit tests
Manual tests

What changes did you make?

add plugin which embed images while pasting from Word

Related to #662

…to functions, etc.

…nt-family from expected fixtures files.

f1ames

Please take a look at review comments. Apart from that:

Tests

Helpers for generated tests

It would be better to extend helpers from pastefromworld plugin instead of copying and slightly modifying them (especially that only 3 of them changed from what I see). Remember that you can always use longer, relative path in bender-include, like:

/* bender-include: ../widget/_helpers/tools.js, ../embedbase/_helpers/tools.js */

So my proposition here is to extend helpers from pastefromword plugin. You may introduce an additional flag like includeRTF:

bender.test( createTestSuite( {
	browsers: [
		'chrome',
		'firefox'
	],
	wordVersions: [
		'word2013',
		'word2016',
		'macos'
	],
	tests: {
		'SimpleImages': true,
		'MixedOnline': true,
		'MixedOnlineAndShapes': true,
		'WrappedImage': true
	},
	ignoreAll: CKEDITOR.env.ie,
        includeRTF: true
} ) );

and then the flow will slightly differ (by the changes you made in a copied files) from the default one.

When it comes to pfwTools.js you may simply add another config there, like:

// Preferred editor config for generated tests.
defaultConfig: {
	language: 'en',
	removePlugins: 'dialogadvtab,flash,showborders,horizontalrule',
	colorButton_normalizeBackground: false,
	extraAllowedContent: 'span{line-height,background,font-weight,font-style,text-decoration,text-underline,display,' +
		'page-break-before,height,tab-stops,layout-grid-mode,text-justify,-ms-layout-grid-mode,-ms-text-justify,' +
		'unicode-bidi,direction,dir,lang,page-break-after};td[valign]',
	disallowedContent: 'td{vertical-align}'
},
// Preferred editor config for generated tests for image pasting.
imagePasteConfig: {
	language: 'en',
	removePlugins: 'dialogadvtab,flash,showborders,horizontalrule',
	colorButton_normalizeBackground: false,
	extraAllowedContent: 'span{line-height,background,font-weight,font-style,text-decoration,text-underline,display,' +
	'page-break-before,height,tab-stops,layout-grid-mode,text-justify,-ms-layout-grid-mode,-ms-text-justify,' +
	'unicode-bidi,direction,dir,lang,page-break-after};td[valign]',
	disallowedContent: 'td{vertical-align};*[data-cke-*];span{font-family}'
},

or alternatively just edit it in test file, overwriting properties which should be changed same as in https://github.com/ckeditor/ckeditor-dev/blob/19e23a414fdb62ec903d2451049861546a7de7e9/tests/plugins/pastefromword/generated/excel.js#L17

Fixtures naming

I find fixtures naming a little inconsistent. There is MixedOnline, MixedOnlineAndShapes, SimpleImages and WrappedImage. It will be good to decide if Image word should be used in all the names or if should be skipped (as this is pastefromwordimage plugin one may assume it contains images, however I would be for adding Image suffix to all fixtures names).

I would also consider changing names of MixedOnline, MixedOnlineAndShapes to something more readable as OnlineImage, ShapesAndOnlineImage. Probably SimpleImages could be renamed to OfflineImages? WDYT?

Failing docx

I created a sample document with various images, shapes, etc. Generally it worked fine unless I inserted a shape containing text:

I know the shapes are not inserted which is fine, but when this shape is present no image is inserted (so it breaks pasting images). I'm not entirely sure how it is connected. It also may be an issue with pastefromword itself so then should be upstreamed to it, but it will be worth to take a look. Here is a troublesome docx:
Images.Test1.docx.txt

f1ames · 2017-10-18T09:38:33Z

plugins/pastefromwordimage/plugin.js

+			editor.on( 'afterPasteFromWord', this.pasteListener, this );
+		},
+
+		pasteListener: function( evt ) {


When you call CKEDITOR.plugins.add as a second parameter it accepts plugin definition. The pasteListener method and two others (createSrcWithBase64, hexToBase64 ) should not be placed in the definition as it should only provide afterInit, beforeInit, init and onLoad methods.

Ofc, this code works fine, but is conceptually incorrect. You may move these functions:

inside init method,

before (or after) CKEDITOR.plugins.add( 'pastefromwordimage', ... ),

inside CKEDITOR.plugins.pastefromwordimage probably as a private ones.

I am for 2nd or 3rd option.

f1ames · 2017-10-18T09:42:11Z

plugins/pastefromwordimage/plugin.js

+
+		hexToBase64: function( hexString ) {
+			return CKEDITOR.tools.convertBytesToBase64( CKEDITOR.tools.convertHexStringToBytes( hexString ) );
+		}


I know the above two looks nice and super clear, but hexToBase64 is only used once inside createSrcWithBase64 so maybe it could be merged into one function?

f1ames · 2017-10-18T10:42:22Z

tests/plugins/pastefromwordimage/manual/pastewordwithimage.md

+1. Clear editor with `New page` and repeat above steps for next two files:
+  * [Online and offline img](../generated/_fixtures/MixedOnline/Mixed_drawings_and_online.docx),
+  * [Document with shape](../generated/_fixtures/MixedOnlineAndShapes/Mixed_drawings_and_online_and_shapes.docx). _Please notice that shape should be ignored while pasting._
+


When you have same scenario for multiple files/editors/etc you may just put a generic steps like:

Open document and copy its whole content.

Clear editor contents / empty editor / etc.

Paste copied content into CKEditor instance.

Perform above scenario for all files:
... list of files

f1ames · 2017-10-18T13:06:43Z

dev/pastefromwordimage/getclipboard.html

+	<div style="width: 100%; float: left;">
+		<p>After Paste Processing:</p>
+		<textarea id="output" readonly="readonly"></textarea>
+	</div>


Maybe you could also add a container which will show all images pasted to the CKEditor (in the form already transformed by CKEditor), something like Extracted Images? If it's relatively easy to do (like 0,5h) you could take a look at it. From the other hand you can see pasted images inside editor instance, so not sure if it is needed. WDYT?

On this stage of implementation it might be problematic. I could process RTF clipboard, but it might contains also Word Shapes, which are ignored for processing while pasting. That's why preparing similar solution here might be repeating functionality of the plugin, which seems to be not a good thing.
It should be easier, when further changes related to file transfer will be implemented:
https://github.com/ckeditor/ckeditor-dev/blob/128516a52485505176189743962eaf97d52aa067/plugins/pastefromwordimage/plugin.js#L60-L63
There is nice event when will be exposed processed URL and image which is going to be embed. With that implementation it should be like 0,5h of work :)

@msamsel Ok, so let's leave it as it is for now.

f1ames · 2017-10-18T13:07:50Z

tests/plugins/pastefromwordimage/manual/pastewordwithimage.md

@@ -0,0 +1,16 @@
+@bender-tags: bug, 4.8.0, 662, pastefromwordimage
+@bender-ui: collapsed
+@bender-ckeditor-plugins: wysiwygarea, toolbar, undo, pastefromwordimage, sourcearea, elementspath, newpage


I would add resize plugin so you can enlarge editor to see whole pasted content.

f1ames · 2017-10-18T13:08:22Z

tests/plugins/pastefromwordimage/manual/pastewordwithimage.html

@@ -0,0 +1,4 @@
+<textarea id="editor" cols="30" rows="10"></textarea>
+<script>


This test should be ignored in browsers not supporting Clipboard API IMHO.

Alternatively you may describe what is the expected behaviour for such browsers.

…enerators for PFW Image. Change Image test to us PFW tests generator.

msamsel · 2017-10-19T12:47:43Z

I redesigned PFW helpers to have possibility for using it in PFW Image tests, but in future most probably it will have to be separate again. Because there will be required to process files outside of PFW event, so logic which make assertion will have to change.
https://github.com/ckeditor/ckeditor-dev/blob/128516a52485505176189743962eaf97d52aa067/tests/plugins/pastefromwordimage/generated/_helpers/createTestCase.js#L40-L115
I rename tests to include "Image" word at the end.
Failing example is caused by wrong processing of Shapes by PFW. I've created an upstream issue with it Wrong shape processing by Paste from Word #1069.

f1ames

I found one issue when offline linked image is pasted (LinkedImage.docx.txt). Instead of one there are two image elements created and src is not transformed to base64 so two broken images appears.

f1ames · 2017-10-20T06:35:57Z

tests/plugins/pastefromword/generated/_helpers/assertWordFilter.js

@@ -1,17 +1,20 @@
 /* global promisePasteEvent */
 /* exported assertWordFilter */
 function assertWordFilter( editor, compareRawData ) {
-	return function( input, output ) {
+	return function( inputHtml, output, inputRtf ) {


Maybe you could use an object here input: { 'text/html': ..., 'text/rtf': ... }?

f1ames · 2017-10-20T06:42:35Z

tests/plugins/pastefromword/generated/_helpers/createTestCase.js

@@ -15,11 +15,13 @@
 * @param {Boolean} [options.compareRawData=false] If `true` test case will assert against raw paste's `data.dataValue` rather than
 * what will appear in the editor after all transformations and filtering.
 * @param {Array} [options.customFilters] Array of custom filters (like [ pfwTools.filters.font ]) which will be used during assertions.
+ * @param {Boolean} options.includeRTF Flag infor if RTF clipboard should be loaded in test case


Make it optional and false by default @param {Boolean} [options.includeRTF=false]. Also I would update a description to sth like Whether RTF fixture should be loaded.

f1ames · 2017-10-20T06:44:32Z

tests/plugins/pastefromword/generated/_helpers/createTestCase.js

 * @returns {Function}
 */
 function createTestCase( options ) {
 	return function() {
-		var inputPath = [ '_fixtures', options.name, options.wordVersion, options.browser ].join( '/' ) + '.html',
+		var inputPathHtml = [ '_fixtures', options.name, options.wordVersion, options.browser ].join( '/' ) + '.html',


You may extract [ '_fixtures', options.name, options.wordVersion, options.browser ] like:

var inputPath = [ '_fixtures', options.name, options.wordVersion, options.browser ].join( '/' ), inputPathHtml = inputPath + '.html, inputPathRtf = options.includeRTF ? inputPath + '.rtf' : null,

Also options.includeRTF ? inputPath + '.rtf' : null, is not needed here as further in the code options.includeRTF is checked so the inputPathRtf may be set even when options.includeRTF is false.

f1ames · 2017-10-20T06:53:43Z

tests/plugins/pastefromword/generated/_helpers/createTestSuite.js

@@ -24,7 +24,8 @@ function createTestSuite( options ) {
 		testData: { _should: { ignore: {} } },
 		ignoreAll: false,
 		compareRawData: false,
-		customFilters: null
+		customFilters: null,
+		includeRTF: false


Missing docs for includeRTF (could be the same as in createTestCase) in the method @params list.

f1ames · 2017-10-20T07:11:17Z

plugins/pastefromwordimage/plugin.js

@@ -8,9 +8,55 @@

 	CKEDITOR.plugins.add( 'pastefromwordimage', {
 		requires: 'pastefromword',
-		init: function() {}
+		init: function( editor ) {
+			if ( CKEDITOR.env.ie || CKEDITOR.env.iOS ) {


Was there any particular reason why it was changed from !CKEDITOR.plugins.clipboard.isCustomDataTypesSupported to CKEDITOR.env.ie || CKEDITOR.env.iOS?

If it is about iOS you may always go with !CKEDITOR.plugins.clipboard.isCustomDataTypesSupported || CKEDITOR.env.iOS which is more flexible (as CKEDITOR.plugins.clipboard.isCustomDataTypesSupported may change at some point and then it will automatically work here too).

I've change wrong file :(
It supposed to be switched in manual test here: https://github.com/ckeditor/ckeditor-dev/blob/aa75e0874a372dd89168314eaae2162c248a1ff9/tests/plugins/pastefromwordimage/manual/pastewordwithimage.html#L3
To avoid problem with loading plugins. As manual test are usually fired only while releasing, then such change should be sufficient there.

f1ames · 2017-10-20T07:29:28Z

tests/plugins/pastefromwordimage/manual/pastewordwithimage.html

@@ -1,4 +1,7 @@
 <textarea id="editor" cols="30" rows="10"></textarea>
 <script>
+	if ( !CKEDITOR.plugins.clipboard.isCustomDataTypesSupported ) {


This code should be called at least after pluginsLoaded event as at this point it throws an error

because CKEDITOR.plugins.clipboard is not initialized yet.

f1ames · 2017-10-20T07:59:27Z

but in future most probably it will have to be separate again. Because there will be required to process files outside of PFW event, so logic which make assertion will have to change.

@msamsel Ok, I hope we will be able to extract the different logic/flows to some separate helper and still keep it DRY, without duplicating too much code.

msamsel · 2017-10-20T08:45:39Z

I found one issue when offline linked image is pasted ...

@f1ames linking images problem was reported here: #995

msamsel · 2017-10-20T09:22:45Z

but in future most probably it will have to be separate again. Because there will be required to process files outside of PFW event, so logic which make assertion will have to change.

Ok, I hope we will be able to extract the different logic/flows to some separate helper and still keep it DRY, without duplicating too much code.

What I think now it might be possible to prepare alternative assert file, similar to current assertWordFilter.js, but dedicated for assert with images.

f1ames · 2017-10-23T09:21:38Z

What I think now it might be possible to prepare alternative assert file, similar to current assertWordFilter.js, but dedicated for assert with images.

@msamsel I think it might make a lot of sense for this case. We will see after starting working on it 👍

f1ames

Looks good.

I have one doubt about fixtures naming, the thing I didn't noticed before 😞 . For each fixtures folder there is original .docx file, expected.html and folders with input data for different versions of Word word2013, word2016, etc. And now:

word2013, word2016 is quite clear, however there is a macos directory without any Word version number. If there is only one Word version on OS X, it is ok. But if not I think we should keep naming consistent like word2013_win, word2016_win, word2013_osx.

Also some fixtures like e.g OnlineAndOfflineImage/macos/firefox.html and OnlineAndOfflineImage/macos/chrome.html are identical so I was thinking about merging them to one (no sense in keeping both identical as then it will generate two identical tests). However, the corresponding RTF fixtures (OnlineAndOfflineImage/macos/firefox.rtf and OnlineAndOfflineImage/macos/chrome.html) are different so I think that is the main reason there are two (or rather four) separate files @msamsel ?

To avoid problem with loading plugins. As manual test are usually fired only while releasing, then such change should be sufficient there.

That's true, but sometimes when some other changes are developed, one may like to go through some manual tests so it is good to keep them up to date (or even better to keep them "update itself":) ). I updated this test, see 31f7874#diff-330cd6c0d0e494342e88f914d876a26dR3

f1ames · 2017-10-23T10:17:53Z

tests/plugins/pastefromwordimage/manual/pastewordwithimage.md

+1. Paste copied content into CKEditor.
+
+### Perform above steps for all files:
+  * [Offline img](../generated/_fixtures/SimpleImages/SimpleImages.docx).


Links doesn't work anymore as folder names were changed.

f1ames · 2017-10-23T10:19:34Z

tests/plugins/pastefromwordimage/manual/pastewordwithimage.md

+  * [Online and offline img](../generated/_fixtures/MixedOnline/Mixed_drawings_and_online.docx),
+  * [Document with shape](../generated/_fixtures/MixedOnlineAndShapes/Mixed_drawings_and_online_and_shapes.docx). _Please notice that shape should be ignored while pasting._
+
+**Expected:** Images are pasted to the editor.


As one of the files contains a shape which is not pasted it should be also described here, e.g. Keep in mind that shapes (like last object inMixed_drawings_and_online_and_shapes.docx) will not be pasted.

msamsel · 2017-10-24T08:59:06Z

Also some fixtures like e.g OnlineAndOfflineImage/macos/firefox.html and OnlineAndOfflineImage/macos/chrome.htm are identical so I was thinking about merging them to one (no sense in keeping both identical as then it will generate two identical tests). However, the corresponding RTF fixtures (OnlineAndOfflineImage/macos/firefox.rtf and OnlineAndOfflineImage/macos/chrome.html) are different so I think that is the main reason there are two (or rather four) separate files.
It wasn't a reason. I just haven't check it and use both available browsers supported RTF under OSX.

About merging it won't possible easily. Currently test generator assume that html and rtf files has the same names, so it won't work with merged file. Such possibility would require changes in test generator.
From what I see RTFs differ with some ID values inside. So those might be removed also. WDYT? If you consider, that those test doesn't provide additional value feel free to remove it.

f1ames · 2017-10-25T11:08:04Z

About merging it won't possible easily. Currently test generator assume that html and rtf files has the same names, so it won't work with merged file. Such possibility would require changes in test generator.
From what I see RTFs differ with some ID values inside. So those might be removed also. WDYT? If you consider, that those test doesn't provide additional value feel free to remove it.

I see. I was think about it as some kind of optimization, but as the overhead is very little we may leave it as is for now.

f1ames

LGTM 👍

msamsel force-pushed the t/662-2974 branch 2 times, most recently from 406681f to ba7ccbe Compare October 5, 2017 11:54

mlewand force-pushed the t/662-2974 branch from ba7ccbe to c1e5768 Compare October 11, 2017 13:32

msamsel force-pushed the t/662-2974 branch from c1e5768 to dc5f37a Compare October 12, 2017 13:07

Mateusz Samsel added 11 commits October 17, 2017 12:08

Changes in PFW required for images.

d84763d

Some working example. Base for further improvements.

eae8e25

Add fixture generator for PFWImage.

c434d75

Adding library necessary for auto-generated tests.

242c8cf

Add auto-generated unit tests.

08b1490

Small code improvements such as: renaming variables, extracting code …

693e3f1

…to functions, etc.

Adding manual tests.

091aa2f

Hot fixes after rebasing on t/662-2973.

5da4676

Adding new fixtures.

22f7e81

Little code improvement, adding macos support for test.

5d870ea

Set font-family as disallowed to fix Unit tests on Firefox. Remove fo…

54d2498

…nt-family from expected fixtures files.

msamsel force-pushed the t/662-2974 branch from dc5f37a to 54d2498 Compare October 17, 2017 10:08

Fix test name for helpers.

17cc5dd

f1ames self-requested a review October 18, 2017 09:11

Fix typos.

d2d8a13

f1ames suggested changes Oct 18, 2017

View reviewed changes

Mateusz Samsel added 2 commits October 19, 2017 12:38

Update PFW autogenerated test to support RTF clipboard. Remove test g…

2e852c0

…enerators for PFW Image. Change Image test to us PFW tests generator.

Rename test cases to be more consistent.

ab1ff15

Move PFW Image logic outside of plugin definition. Update Manual tests.

aa75e08

msamsel force-pushed the t/662-2974 branch from 9a2e432 to aa75e08 Compare October 19, 2017 15:06

f1ames self-requested a review October 20, 2017 06:32

Typo fixes.

bd65d9c

f1ames suggested changes Oct 20, 2017

View reviewed changes

Test updates. Small code improvements in plugin.

b9b878a

Manual test - ignore on loaded.

31f7874

f1ames force-pushed the t/662-2974 branch from 3705b6b to 31f7874 Compare October 23, 2017 10:35

f1ames suggested changes Oct 23, 2017

View reviewed changes

Review: rename tests, fix links

2055b4d

f1ames self-requested a review October 25, 2017 11:02

Some rewording.

6388b13

f1ames approved these changes Oct 25, 2017

View reviewed changes

f1ames merged commit 734f957 into t/662 Oct 25, 2017

f1ames deleted the t/662-2974 branch October 25, 2017 12:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Paste replacement #976

Paste replacement #976

msamsel commented Sep 28, 2017 •

edited

Loading

f1ames left a comment

f1ames Oct 18, 2017

f1ames Oct 18, 2017

f1ames Oct 18, 2017

f1ames Oct 18, 2017

msamsel Oct 19, 2017 •

edited

Loading

f1ames Oct 19, 2017

f1ames Oct 18, 2017

f1ames Oct 18, 2017

f1ames Oct 18, 2017

msamsel commented Oct 19, 2017

f1ames left a comment

f1ames Oct 20, 2017

f1ames Oct 20, 2017

f1ames Oct 20, 2017

f1ames Oct 20, 2017

f1ames Oct 20, 2017

f1ames Oct 20, 2017

msamsel Oct 20, 2017

f1ames Oct 20, 2017

f1ames commented Oct 20, 2017

msamsel commented Oct 20, 2017 •

edited

Loading

msamsel commented Oct 20, 2017

f1ames commented Oct 23, 2017

f1ames left a comment

f1ames Oct 23, 2017

f1ames Oct 23, 2017 •

edited

Loading

msamsel commented Oct 24, 2017

f1ames commented Oct 25, 2017

f1ames left a comment

		@@ -0,0 +1,4 @@
		<textarea id="editor" cols="30" rows="10"></textarea>
		<script>

Paste replacement #976

Paste replacement #976

Conversation

msamsel commented Sep 28, 2017 • edited Loading

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

f1ames left a comment

Choose a reason for hiding this comment

Tests

Helpers for generated tests

Fixtures naming

Failing docx

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 Oct 19, 2017 • edited Loading

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 Oct 19, 2017

f1ames 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

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

f1ames commented Oct 20, 2017

msamsel commented Oct 20, 2017 • edited Loading

msamsel commented Oct 20, 2017

f1ames commented Oct 23, 2017

f1ames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f1ames Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

msamsel commented Oct 24, 2017

f1ames commented Oct 25, 2017

f1ames left a comment

Choose a reason for hiding this comment

msamsel commented Sep 28, 2017 •

edited

Loading

msamsel Oct 19, 2017 •

edited

Loading

msamsel commented Oct 20, 2017 •

edited

Loading

f1ames Oct 23, 2017 •

edited

Loading