Opened 3 years ago

Closed 2 years ago

#13812 closed Bug (fixed)

When aborting file upload the placeholder for image is left.

Reported by: Jakub Ś Owned by: Tade0
Priority: Normal Milestone: CKEditor 4.5.11
Component: General Version: 4.5.0
Keywords: Cc:

Description

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.

Attachments (2)

placeholder.png (109.5 KB) - added by Jakub Ś 3 years ago.
Screen Shot 2016-08-23 at 12.41.01.png (25.4 KB) - added by kkrzton 2 years ago.
Failing unit test (same error for IE10, IE11 and Edge).

Download all attachments as: .zip

Change History (20)

Changed 3 years ago by Jakub Ś

Attachment: placeholder.png added

comment:1 Changed 3 years ago by Jakub Ś

Status: newconfirmed

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

comment:3 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.5

comment:4 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.5CKEditor 4.5.6

comment:5 Changed 3 years ago by Tade0

Owner: set to Tade0
Status: confirmedassigned

comment:6 Changed 3 years ago by 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();

comment:7 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.6CKEditor 4.5.7

comment:8 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.7CKEditor 4.5.8

comment:9 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.8CKEditor 4.5.9

comment:10 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.9CKEditor 4.5.10

comment:11 Changed 3 years ago by Tade0

Status: assignedreview

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.

comment:12 Changed 2 years ago by Tomasz Jakut

Status: reviewreview_failed

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.

comment:13 Changed 2 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.10CKEditor 4.5.11

Moving tickets to the next milestone.

comment:14 Changed 2 years ago by Tade0

Status: review_failedreview

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

comment:15 Changed 2 years ago by kkrzton

Status: reviewreview_failed

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.

Changed 2 years ago by kkrzton

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

comment:16 Changed 2 years ago by Tade0

Status: review_failedreview

Fixed unit test, updated manual test.

Changes pushed to branch:t/13812.

comment:17 Changed 2 years ago by kkrzton

Status: reviewreview_passed

LGTM, R+.

comment:18 Changed 2 years ago by kkrzton

Resolution: fixed
Status: review_passedclosed

Merged to master with 50f9765.

Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy