Opened 4 years ago

Closed 4 years ago

#14375 closed Bug (fixed)

No progress bar, notifications in uploadwidget.

Reported by: Tade0 Owned by: Tade0
Priority: Normal Milestone: CKEditor 4.5.10
Component: General Version: 4.5.6
Keywords: Cc:

Description

Steps to reproduce

  1. Open http://tests.ckeditor.dev:1030/tests/plugins/uploadwidget/manual/image
  2. Drag and drop an image from the file system.

Expected result

There should appear a progress bar, and after the upload is done a success notification should be displayed.

Actual result

There are no notifications and the image is "uploaded" almost immediately(no artificial delay - which should be present).

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

IE, Chrome, Firefox.

Bug appeared along with commit d691b1a152cae852f1165b78a46133f9875e7c44.

Change History (12)

comment:1 Changed 4 years ago by Tade0

Status: newconfirmed
Version: 4.5.6

comment:2 Changed 4 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.8CKEditor 4.5.9

comment:3 Changed 4 years ago by Tade0

Owner: set to Tade0
Status: confirmedassigned

comment:4 Changed 4 years ago by Tade0

The reason for this error is that the form data object mock that is used in the test does not support appending more than one item. I'll modify the mock accordingly.

comment:5 Changed 4 years ago by Tade0

Status: assignedreview

Modified the mock so that now it detects the function signature(simply the number of arguments in this case).

Changes pushed to branch:t/14375.

comment:6 Changed 4 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.9CKEditor 4.5.10

comment:7 Changed 4 years ago by kkrzton

Status: reviewreview_failed

Indeed bug is cause by d691b1a152cae852f1165b78a46133f9875e7c44 where ckCsrfToken was added to upload form. The additional request with ckCsrfToken which is called after image request messes up total and filename variables in the mock.

While checking number of arguments works fine in this case:

  • Adding another elements to the upload form (with 3 arguments) in the future may break this code again.
  • It is not very clear solution, I have to see how file upload works to understand it.

While every field in the form has its own name which is visible in the requests arguments:

["upload", File, "car-8.jpg"]
["ckCsrfToken", "90BQ0iwVYaEqQ0GiZmrunbtQSS3dG5343L4l4Fm8"]

maybe detecting it using this name will be more reasonable solution?


I also added one commit fixing variable names conflict / overshadowing.

Last edited 4 years ago by kkrzton (previous) (diff)

comment:8 Changed 4 years ago by Tade0

Status: review_failedreview

Made it so. Rebased with master, changes pushed to branch:t/14375.

comment:9 Changed 4 years ago by kkrzton

Wouldn't it be better to check for name == 'upload' instead of name !== 'ckCsrfToken' in this line? Adding another elements to the upload form (with 3 arguments) in the future will still break this test.

It's just a detail and I'm not sure if there will be any changes in this code in the future, what do you think @Tade0?

comment:10 Changed 4 years ago by Tade0

I think this is better than my current solution.

comment:11 Changed 4 years ago by kkrzton

Status: reviewreview_passed

Looks good, I only changed this line as mentioned in the comment above.

comment:12 Changed 4 years ago by kkrzton

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