When aborting file upload the placeholder for image is left.

[Jakub Ś]

Steps to reproduce

1. Use below code in CKEditor

   var editor1 = CKEDITOR.replace( 'editor1', {
   	extraPlugins: 'uploadimage,image2',
   	imageUploadUrl: 'http://your-url?command=QuickUpload&type=Images&responseType=json'
    );
   
   editor1.on( 'fileUploadRequest', function( evt ) {
   	evt.data.fileLoader.abort();
   	editor1.showNotification( 'File too big', 'warning' );
   	evt.cancel();
   });
   

2. D&D the file

Expected result

File upload is aborted, message is displayed and placeholder for the image is removed.

Actual result

File upload is aborted, message is displayed but placeholder for the image is left.

Other details (browser, OS, CKEditor version, installed plugins)

Browsers: all, CKEditor version 4.5.0.

[Piotr Jasiun]

It looks like xml.onabort is not called when xml.abort() is called. I pushed quick fix to the ​t/13812 branch, but this needs to be investigated.

[Tade0]

From what I have established xhr.onabort is never fired, because xhr.abort() is called before xhr.send().

This code demonstrates this behavior:

var xhr = new XMLHttpRequest();

xhr.onabort = function() {
  console.log('not called');
};

xhr.open('get', 'http://www.google.com');
//xhr.send();
xhr.abort();

[Tade0]

onAbort() has a safeguard, that prevents anything of consequence from happening if it is called twice or more, so this solution should fit.

Added manual test and rebased. Changes pushed to ​branch:t/13812.

[Tomasz Jakut]

We should have also a unit test to check if onAbort is really called on abort and how many times.

Interestingly in the manual test I don't see any placeholder – even without the fix.

Despite the issue with test, LGTM.

[Marek Lewandowski]

Moving tickets to the next milestone.

[Tade0]

Added unit test, Rebased with master, changes pushed to ​branch:t/13812.

[kkrzton]

The image2 plugin (which is also the cause of this issue) was missing in the manual test (that's why @t.jakut was unable to reproduce the issue) - I added this dependency ​acfbf30.

The fix works fine, however:

1. The unit test fails on IE10, IE11 and Edge.
2. I would add information in the manual test that it won't work on IE8 and IE9 (because of lack of support for D'n'D).
3. In the manual test, before notification error is shown, there is another notification Upload aborted by the user. - I would also add this to Expected result section so it is clear exactly what should happen.

[kkrzton]

Failing unit test (same error for IE10, IE11 and Edge).

[Tade0]

Fixed unit test, updated manual test.

Changes pushed to ​branch:t/13812.

[kkrzton]

LGTM, R+.

[kkrzton]

Merged to master with ​50f9765.