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

Comandeer · 2020-10-19T16:08:29Z

What is the purpose of this pull request?

New feature

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?

* [#1134](https://github.com/ckeditor/ckeditor4/issues/1134): [Safari] Fixed: [Paste from Word](https://ckeditor.com/cke4/addon/pastefromword) does not embed images.

What changes did you make?

I've basically taken the contents from @msamsel's #1976 and integrated it with our CKEDITOR.plugins.pastetools.filters.image filter.

Which issues does your PR resolve?

Closes #1134.

jacekbogdanski

Works pretty nice! I see that this code had already some partial review in original PR, so there is not much left. For now, I've tested it with jpg, png, and gif. Please, see the review comments.

I also see that you marked this PR as a feature, although the original issue has been reported as a bug. I suppose that we could change it to feature as image support for OS X Word edition.

plugins/pastetools/filter/image.js

jacekbogdanski · 2020-10-26T10:29:06Z

plugins/pastetools/filter/image.js

+		var blobUrls = removeDuplicates( CKEDITOR.tools.array.filter( imgTags, function( imgTag ) {
+				return imgTag.match( /^blob:/i );
+			} ) ),


I suppose we could extract removeDuplicates function to array tools as something like CKEDITOR.tools.array.unique?

jacekbogdanski · 2020-10-26T10:29:37Z

plugins/pastetools/filter/image.js

+				return imgTag.match( /^blob:/i );
+			} ) ),
+			promises = CKEDITOR.tools.array.map( blobUrls, function( blobUrl ) {
+				return convertBlobUrlToBase64( blobUrl );


convertToBlobUrlToBase64 could be probably used as higher-order function?

jacekbogdanski · 2020-10-26T11:13:21Z

plugins/pastetools/filter/image.js

+				fileType = 'image/png';
+				break;
+			case '47494638':
+				fileType = 'image/gif';


It was unexpected to me to find out that actually gif images are supported by PFW. They seem to work on Chrome and Firefox, but not in Safari:

I've used this GIF:

Wow, I didn't know that Word supports GIFs 😅 I was pretty sure they are converted to PNGs.

Actually they are converted to PNG… So it works because most browsers support APNG.

Safari in such a case use GIF format. It's strange that it isn't rendered correctly as we have GIF recognition 🤔

Adding GIF mime type to array of supported image formats seems to do the trick.

jacekbogdanski · 2020-10-26T12:30:10Z

plugins/pastetools/filter/image.js

+			header += bytesHeader[ i ].toString( 16 );
+		}
+
+		switch ( header ) {


That's some dark magic 😄 I'm not sure if it should be named header, probably File signature is more accurate here. It would be good to have real, existing images with these signatures, to confirm that the feature covers all expected image types. These can be checked e.g. here: https://www.filesignatures.net/index.php?search=FFD8FFE0&mode=SIG although I suppose it may be hard to get all spf, jfif etc extension examples and I'm not sure if Word is not converting them also into a more unified format.

jacekbogdanski · 2020-10-26T12:45:29Z

plugins/ajax/plugin.js

@@ -128,12 +136,16 @@
 			 * @param {String} url The URL from which the data is loaded.
 			 * @param {Function} [callback] A callback function to be called on
 			 * data load. If not provided, the data will be loaded
-			 * synchronously.
-			 * @returns {String} The loaded data. For asynchronous requests, an
+			 * synchronously. Please notice that only text data might be loaded synchrnously.


Suggested change

* synchronously. Please notice that only text data might be loaded synchrnously.

* synchronously. Please notice that only text data might be loaded synchronously.

jacekbogdanski · 2020-10-26T12:48:15Z

tests/plugins/ajax/ajax.js

+		},
+
+		// (#1134)
+		'test load async arraybuffer': function() {


We are also missing text and xml types. I guess they are probably tested already as we are using them indirectly in ajax logic, but it would be good to check if API works correctly.

jacekbogdanski · 2020-10-26T12:49:27Z

tests/plugins/pastefromword/_helpers/blob.js

+
+	function simulatePasteBlob( editor, assertion, options ) {
+		// jscs:disable maximumLineLength
+		var imgBase64 = 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAIAAAACACAYAAADDPmHLAAAEDWlDQ1BJQ0MgUHJvZmlsZQAAOI2NVV1oHFUUPrtzZyMkzlNsNIV0qD8NJQ2TVjShtLp/3d02bpZJNtoi6GT27s6Yyc44M7v9oU9FUHwx6psUxL+3gCAo9Q/bPrQvlQol2tQgKD60+INQ6Ium65k7M5lpurHeZe58853vnnvuuWfvBei5qliWkRQBFpquLRcy4nOHj4g9K5CEh6AXBqFXUR0rXalMAjZPC3e1W99Dwntf2dXd/p+tt0YdFSBxH2Kz5qgLiI8B8KdVy3YBevqRHz/qWh72Yui3MUDEL3q44WPXw3M+fo1pZuQs4tOIBVVTaoiXEI/MxfhGDPsxsNZfoE1q66ro5aJim3XdoLFw72H+n23BaIXzbcOnz5mfPoTvYVz7KzUl5+FRxEuqkp9G/Ajia219thzg25abkRE/BpDc3pqvphHvRFys2weqvp+krbWKIX7nhDbzLOItiM8358pTwdirqpPFnMF2xLc1WvLyOwTAibpbmvHHcvttU57y5+XqNZrLe3lE/Pq8eUj2fXKfOe3pfOjzhJYtB/yll5SDFcSDiH+hRkH25+L+sdxKEAMZahrlSX8ukqMOWy/jXW2m6M9LDBc31B9LFuv6gVKg/0Szi3KAr1kGq1GMjU/aLbnq6/lRxc4XfJ98hTargX++DbMJBSiYMIe9Ck1YAxFkKEAG3xbYaKmDDgYyFK0UGYpfoWYXG+fAPPI6tJnNwb7ClP7IyF+D+bjOtCpkhz6CFrIa/I6sFtNl8auFXGMTP34sNwI/JhkgEtmDz14ySfaRcTIBInmKPE32kxyyE2Tv+thKbEVePDfW/byMM1Kmm0XdObS7oGD/MypMXFPXrCwOtoYjyyn7BV29/MZfsVzpLDdRtuIZnbpXzvlf+ev8MvYr/Gqk4H/kV/G3csdazLuyTMPsbFhzd1UabQbjFvDRmcWJxR3zcfHkVw9GfpbJmeev9F08WW8uDkaslwX6avlWGU6NRKz0g/SHtCy9J30o/ca9zX3Kfc19zn3BXQKRO8ud477hLnAfc1/G9mrzGlrfexZ5GLdn6ZZrrEohI2wVHhZywjbhUWEy8icMCGNCUdiBlq3r+xafL549HQ5jH+an+1y+LlYBifuxAvRN/lVVVOlwlCkdVm9NOL5BE4wkQ2SMlDZU97hX86EilU/lUmkQUztTE6mx1EEPh7OmdqBtAvv8HdWpbrJS6tJj3n0CWdM6busNzRV3S9KTYhqvNiqWmuroiKgYhshMjmhTh9ptWhsF7970j/SbMrsPE1suR5z7DMC+P/Hs+y7ijrQAlhyAgccjbhjPygfeBTjzhNqy28EdkUh8C+DU9+z2v/oyeH791OncxHOs5y2AtTc7nb/f73TWPkD/qwBnjX8BoJ98VVBg/m8AAAwTSURBVHgB7V15WBXXFf/x2PdVEFkU0KBGUVAUSExjtOqn9UsTd+PSGLcarW20TbBp0q+mTaJNE60xVqUxqbhETbQmxs8lGo0aLQiIRlQWyyIgIjvI3jmvEkB48BZm5g5z7z9v5s49957zO793Z+bOveeaTWm0bgRPqkVAo1rLueFaBDgBVE4ETgBOAJUjoHLzeQ/ACaByBFRuPu8BOAFUjoDKzec9ACeAyhFQufm8B+AEUDkCKjef9wCcACpHQOXm8x6AE0DlCKjcfAuV22+U+eWpQbh74hkUfDsKZSnBqCl0w8TMfkbVJbcQJ4CeHmgU5k3lHZmA1A9WCM4f00aqodYCGsu6NvmsZ3ACdOKhxgYzZGxfgFt/W4nyG4/pLN1Qbc0JoBMdhV6ozPZB3LwYFJx6umMLzBphbl/RcRlGr/IeQIdjsvc/h8uLN6O2yFVHieZsK/dCmJk1nyvpiL8GtuOtq2v+hIvTduvlfBJ3HJDSTi3KyOIEeMRP19dG48bbv3skt+NTl9CkjgswfJUToIVzbr2/Aj+88WaLHP0OvcYf068gg6U4AR46he75V15Zb7CLLJxL0OPpMwbLsSLACSB4ovqeOxKWbTTKJ4FLtsPCrsooWRaEOAEELyStfA81BT0M9oeZVQ36rtxksBxLAqonQO5XE5C1a6ZRPvGfvQe2vXKNkmVFSPUESHkr2ihfmNtVov/v3zVKliUhVROgOGkw7n8/0ih/hLz/Wzj0TTNKliUhVRMgfcsio3zh/exhBC6OMUqWNSEztcYHaKgzx2G3PNSVORrkE1vfbIxJGAlrj0KD5FgtrNoeoPxWX8Od75+Jp06N7zbOJ1Kq9mNQ6dXHDfpT2gemY9Q3E2DfO9MgOdYLq7YHKL02UG/fOIUk4ydnxnY75xMAqiVA1R3vTglAAz0D/rgWY+IiYetzp9PySiyg2luAxqLj6VtuURcwbNsv4TRQuZ969SGkYgnQWK9BLT3BN5rBTHCmpWO5Pvb+WMamnRE8jW0Vek44hqDlH8HzmdM/lu3soL7aClXZvoAwM8hCGCCy8rgHjUV9Z2JMXFcEAQjguydHo/BclDBwMwL0BP8g1xuNdc3q05QsW98cuIQmwj3qe3iNOw7H4Fs6QSYHl9+MhbngdPugdDiHXIXHU2c7/bBTV26P/ONjkPvlRNCDZGWmH6rzvbREbGqMCGkfcFs7UcQt4qJWH/cnzjNJCqbHAQovjET6R4tx59Bk1JU6NeGr969LWAL858ai9/ydsHIt1lvu0YL1VTbI+eJZZP5rtnZ+IE0ANTRZ98xDnxc/ReDSbbDzzzJUXLTyTBLg3ndRSH71z7h/PrJLDKdxe79Ze+E/LxYewj/RzLyh03obaixxV5gMmnPg58jeO80oArbXCD1YBr28BQNefxtWbkXtFZE0jykC1JY54Gr0WqRvXtqqS+1KRGgCp+e4E3AddhnOg6/B2vMuLBwq8CDfE1VCd05detHlUOR/Pd7ggSJD9LR0u4/QD1fCb+Y+Q8S6vCwzBMg9Mh4JSzehKsuvy41kuUKf6fsxbPtSgx9iu8omJsYBkl97C+cnHVKd88mJOZ9NxamIs6DlZnIk2QmQKMzGufnuajlsZ6bNsh8G4FTUaRQnDJFcJ9kIQGvtLi/ZhLSNL0tuNIsN0pS0b0cfE54/hkqqnmwEiF+wFRlbF0pqLOuN1ZU449ykg6jMEgaVJEqyECD9Hwvx3x3zJDJRWc3U3PPQLj2XSuvmoTSJWiy7FYQrq5Q/l04MuBz6pSI8dj7cwuPFqL7dOiUlAM3C+c+cHaivsG9XGTVnek/+CiN2z4WFfaWkMEh6C6AZuEWXwiU1UAmN+c78DBGfT5fc+YSNZANBtNb+aGAKGmstleATyXT0m7ML4Z+8BDONPFs3SdYD0Osed35rXrkMj9eOAsrlfNJGEgLQJ9T0rS+1tl7lZ/RNIvLATJhb18iKhCQEyIj5Begdl6dmBIZ8sJqJz8KiE4Bm7qRuWN5sOT+Ca3gc/F7YzQQSohOg4MwoVGYEMGEsK0qEvPcqMzGFRCdAUVwYK7gzoQdNMfcYdY4JXUgJ0QlQHM8J0NLbfRbsaHkq+7HoBCiKD5XdSGYUEGYN+8/ey4w6pIioBKgtcUJFmjwTHZhC+aEyjsE3Yd3jHlOqiUqAiow+os3tYwpFPZWhxSasJVEJUKNHlE3WABFTH2fhAZC1JCoBaotcWLNXVn1sPAtkbb+9xkUlAO8BWkNuxdj9n7QTlQC8B2hNAI1lbesMBs5EJUBjg6jVMwCfYSrUlhoWjsaw2o0rLaqHaMUNT80IGLO+sVlanCORCWDYkm1xTGSn1vK0QHaUeagJJ4CELilJlH7hR2fmiUwAfgto6QAWh8VFJYC1V35L+1V/XJXpj5Lkx5nCQVQC0Dx3CpvCUzMCOfufbz5h4EhUAlAcfZte3TO6lrG+y9w1A7QVHStJVAKQkY6P6Y7TwwoIUupRkdoX2fumSNlkh22JTgDtbaBDFdR3kTalotXRLCTRCUDRt3hqjUBJUgjStyxunSnTmegE8Bj1nUymsd3sVSEIlpTLwHWhIToBnAZdg6WL8SHadCmu9HwKU3/phR2gaGRyJtEJQMue3J88L6eNzLZdePZJxAmBMuRMohOAjOO3Ad0uzoqdhaRf/1W2h0JJCOA59hvdCPAr2pVTF6fvQv0DwyOQmgqfJARwDUsEbbjAk24EaITw9BOnUXJlkO5CIlyRhACkt8/UL0RQv3tVWSxEKD05/AKu/eFNIRK6gyTGSUYA32kHJDFI6Y1QDAWKpHLEL00bL/lBnhCJXMQkGQFch1+GXZ/bIprSvaqm5fQ3161CqRBEUswkGQHICP+5u8S0pdvV7dj/hkEbVxgDgKQEoJ22aTMFnvRDoN8rG/QraEIpSQlAGy/1ev6gCeqqR9TGO1e72YXYFktKADKm74rNYtvULeoPXrMO5jbVotsiOQE8hGFh56FJohum5AZs/bIQsChGEhMkJwBZ1V/YLoUn3Qj0f/0dyaKHyUIA3ykHQTHyeGqLAI2Y9nnxk7YXRMqRhQBky6C/vCGSScquNkQIH6exlO5NSTYCeP30JHqMPq1sb3Wx9j1/dgS9Jh/p4lo7rk42ApBag9dHA+b1HWuokqsamwcYsmGV5NbKSgDXYQno95uNkhvNYoPBr62HQ2CG5KpJFi1cl2W0K+fxwfGqDiblIASPGpsYLsl7/6N+kLUHIGXMbR8gbKt6N47SWFdj5J45sjif8JedAKQEbeQc9KsP6VB1afC6aLgMvSKb3bLfAposb6i1wJkxR0ETJdWS6Kn/icPyrhVkogcgh9O7b8S+2bDxyVGF/2nN5PCPF8luKzMEICRsvO4i8vMZoFei7pw0tlWIEDaLsPYolN1MpghAaLiNiEPUoandlwSaBowQtoZzj7gku/NJAeYIQEp5jTuBKOHeSP+UbpWEWAlhW5bD57l/M2MWkwQgdLyEtQT0gGThWMYMWCYpIvzzh8UsET7z/tOkarpamJm3AF2G0U6jF2fEoiShazZVpvdul7AEuAldMEXvJoJZOpVCI2zeVJ3viao73qi83RuF5yOFcC7CHP0uiHVo7lCO8E8XMPXPb8KbeQKQovXVVkhe/Q7SNi1r0tugX3KA36y96D1/p7Ataxw0VvpF7Kwpdkb+1+ORuXM28o+NRWOd4Rut0ihfpLAppNPAFIN0lqqwIgjQBMb9S8Nxfe0a5H05sSmrw1/n0EQELt0mbNKwB6YGrazK6YVUYe9D2vhanx3QqKeh8f3gaGFql8xbw3UEkqII0GRIccIQrSMKz0X9f958i26aXiHpU3PAkm3wnnS0SaTLfqlXSPv7MmTtnoGy6/3b1GvdMw8BCz9GwOIY2Pllt7nOWoYiCdASRIq/W3ptoLDO3krYjaMA9kHpkv3jym70Q1lKMGpLnWBuVwmXIclC+2nM7AjWEiddx4ongC7DeL5+CDD7Gqif+ryUqQhwApiKoMLlOQEU7kBT1ecEMBVBhctzAijcgaaqzwlgKoIKl+cEULgDTVWfE8BUBBUuzwmgcAeaqj4ngKkIKlyeE0DhDjRVfU4AUxFUuDwngMIdaKr6nACmIqhweU4AhTvQVPX/B7+LJxLB2zHEAAAAAElFTkSuQmCC',


Do you think it would be possible to use real images instead with more test cases?

jacekbogdanski · 2020-10-26T12:50:59Z

tests/plugins/pastefromword/manual/pasteimagessafari.html

+	if ( !CKEDITOR.env.safari ) {
+		bender.ignore();
+	}


Maybe it makes sense to keep this test also for different browsers to make sure we didn't introduce regression?

All other browsers are covered by unit tests for Paste Tools, Paste from Word and Paste from Libre Office, so I don't think it would be necessary.

We still need to perform manual tests for images during the testing phase to make sure that we didn't introduce any regression. Enabling these tests on browsers which should support images significantly improves odds that we will actually test it, as generic tests may not have enough test samples.

Please, also ignore this test on mobile.

jacekbogdanski · 2020-10-26T12:51:51Z

tests/plugins/pastefromword/manual/pasteimagessafari.md

+@bender-ckeditor-plugins: wysiwygarea, toolbar, undo, basicstyles, pastefromword, elementspath, image, format, sourcearea
+
+1. Open [attached Word document](_assets/Image_safari.docx).


It would be good to add a more complicated document with more image sources, especially gif and different flavors of jpg.

How can I produce different flavors of JPG? 🤔

Actually we probably don't need to test different flavors of JPG, as all of its signatures starts with the same sequence: FF D8 FF. Checking if the image file starts with it should be enough for our case.

How can I produce different flavors of JPG?

By using some conversion tool. But indeed making signature-less strict should work. I've checked File Signature Database and it doesn't seem that a less specific signature will catch any other filetype 🤞🏻

jacekbogdanski · 2020-10-26T13:42:32Z

I also see that there are some jsduck errors

Please, update docs to match the correct types.

Comandeer · 2020-10-26T13:44:22Z

Please, update docs to match the correct types.

Hmm, that can be a little problematic, as Uint8Array is correct… Seems like jsduck is old enough to not recognize Typed Arrays 🤔

github-actions · 2020-11-03T09:09:13Z

It's been a while since we last heard from you. We are marking this pull request as stale due to inactivity. Please provide the requested feedback or the pull request will be closed after next 7 days.

Comandeer · 2020-11-08T17:43:16Z

Ok,

support for GIFs added by adding them to the array of supported image types,
tests with real images added,
I've also added throwing error when the image in blob is in unsupported format (reused the error introduced in Improve importing images from Microsoft Word #4291,
added CKEDITOR.tools.array.unique(),
some refactoring.

jacekbogdanski

Hmm, that can be a little problematic, as Uint8Array is correct… Seems like jsduck is old enough to not recognize Typed Arrays 🤔

I see that Blob is correctly recognized, so it's indeed weird that jsduck is not able to read this type. Maybe there is some workaround for custom type? If not, I suppose we will have to rename it into Object to avoid building warnings? Although, it's pity going some weird workarounds because the tooling doesn't work correctly.

Currently, ajax methods are a bit mixed up. Previously, we had:

load - loading with text response
loadXml - loading with xml response

now we have:

load - by default text, but also supports xml and arraybuffer
loadXml - still supporting only xml

Maybe, we should add explicit loadBinaryData/loadData? It probably doesn't really make sense deprecating loadXml as explicitness of this method is actually nice. Most readable seems to have load (without default type), loadText, loadXml, loadBinaryData but it's a bit too late for that.

jacekbogdanski · 2020-11-09T13:14:31Z

core/tools.js

+			 * 	arrayWithoutDuplicates = CKEDITOR.tools.array.unique( array );
+			 * console.log( arrayWithoutDuplicates ); // [ 1, 2, 3 ]
+			 * ```
+			 *


Missing @since tag and @member CKEDITOR.tools.array.

jacekbogdanski · 2020-11-10T10:30:22Z

plugins/pastetools/filter/image.js

@@ -11,7 +11,9 @@
 	'use strict';

 	/**
-	 * Filter handling pasting images from RTF content.
+	 * Filter handling pasting images. In Safari images are extracted


Do we have any strict check if it's actually Safari? I suppose it should rather tell that in case of missing RTF it will be using this method?

jacekbogdanski · 2020-11-10T11:52:17Z

tests/plugins/pastefromword/manual/imagesgif.md

+
+## Unexpected
+
+* Iage is not rendered correctly.


jacekbogdanski · 2020-11-10T11:55:29Z

tests/plugins/pastefromword/manual/pasteimagessafari.html

+	if ( !CKEDITOR.env.safari ) {
+		bender.ignore();
+	}


We still need to perform manual tests for images during the testing phase to make sure that we didn't introduce any regression. Enabling these tests on browsers which should support images significantly improves odds that we will actually test it, as generic tests may not have enough test samples.

Please, also ignore this test on mobile.

jacekbogdanski · 2020-11-10T12:17:24Z

tests/plugins/pastefromword/manual/pasteimagessafari.md

+@bender-ckeditor-plugins: wysiwygarea, toolbar, undo, basicstyles, pastefromword, elementspath, image, format, sourcearea
+
+1. Open [attached Word document](_assets/Image_safari.docx).


How can I produce different flavors of JPG?

By using some conversion tool. But indeed making signature-less strict should work. I've checked File Signature Database and it doesn't seem that a less specific signature will catch any other filetype 🤞🏻

jacekbogdanski · 2020-11-10T12:20:11Z

plugins/pastetools/filter/image.js

+	function convertBlobUrlToBase64( blobUrlSrc ) {
+		return new CKEDITOR.tools.promise( function( resolve ) {
+			CKEDITOR.ajax.load( blobUrlSrc, function( arrayBuffer ) {
+				var data = new Uint8Array( arrayBuffer ),


I'm curious about old IE support here (IE8-9) as Uint8Array is not supported there. However, nothing breaks - seems like we are not supporting image pasting there at all.

Yes, in old IEs this code is just ignored.

jacekbogdanski · 2020-11-10T12:29:40Z

tests/plugins/pastefromword/manual/imagesgif.md

@@ -0,0 +1,16 @@
+@bender-tags: feature, 4.16.0, 1134, 2800, pastefromword
+@bender-ui: collapsed
+@bender-ckeditor-plugins: wysiwygarea, toolbar, undo, pastefromword, image, pagebreak, autogrow


It would be good to add sourcearea test steps similar to pasteimagessafari manual test to make sure that the image is transformed to base64.

jacekbogdanski · 2020-11-10T12:36:35Z

plugins/pastetools/filter/image.js

@@ -179,6 +164,111 @@
 		}
 	];

+	/**
+	 * Array of all recognizable image file signatrues with their respective types.


Suggested change

* Array of all recognizable image file signatrues with their respective types.

* Recognizable image file signatures with their respective types.

jacekbogdanski · 2020-11-10T13:20:59Z

tests/plugins/pastefromword/_helpers/blob.js

+		var image = options && options.image || SAMPLE_PNG,
+			imageDataUrl = options && options.imageDataUrl || SAMPLE_PNG,
+			url = getObjectUrl( image, options && options.imageType ),
+			template = options && options.template || '<p{%CLASS%}>Hello<img style="height:200px; width:200px" src="{%URL%}" />world.</p>',
+			input = template.replace( /{%URL%}/g, url ).replace( /{%CLASS%}/g, ' class="MsoNormal"' ),
+			expected = template.replace( /{%URL%}/g, imageDataUrl ).replace( /{%CLASS%}/g, '' );


The code is a bit unreadable with all default option values. Maybe we could simplify it with some merging logic?

CKEDITOR.tools.object.merge( defaultOptions, options || {} );

jacekbogdanski · 2020-11-10T13:52:11Z

plugins/pastetools/filter/image.js

+		return new CKEDITOR.tools.promise( function( resolve ) {
+			CKEDITOR.ajax.load( blobUrlSrc, function( arrayBuffer ) {
+				var data = new Uint8Array( arrayBuffer ),
+					imageType = getImageTypeFromSignature( data.subarray( 0, 4 ) ),


Shouldn't we pass here the whole arrayBuffer? It seems like taking the first 4 characters is already done in getImageTypeFromSignature function? In that case, this should be probably renamed again to just getImageType.

jacekbogdanski · 2020-11-10T14:04:26Z

Could you also rebase PR onto the latest major? Seems like there are some conficts.

Comandeer · 2020-11-16T17:11:33Z

Ok, I've addressed issues found during the review.

Maybe, we should add explicit loadBinaryData/loadData? It probably doesn't really make sense deprecating loadXml as explicitness of this method is actually nice. Most readable seems to have load (without default type), loadText, loadXml, loadBinaryData but it's a bit too late for that.

TBH I don't find it's too late. We can always have additional methods and load with a default response type.

I'll raise an issue for it.

…gnature.

…nversion of TypedArray is no longer needed.

jacekbogdanski

Good job!

jacekbogdanski self-requested a review October 25, 2020 09:04

jacekbogdanski self-assigned this Oct 25, 2020

jacekbogdanski requested changes Oct 26, 2020

View reviewed changes

CKEditorBot unassigned jacekbogdanski Oct 26, 2020

github-actions bot added the stale The issue / PR will be closed within the next 7 days of inactivity. label Nov 3, 2020

Comandeer self-assigned this Nov 3, 2020

Comandeer requested a review from jacekbogdanski November 8, 2020 17:43

CKEditorBot unassigned Comandeer Nov 8, 2020

Comandeer mentioned this pull request Nov 8, 2020

Rephrase info about unsupported image error ckeditor/ckeditor4-docs#333

Merged

github-actions bot removed the stale The issue / PR will be closed within the next 7 days of inactivity. label Nov 9, 2020

jacekbogdanski self-assigned this Nov 9, 2020

jacekbogdanski requested changes Nov 10, 2020

View reviewed changes

CKEditorBot unassigned jacekbogdanski Nov 10, 2020

Comandeer self-assigned this Nov 16, 2020

Comandeer force-pushed the t/1134-b branch from 010f9aa to 4bb8c24 Compare November 16, 2020 17:10

Comandeer requested a review from jacekbogdanski November 16, 2020 17:11

CKEditorBot unassigned Comandeer Nov 16, 2020

jacekbogdanski self-assigned this Nov 23, 2020

Comandeer added 6 commits November 23, 2020 11:10

Import PfW tests from #1976.

246f04f

Introduce blob flow for CKEDITOR.pasteFilters.image.

cde6879

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

d8076d0

Import changes to Ajax plugin from #1976.

a0cbde9

Add tests for blob image helpers.

8e28281

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

8a4ccd8

Comandeer and others added 18 commits November 23, 2020 11:10

Ignore test in not supported environments.

dc1d1ee

Fix license header in Ajax plugin.

7ed7c65

Move removeDuplicates to CKEDITOR.tools.array.unique.

b7ece7c

Add tests for pasting GIFs.

9218ccc

Add GIF to list of supported image formats.

4fbcca5

Refactor getImageTypeFromHeader and rename it into getImageTypeFromSi…

eab066b

…gnature.

Use map from CKEDITOR.tools – it forces normal array so additional co…

86d62cb

…nversion of TypedArray is no longer needed.

Add tests for pasting real images as blobs.

3fbe17d

Add error when blob image is in unsupported format.

8e459d4

Add missing tests for Ajax plugin.

260d000

Code styles and API docs fixes.

d3c1022

Add JPG and GIF images to manual test fixture.

a68a54d

Update API docs.

79a3753

Update manual tests.

cc68de3

Update tests helper.

a5ed292

Remove unnecessary array trimming.

7008f41

Corrected array.unique docs.

f2d7521

Added changelog entry.

8e6392c

jacekbogdanski force-pushed the t/1134-b branch from a869c01 to 8e6392c Compare November 23, 2020 10:11

jacekbogdanski approved these changes Nov 23, 2020

View reviewed changes

CKEditorBot unassigned jacekbogdanski Nov 23, 2020

jacekbogdanski merged commit 0400081 into major Nov 23, 2020

jacekbogdanski deleted the t/1134-b branch November 23, 2020 10:21

CKEditorBot mentioned this pull request Nov 23, 2020

Integration PFW Image with Safari #1134

Closed

jacekbogdanski mentioned this pull request Nov 23, 2020

Explicit Ajax helpers for new response types #4394

Closed

	* synchronously. Please notice that only text data might be loaded synchrnously.
	* synchronously. Please notice that only text data might be loaded synchronously.

		@bender-ckeditor-plugins: wysiwygarea, toolbar, undo, basicstyles, pastefromword, elementspath, image, format, sourcearea

		1. Open [attached Word document](_assets/Image_safari.docx).

	* Array of all recognizable image file signatrues with their respective types.
	* Recognizable image file signatures with their respective types.

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

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

Conversation

Comandeer commented Oct 19, 2020

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

Did you follow the CKEditor 4 code style guide?

What is the proposed changelog entry for this pull request?

What changes did you make?

Which issues does your PR resolve?

jacekbogdanski 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

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

jacekbogdanski commented Oct 26, 2020

Comandeer commented Oct 26, 2020

github-actions bot commented Nov 3, 2020

Comandeer commented Nov 8, 2020 • edited Loading

jacekbogdanski 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

Choose a reason for hiding this comment

jacekbogdanski commented Nov 10, 2020

Comandeer commented Nov 16, 2020

jacekbogdanski left a comment

Choose a reason for hiding this comment

Comandeer commented Nov 8, 2020 •

edited

Loading