When you have multiple upload fields, and click the upload button, it uploads the file and disables the other upload fields as expected, however the upload fields stay disabled and you need to click the upload button to re-enable them. See attached image.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

henwan’s picture

After looking at the core file/file.js code, I noticed that it is using this to re-enable fields:

$fieldsToTemporarilyDisable.attr('disabled', '');

That will still keep the fields disabled. You will need to remove the disabled attribute in order to re-enable the field:

$fieldsToTemporarilyDisable.removeAttr('disabled');

I've attached a patch for it.

Mark Theunissen’s picture

Seems like a sensible change.

gagarine’s picture

Version: 7.12 » 7.14
Status: Active » Reviewed & tested by the community

I agree and tested #1. Works for me.

We can also use attr but set to false

      $fieldsToTemporarilyDisable.attr('disabled', false);

But, I'm for pushing this patch, it's better to remove the attribute because it's what we want to do...

I think this is linked to jqueryUpdate and/or HTML5 theme.
FYI they are some change in jQuery 1.7 than made bad code incompatible. #1498858: [meta] attr and prop.

I put as RTBC because it look predictable for me. But feel free to ask more test if you feel uncomfortable.

marcingy’s picture

Version: 7.14 » 8.x-dev
Status: Reviewed & tested by the community » Needs work

This needs to be fixed in d8 first and then backported.

gagarine’s picture

I create also a slightly different issue for D8 as we can use prop #1597038: Use .prop instead of .attr when we deal with properties

After looking around I saw some browser inconsistently with .removeAttr on some kind of attribute/properties. So at the end it will be safer to use attr('disabled', false);.

We should do a quick search in all core's javascript files and replace .attr(myAttribut, '') by .attr(myAttribut, false). I can do it if you agree on the idea.. The patch shouldn't be that big.

gagarine’s picture

Title: Upload fields not enabled after clicking upload button during AJAX uploads » use $.attr with false instead of empty string
Status: Needs work » Needs review
FileSize
569 bytes
1.07 KB

I found only two place where we have this problem on D7 and one on D8.
regex used

\.attr\('([a-z]+)',.*''\)
ericduran’s picture

Status: Needs review » Reviewed & tested by the community

Don't really have much to say. Patch is pretty straight forward.

gagarine’s picture

I new patch for D7 as it didn't past the test. I also use full if syntax because I think short if syntax is not really clear in this context.

EDIT: Ah I understand the system try to apply the patch again 8.x only... so both patch works.
EDIT 2: don't use this one.. look at #10

Status: Reviewed & tested by the community » Needs work

The last submitted patch, d7-use_attr_with_false-1480568-8.patch, failed testing.

gagarine’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.52 KB

Oups I forget to edit the second file in #8.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, d7-use_attr_with_false-1480568-8.patch, failed testing.

gagarine’s picture

Status: Needs work » Needs review

It fail because it's for D7. D8 patch already passed test in #6.

nod_’s picture

Issue tags: +JavaScript

tag please.

gagarine’s picture

To test it, patch (#6 for D8, #10 for D7) and check this:
- On the page admin/structure/types/manage/YOUNODETYPE/fields add a field, move a field, save..
- On a node with two file fields upload a file (ajax upload) on the first field, upload a file on the second field, save the node.

ryan.gibson’s picture

Status: Needs review » Reviewed & tested by the community

The patch applied cleanly, I reviewed the code and it is not complicated and does what it says, I also completed the tests from #14 and everything works fine after applying the patch.

JamesK’s picture

Status: Reviewed & tested by the community » Needs work
-    $(this).html(html).attr('disabled', disabled ? 'disabled' : '');
+    if(disabled){
+        $(this).html(html).attr('disabled', 'disabled');
+    }else{
+        $(this).html(html).attr('disabled', false);
+    }
   });
 };

Why don't you leave the condition syntax the way it was?
The line should become $(this).html(html).attr('disabled', disabled ? 'disabled' : false);

JamesK’s picture

Status: Needs work » Reviewed & tested by the community

Sorry my problem is with D7 patch, D8 is still RTBC.

gagarine’s picture

@JamesK I actually wondering myself about this point. So I read http://drupal.org/node/172169 and so nothing about short syntax... and I suppose it was not encouraged ( http://drupal.org/node/172169#comment-6044038) . I have to say personally I never liked short syntax.

But once the D8 get committed I can provide a patch with the short syntax if needed.

MustangGB’s picture

After some chitchat on IRC the general consensus is that ternary operators are fine to use, and in this case should be, as long as we're sensible about it. e.g. 80 character per line rule
So lets go with #16 and get a D7 patch rolled as well.

sun’s picture

Component: file system » file.module
catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs backport to D7

Thanks. Committed/pushed to 8.x.

MustangGB’s picture

Status: Needs review » Needs work

As per #16 a reroll in needed for D7

gagarine’s picture

Status: Needs work » Needs review
FileSize
1.07 KB

reroll

JamesK’s picture

Status: Needs review » Reviewed & tested by the community

Looks great.

Macronomicus’s picture

Nice to have this sorted, tested perfect for me.
Cheers!

MustangGB’s picture

#23 Has been working fine over here also.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

Automatically closed -- issue fixed for 2 weeks with no activity.