Hello,

I just do the update of your last release 7.x-4.9.

I have checked my html code by this validator : https://validator.w3.org/
The website that I develop is : http://sublim.recursive-solution.com/
(this is actualy under developpment).

The validator give me 2 errors that concerning webform (file field only) :

  • Duplicate ID edit-submitted--upload.
  • The for attribute of the label element must refer to a form control.

I done some search into your bug tracker but I did'nt find something.

Best regards,

CommentFileSizeAuthor
#6 webform_duplicate_id_managed_file-2495821-6.patch2.89 KBdanchadwick

Comments

danchadwick’s picture

danchadwick commented
Related issues: +#2077237: Accessibility Issue with File Elements: Form Label Not Associated With Input

This seems to be a regression resulting from #2077237: Accessibility Issue with File Elements: Form Label Not Associated With Input.

The issue seems to be two-fold:

  1. In theme_webform_element, we are adding '-upload' to the #id, when it already has been appended by file_manage_file_process(). I don't know why.
  2. The div wrapper around the file element has an id, rather than a class. This may be required for conditionals for other functionality -- not sure.
danchadwick’s picture

danchadwick commented
Related issues: +#1147994: Wrap composite elements in a fieldset element

This is also related to #1147994: Wrap composite elements in a fieldset element. We are using a label element and for some the target of the for attribute doesn't exist when the component has multiple elements within it, such as a date (month, day, year), time (hour, minute, am/pm, select components (checkboxes, radio buttons), grids, file components, and perhaps others.

The other issue is we seem to have two elements with the same id -- the wrapper and the file element.

And last, I don't know why the patch to add -upload to the id was needed, nor why it isn't needed now. In my testing, the id already ends in -upload.

danchadwick’s picture

danchadwick commented

And there's this issue, which was bumped to D8: #945438: Prevent label @for from pointing at non-existent @id.

I think the "for" attribute will have to wait for a fix, maybe the duplicate id can be addressed.

Jeremy JOCHEMSKI’s picture

Jeremy JOCHEMSKI commented

Hello Dan,

I find the location where the item "-upload" is added :

file ./webform.module
line 3579
I just comment this line and it's ok ! ( I need an hotfix quickly :) )

For the duplicate id, i think it's a core issue :
See the function "theme_file_managed_file($variables)"
https://api.drupal.org/api/drupal/modules%21file%21file.module/function/...

It create an attribute id with the same element id (the file input id) :

  if (isset($element['#id'])) {
    $attributes['id'] = $element['#id'];
  }

And after, it add the div before render the file input :

  $output = '';
  $output .= '<div' . drupal_attributes($attributes) . '>';
  $output .= drupal_render_children($element);
  $output .= '</div>';
  return $output;

So, every time, it do duplicate id, into the div and file input elements.

Best regards,

Jeremy JOCHEMSKI’s picture

Jeremy JOCHEMSKI commented

I have open this issue for the duplicate id :
https://www.drupal.org/node/2497909

danchadwick’s picture

danchadwick commented
Status: Active » Needs review
StatusFileSize
new webform_duplicate_id_managed_file-2495821-6.patch2.89 KB

Please review this patch to see if it

  1. Resolves the duplicate id issue.
  2. Resolves the 'for' attribute issue.
  3. Does not create any regressions with javascript or css.

Jeremy -- please remove any theme overrides or webform patches you may have used to work around this issue before applying the patch.

Status: Needs review » Needs work

The last submitted patch, 6: webform_duplicate_id_managed_file-2495821-6.patch, failed testing.

danchadwick’s picture

danchadwick commented
Version: 7.x-4.9 » 7.x-4.x-dev
Category: Bug report » Task
Status: Needs work » Needs review

Since the crux of this seems to be a core issue, I think we should call this a task, rather than a bug report. Also changing the version so testbot can work.

DanChadwick queued 6: webform_duplicate_id_managed_file-2495821-6.patch for re-testing.

danchadwick’s picture

danchadwick commented

Test failures are unrelated to this patch.

fenstrat’s picture

fenstrat
he/him
Primary language English
Location Australia
commented

Tricky issue. The fix does point towards the issue laying with core, #6 seems like it will work but the key is checking for regressions with js/css.

@Jeremy JOCHEMSKI can you confirm #6 fixes your issues?

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
Status: Needs review » Reviewed & tested by the community

#6 looks good to me.

Unavoidably, custom CSS and JS could be effected since the selectors are changing. It would be an easy fix, however. Core must have made changes in the same area without creating a huge issue, however, so this may not be a big deal. Still, there should be a warning in the release notes.

danchadwick’s picture

danchadwick commented
Component: User interface » Code
Category: Task » Bug report
Status: Reviewed & tested by the community » Fixed
Parent issue: » #2497909: Duplicate HTML IDs are created for file_managed_file fields

With some hesitation, I committed this even though most of the patch is working around the core bug. Should this be fixed in core, the theme function should be removed, if possible, before webform-8.x is stable.

Committed to 7.x-4.x and 8.x.

Changing this back to a bug report since this patch also removes the duplicated '-upload' that was added to the id.

  • DanChadwick committed 807d468 on 8.x-4.x 
    Issue #2495821 by DanChadwick: Fixed duplicate ID for file field.

  • DanChadwick committed 69f8ae1 on 
    Issue #2495821 by DanChadwick: Fixed duplicate ID for file field.

Status: Fixed » Closed (fixed)

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