Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#13533 closed Bug (fixed)

No progress during upload

Reported by: Piotr Jasiun Owned by: Szymon Kupś
Priority: Normal Milestone: CKEditor 4.5.4
Component: General Version: 4.5.0 Beta
Keywords: Cc:

Description (last modified by 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.

Change History (22)

comment:1 Changed 3 years ago by 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.

comment:2 Changed 3 years ago by Piotr Jasiun

Description: modified (diff)

comment:3 Changed 3 years ago by 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.

comment:4 Changed 3 years ago by Jakub Ś

Status: newconfirmed

comment:5 Changed 3 years ago by Piotr Jasiun

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

Last edited 3 years ago by Piotr Jasiun (previous) (diff)

comment:6 Changed 3 years ago by Jonathan

Tracking.

comment:7 Changed 3 years ago by Piotrek Koszuliński

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

comment:8 Changed 3 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.5.3

comment:9 Changed 3 years ago by 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).

Last edited 3 years ago by Piotr Jasiun (previous) (diff)

comment:10 Changed 3 years ago by Maciej

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

comment:11 Changed 3 years ago by Szymon Kupś

Owner: set to Szymon Kupś
Status: confirmedassigned

comment:12 Changed 3 years ago by 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.

comment:13 Changed 3 years ago by 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.

Last edited 3 years ago by Piotrek Koszuliński (previous) (diff)

comment:14 Changed 3 years ago by Szymon Kupś

Status: assignedreview

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.

comment:15 Changed 3 years ago by Piotr Jasiun

Status: reviewreview_failed

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

comment:16 Changed 3 years ago by Olek Nowodziński

Milestone: CKEditor 4.5.3CKEditor 4.5.4

comment:17 Changed 3 years ago by Szymon Kupś

Status: review_failedassigned

comment:18 Changed 3 years ago by Szymon Kupś

Status: assignedreview

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.

comment:19 Changed 3 years ago by Piotr Jasiun

Status: reviewreview_failed

The solution looks good, but needs some more polishing.

Don't repeat yourself:

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:

Code style:

comment:20 Changed 3 years ago by Szymon Kupś

Status: review_failedassigned

comment:21 Changed 3 years ago by Szymon Kupś

Status: assignedreview

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.

comment:22 Changed 3 years ago by Piotrek Koszuliński

Resolution: fixed
Status: reviewclosed

Closed with git:3a5b1ff2.

Last edited 3 years ago by Piotrek Koszuliński (previous) (diff)
Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy