Use XHR as possible uploading mechanism in filebrowser plugin #1352

msamsel · 2017-12-15T15:50:25Z

What is the purpose of this pull request?

New feature

Does your PR contain necessary tests?

All patches which 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

What changes did you make?

change way how filebrowser upload files - prefer using XHR
allow on providing custom headers to XHR used when files are uploaded
add config flag which will force to use form submit behaviour when files are uploaded

Closes #643.

mlewand

Let's revisit configuration:

We could add this feature (sending file browser stuff over XHR) in a minor release (4.8.1), but we can't change the default behavior in a minor release.

That's why, for now, the default behavior should be to send <form> using element, as it was. But it should be possible to change this by setting a configuration option to 'xhr'.

Another task will be to use xhr by default in a next major release - 4.9, while still allowing the config variable to enforce old <form> mode.

So that requires us to change the config a bit, it should work the following way:

filebrowser_forceFormSubmit should be renamed to filebrowserUploadMethod. API docs should explain that for wide browser support CKEditor provides multiple uploading strategies and this option can allow you to force usage of a given strategy.

The default strat. is a form strategy.

This config can take following values:

'form' - files are upload by sending traditional HTML <form>
'xhr' - files are uploaded using XHR
null (default value) - the default upload method applies

It also must include @since tag

There are no API docs for config.xmlHttpRequestHeaders option. Please don't forget about @since tag.

mlewand · 2017-12-18T14:38:18Z

plugins/filebrowser/plugin.js


 						if ( uploadFile.call( sender, evt ) ) {
-							var fileInput = sender.getDialog().getContentElement( this[ 'for' ][ 0 ], this[ 'for' ][ 1 ] ).getInputElement();
+							// Backward compatibility for IE8 and IE9 (https://cksource.tpondemand.com/entity/3117).
+							if ( editor.config.filebrowser_forceFormSubmit || !CKEDITOR.fileTools.isFileUploadSupported ) {


This line will break if no filetools plugin is added. Following exception will be thrown:

uploadxhr:105 Uncaught TypeError: Cannot read property 'isFileUploadSupported' of undefined at Object.<anonymous> (uploadxhr:105) at Object.listenerFirer (event.js:144) at Object.CKEDITOR.event.fire (event.js:290) at Object.<anonymous> (editor.js:625) at doCallback (scriptloader.js:70) at Array.checkLoaded (scriptloader.js:84) at onLoad (scriptloader.js:98) at scriptloader.js:135

Just remove filetools plugin from uploadxhr manual test and follow the steps in the description. You'll see that it will throw this error.

mlewand · 2017-12-19T11:22:07Z

plugins/filebrowser/plugin.js

+							} else {
+								var loader = editor.uploadRepository.create( fileInput.$.files[ 0 ] );
+
+								loader.on( 'uploaded', function( evt ) {


We also need to provide an error handling. E.g. user is connected to the wifi, and he losts the connection, upload returns 404.

We can simplify it to showing an alert. I'll save you some time, and here is the snippet for it:

loader.on( 'error', function( evt ) { alert( this.message ); } );

however you still need to enable the upload button (note that it gets disabled).

Please add a separate unit and manual test for the above. You could just stub window.alert and then ensure it was called with an error message.

Remember to set config.language to 'en' in the unit test, otherwise person opening on a different locale could have alert called with message other than expected. 🙂

…ig changes to docs related to the feature.

…nual test, code clean up.

…plugin.

…equest headers.

mlewand · 2017-12-20T14:22:45Z

plugins/filebrowser/plugin.js

+ *
+ *	* `'xhr'` - XMLHttpRequest is used to upload file
+ *	* `'form'` - Form submit is used to upload file
+ *	* `undefiend` - when configuration option is not specified `'form'` configuration is used


Typo -> undefined.

mlewand · 2017-12-20T14:23:31Z

plugins/filebrowser/plugin.js

+ * 	config.filebrowserUploadMethod = 'xhr';
+ *
+ * @since 4.8.1
+ * @cfg {Boolean} [filebrowserUploadMethod=undefined]


Wrong type. This is not a bool but a string. Also I have asked for a null value rather than undefined - I'll correct that.

mlewand · 2017-12-20T14:24:48Z

tests/plugins/filebrowser/manual/uploadxhr.html

+<div id="output" style="background-color:lightgreen;"></div>
+
+<script>
+	var xhr, requests;


Please do not specify multiple variables in a single line.

mlewand · 2017-12-20T14:35:13Z

tests/plugins/filebrowser/manual/uploadxhr.html

+	if ( CKEDITOR.env.ie && CKEDITOR.env.version < 10 ) {
+		bender.ignore();
+	} else {
+		// Mock XHR


Missing dot.

mlewand · 2017-12-20T14:53:40Z

tests/plugins/filebrowser/manual/uploadxhr.html

+		var output = document.getElementById( 'output' );
+		var outputString = CKEDITOR.tools.array.reduce( requests, function( acc, item ) {
+			var line = '';
+			for ( header in item.requestHeaders ) {


I think it could be much simplified, you could just add JSON.stringify( item.requestHeaders ) 🙂

I was able to get:

instead of

Which seems to be a bit more readable.

mlewand · 2017-12-20T15:01:05Z

tests/plugins/filebrowser/manual/uploadxhrwitherror.html

+<div id="output" style="background-color:lightgreen;"></div>
+
+<script>
+	var xhr, requests;


Unused variables.

mlewand · 2017-12-20T15:46:04Z

plugins/filetools/plugin.js

@@ -885,3 +911,16 @@
 * @cfg {String} [fileTools_defaultFileName='']
 * @member CKEDITOR.config
 */
+
+/**
+ * Additional headers of XMLHttpRequest used during file upload with plugins: {@link CKEDITOR.fileTools} and filebrowser.


These headers are not added only for file uploading but any request made by filetools.

mlewand · 2017-12-20T16:09:08Z

plugins/filetools/plugin.js

+ * 	};
+ *
+ * @since 4.8.1
+ * @cfg {Object} [xmlHttpRequestHeaders=null]


This config property should be prefixed with fileTools_ just like the other filetools config property.

mlewand

Mistakenly gave a wrong review result for the previous one, sorry.

mlewand · 2017-12-20T16:10:22Z

I'll take over this one from here.

… refactorings.

Corrections to XHR support for File Browser plugin uploads

Comandeer

LGTM!

msamsel changed the title ~~Use XHR as default mechanism of uploading in filebrowser plugin~~ Use XHR as default uploading mechanism in filebrowser plugin Dec 15, 2017

msamsel force-pushed the t/tp3117 branch 3 times, most recently from 42ece87 to c3c075b Compare December 18, 2017 10:28

mlewand self-requested a review December 18, 2017 14:34

mlewand suggested changes Dec 19, 2017

View reviewed changes

Mateusz Samsel added 11 commits December 20, 2017 11:53

Some working solution for using XHR with filebrowser.

9791777

Few improvements to using XHR with uploading images.

7bf2375

Unit test for adding XHR headers.

f3449bd

Change condition to be independent from clipboard plugin.

56259ab

Unit test for uploading images.

b491787

Rename config flag, add docs.

156fb10

Code optimization, adding feature detection for file uploading.

ef08686

Preserve backward compatybility with returned data formats.

ccfffa1

Little changes in unit tests.

9642a81

Manual tests for XHR upload.

5fcd547

Review fixes: chnage in behaviour to have 'form' as default action, b…

9985680

…ig changes to docs related to the feature.

msamsel force-pushed the t/tp3117 branch from 515cc5c to b4183d1 Compare December 20, 2017 10:53

Review fixes: add error handling to loader, provide extra unit and ma…

4cd6c1f

…nual test, code clean up.

msamsel force-pushed the t/tp3117 branch from b4183d1 to 4cd6c1f Compare December 20, 2017 10:55

msamsel changed the title ~~Use XHR as default uploading mechanism in filebrowser plugin~~ Use XHR as possible uploading mechanism in filebrowser plugin Dec 20, 2017

msamsel requested a review from mlewand December 20, 2017 11:49

mlewand added 6 commits December 20, 2017 15:40

Simplified file upload api detection.

f9f54df

Test: simplified manual test for xhr upload in filebrowser plugin.

40a1d06

Tests: adjusted manual tests for 404 errors using XHR in filebrowser …

d2da180

…plugin.

Docs: corrections for config.filebrowserUploadMethod API docs.

1886055

Docs: adjusted the docs for a config property allowing to customize r…

f299c36

…equest headers.

Renamed xmlHttpRequestHeaders config to fileTools_requestHeaders.

ce4afc2

mlewand approved these changes Dec 20, 2017

View reviewed changes

mlewand suggested changes Dec 20, 2017

View reviewed changes

mlewand added 2 commits December 21, 2017 10:46

Docs: adjusted ticket reference.

c2c7565

Simplified feature detection for file uploading over XHR. Minor other…

cfd75e8

… refactorings.

mlewand mentioned this pull request Dec 21, 2017

Corrections to XHR support for File Browser plugin uploads #1369

Merged

2 tasks

mlewand and others added 8 commits December 21, 2017 11:42

Changelog entry.

2c7bf71

Tests: corrected file browser xhr upload manual tests.

4c95176

Fixed unit tests for Edge, IEs.

8276465

Extracted common workarounds.

76cacbe

Tests: removed dummy assertion.

1eeab6f

Tests: corrected language config property.

5a577fe

Tests: corrected manual test expected result.

cd36fe8

Merge pull request #1369 from ckeditor/t/643

1140dbe

Corrections to XHR support for File Browser plugin uploads

Comandeer approved these changes Dec 22, 2017

View reviewed changes

mlewand approved these changes Dec 22, 2017

View reviewed changes

mlewand merged commit b849b9a into master Dec 22, 2017

mlewand deleted the t/tp3117 branch December 22, 2017 20:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use XHR as possible uploading mechanism in filebrowser plugin #1352

Use XHR as possible uploading mechanism in filebrowser plugin #1352

msamsel commented Dec 15, 2017 •

edited by mlewand

Loading

mlewand left a comment

mlewand Dec 18, 2017

mlewand Dec 19, 2017

mlewand Dec 19, 2017

mlewand Dec 19, 2017

mlewand Dec 20, 2017

mlewand Dec 20, 2017

mlewand Dec 20, 2017

mlewand Dec 20, 2017

mlewand Dec 20, 2017

mlewand Dec 20, 2017

mlewand Dec 20, 2017

mlewand Dec 20, 2017

mlewand left a comment

mlewand commented Dec 20, 2017

Comandeer left a comment

Use XHR as possible uploading mechanism in filebrowser plugin #1352

Use XHR as possible uploading mechanism in filebrowser plugin #1352

Conversation

msamsel commented Dec 15, 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?

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

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

mlewand left a comment

Choose a reason for hiding this comment

mlewand commented Dec 20, 2017

Comandeer left a comment

Choose a reason for hiding this comment

msamsel commented Dec 15, 2017 •

edited by mlewand

Loading