Prevent label @for from pointing at non-existent @id

Hmm, this is because date fields aren't just one field, they're 3. So you can't use an ID to target all of them at once. We'll need to figure out a way to disable the target attribute while still keeping the ID on the field.

This problem may also affect Time and Grid components.

The same problem still in version 3.6.

Is there a way to fix this?

Thx

There is not an "easy" solution here that I know of, since Webform does not have the level of control needed on the output of form elements in Drupal 6. In Drupal 6, the "target" attribute is output by theme_form_element(), which is part of core. In Drupal 7 we point this to theme_webform_element() instead, but in both cases we haven't yet solved the problem.

Okay I understand. Thank you for your fast answer!

This issue appear on: date, file, grid, select and time components.

I don't know drupal 6, but for D7 the label is output by theme_form_element_label() on includes/form.inc and "for" attribute could be shutdown by unsetting $element['#id']

I would attach a proposed patch for 7.x-3.x-dev

Seems like a pretty big hack to me. We could easily just check if the field has any child-elements and automatically determine this property (I think).

We could easily just check if the field has any child-elements and automatically determine this property (I think).

Thanks for the hint :)

I've re-rolled patch on dev.

Thanks this looks good to me. Could you confirm that we're not going to accidentally nix the "for" attribute on a field that should have it (like the File component)?

Also File component rise validation error:

Error Line 177, Column 35: The for attribute of the label element must refer to a form control.

  <label for="edit-submitted-file">File </label>

this patch remove also this for attribute, but is correct.

The real (and correct) id of file is $element['file']['#id'] and it is "edit-file" instead of "edit-submitted-file".

All other default elements seems correct to me

I was looking at this patch and trying to have it make sense for all components and I think this approach may still be a bit heavy-handed. If we can figure out a way to make it work with the File component also (or fix the File component in some other way) we can commit it to the project, but right now this still seems hacky.

Attached are patches with cleaned up documentation and a D6 backport (even though it doesn't do much in D6, since theme_webform_element is not used for form elements).

This looks like a useful addition. What needs to be done to bet it into the next release?

@mgifford: Since the patch in #13 we completely rewrote the File component, so perhaps that problem no longer applies. We need testing to ensure that the "for" attributes on labels are properly associated with the related element.

Downloaded 7.x-4.0-alpha3 which I assume would have the reworked code. Created a new webform with a date. Grabbed the html output and tossed it into http://validator.w3.org got this error:

Warning Line 200, Column 15: reference to non-existent ID "edit-submitted-date"

today's date

✉

This error can be triggered by:

A non-existent input, select or textarea element
A missing id attribute
A typographical error in the id attribute

Try to check the spelling and case of the id you are referring to.

I also ran it through WAVE, and got a report of an orphan form label. It's looking for an ID of edit-submitted-date but that doesn't exist. There is a day, month, year but no date ID. Therefore there is still a validation issue.

Date is one of the complex form elements that's tied up in #504962: Provide a compound form element with accessible labels

Downloaded 7.x-4.0-alpha3 which I assume would have the reworked code.

No version has the code applied to it yet.

Ahh, ok, I misunderstood.

I updated the D7 patch to fit with the git repository. That does eliminate the one validation issue.

However, I also uploaded a patch that rather than hacking around to avoid Drupal's fieldset conventions, it just uses it properly. webform_for_attribute-d7-18-correct.patch is semantically better as it does provide a logical grouping and follows best practices like:
http://www.nomensa.com/blog/2010/three-rules-for-creating-accessible-forms/

But ya, it's totally ugly without unique styling and I don't expect to see it in D7 as it would be a new pattern.

oops, didn't bump the bot.

@mgifford:
You are referring to 7.x-4.0-alpha3 but the issue is for 7.x-3.x-dev.

Are the patches for 7.x-3.x-dev?

@quicksketch:

@mgifford: Since the patch in #13 we completely rewrote the File component, so perhaps that problem no longer applies. We need testing to ensure that the "for" attributes on labels are properly associated with the related element.

No version has the code applied to it yet.

So this change is not in any dev versions/git yet? I cannot see any commit messages relating to it.

Any idea when this will be testable?
I am running into file element label validation issues also.

Well, I would assume it was the master branch in git which would probably be closest to 7.x-4.0, but I posted that over a month ago, so really not sure.

From memory I think I checked master and it has no commits since december or something.

I have an easy workaround for my use case by overriding theme_form_element_label()

I think the fieldset approach is an excellent candidate for the 4.x branch, while we're in the business of breaking theming/apis. Considering the effects this would have on existing sites, it probably can't be applied to the 3.x branch.

Well upon attempting this I found that fieldsets are pretty much a pain to work with because the <legend> tag for fieldsets can't be displayed inline through CSS (as reported throughout the internet). So instead this approach keeps the normal label and displays it exactly as before, but adds a fieldset and a legend that is entirely hidden through CSS. It's completely redundant markup, but something that works. The label is shown (and read through screenreaders going through the page initially), but the legend text is repeated each time focused is changed between elements. It's certainly a usability improvement, but I'm not sure how I feel about it from a code-cleanliness standpoint. I'm sure the designers/themers will lament it... but as far as end-users go (and goverments and organizations that demand accessibility), it's a win. Thoughts?

I wasn't able to apply this to the master branch from git. Were there changes or was this against dev?

The Drupal legacy use of fieldsets is a pain. I'm hoping we can remove it in D8 with - #1168246: Freedom For Fieldsets! Long Live The DETAILS.

Then we can use fieldsets as the W3C requires. Until then the html's going to be a bit messy to deal with accessibility issues properly.

I wasn't able to apply this to the master branch from git. Were there changes or was this against dev?

There isn't a master branch I don't think. It's against 7.x-4.x.

Ok, it applies perfectly against that repo.. However, I got a PDOException: SQLSTATE[23000] error when working to set up my dev environment so will report that and come back and look at this letter.

Here's a reroll that applies to the current 4.x branch.

Okay, so in my testing this patch does not have adequate results. Yes it makes it so that Webform passes validators, but it has no real impact in my accessibility testing. JAWS and Voiceover won't read the fieldset legends if they're hidden, so this don't help with accessibility for blind users in any real way. Putting this back to needs work.

I think the only real solution here for accessibility is to do the following:

- Use fieldsets and legends on all components that contain multiple elements (radios, checkboxes, files, date, time, etc).
- Remove the label elements for these components, since they can't point at multiple elements (that's what the legend is for).
- Style the legends to match the appearance of labels as closely as we can in a generic way.
- Unfortunately this means that "inline" label display needs to be removed from these components, because legends simply cannot be displayed inline using CSS and HTML. Legends are very difficult to work with.

This is how I'm hoping to hide the fieldsets in D8 Core [#7980597]

That should address some of the visual changes.

What do you need to help push this along?

just a re-roll.

I thought i attached it...

Patch from comment #36 applies correctly to commit d12f23225d9bdca8c988a95fddddb6bf61cdb84d of the 7.x-4.x dev branch of webform.

see comments from gdaw below

Testing results for Patch from comment #36

Label now applies to the Input, but describedby does not work.

New error = The aria-describedby attribute must point to an element in the same document.

Ex.

<label for="edit-submitted-keywords">Keywords <span class="form-required" title="This field is required.">* required</span></label>
<input aria-describedby="edit-submitted-keywords--description" type="text" id="edit-submitted-keywords" name="submitted[keywords]" value="" size="60" maxlength="128" class="form-text required" />

@gdaw - does this work properly before the patch is applied? Just looking at the code in the patch:

+  // If there are multiple elements, a label cannot point to all of them.
+  // Removing the ID on the element will remove the label "for" attribute.
+  $multiple_elements = count(element_children($element)) > 1;
+  if ($multiple_elements) {
+    $variables['element']['#id'] = NULL;
+  }

It looks like this would happen in cases where there are multiple elements. I don't have enough information in your case to determine if this is the case or not.

Can you give a bit more context on how to replicate this? The keyword wouldn't be wrapped in a fieldset. Looks like this might be a 2 part problem, where I just fixed one of the issues in my patch above.

Hi MGifford, looks like your patch is working. We debugged a bit more and it looks like the issue we have is comming from our jquery libraries version 1.10.2 .

When we did a search for the string "aria-describedby" it showed up in our jquery files (outside of webforms)
./jquery_update/replace/ui/ui/minified/jquery.ui.tooltip.min.js
./jquery_update/replace/ui/ui/minified/jquery-ui.min.js
./jquery_update/replace/ui/ui/minified/jquery.ui.dialog.min.js
./jquery_update/replace/ui/ui/jquery.ui.tooltip.js
./jquery_update/replace/ui/ui/jquery-ui.js
./jquery_update/replace/ui/ui/jquery.ui.dialog.js

The javascript is placing the aria-describedby= into the tag attributes.

We'll have to re-test in a vanilla drupal environment. Just wondering mgifford , what is your jquery version?

We're moving to bootstrap in the next month or so, I imagine this will change our javascript version and we'll have to retest at that time.

So it's likely that mgiffords patch is good, it resolved the problem of " labels on multiple elements " , our other issue seems to be unrelated to webforms and more to our drupal distro.

It's just with with Core. I didn't upgrade jQuery. Glad that it helped with your label issue Joseph.

What's the status of this? Is #36 RTBC? It looks like #46 may have been a red herring.

I have two related concerns:

1. Some themes may have CSS which is more specific and causes the new fieldset to have a visual effect or the new legend to actually be displayed.
2. Some themes may have overly-specific CSS which breaks when the new fieldset is introduced.

Absent further discussion, I'm moving this to D8, where I think #36 would be a good solution.

Too bad it didn't make it to 7.x.4 cause this would solve a lot of accessibility issues.

Closing to clear out the old Webform 8.x-4.x branch. See #2827845: [roadmap] YAML Form 8.x-1.x to Webform 8.x-5.x.

#36 is the same sort of fix as in #2192419: Use a WCAG-compliant fieldset (fieldgroup) for #type radios/checkboxes. It would be difficult to do that here without messing up themes. However, the title of this issue is specifically about invalid "for" attributes, which can be fixed.

This patch just removes the invalid for attributes.

Here is another way to do it, directly checking for the existence of the @id. I'm not sure which is better.

The CI error appears to be a problem with drupalci_testbot , something is up with it.

failed to open stream: Permission denied in /opt/drupalci_testbot

We did a drupalci deployment today, and there were things that didnt go as planned. Sorry. I have requeued the test.

Here's a 7.x-3.x version of the patch from #36. Leaving this here just in case someone like me needs it.

#69 works for me! Thanks @jenna.tollerson.