media-manager-improvement icon indicating copy to clipboard operation
media-manager-improvement copied to clipboard

Bug: Save & Close edited image do not save the image

Open schnuti opened this issue 7 years ago • 23 comments

Steps to reproduce the issue

Edit an image. e.g. resize. Click on button Save & Close. Load the same image for edit. Has original size - not changed. 2. Edit an image. e.g. resize. Click on button Save. Click on button Cancel. Load the same image for edit. Has changed size.

Expected result

Changed image saved in case 1 as in case 2.

schnuti avatar Jan 17 '18 13:01 schnuti

No one tested this yet? I could have a look but it has first to be confirmed.

System local Windows 10, XAMPP PHP 7.1 if that could be a problem.

schnuti avatar Jan 20 '18 06:01 schnuti

I'm pretty sure this comes from the redirect that breaks the upload in the save task - edit-images.js. When debugging it works!

    switch (task) {
        case 'apply':
             Joomla.UploadFile.exec(name, JSON.stringify(forUpload), uploadPath, url, type);
            Joomla.MediaManager.Edit.Reset(true);
            break;
        case 'save':
            Joomla.UploadFile.exec(name, JSON.stringify(forUpload), uploadPath, url, type);
===>        window.location = pathName + '?option=com_media&path=' + fileDirectory;
            break;
        case 'cancel':

schnuti avatar Feb 07 '18 01:02 schnuti

@schnuti I tried to reproduce this issue but I was not able to get the same result as yours. The image file got changed in each case. I am having the same configuration as your's

geetanshu-m avatar Mar 18 '18 19:03 geetanshu-m

@proman24 Thanks for testing. I gave it a new try with code version from today March 18. Still not saving the changed file after clicking on Save & Close. My guess is, as explained above, that my system for some reason is slower and the redirect breaks the save -> Joomla.UploadFile.exec.

It works in debug mode if I set a javascript breakpoint at the next line i.e the redirect -> window.location

schnuti avatar Mar 18 '18 22:03 schnuti

Is this another Windows issue? @proman24 did you test on Windows as well?

laoneo avatar Mar 19 '18 08:03 laoneo

Your question about Windows remembered me to test with another browser. And ... with Chrome it works. Maybe it's the combination of Windows and FF that is the problem.

schnuti avatar Mar 19 '18 11:03 schnuti

@laoneo I tested this in the windows environment. Also, I have used Chrome browser.

geetanshu-m avatar Mar 20 '18 08:03 geetanshu-m

@proman24 Is it possible for you to test with Firefox? I've tried to find some hint on problems with "windows.location" in FF. So far no luck.

schnuti avatar Mar 20 '18 09:03 schnuti

@schnuti Got the perfect results again while testing with firefox in the windows environment.

geetanshu-m avatar Mar 20 '18 14:03 geetanshu-m

@proman24 with which PHP version did you test?

laoneo avatar Mar 20 '18 14:03 laoneo

@laoneo Tested with PHP 7.2.1 and PHP 5.6.19

geetanshu-m avatar Mar 20 '18 15:03 geetanshu-m

Not a solution but proof of problem. I made the ajax request synchronous and it works.

line 175 in edit-images.js - the false makes it

xhr.open("PUT", url, false);

I'm pretty sure that I'm not the only FF user with my configuration.

Alternative asynchronous solutions on MDN

schnuti avatar Mar 20 '18 18:03 schnuti

Did you try to run it with true?

laoneo avatar Mar 21 '18 07:03 laoneo

true (= asynchronous) is the original value i.e that's when it doesn't work with my FF.

Now I think my FF has the correct behaviour in the asynchronous world of Javascript. When searching the web I found issues with the synchronous behaviour of windows.location in browsers. e.g. if a page have a button "Go to latest Article" and a long running background request, the user has to wait for the request to end. (or some code for "Stop request" has to be added)

@proman24 Do you use Firefox 64Bit? I do.

schnuti avatar Mar 21 '18 12:03 schnuti

Can you try it with the latest version of FF? For me it worked as expected.

laoneo avatar May 24 '18 17:05 laoneo

@laoneo I'll test this and the new save events over the weekend.

schnuti avatar May 26 '18 08:05 schnuti

@laoneo After a fresh J!MM install, code from May 26. Using Firefox latest (normal and developer edition) with my own FF profile and the one created by the developer edition. It's the same. The resized image do not get saved. (save&close) As before: it works with chrome. Did you use Windows 10?

I found a new issue.

schnuti avatar May 27 '18 11:05 schnuti

No I was using Linux. But if the save action is working, then should also the save and close working. Really strange.

laoneo avatar May 28 '18 07:05 laoneo

I'm not that surprised. The difference is the line following the save (apply) action. As I wrote, the window.location somehow breaks the asynchronous Ajax call before the server can start the save method. It works in debug mode (with a breakpoint at those lines) or if the Ajax call is made synchronous as described above. My local client/server combination is probably to slow (or to fast? only FF?)
I've tried to somehow add a promise, callback or worker but my JS knowledge is too limited. window.location should wait until the Ajax call returns.

    case 'apply':
       Joomla.UploadFile.exec(name, JSON.stringify(forUpload), uploadPath, url, type);
       Joomla.MediaManager.Edit.Reset(true);
       break;
    case 'save':
       Joomla.UploadFile.exec(name, JSON.stringify(forUpload), uploadPath, url, type);
       ==> window.location = pathName + '?option=com_media&path=' + fileDirectory;
       break;

Most of the problems and info I've found are related to the use of JQuery but here is one link. Without date but with JS worker. The image under "Irreplaceability of the synchronous use" is interesting.

Synchronous and Asynchronous_Requests

schnuti avatar May 28 '18 10:05 schnuti

I made some tests. Chrome Developer Tools told me my system is slow and also detected [Violation] Forced reflow while executing JavaScript took 80ms. Probably Chrome handles this violation and waits for the Javascript to come to an end. I added some console.log

Console logs from Chrome, Firefox and Edge.

Chrome System is slow. [Violation] Forced reflow while executing JavaScript took 80ms cropper.min.js?... [Violation] 'resize' handler took 183ms

start ajax send ajax data before window.locaton (proves it is asynchronous) success true (returns from ajax) Navigated to "...//...option=com_media&path=local-0:/sampledata"

Firefox start ajax send ajax data before window.locaton (proves it is asynchronous) Navigated to "...//...option=com_media&path=local-0:/sampledata" error ajax (from ajax error handling)

Edge works. The HTML message could get sent before waiting for the response? start ajax send ajax data before window.locaton (proves it is asynchronous) HTML1300: User has navigated success true (returns from ajax)

I optimized apache and mySql a bit. The web server got faster but error remains.

schnuti avatar May 29 '18 09:05 schnuti

@laoneo Do you know the reason for not using Promise and Joomla.request (as is) for the requests as for upload, rename ....? The Promise would solve this issue.

    upload(name, parent, content, override) {
        // Wrap the ajax call into a real promise
        return new Promise((resolve, reject) => {
    ...
            Joomla.request({
                url: url,
                method: 'POST',

I managed to add callbacks and then it works in my tests. As my own experiment I'll try to follow the upload example and use Promise and Joomla.request instead.

Shouldn't Notifications.js be moved to media/com_media/js and in the end to media/system/js ? Reuse!

schnuti avatar Jun 02 '18 11:06 schnuti

Shouldn't Notifications.js be moved to media/com_media/js and in the end to media/system/js ?

Yes... Notifications.js is/was just an adapter for a non-existing joomla api. Our goal was to focus on media manager and not spent too much time modernizing outdated core apis. As soon as we have promise support or a good notification and request api in joomla, we can change the internals.

dneukirchen avatar Jun 04 '18 08:06 dneukirchen

@dneukirchen I copied a small part of Notifications into the code I edit. A nice helper. There is a colourful bug in there. I'll add an issue.

i changed the edit-images to use promises. That solves this issue. I also added progress using tag with the same name. I'll add it as a PR for review.

schnuti avatar Jun 04 '18 10:06 schnuti