File field form label has incorrect id in @for attribute

This should be relatively easy to solve. Changed to 8.x as this is where all new work is done.

Tagging.

This is tests only patch, should fail.

Test only fails as expected
And again with the patch + tests

Patch works as expected. When clicking the label, the file upload window opens and the 'for' value is correct.

<div class="form-item form-type-managed-file form-item-field-resume-und-0">
  <label for="edit-field-resume-und-0-upload">Resume </label>
 <div class="file-widget form-managed-file clearfix"><input type="file" id="edit-field-resume-und-0-upload" name="files[field_resume_und_0]" size="22" class="form-file"><input formnovalidate="formnovalidate" type="submit" id="edit-field-resume-und-0-upload-button" name="field_resume_und_0_upload_button" value="Upload" class="form-submit drupal-ajax-processed"><input type="hidden" name="field_resume[und][0][fid]" value="0">
<input type="hidden" name="field_resume[und][0][display]" value="1">
</div>
<div class="description" id="edit-field-resume-und-0-upload--description">Files must be less than <strong>128 MB</strong>.<br>Allowed file types: <strong>pdf</strong>.</div>
</div>

Agreed, applies and works.

Seems like a good candidate to backport to d7

Doing this in preprocess doesn't feel right to me, can't it happen when building the form somewhere? Or can we find the code that's adding the -upload in the first place?

Hi catch, it's not using preprocess, it's using process - do you have the same objection to that?
It's because the label is on the parent element, the actual field is $element['upload'], ie a child. But that is a file upload field so the title is used for a different purpose. This patch updates the id of the field so that the parent label is given to the child, there is no other markup output for the parent so we don't get duplicate ids.
/me cheekily sets status back.

Forgot to switch status back

OK I spoke to chx and he didn't have a good reason not to do this here, so I've gone ahead and committed/pushed this to 8.x.

This would be a markup change so it'll need some consideration as to how backportable it is, but will leave that for David to decide.

Backported #3 to D7.

#17: file-label-508-d7-1734716-17.patch queued for re-testing.

Looks like a good backport, thanks for the proof of fail with the test only.

In general changing IDs is OK to fix accessibility issues (outside code should not be relying on specific IDs anyway), but if you look at how $element['#id'] is used later on in this function, the patch is changing a lot more of them than necessary.

More to the point, this ID gets added as a JavaScript setting, so it totally breaks the JavaScript validation for allowed file extensions, i.e. as set up by this code later on in the same function

  // Add the extension list to the page as JavaScript settings.
  if (isset($element['#upload_validators']['file_validate_extensions'][0])) {
    $extension_list = implode(',', array_filter(explode(' ', $element['#upload_validators']['file_validate_extensions'][0])));
    $element['upload']['#attached']['js'] = array(
      array(
        'type' => 'setting',
        'data' => array('file' => array('elements' => array('#' . $element['#id'] . '-upload' => $extension_list)))
      )
    );
  }

and as used by Drupal.behaviors.fileValidateAutoAttach() later on.

I just checked that the regression is in Drupal 8 also (the JavaScript-based validation of allowed file extensions doesn't work).

Not sure if it makes sense to split that off to a different issue at this point, but rolling this patch back fixes it and it's clear we need a different fix here anyway, so I'm leaving it as part of this issue for now...

Anyway, wouldn't a better way to fix this be to change the label's "for" attribute to match the desired ID, rather than the other way around? I think this would just involve making some changes to theme_form_element_label() to be smart enough to allow elements to set an explicit "for" attribute for the label and not overwrite it if it's already set... (I actually even thought there was an issue for that in the queue already, but I can't find it at the moment.)

tag even if not really a JS issue.

Tagging

If you remove the concatenation of -upload from the js setting, would that break things downstream, after applying the patch?

http://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.

The summary may need to be updated with information from comments.

Just wanted to chime in to say that Section 508 isn't the goal that Drupal is working towards. The global standard for accessibility is still WCAG 2.0 and the Drupal community is working to ensure that it is AA compliant. That being said, it's a real challenge and there are sometimes issues like this that slip through.

@larowlan or @dcam any suggestions on how to deal with the downstream issues that @David_Rothstein & @scottshipman raised?

@mgifford I'll see if I can prod someone at work to have a look at this

Updated issue summary using comments.

Updated issue summary using template. -ekl1773

Adding related meta issue #2035139: [meta] Comply with WCAG guideline 4.1: Compatibility

Updated issue summary.

Testing the patch again (just a re-roll) in D7. Then will set it back to D8 so that we can look at @David_Rothstein's concerns in #22.

Ok, that passed but as @David_Rothstein said

wouldn't a better way to fix this be to change the label's "for" attribute to match the desired ID, rather than the other way around?

by making theme_form_element_label() allow elements to set an explicit "for" attribute for the label and not overwrite it if it's already set.

This is in Core now in D8 core/modules/file/file.module

function file_managed_file_process($element, &$form_state, $form) {
  // Append the '-upload' to the #id so the field label's 'for' attribute
  // corresponds with the file element.
  $element['#id'] .= '-upload';

I don't know if this presents a problem for the Javascript wrapper:

  $ajax_settings = array(
    'path' => 'file/ajax',
    'options' => array(
      'query' => array(
        'element_parents' => implode('/', $element['#array_parents']),
        'form_build_id' => $form['form_build_id']['#value'],
      ),
    ),
    'wrapper' => $element['#id'] . '-ajax-wrapper',
    'effect' => 'fade',
    'progress' => array(
      'type' => $element['#progress_indicator'],
      'message' => $element['#progress_message'],
    ),
  );

At the moment it just produces <div id="edit-field-image-0-upload-ajax-wrapper">

I don't see where the commit message was for this code, but perhaps it was added elsewhere.

In anycase, the accessibility problem is fixed in D8 now. So it should be fine to move this back to D7.

Fixes the issues with JavaScript wrappers/ajax etc

I confirmed that this is still an issue in D7 Core and that this patch fixes the problem which is visible here /node#overlay=node/add/article

The patch seems to work ok for the first filefield, but not sure the patch fixes it for multiple filefields on the same page.

Heres what I get on my second one:

for="edit-field-comment-attachment-und-0--2-upload"

id="edit-field-comment-attachment-und-0-upload--2"

Ok, so it's a bit better, but we should figure out why it doesn't work for the second filefield.

Also, this should be tested for D8 as well.

Actually, just tested it and it seems to be working fine so setting this back to RTBC:

Need more information to be able to replicate issue in #37.

That was a bogus test failure, so back to RTBC for now. (I didn't review the patch myself.)

This looks and works great. And the regression in Drupal 8 indeed appears to have been fixed elsewhere, so I'm removing that from the title.

Committed to 7.x - thanks!

The 7.x commit from this issue has caused a regression, which causes AJAX file uploads to fail. I discovered this issue after updating from Drupal 7.35 to Drupal 7.36 when Plupload ceased to upload files over AJAX.

Preserving the original element ID for use in $form['form_build_id']['#value']['wrapper'] and $element['#prefix'] appears to be unnecessary. The $original_id variable first appeared in patch #35.

Patch attached that eliminates the use of the original element ID.

• Drupal 7.36 (Standard install profile)
• FileField Sources 7.x-1.9
• FileField Sources Plupload 7.x-1.1
• Plupload 7.x-1.7
• Plupload external library (v 1.5.8)

Looks like we have these two issue so far:
#2466247: Drupal core 7.36 broke Filefield Sources Ajax upload previews (FileField Sources)
#2466475: Drupal core 7.36 broke IMCE for FileField Ajax upload previews (IMCE for FileField)

Is Plupload broken specifically? It didn't look to me like it would be.

The original goal was to change as little HTML as possible, and I'm not sure we want to go back on that and start changing wrapper IDs (we could break even more if we do that)...

We could consider just rolling this back, or something like #22.

But at this point we also have the problem that it's already out there so rolling it back could re-break things.

I guess what we missed here is that some modules are expecting a particular relationship between $element['#id'] and the Ajax wrapper ID, and that relationship is no longer true after this issue? I'm not sure that's a great assumption to make but perhaps they have no choice...

I just posted patches on both the above issues that fix this by making those modules use the actual Ajax wrapper ID rather than assuming a particular structure for it. It seems like a clean way to fix this, and the patches appear to work correctly.

I also added a note about these issues to the release notes (https://www.drupal.org/drupal-7.36-release-notes) - thanks!

Is Plupload broken specifically? It didn't look to me like it would be.

Looks like FileField Sources Plupload was the culprit actually: #2466505: Drupal core 7.36 broke FileField Sources Plupload Ajax upload previews

I agree that patching the affected modules is better than reverting the core commit from this issue. Patches for two of the affected modules tested successfully for me: #2466505: Drupal core 7.36 broke FileField Sources Plupload Ajax upload previews and #2466505: Drupal core 7.36 broke FileField Sources Plupload Ajax upload previews. @David_Rothstein - Thanks for working through this issue so quickly. I think we're OK to change the status of this issue back to Fixed.

Thanks for testing those! Yeah, let's close this again for now.

I did take a look through some other FileField Sources-related modules and found 4 others that look like they might be affected (I just added them all to the list at https://www.drupal.org/drupal-7.36-release-notes now). I didn't test any of them myself, but did put up patches for each for people to test, if anyone is using those modules - since the code for the patch looked like it would be a completely straightforward extension of the code used to fix the other modules.

Note that we're probably going to have to roll this back and fix it differently after all. See discussion/analysis in #2594955: [D7] Duplicate HTML IDs are created for file_managed_file fields

After using patch #35 label starts works correct for first time. But with ajax we still get wrong for attribute.

  // Append the '-upload' to the #id so the field label's 'for' attribute
  // corresponds with the file element.
  $original_id = $element['#id'];
  $element['#id'] .= '-upload';

  // Get actual id.
  $element['#id'] = str_replace($original_id, $element['#id'], drupal_html_id($original_id));

Maybe it will helps to somebody.