No progress during upload

[Piotr Jasiun]

When a file is dropped into the editor, progress notification show 0% and then jump to the 100%. It might be because browsers do not sent onprogress event but I see other libraries do it better. Maybe it is because we use xhr.onprogress and maybe we should use xhr2.upload.onprogress. Maybe the order of adding listeners is wrong (now they are added before xhr.open what seems to be correct but maybe it is not). Maybe it is because we show only progress of uploading, not loading, but in fact it is loading what is the most time consuming. Or maybe it is simple a bug. This should be investigated.

[Piotr Jasiun]

On the other hand everything works fine with the XHR mock, so the progress handling should be fine. It is an integration bug. It appears does not matter what is the size of the file and what browser I use.

[Jakub Ś]

Bonus: When you drop file and then deleted in the editor it is still uploaded to the server. Te reason is the same - no onprogess event is fired.

[Piotr Jasiun]

I tested it and using xhr.upload.onprogress instead on xhr.onprogress works much better on Chrome.

[Jonathan]

Tracking.

[Piotrek Koszuliński]

This was worrying me too. Curious what's the problem.

[Piotr Jasiun]

This seems to be matter of using xhr.upload.onprogress instead of xhr.onprogress. But the problem is that it is XHR2 feature and we need to check if it is supported on all browsers we handle (in this case: IE10+, Chrome, Firefox, Safari). And the testing may be a little tricky, because we need to slow down the upload to check if progress works fine with the real XHR (not a mock).

[Maciej]

For CKFinder 3 we're using xhr.upload.onprogress with check for xhr.upload existance.

[Szymon Kupś]

AFAIK xhr.onprogress method is used for monitoring data of request's response, so its not reporting uploaded file size but response size.

This is why FileLoader should use xhr.upload.onprogress if available and nothing else to have proper values in FileLoader.uploaded property. If it's not available no upload progress should be reported.

[Piotrek Koszuliński]

Sounds totally logical. There's a request and a response... and we've been listening on a progress on the response :D.

[Szymon Kupś]

Pushed ​branch:t/13533

Changed current upload progress reporting:

• Using xhr.upload.onprogress when is available.
• Introduced new FileLoader property called uploadSize. This property will hold upload payload size, which can be different than total file size. Before uploading file is embedded into FormData and that is why payload size is different than file size.
• Introduced uploadSizeRecieved event which is fired when uploadSize is received.

These modifications entailed changes in uploadwidget plugin:

• bindNotification function is creating tasks in asynchronous manner by listening to uploadSizeReceived event.

[Piotr Jasiun]

This solution unfortunately needs some more improvements:

• It was a good point that xhr.upload, not xhr, is the proper object for upload events. But beside xhr.upload.onprogress we should listen also on xhr.upload.abort and xhr.upload.error to remove uploadwidget not only when response errors occurred, but also on request errors.

• Why did not you create a simple function function createAggregator() but declare createAggregator and then assign function to it: createAggregator = function(). Also createAggregator function should be at the end of bindNotifications - generic code first, specific code later (​https://github.com/cksource/ckeditor-dev/blob/f1330df/plugins/uploadwidget/plugin.js#L446-L448).

• There is a mess then there is no xhr.upload. uploadSize is not set then, but it is used in two other places: onload and uploadSizeReceived. The fact that uploadSizeReceived is fired even if uploadSize is not set the reason of that bug. I think that loader.uploadSize should be set even if xhr.upload is no supported. It could be equal loader.total in such case. User may check if loader.uploaded == loader.uploadSize to know that the upload is done or we could show the progress. Of course in such case it will be a binary progress (0% or 100%) but it makes sense if user is uploading multiple files he will see the percentage of uploaded files thanks to aggregator.

• I am not sure if uploadSize is a good name. It sounds like the size of the uploaded data, not the total. I think that uploadTotal is better.

• I am not sure if we need uploadSizeReceived event. Having too many events is often reason of problems. You can listen on the update event and check if uploadSize is set and task is null and then create a task.

[Szymon Kupś]

Pushed ​branch:t/13533:

• Added error/abort handling to xhr.upload.
• createAggregator function expriession changed to function declaration and moved to the end of bindNotification.
• Changed uploadSize to uploadTotal. When xhr.upload is not available it has value of loader.total.
• Event uploadSizeReceived removed. As described in comment:15 - it is not necessary. Value of uploadTotal is set to null before data is available so it can be handled in update event listeners.
• Updated tests.

[Piotr Jasiun]

The solution looks good, but needs some more polishing.

Don't repeat yourself:

• ​https://github.com/cksource/ckeditor-dev/blob/b4dc79ff528b5b99b5817d4d5b2a6b911451c83d/plugins/filetools/plugin.js#L563-L580 and ​https://github.com/cksource/ckeditor-dev/blob/b4dc79ff528b5b99b5817d4d5b2a6b911451c83d/plugins/filetools/plugin.js#L530-L547 You can define these functions and use them twice.

Manual test:

• This solution support two situations: when xhr.upload is defined and when it is not. You created manual test only for the first one, as far as I can see. It would be usefull to have MT for the situation when there is no xhr.upload. Mark it with a 4.5.4 tag.

Code comments needed:

• there should be a ticket number at the end of the comment (for sure here ​https://github.com/cksource/ckeditor-dev/blob/b4dc79ff528b5b99b5817d4d5b2a6b911451c83d/plugins/filetools/plugin.js#L583, possibly in other not obvious places),
• uploadTotal description could be extend with the note that uploadTotal is set to be equal total if we can not get the real total upload file ​https://github.com/cksource/ckeditor-dev/blob/b4dc79ff528b5b99b5817d4d5b2a6b911451c83d/plugins/filetools/plugin.js#L58
• this condition definitely needs a comment: ​https://github.com/cksource/ckeditor-dev/blob/b4dc79ff528b5b99b5817d4d5b2a6b911451c83d/plugins/uploadwidget/plugin.js#L450

Code style:

• there should be not empty line here ​https://github.com/cksource/ckeditor-dev/blob/b4dc79ff528b5b99b5817d4d5b2a6b911451c83d/plugins/uploadwidget/plugin.js#L449

[Szymon Kupś]

Pushed ​branch:t/13533b.
Fixed issues pointed out in comment:19.

One more issue was found thanks to the manual test. When XHR.upload object is not available no update events are fired during the upload process. Because of that plugins using FileLoader are not informed about progress and cannot check if upload is still valid. To resolve this I've added one more update event which is called during XHR.onload callback.

[Piotrek Koszuliński]

Closed with ​git:g3a5b1ff2.