#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 )
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 9 years ago by
comment:2 Changed 9 years ago by
Description: | modified (diff) |
---|
comment:3 Changed 9 years ago by
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 9 years ago by
Status: | new → confirmed |
---|
comment:5 Changed 9 years ago by
I tested it and using xhr.upload.onprogress
instead on xhr.onprogress
works much better on Chrome.
comment:8 Changed 9 years ago by
Milestone: | → CKEditor 4.5.3 |
---|
comment:9 Changed 9 years ago by
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).
comment:10 Changed 9 years ago by
For CKFinder 3 we're using xhr.upload.onprogress
with check for xhr.upload
existance.
comment:11 Changed 9 years ago by
Owner: | set to Szymon Kupś |
---|---|
Status: | confirmed → assigned |
comment:12 Changed 9 years ago by
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 9 years ago by
Sounds totally logical. There's a request and a response... and we've been listening on a progress on the response :D.
comment:14 Changed 9 years ago by
Status: | assigned → review |
---|
Pushed branch:t/13533
Changed current upload progress reporting:
- Using
xhr.upload.onprogress
when is available. - Introduced new
FileLoader
property calleduploadSize
. This property will hold upload payload size, which can be different thantotal
file size. Before uploading file is embedded intoFormData
and that is why payload size is different than file size. - Introduced
uploadSizeRecieved
event which is fired whenuploadSize
is received.
These modifications entailed changes in uploadwidget
plugin:
bindNotification
function is creating tasks in asynchronous manner by listening touploadSizeReceived
event.
comment:15 Changed 9 years ago by
Status: | review → review_failed |
---|
This solution unfortunately needs some more improvements:
- It was a good point that
xhr.upload
, notxhr
, is the proper object for upload events. But besidexhr.upload.onprogress
we should listen also onxhr.upload.abort
andxhr.upload.error
to removeuploadwidget
not only when response errors occurred, but also on request errors.
- Why did not you create a simple function
function createAggregator()
but declarecreateAggregator
and then assign function to it:createAggregator = function()
. AlsocreateAggregator
function should be at the end ofbindNotifications
- 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
anduploadSizeReceived
. The fact thatuploadSizeReceived
is fired even ifuploadSize
is not set the reason of that bug. I think thatloader.uploadSize
should be set even ifxhr.upload
is no supported. It could be equalloader.total
in such case. User may check ifloader.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 thatuploadTotal
is better.
- I am not sure if we need
uploadSizeReceived
event. Having too many events is often reason of problems. You can listen on theupdate
event and check ifuploadSize
is set andtask
isnull
and then create a task.
comment:16 Changed 9 years ago by
Milestone: | CKEditor 4.5.3 → CKEditor 4.5.4 |
---|
comment:17 Changed 9 years ago by
Status: | review_failed → assigned |
---|
comment:18 Changed 9 years ago by
Status: | assigned → review |
---|
Pushed branch:t/13533:
- Added error/abort handling to
xhr.upload
. createAggregator
function expriession changed to function declaration and moved to the end ofbindNotification
.- Changed
uploadSize
touploadTotal
. Whenxhr.upload
is not available it has value ofloader.total
. - Event
uploadSizeReceived
removed. As described in comment:15 - it is not necessary. Value ofuploadTotal
is set tonull
before data is available so it can be handled inupdate
event listeners. - Updated tests.
comment:19 Changed 9 years ago by
Status: | review → review_failed |
---|
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 noxhr.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 thatuploadTotal
is set to be equaltotal
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
comment:20 Changed 9 years ago by
Status: | review_failed → assigned |
---|
comment:21 Changed 9 years ago by
Status: | assigned → review |
---|
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 9 years ago by
Resolution: | → fixed |
---|---|
Status: | review → closed |
Closed with git:3a5b1ff2.
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.