Embed images pasted from Word in Safari #1976

msamsel · 2018-05-15T14:37:33Z

What is the purpose of this pull request?

Bug fix

Does your PR contain necessary tests?

yes

This PR contains

Unit tests
Manual tests

What changes did you make?

Provide possibility to embed images pasted in safari browser
Fix saves blobUrls detected during pasting images from Word and replacing them later asynchronously in editor's content.

Close #1134

Comandeer

I can't get it working with Safari 11.1 (13605.1.33.1.2) or Safari Technology Preview Release 56 (Safari 11.2, WebKit 13606.1.17.2.2) and Microsoft Word 15.34 (170330). The image is not pasted at all.

We're also missing real fixtures for pasting images in Safari, generated from Word.

Comandeer · 2018-05-19T12:32:43Z

CHANGES.md

@@ -17,6 +17,7 @@ Fixed Issues:
 * [#1592](https://github.com/ckeditor/ckeditor-dev/issues/1592): [Image Base](https://ckeditor.com/cke4/addon/imagebase) caption is not visible after paste.
 * [#620](https://github.com/ckeditor/ckeditor-dev/issues/620): Fixed: [`forcePasteAsPlainText`](https://docs.ckeditor.com/ckeditor4/latest/api/CKEDITOR_config.html#cfg-forcePasteAsPlainText) will be respected when internal and cross-editor pasting happen.
 * [#1467](https://github.com/ckeditor/ckeditor-dev/issues/1467): Fixed: [Table Resize](https://ckeditor.com/cke4/addon/tableresize) resizing coursor appearing in middle of merged cell.
+* [#1134](https://github.com/ckeditor/ckeditor-dev/issues/1134): Fixed: [Paste from word](https://ckeditor.com/cke4/addon/pastefromword) will embed images in Safari browser.


For browsers the style of changelog is:

[Safari] Fixed: Description of the issue.

Please also note that it's "Paste from Word", not "Paste from word"

Comandeer · 2018-05-19T14:04:45Z

plugins/ajax/plugin.js

@@ -65,14 +65,25 @@
 			return null;
 		}

-		function load( url, callback, getResponseFn ) {
+		function getCustomResponse( xhr ) {


Won't it be better to create one getResponse function that would return appropriate response instead of creating one function for every response type? Especially that now ArrayBuffer is supported only in load method and having one function would enable it everywhere with minimal amount of work.

If not, then at least change the function name to something more appropriate, e.g. getResponseArrayBuffer.

Comandeer · 2018-05-19T14:07:19Z

plugins/pastefromword/plugin.js

+						} else if ( typeof filteredData === 'object' ) {
+							editor.fire( 'lockSnapshot' );
+							pfwEvtData.dataValue = filteredData.dataValue;
+							handleBlobs( filteredData.blobUrls, function() {


I'm wondering about time between lockSnapshot and unlockSnapshot. If user could write anything in this time, it could result in broken or at least weird snapshot.

@Comandeer, yes it's a drawback of this approach. Another option is to make editor somehow readOnly, but I'm not sure what would be better here. WDYT? Should editor be read only during this paste? Or maybe you have some other solutions which I don't see?

Making editor read-only sounds even worse. Let's leave it for now as it shouldn't be so critical issue.

Comandeer · 2018-05-19T14:09:37Z

plugins/pastefromword/plugin.js

@@ -7,7 +7,7 @@
 	/* global confirm */

 	CKEDITOR.plugins.add( 'pastefromword', {
-		requires: 'clipboard',
+		requires: 'clipboard,ajax',


Isn't it any other way to not require ajax for PfW? It looks a little bit strange. I wonder if FileReader would be an alternative here.

I've considered using FileTools here. Unfortunately FileTools contains logic responsible for uploading files to the server, there is no methods helping downloading files from given URL, where Ajax has it and only requires to add new file type to support. That's it was chosen here. I could extend FileTools, with new functions, but this seems like new separate task, or hide completely XHR inside PFW and do not rely on Ajax plugin.
Using Ajax seems to be most convenient and time efficient for me.

@mlewand WDYT? Should we require Ajax in PFW or use some solution?

@Comandeer what are the downsides of using ajax plugin for this case? Using FileReader just for sake of using it results with more code to worry about.

@mlewand it wasn't exactly a downside, I just felt that current use of ajax plugin is inappropriate, but I haven't spend much time on it as whole fix wasn't working for me. You're comment about using plugin in core explains my anxiety ;)

Comandeer · 2018-05-19T14:09:42Z

plugins/pastefromword/plugin.js

+				function replaceBlobUrlsInEditor( map ) {
+					for ( var blob in map ) {
+						var nodeList = editor.editable().find( 'img[src="' + blob + '"]' ).toArray();
+						CKEDITOR.tools.array.forEach( nodeList, function( element ) {


Amount of CKEDITOR.tools in handleBlobs is too high. Let's make some short alias, which will increase the readability.

msamsel · 2018-05-28T08:42:09Z

I can't get it working with Safari 11.1 (13605.1.33.1.2) or Safari Technology Preview Release 56 (Safari 11.2, WebKit 13606.1.17.2.2) and Microsoft Word 15.34 (170330). The image is not pasted at all.

I test it on Safari 11.1 (13605.1.33.1.4) and MS Word 15.41 (171205) MacOS HighSierra (10.13.4) and it is working completely fine. Maybe you have some strange picture which is not embed as blobUrl by Safari? Could you provide your docx file?

We're also missing real fixtures for pasting images in Safari, generated from Word.

What do you have on your mind by "real fixtures". Do you want just to have soem real markup in test or you think about integrating it with generated tests? In case of 2nd option, it would require tremendous amount of work to adapt generated test to work with Safari fixtures:

Fixture would contain bloburl which will be invalid and should be changed or mocked with every single test run, as blob url won't be available
test should be fired completely asynchronously (it's not possible to read data as image from url synchrnously - only text can be done like that, and converting text to image isn't great idea). We should wait when editors content will be updated after asynchronous events and then compare its content. Right now PFW test check for data in event with high priority and cancel it before content is embed in editor. https://github.com/ckeditor/ckeditor-dev/blob/f4e106d5736e5d4211f7371a863d17dd46270e30/tests/plugins/pastefromword/generated/_helpers/promisePasteEvent.js#L23-L29

Comandeer · 2018-05-28T10:52:51Z

Word document with image – not working with Safari 11.1 (13605.1.33.1.4). I tried every single method of inserting image (pasting, via menu, d&d…), nothing works.

Comandeer · 2018-05-28T11:05:43Z

Uh, after update of Word to 16.13.1 (180523) it started working.

mlewand · 2018-05-29T11:28:29Z

core/tools.js

+		 */
+
+		convertBlobUrlToBase64: function( blobUrlSrc, callback ) {
+			CKEDITOR.ajax.load( blobUrlSrc, function( arrayBuffer ) {


ajax plugin function can not be used in core, since it's not guaranteed it's going to be there. If anything the logic should go to the ajax plugin itself.

Comandeer · 2018-05-29T11:55:54Z

Taking over this PR!

… to be full asynchronous.

…tion to handleBlob function.

Comandeer · 2018-06-01T12:13:20Z

I modified handling snapshots as the last version seemed to disable undo.

Taken over PR.

Comandeer previously requested changes May 19, 2018

View reviewed changes

mlewand reviewed May 29, 2018

View reviewed changes

Mateusz Samsel and others added 17 commits June 1, 2018 14:10

New feature bloburl to base64 converter in PFW.

15c8fda

Remove unnecessary change in ajax plugin

d6e2d48

Unit tests.

4c40476

Update tests, provide docs for ajax. Modify converting blob to base64…

2e80c60

… to be full asynchronous.

Move blob conversion to tools add unit test for new tools.

a85001d

Remove wrong test case.

3365561

Update unit test, add manual tests.

ba8371a

Changelog entry.

f5c50c3

Review fixes: update changelog entry, refactor CKEDITOR.ajax.

6a68173

Review fixes, provide alias for CKEDITOR.tools.array, provide descrip…

7a0ac51

…tion to handleBlob function.

Move convertBlobUrlToBase64 into ajax plugin.

b1305d2

Refactoring of Ajax plugin.

c958fd7

Update issue references.

dc20773

Refactor tests.

d6d2fa5

Fix undo integration.

5ec567b

Add unit test for multiple blobs.

29940fa

Remove redundant white space.

12076d7

Comandeer force-pushed the t/1134 branch from 4a8cdf9 to 12076d7 Compare June 1, 2018 12:11

Update changelog entry.

25f0833

Comandeer requested a review from mlewand June 1, 2018 12:16

f1ames added the pr:frozen ❄ The pull request on which work was suspended due to various reasons. label Jan 16, 2020

f1ames closed this Jan 16, 2020

Comandeer added a commit that referenced this pull request Oct 19, 2020

Import PfW tests from #1976.

0735f53

Comandeer added a commit that referenced this pull request Oct 19, 2020

Add test for loading ArrayBuffer via Ajax plugin from #1976.

9fa0239

Comandeer added a commit that referenced this pull request Oct 19, 2020

Import changes to Ajax plugin from #1976.

398089c

Comandeer added a commit that referenced this pull request Oct 19, 2020

Convert blob images to Data URLs. Based on #1976.

06759a7

Comandeer mentioned this pull request Oct 19, 2020

Add support for Word's images pasting to Safari #4339

Merged

3 tasks

Comandeer added a commit that referenced this pull request Nov 16, 2020

Import PfW tests from #1976.

3386ec7

Comandeer added a commit that referenced this pull request Nov 16, 2020

Add test for loading ArrayBuffer via Ajax plugin from #1976.

b25290d

Comandeer added a commit that referenced this pull request Nov 16, 2020

Import changes to Ajax plugin from #1976.

9bee241

Comandeer added a commit that referenced this pull request Nov 16, 2020

Convert blob images to Data URLs. Based on #1976.

9ed352f

jacekbogdanski pushed a commit that referenced this pull request Nov 23, 2020

Import PfW tests from #1976.

246f04f

jacekbogdanski pushed a commit that referenced this pull request Nov 23, 2020

Add test for loading ArrayBuffer via Ajax plugin from #1976.

d8076d0

jacekbogdanski pushed a commit that referenced this pull request Nov 23, 2020

Import changes to Ajax plugin from #1976.

a0cbde9

jacekbogdanski pushed a commit that referenced this pull request Nov 23, 2020

Convert blob images to Data URLs. Based on #1976.

8a4ccd8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Embed images pasted from Word in Safari #1976

Embed images pasted from Word in Safari #1976

msamsel commented May 15, 2018 •

edited

Loading

Comandeer left a comment

Comandeer May 19, 2018

Comandeer May 19, 2018

Comandeer May 19, 2018

msamsel May 28, 2018

Comandeer May 28, 2018

Comandeer May 19, 2018

msamsel May 28, 2018

mlewand May 29, 2018

Comandeer May 29, 2018

Comandeer May 19, 2018

msamsel commented May 28, 2018

Comandeer commented May 28, 2018

Comandeer commented May 28, 2018

mlewand May 29, 2018

Comandeer commented May 29, 2018

Comandeer commented Jun 1, 2018

Embed images pasted from Word in Safari #1976

Embed images pasted from Word in Safari #1976

Conversation

msamsel commented May 15, 2018 • edited Loading

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

Comandeer 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msamsel commented May 28, 2018

Comandeer commented May 28, 2018

Comandeer commented May 28, 2018

Choose a reason for hiding this comment

Comandeer commented May 29, 2018

Comandeer commented Jun 1, 2018

msamsel commented May 15, 2018 •

edited

Loading