Issue #2497909 by Andrew.Mikhailov, pfrenssen, claudiu.cristea, cilefen, yogeshmpawar, jibran, kim.pepper, alexpott, Jeremy JOCHEMSKI, Cottser: Duplicate HTML IDs are created for file_managed_file fields

core/includes/form.inc

@@ -423,11 +423,14 @@ function template_preprocess_textarea(&$variables) {
  * but the parent element should have neither. Use this carefully because a
  * field without an associated label can cause accessibility challenges.
  *
+ * To associate the label with a different field, set the #label_for property
+ * to the ID of the desired field.
+ *
  * @param array $variables
  *   An associative array containing:
  *   - element: An associative array containing the properties of the element.
  *     Properties used: #title, #title_display, #description, #id, #required,
- *     #children, #type, #name.
+ *     #children, #type, #name, #label_for.
  */
 function template_preprocess_form_element(&$variables) {
   $element = &$variables['element'];
@@ -439,6 +442,7 @@ function template_preprocess_form_element(&$variables) {
     '#title_display' => 'before',
     '#wrapper_attributes' => [],
     '#label_attributes' => [],
+    '#label_for' => NULL,
   ];
   $variables['attributes'] = $element['#wrapper_attributes'];
 
@@ -487,6 +491,12 @@ function template_preprocess_form_element(&$variables) {
   $variables['label'] = ['#theme' => 'form_element_label'];
   $variables['label'] += array_intersect_key($element, array_flip(['#id', '#required', '#title', '#title_display']));
   $variables['label']['#attributes'] = $element['#label_attributes'];
+  if (!empty($element['#label_for'])) {
+    $variables['label']['#for'] = $element['#label_for'];
+    if (!empty($element['#id'])) {
+      $variables['label']['#id'] = $element['#id'] . '--label';
+    }
+  }
 
   $variables['children'] = $element['#children'];
 }
@@ -507,10 +517,13 @@ function template_preprocess_form_element(&$variables) {
  * required. That is especially important for screenreader users to know
  * which field is required.
  *
+ * To associate the label with a different field, set the #for property to the
+ * ID of the desired field.
+ *
  * @param array $variables
  *   An associative array containing:
  *   - element: An associative array containing the properties of the element.
- *     Properties used: #required, #title, #id, #value, #description.
+ *     Properties used: #required, #title, #id, #value, #description, #for.
  */
 function template_preprocess_form_element_label(&$variables) {
   $element = $variables['element'];

core/modules/file/src/Element/ManagedFile.php

@@ -305,12 +305,24 @@ class ManagedFile extends FormElement {
       $element['upload_button']['#ajax']['event'] = 'fileUpload';
     }
 
+    // Use a manually generated ID for the file upload field so the desired
+    // field label can be associated with it below. Use the same method for
+    // setting the ID that the form API autogenerator does.
+    // @see \Drupal\Core\Form\FormBuilder::doBuildForm()
+    $id = Html::getUniqueId('edit-' . implode('-', array_merge($element['#parents'], ['upload'])));
+
     // The file upload field itself.
     $element['upload'] = [
       '#name' => 'files[' . $parents_prefix . ']',
       '#type' => 'file',
+      // This #title will not actually be used as the upload field's HTML label,
+      // since the theme function for upload fields never passes the element
+      // through theme('form_element'). Instead the parent element's #title is
+      // used as the label (see below). That is usually a more meaningful label
+      // anyway.
       '#title' => t('Choose a file'),
       '#title_display' => 'invisible',
+      '#id' => $id,
       '#size' => $element['#size'],
       '#multiple' => $element['#multiple'],
       '#theme_wrappers' => [],
@@ -321,6 +333,10 @@ class ManagedFile extends FormElement {
       $element['upload']['#attributes'] = ['accept' => $element['#accept']];
     }
 
+    // Indicate that $element['#title'] should be used as the HTML label for the
+    // file upload field.
+    $element['#label_for'] = $element['upload']['#id'];
+
     if (!empty($fids) && $element['#files']) {
       foreach ($element['#files'] as $delta => $file) {
         $file_link = [
@@ -353,13 +369,9 @@ class ManagedFile extends FormElement {
     // 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']['drupalSettings']['file']['elements']['#' . $element['#id']] = $extension_list;
+      $element['upload']['#attached']['drupalSettings']['file']['elements']['#' . $id] = $extension_list;
     }
 
-    // Let #id point to the file element, so the field label's 'for' corresponds
-    // with it.
-    $element['#id'] = &$element['upload']['#id'];
-
     // Prefix and suffix used for Ajax replacement.
     $element['#prefix'] = '<div id="' . $ajax_wrapper_id . '">';
     $element['#suffix'] = '</div>';

core/modules/file/tests/src/Functional/FileFieldDisplayTest.php

@@ -2,6 +2,7 @@
 
 namespace Drupal\Tests\file\Functional;
 
+use Drupal\Component\Render\FormattableMarkup;
 use Drupal\Core\Field\FieldStorageDefinitionInterface;
 use Drupal\file\Entity\File;
 use Drupal\node\Entity\Node;
@@ -107,6 +108,9 @@ class FileFieldDisplayTest extends FileFieldTestBase {
     $this->assertRaw($field_name . '[0][display]', 'First file appears as expected.');
     $this->assertRaw($field_name . '[1][display]', 'Second file appears as expected.');
     $this->assertSession()->responseContains($field_name . '[1][description]', 'Description of second file appears as expected.');
+
+    // Check that the file fields don't contain duplicate HTML IDs.
+    $this->assertNoDuplicateIds();
   }
 
   /**
@@ -220,4 +224,32 @@ class FileFieldDisplayTest extends FileFieldTestBase {
     $this->assertFieldByXPath('//a[@href="' . $node->{$field_name}->entity->url() . '"]', $description);
   }
 
+  /**
+   * Asserts that each HTML ID is used for just a single element on the page.
+   *
+   * @param string $message
+   *   (optional) A message to display with the assertion.
+   */
+  protected function assertNoDuplicateIds($message = '') {
+    $args = ['@url' => $this->getUrl()];
+
+    if (!$elements = $this->xpath('//*[@id]')) {
+      $this->fail(new FormattableMarkup('The page @url contains no HTML IDs.', $args));
+      return;
+    }
+
+    $message = $message ?: new FormattableMarkup('The page @url does not contain duplicate HTML IDs', $args);
+
+    $seen_ids = [];
+    foreach ($elements as $element) {
+      $id = $element->getAttribute('id');
+      if (isset($seen_ids[$id])) {
+        $this->fail($message);
+        return;
+      }
+      $seen_ids[$id] = TRUE;
+    }
+    $this->assertTrue(TRUE, $message);
+  }
+
 }