Fix file upload progress bar

Here's a patch.
The problem is that the file upload starts asynchronously before file module's event handlers are called.
The code also prevents calling these event handlers twice.

Here's a fix for multiple event handler function calls at once.

Here's an other small fix.

The progressbar should work using the latest patch, however some problems still may exist:
The disableFields() and progressBar() methods are called multiple times when the user selects a file to upload and the upload progress starts automatically, that's why I tried to prevent them to be called multiple times.
When the user clicks a submit button, these methods are called only once, but I'm not sure if they're executed in time (before the upload is actually started).
I've tested with and without Javascipt, with throbber and progressmeter, and even with large files.

Some feedbacks would be appreciated when used on forms having multiple file fields, multi-value fields, and using APCu instead of upload-progress.

You are right, those spaces shouldn't be there.
Even if it works, I had the impression I'm on a wrong track.
I mean maybe someone else can solve it an other way to ensure the form is always submitted later than our handlers are executed.

Finally I could fix this issue in a more clean and nice way without modifying actual code of event handlers using Drupal.debounce() based on the suggestion of nod_ in #2664216: Drupal.debounce() can not access function arguments within closure.. :)

I can't see the progressbar so it's sort of a blind patch. Went ahead and simplified a bit the code, a couple of temporary variables were not needed and placed debounce so that we don't need a separate objects for the callbacks.

Anyway, the only thing I'm not sure will work as expected is what I did in triggerUploadButton, which is sort of the whole point of the patch, In theory it works, but you never know… If I'm wrong I'll eat some turnips for punishement :D

No interdiff since the updated version is very simple compared to existing code.

I think it's as simple as it should be now. :) It works for me.

1. +++ b/core/modules/file/file.js
   @@ -170,7 +170,9 @@
   +        .trigger('mousedown.fileButtons')
   +        .trigger('mousedown');
   

   Needed docs if there's intended to do so.

2. +++ b/core/modules/file/file.js
   @@ -209,7 +211,7 @@
   +    }, 100, true),
   

   I'm interested why it chosen `100ms`

3. +++ b/core/modules/file/file.js
   @@ -88,13 +88,13 @@
   +      $context.find('.js-form-submit').on('mousedown.fileButtons', Drupal.file.disableFields);
   

   added namespace, it can't land on D8.0 :(

1. Comments are added.
2. 100 is less than 1000 (until the form's file elements are disabled and the form can not be used normally), it's more than 16.667 (if we assume 60 fps), and is an acceptable number since I don't know any human who can attach 10 files to a form in a second clicking on multiple file upload elements. 100 is even acceptable on slow machines.
3. No regressions. That's an event namespace, not an API namespace. A red apple is still an apple. :)
https://api.jquery.com/event.namespace/
http://www.w3schools.com/jquery/event_namespace.asp
Bug fixes should go to 8.0 as stated here: https://www.drupal.org/node/2494073#comment-10833258

ahh. I may understand what we're trying to do now. We (I) fooled by the event name. @see attached patch. I've tested on my local and working.

Yep, that's clearer like that. New patch with trivial doc fixes.

If we use a custom event I'm not sure we need the event namespace.

Can this patch get any smaller? :)

I just realized that NON-AUTO-UPLOAD BUTTON do not working on both patches in #16 & #17. But this is working on D7 for some reason.

Should we setup Drupal.file.progressBar() on mousedown ?

Does the remove / re-upload functionality work in latest?

Nice. I'll get back and test regressions, because ('mousedown') is removed in #17.

Until that I share my opinions:

#12 & #16 is RTBC for me, and can go to 8.0 because original handlers are not removed, functionality is injected between attach & event handlers. It's also tested.

#17 & #18 Much cleaner. :)
('mousedown') is removed, so we need to test edge cases.
Even if these buttons are hidden it can only go to 8.1 because theoretically someone has a custom JS which triggers mousedown on these buttons, and will break after update, what means API change. (Whoever triggers this button if upload starts automatically? :D Tests or bots maybe? )
If we agree we don't need mousedown handlers any more it's a nice solution. ;)

I minimized the patch for auto upload features.

I think we better kick it in first and split non-auto upload to another issue. (no regressions)

Also, I've confirmed that non-auto upload problem caused by:

* jQuery Form Plugin
* version: 3.51.0-2014.06.20

I also uploaded a patch to demo non-auto buttons fixing. IMO, this is the best for both cases, but it may not able to kick in D8.0.x ?
(I commented out some code for quick testing)

**Sidenotes: we also have to address the `fileAutoUpload` on slow networks. Now, the upload button is hidden by default (HTML). Since all behaviors run on Dom Ready, onChange won't be triggered.

And finally you got it even shorter. :)
Patch #22 works here. RTBC?

Do you think the other (NON-AUTO) can be solved by updating/downgrading or patching JQuery Form?

NON-AUTO is not patching jQuery Form, it sorted by Drupal internal. So that, it triggered Drupal.ajax with custom event `fileUpload`:

Drupal.Ajax.prototype.eventResponse
...
..
 ajax.$form.ajaxSubmit(ajax.options);

It's also standardized all UPLOAD handlers to triggerUploadButton only.

** Even said jQuery Form fixing the problem in next version, non-auto.patch still good to go IMO.

@nod_ #18,

If we use a custom event I'm not sure we need the event namespace.

IMO, we should namespaced all events. eg. mousedown.drupal.fileUpload. (event.drupal.moduleNameOrFunctionName).
I will open an issue to discuss it soon. If someone can do it, please do so.

We have multiple different solutions for the same problem here, so I decided to test all of them using multiple fields attached to a node.
I've added file 1 & file 2 fields to a content type, and started two simultanous upload testing how they work in this case.
I've applied the patch, clear caches, and refreshed the page in all cases, and executed the test multiple times for each not to get any false results by mistake.

Of course I skipped testing trivial fixes.

I'm working on a system where we need to handle large files that's why I'm really interested in fixing this issue.

Let's see the results from the latest to the first:

• #22: Can't handle this case, first progress bar is jumping back & forward.
• #18: First progressbar works, second doesn't seem to be initialized.
• #16: Same as #18.
• #10: First progressbar works, second is initialized, but doesn't show progress until first is finished.
• #6: Same as #10. (Not a nice solution, I was interested if it works or not)

#10 gave the best results, but it's not a nice solution because a new helper class is introduced.

I suggest review why #18 and #16 are failing or how can we remove utility class from #10.

IMO #16 is the less obstrusive solution but I'm not sure why it fails in this case, maybe we don't need debounce at all?
Actually I wouldn't go with #18 because if someone needs a custom widget, triggering mousedown on submit button is the most straightforward way to implement.

#23 back & fore, we need to take the mousedown event down.

---

It seems a bug of simultanous uploading. Each upload posting FULL FORM data to backend. therefore,

First Upload is sending FIRST UPLOAD
Second Upload is sending SECOND UPLOAD, also the FIRST UPLOAD AGAIN.

#2666746: Fix simultaneous file uploads re-posting data

This isn't the final workaround but can you help to test. Thanks. ( It's worked on my side )

(wait 2 sec between 2 uploads, less side effects )

@swentel
I've tried this case, and worked normally (I think there's no difference between patches).

@droplet
Could you please provide interdiff and state which patch did you start from?
I'm afraid of quick & untested patches, since I've spent a lot of time with testing.

@imre.horjan #33,

+++ b/core/modules/file/file.js
@@ -89,7 +89,7 @@
+      // $context.find('.js-form-managed-file .js-form-submit').on('mousedown', Drupal.file.progressBar);

@@ -230,7 +233,10 @@
+          $clickedButton.closest('div.js-form-managed-file').find('input.form-file').remove();

Only 2 lines diff

@swentel
I'll test this case also, maybe there are differences, I'm not sure now.
@droplet
If you don't provide information what and why and in which patch did you change that is not so useful and lead to having more different patches instead of having only one which works and tested.

@imre.horjan,

the patch is pretty simple, it's actually trying to test the simultaneous uploading bugs. I have no more additional info to my patch at the moment (As you can see, I didn't set the status to `Needs Review`. It's not ready as I told.). If you have no time or didn't understand the patch, just skip it. I'm pretty sure we have to test it at least 3 ~ 50 more times.

As I said before in #28, it worked on my side. The reason I asked you to test is I wonder if there's anymore scenario I didn't catch on my side.

We needed issue summary update to help maintainers to reproduce different bugs.

Also, we may have to sort following issue first:
#2666746: Fix simultaneous file uploads re-posting data

Well, I got some time to address this issue in my worktime, and finally finished in my free time.

Testing this issue is time-consuming since we need to upload large files all the time. We need to ensure multi-value fields, multiple fields for single entities, while keeping in mind non-JS submissions, not to break up throbber, custom widgets or any other part of any D8 site ever been installed, and without adding new side effects.

My current solutions:
#6 Solves the problem using helper variables like Drupal.file.preventProgressBarCall.
#10 Using helper class (Drupal.fileHandlerDebounce) to debounce event handlers.
#16 debounced event handlers in Drupal.file.

That's sad. I'm doing it 100% in my free time. I should have a rest now. I will drop out awhile. But I must jump out for final reviewing since @nod_ said he can't test it in #12.

I don't care which solution kick into CORE also. My goal is move this forward WITHOUT errors & Redundant code.

According to #27 & #37, the problem doesn't address yet.

Thanks ALL.

Sorry about your feelings...
I'll get back with other test scenarios.

Interdiffs save lives.

Finally it turned out I have no time for other test scenarios, so I put it back to needs review since we have some working solutions. Actually, I'm using patch #10 on my project.
Progressbar works until ~2GB file size is reached (higher than I expected). Progress meter starts to show negative values when it reaches 2GB, however that's the limitation of Upload progress. (Regarding to specification it won't work for 1GB+ files)

Can anybody explain to me why do we even do all this JavaScript dynamic name changing of upload progress hidden field in Drupal::file::progressBar()?

Looks to me like all we're trying to achieve is to temporarily adjust name of this element from something like name="field_attachment[0][UPLOAD_IDENTIFIER]" to name="UPLOAD_IDENTIFIER" which is required by Uploadprogress extension to work correctly.

Why not just get to the root cause of the issue and tell ManagedFile::processManagedFile() method responsible for generating file upload form to set the name of the element correctly in the first place '#name' => 'UPLOAD_IDENTIFIER',?

I've tried it and progress bar with Uploadprogress worked fine.

Adding patch for ManagedFile::processManagedFile() in case somebody wants to try it. Of course if we go this route then we can remove a bunch of unnecessary JavaScript code from Drupal::file that deals with the dynamic name replacement.

@Peter,
For parallel uploads?

@droplet

I'm not entirely sure what you mean by parallel uploads?

1. Do you mean uploading multiple files from one File upload field by just selecting more then one file in your Finder/File Explorer?
2. or do you mean there is multiple upload forms on the page with File upload fields on them that would allow user to click on Choose file on each of the form and start uploading files multiple files at the same time?

In first case the upload doesn't work in parallel and files being uploaded sequentially.

In second case the upload works in parallel but the information about the uploaded files is incorrect as Uploadprogress saves information about each upload to the same file in /tmp/upt_xxxx reporting info about first file then overriding it with the info about second file etc.

I believe solution to the problem here might be PHP's native Session Upload https://www.drupal.org/node/1561866#comment-11546855

Patch for 8.6.x , please review it :)

#45 and #55 are incorrect because, when there are multiple file upload elements, each one must have a unique key.

Rerolled patches from droplet that need testing and added interdiff.

Rerolled patch from nod_.

Rerolled patches from imre.horjan and added interdiff.

Removed patch from droplet that failed tests.

Added more interdiffs.

I have reviewed all the patches. There were three different successful approaches:

1. imre.horjan had the auto uploader run the progress bar handler before triggering the upload button and added a debounce wrapper to keep it from running a second time when the upload button was triggered. This approach works only when the upload button is hidden.
2. droplet stripped out the auto upload code and changed the submit event so that the mousedown event would not immediately start the upload.
3. Peter Swietoslawski hard-coded the name of the upload identifier element so that the code to change it would not be needed. This prevents it from working with multiple file uploads. (In my testing, multiple file uploads with the PHP 7 uploadprogress extension were buggy. However, this patch made it worse.)

I tried three other approaches:

• The proper place to modify form data is in the jQuery.form beforeSubmit handler. However, Drupal has a blank placeholder for the beforeSubmit handler that other modules can override. Adding code to it is an API change that could break contributed modules.
• Drupal calls the detach behaviors from the jQuery.form beforeSerialize handler to allow them to modify the form. However, I could not find a way to determine the triggering element from the detach behaviors.
• I tried adding the upload identifier as submit data in the upload button’s #ajax array. This did not work because jQuery.form adds that data after the regular form data. The upload progress extensions require the upload identifier to come before the file upload it identifies.

In the end, I updated droplet's patch to support auto upload. This version of the patch passed all my tests.

Hi Darren Oh,
I appreciate you finally took time to progress on this issue.
#70 seems to be an elegant solution.
(I haven't tested though)

I accidentally changed the event on both the upload button and the remove button. Updated patch attached.

Fixed an undefined variable error.

Patch #74 looks good to me. Marking RTBC.

Thank you all for your work. The changes look good. I tried to test all possible use cases for this: auto file upload, file upload with upload button on both single and multi field configuration including without JavaScript.

Given that this bug fix is still potentially disruptive, I will only commit this for 8.6.x so we have time to fix any regressions.

Committed 8b236d9 and pushed to 8.6.x. Thanks!