Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use XHR as possible uploading mechanism in filebrowser plugin #1352

Merged
merged 28 commits into from
Dec 22, 2017
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
9791777
Some working solution for using XHR with filebrowser.
Dec 12, 2017
7bf2375
Few improvements to using XHR with uploading images.
Dec 13, 2017
f3449bd
Unit test for adding XHR headers.
Dec 13, 2017
56259ab
Change condition to be independent from clipboard plugin.
Dec 15, 2017
b491787
Unit test for uploading images.
Dec 15, 2017
156fb10
Rename config flag, add docs.
Dec 15, 2017
ef08686
Code optimization, adding feature detection for file uploading.
Dec 15, 2017
ccfffa1
Preserve backward compatybility with returned data formats.
Dec 15, 2017
9642a81
Little changes in unit tests.
Dec 18, 2017
5fcd547
Manual tests for XHR upload.
Dec 18, 2017
9985680
Review fixes: chnage in behaviour to have 'form' as default action, b…
Dec 19, 2017
4cd6c1f
Review fixes: add error handling to loader, provide extra unit and ma…
Dec 20, 2017
f9f54df
Simplified file upload api detection.
mlewand Dec 20, 2017
40a1d06
Test: simplified manual test for xhr upload in filebrowser plugin.
mlewand Dec 20, 2017
d2da180
Tests: adjusted manual tests for 404 errors using XHR in filebrowser …
mlewand Dec 20, 2017
1886055
Docs: corrections for config.filebrowserUploadMethod API docs.
mlewand Dec 20, 2017
f299c36
Docs: adjusted the docs for a config property allowing to customize r…
mlewand Dec 20, 2017
ce4afc2
Renamed xmlHttpRequestHeaders config to fileTools_requestHeaders.
mlewand Dec 20, 2017
c2c7565
Docs: adjusted ticket reference.
mlewand Dec 21, 2017
cfd75e8
Simplified feature detection for file uploading over XHR. Minor other…
mlewand Dec 21, 2017
2c7bf71
Changelog entry.
mlewand Dec 21, 2017
4c95176
Tests: corrected file browser xhr upload manual tests.
mlewand Dec 21, 2017
8276465
Fixed unit tests for Edge, IEs.
mlewand Dec 21, 2017
76cacbe
Extracted common workarounds.
mlewand Dec 21, 2017
1eeab6f
Tests: removed dummy assertion.
mlewand Dec 22, 2017
5a577fe
Tests: corrected language config property.
mlewand Dec 22, 2017
cd36fe8
Tests: corrected manual test expected result.
mlewand Dec 22, 2017
1140dbe
Merge pull request #1369 from ckeditor/t/643
Comandeer Dec 22, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions plugins/dialogui/plugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -743,8 +743,15 @@ CKEDITOR.plugins.add( 'dialogui', {
myDefinition.className = ( myDefinition.className ? myDefinition.className + ' ' : '' ) + 'cke_dialog_ui_button';
myDefinition.onClick = function( evt ) {
var target = elementDefinition[ 'for' ]; // [ pageId, elementId ]
if ( !onClick || onClick.call( this, evt ) !== false ) {
dialog.getContentElement( target[ 0 ], target[ 1 ] ).submit();

// If exists onClick in elementDefinition, then it is called and checked response type.
// If it's possible, then XHR is used, what prevents of using submit.
var responseType = onClick ? onClick.call( this, evt ) : false;

if ( responseType !== false ) {
if ( responseType !== 'xhr' ) {
dialog.getContentElement( target[ 0 ], target[ 1 ] ).submit();
}
this.disable();
}
};
Expand Down
73 changes: 62 additions & 11 deletions plugins/filebrowser/plugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@
*/

( function() {
'use strict';
// Default input element name for CSRF protection token.
var TOKEN_INPUT_NAME = 'ckCsrfToken';

Expand Down Expand Up @@ -200,7 +201,7 @@
}
}

// The onlick function assigned to the 'Upload' button. Makes the final
// The onclick function assigned to the 'Upload' button. Makes the final
// decision whether form is really submitted and updates target field when
// file is uploaded.
//
Expand Down Expand Up @@ -296,22 +297,43 @@

if ( url ) {
var onClick = element.onClick;

// "element" here means the definition object, so we need to find the correct
// button to scope the event call
element.onClick = function( evt ) {
// "element" here means the definition object, so we need to find the correct
// button to scope the event call
var sender = evt.sender;
if ( onClick && onClick.call( sender, evt ) === false )
var fileInput = sender.getDialog().getContentElement( this[ 'for' ][ 0 ], this[ 'for' ][ 1 ] ).getInputElement();

if ( onClick && onClick.call( sender, evt ) === false ) {
return false;
}

if ( uploadFile.call( sender, evt ) ) {
var fileInput = sender.getDialog().getContentElement( this[ 'for' ][ 0 ], this[ 'for' ][ 1 ] ).getInputElement();

// Append token preventing CSRF attacks.
appendToken( fileInput );
return true;
// Backward compatibility for IE8 and IE9 (https://cksource.tpondemand.com/entity/3117).
// With version 4.9.0 change: `!== 'xhr'` to `=== 'form'`.
if ( editor.config.filebrowserUploadMethod !== 'xhr' || !( CKEDITOR.fileTools && CKEDITOR.fileTools.isFileUploadSupported ) ) {
// Append token preventing CSRF attacks.
appendToken( fileInput );
return true;

} else {
var loader = editor.uploadRepository.create( fileInput.$.files[ 0 ] );

loader.on( 'uploaded', function( evt ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 🙂

var response = evt.sender.responseData;
setUrl.call( evt.sender.editor, response.url, response.message );
} );

// Return non-false value will disable fileButton in dialogui,
// below listeners takes care of such situation and re-enable "send" button.
loader.on( 'error', xhrUploadErrorHandler.bind( this ) );
loader.on( 'abort', xhrUploadErrorHandler.bind( this ) );

loader.loadAndUpload( url );

return 'xhr';
}
}


return false;
};

Expand All @@ -323,6 +345,12 @@
}
}

function xhrUploadErrorHandler( evt ) {
// `this` is a reference to ui.dialog.fileButton.
this.enable();
alert( evt.sender.message ); // jshint ignore:line
}

// Updates the target element with the url of uploaded/selected file.
//
// @param {String}
Expand Down Expand Up @@ -571,3 +599,26 @@
* @cfg {Number/String} [filebrowserWindowHeight='70%']
* @member CKEDITOR.config
*/

/**
* Define preferred option to submit file upload with filebrowser plugin. If value is not specified,
* `'form'` option is used. Additional XHR headers might be set up with {@link CKEDITOR.config#xmlHttpRequestHeaders}.
*
* Available values:
*
* * `'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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo -> undefined.

*
* Note: please be aware that `'xhr'` requires {@link CKEDITOR.fileTools} for proper work. Missing this plugins
* or using browsers which isn't supported {@link CKEDITOR.fileTools#isFileUploadSupported},
* will force using `'form'` behaviour despite configuration option.
*
* // Modern browsers will use XMLHttpRequest to upload files.
* // IE8 and IE9 will use form submit even tough config option is set to 'xhr'.
* config.filebrowserUploadMethod = 'xhr';
*
* @since 4.8.1
* @cfg {Boolean} [filebrowserUploadMethod=undefined]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* @member CKEDITOR.config
*/
43 changes: 41 additions & 2 deletions plugins/filetools/plugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,9 @@
editor.on( 'fileUploadRequest', function( evt ) {
var fileLoader = evt.data.fileLoader,
$formData = new FormData(),
requestData = evt.data.requestData;
requestData = evt.data.requestData,
configXHRHeaders = editor.config.xmlHttpRequestHeaders,
header;

for ( var name in requestData ) {
var value = requestData[ name ];
Expand All @@ -64,6 +66,12 @@
// Append token preventing CSRF attacks.
$formData.append( 'ckCsrfToken', CKEDITOR.tools.getCsrfToken() );

if ( configXHRHeaders ) {
for ( header in configXHRHeaders ) {
fileLoader.xhr.setRequestHeader( header, configXHRHeaders[ header ] );
}
}

fileLoader.xhr.send( $formData );
}, null, null, 999 );

Expand Down Expand Up @@ -856,7 +864,25 @@
*/
isTypeSupported: function( file, supportedTypes ) {
return !!file.type.match( supportedTypes );
}
},

/**
* Property inform if current browser poses native methods used by {@link CKEDITOR.fileTools} to upload files.
*
* @property {Boolean} isFileUploadSupported
*/
isFileUploadSupported: ( function() {
if ( typeof FileReader === 'function' &&
typeof ( new FileReader() ).readAsDataURL === 'function' &&
typeof FormData === 'function' &&
typeof ( new FormData() ).append === 'function' &&
typeof XMLHttpRequest === 'function' &&
typeof Blob === 'function' ) {

return true;
}
return false;
} )()
} );
} )();

Expand Down Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

*
* config.xmlHttpRequestHeaders = {
* 'Cache-Control': 'no-cache',
* 'X-CUSTOM': 'HEADER'
* };
*
* @since 4.8.1
* @cfg {Object} [xmlHttpRequestHeaders=null]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* @member CKEDITOR.config
*/
48 changes: 48 additions & 0 deletions tests/plugins/filebrowser/manual/uploadxhr.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
<textarea id="classic">

</textarea>

<div id="output" style="background-color:lightgreen;"></div>

<script>
var xhr, requests;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not specify multiple variables in a single line.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing dot.

xhr = sinon.useFakeXMLHttpRequest();
requests = [];
xhr.onCreate = function( xhr ) {
requests.push( xhr );
};
window.onunload = function() {
xhr.restore();
}
}

var classic = CKEDITOR.replace( 'classic', {
xmlHttpRequestHeaders: {
'hello': 'world',
'foo': 'bar'
},
filebrowserUploadUrl: 'fake-url',
filebrowserUploadMethod: 'xhr'
} );

// Display XHR details when CKEDITOR process entire request.
classic.on( 'fileUploadRequest', function( evt ) {
var output = document.getElementById( 'output' );
var outputString = CKEDITOR.tools.array.reduce( requests, function( acc, item ) {
var line = '';
for ( header in item.requestHeaders ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

I was able to get:

image

instead of

image

Which seems to be a bit more readable.

if ( item.requestHeaders.hasOwnProperty( header ) ) {
line += ' | <code>header: ' + header + ', value: ' + item.requestHeaders[ header ] + '</code> |';
}
}
acc += '<li>' + line + '</li>';
return acc;
}, '' );
output.innerHTML = '<ol>' + outputString + '</ol>';
}, null, null, 1000 )
</script>
18 changes: 18 additions & 0 deletions tests/plugins/filebrowser/manual/uploadxhr.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
@bender-tags: 4.8.1, feature, tp3117
@bender-ui: collapsed
@bender-ckeditor-plugins: wysiwygarea, toolbar, filebrowser, filetools, image, link, flash

----
1. Open image dialog
2. Go to upload tab
3. Select some image and send it to server
4. Close dialog (There might appear warning that image url is not set up)

Repeat those steps for Link and Flash plugin.

_Note:_ When new upload request is made, it should be visible as separate line below.

**Expected:** Below editor will appear green div with listed headers attempted to send. There are 2 headers in single line:
`hello: world, foo: bar`.

**Unexpected:** Headers are not listed below.
18 changes: 18 additions & 0 deletions tests/plugins/filebrowser/manual/uploadxhrwitherror.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<textarea id="classic">

</textarea>

<div id="output" style="background-color:lightgreen;"></div>

<script>
var xhr, requests;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused variables.


if ( CKEDITOR.env.ie && CKEDITOR.env.version < 10 ) {
bender.ignore();
}

var classic = CKEDITOR.replace( 'classic', {
filebrowserUploadUrl: 'fake-url',
filebrowserUploadMethod: 'xhr'
} );
</script>
16 changes: 16 additions & 0 deletions tests/plugins/filebrowser/manual/uploadxhrwitherror.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
@bender-tags: 4.8.1, feature, tp3117
@bender-ui: collapsed
@bender-ckeditor-plugins: wysiwygarea, toolbar, filebrowser, filetools, image, link, flash

----
1. Open image dialog
2. Go to upload tab
3. Select some image and send it to server
4. **Alert** should be displayed
5. Try to upload file again

Repeat those steps for Link and Flash plugin.

**Expected:** After first upload attempt, send button is enabled. Which means that every next click trigger new uploading process and generate new alert message.

**Unexpected:** After first upload send button is disabled. Second and further upload attempts don't trigger upload process, and alert is not shown.
Loading