Broken aria-describedby in radios and checkboxes

Tested this in 8.3 & can repeat the problem. This is what the WAVE Toolbar sees:

Hard to argue with "the radio buttons don't have aria-describedby attribute since they don't have description."

I wonder if it would make sense to open up a new issue for this relationship:

<fieldset data-drupal-selector="edit-user-cancel-method" aria-describedby="edit-user-cancel-method--wrapper--description" id="edit-user-cancel-method--wrapper" class="fieldgroup form-composite js-form-item form-item js-form-wrapper form-wrapper" aria-describedby="edit-user-cancel-method--wrapper--description">

<div id="edit-user-cancel-method--wrapper--description" class="description">Users with the <em class="placeholder">Select method for cancelling account</em> or <em class="placeholder">Administer users</em> <a href="/admin/people/permissions">permissions</a> can override this default method.</div>

Agreed that they should be semantically linked, especially since that DIV isn't inside the fieldset.

Is there any status on fixing this?
Thanks...

@lstrange - thanks for waking this issue up after 3 years :-)

The accessibility maintainers are aware of several other scenarios where an aria-* ID reference wasn't working. Some issues have already been fixed, other issues have work-in-progress.

This issue has a vague title (aria-* ID refs in general), but a very detailed description of a problem with the registration/cancellation radio options on the user admin form.

So for now, let's narrow the issue scope to the User module. Maybe it has a more general-purpose solution for all radio buttons; we'll see.

I'm guessing that this is a widespread problem in Drupal form widgets with containers since I'm also running into this on the Content Moderation module "Save As" select list widget. The form-wrapper div element has a an aria-describedby value of edit-moderation-state-0--description but no corresponding ID exists on the form. This was confounding me since the description string "The moderation state of this piece of content." exists, but is never actually rendered on the node form.

This may be a naive approach, but may be useful as an initial patch.

By removing the attribute from radios, the description associated with the fieldset may be relied upon instead.

An amendment to the previous patch.

This is an issue with all radios and checkboxes elements (and maybe other elements), and has to do with how attributes are applied to these composite elements and their children.

What happens now is that:

1. The code that connects any #description present, with aria-describedby does not account for the fact that these elements are "special" composite elements with children.
2. Attributes are simply pushed down to the children elements which results in the same attributes on BOTH the wrapper (fieldset) and the children (input type checkbox/radio).

This patch needs work because adding a #description to a radios or checkboxes and wanting to use it with aria-describedby is perfectly fine. The patch just removes that functionality (for radios) entirely, preventing the aria-describedby connection from being made, instead of addressing the root issue, which IMO is that there should be a formal separation of attributes for composite form elements like this.

$form['checkboxes_or_radios_element'] = [
  '#type' => 'checkboxes',
  '#title' => $this->t('Element label'),
  // Should only apply to the fieldset wrapper for checkboxes/radios.
  '#description' => $this->t('Some description'),
  // Should only apply to the fieldset wrapper, but gets pushed to children.
  '#attributes' = [
    'whatever' => 'attribute'
  ],
  '#options' => [
    // These options should NOT inherit #attributes, but they do, and at this point, aria-describedby from the parent is there.
    'option1' => $this->t('Option 1'),
  ],
];

Thanks for the feedback, Janice.

I believe I would agree with your suggestions entirely if the form API currently provided some mechanism for associating specific #descriptions with specific #options. Perhaps it would look something like this:

$form['checkboxes_or_radios_element'] = [
  '#type' => 'checkboxes',
  '#title' => $this->t('Element label'),
  '#description' => $this->t('Element description'),
  '#options' => [
    'option1' => [
      '#title' => $this->t('Option label'),
      // The following would perhaps generate an associated HTML element,
      // give it a suitable ID, and assign this ID to the aria-describedby attribute
      // of the option's input.
      '#description' => $this->t('Option description'),
    ],
  ],
];

However, perhaps this is beyond the scope of this issue, as this would seem to be feature enhancement of the form API, rather than a fix for the incorrect aria-describedby values.

This patch needs work because adding a #description to a radios or checkboxes and wanting to use it with aria-describedby is perfectly fine. The patch just removes that functionality (for radios) entirely...

The patch does not remove any existing functionality. The #description will still be generated with an ID which is associated with the aria-describedby attribute of the wrapper (such as the fieldset that contains the checkboxes). The patch solely seeks to remove the erroneous attributes.

I have amended the patch to also consider checkboxes, as you suggest.

The testing now looks to have passed. It seems to have been a temporary issue in unrelated functional JavaScript tests.

I believe this issue to now be sufficiently re-scoped.

Tested Patch #17 on 9.1, its appearing fine
This can be moved to RTBC

Many thanks, tanubansal.

Moving to RTBC as suggested.

+  public static function isComposite(array $element) {
+    return isset($element['#type']) && in_array($element['#type'], ['radios', 'checkboxes']);
+  }

This doesn't look very extensible. I'd expect this to be some kind of property/method on the class which defines the form element type, so that any element type might be a composite element. There are various places where this might be done, but I'm not sure which would be best.

• A new method on Drupal\Core\Render\Element\ElementInterface or Drupal\Core\Render\Element\FormElementInterface. So then you might have Drupal\Core\Render\Element\Checkboxes::isCompositeElement().
• A new CompositeFormElementInterface, so you can use a instanceof CompositeFormElementInterface test.

Besides #25, this also needs tests.

This issue also affects the Details form element, these accessibility issues are more apparent through the use of the Styleguide module.

https://www.drupal.org/project/styleguide

I think the details element would require a change to the twig template to include the appropriate wrapper and details attributes just like the fieldset template does. Or just simply omitted as well.

Following on from @codebymikey's point #29 I've also identified this happens to the 'container' element too, (albeit with the caveat that the container element does not support the #description attribute):

$form['element'] = [
      '#type' => 'container',
      '#description' => 'A description added to the container.'
];

This will result in a container wrapper being rendered with an incorrect 'aria-describedby' attribute.

Following on from @andrewmacpherson's comment #25, I agree there needs to be a more extensible way to declare a form element as a composite form element. But there are also needs to be some consideration for other render elements which get the 'aria-describedby' attribute added.

I have made a merge request with patch 17 rebased onto 11.x.

Added stack trace

Fixed merge conflict.

I agree with #25, that we should use something more extensible. However, I don't think either of the suggestions is enough information for making it work. The issue is in the FormBuilder class, and at that point, you don't have instances of ElementInterface, you have a render array. So you can't do an instanceof test so easily.

It might be easier to add an attribute #is_composite to the render array?

Or if I'm wrong, then I would say it's probably better to have CompositeFormElementInterface, because there is also a CompositeFormElementTrait. So you could even put isComposite in the trait, and it would be added to any contrib/custom classes using the trait as well.

I agree that this needs to be more extensive, I'm seeing these broken references with selection options as well.

For anyone who needs a workaround, you can implement this in a custom theme or module:

/**
 * Implements hook_element_info_alter().
 */
function my_theme_element_info_alter(array &$info) {
  $info['checkboxes']['#process'][] = '_my_theme_process_options_element';
  $info['radios']['#process'][] = '_my_theme_process_options_element';
}

/**
 * Processes checkboxes and radios elements.
 *
 * Checkboxes and radios have an accessibility bug where the child option
 * elements inherit an empty aria-describedby attribute from the parent.
 * Because the child elements are fully rendered before the parent, this can't
 * be handled in preprocess functions.
 *
 * @param array $element
 *   An options form element.
 *
 * @return array
 *   The processed options form element.
 *
 * @see https://www.drupal.org/project/drupal/issues/2839344
 */
function _my_theme_process_options_element(array $element) {
  foreach ($element as $key => $value) {
    if (substr((string) $key, 0, 1) == '#') {
      continue;
    }
    if (!isset($value['#description']) && isset($value['#attributes']['aria-describedby'])) {
      unset($value['#attributes']['aria-describedby']);
      $element[$key] = $value;
    }
  }
  return $element;
}

The same thing can be accomplished by adding a process function to individual checkbox and radio buttons. I tested it. Doing that means you don't need to loop through all the keys in the parent element to find its children. But my preference was to target the checkboxes and radios since they're the ones that have the problem.

I disagree with the approach in MR 7117 and from reading through comments it was contentious to begin with.

I spent the last couple of evenings studying the checkboxes and radios form elements, trying to figure out what parts of the system were setting wrappers and IDs and descriptions. I wanted to make absolutely certain that I'd found the source of these attributes so that it could be solved there.

It's the Checkboxes and Radios form element classes that do it. They both copy the parent element's attributes to the children, including the aria-describedby. This choice is made unilaterally by those classes. The rest of the form system, e.g. FormBuilder, is just doing what it's supposed to do. The element classes are also managing the child descriptions or rather the lack of them. So if the element classes are setting the attributes, then they should be in charge of unsetting any irrelevant attributes. I opened MR 12287 with this strategy in mind. If this idea is accepted, then MR 7117 should be closed.

I can sympathize with any desire to not duplicate code, but I will note that there is already a lot of duplicated code between the two elements. I probably wouldn't bother. If someone wants that to happen, then I assert that we should put off eliminating duplication for a follow-up.

I'll update the IS when a decision is made.

If there are 2 MRs to choose from usually helps to still update the issue summary but in the proposed solution mention there are 2 solutions

Option 1 solves it by xyz
Option 2 solves it by abc

If there are 2 MRs to choose from usually helps to still update the issue summary but in the proposed solution mention there are 2 solutions

Fair enough. It's done.

Tweaked the Option 2 text.

Thanks Liam. I'll see if I can get someone to renew it.

not a sub-maintainer but let me see if I can't help this along. Of the 2 options it seems like MR 7117 would be the most flexible correct? To add a new composite type I mean.

@smustgrave What do you think about having an interface for composite elements? The check should probably be extensible, so that contrib modules can add composite elements and have them checked by extending the interface or having an attribute or something. See comment #40. Then you could check if the element extends the interface or if it has an attribute.

I can't speak as the form maintainer but if that made it easier to add a new item too I would be a +1

Of the 2 options it seems like MR 7117 would be the most flexible correct?

Let me be blunt: MR 7117 is a workaround for a problem caused by irresponsible copying of parent element attributes to the children. MR 12287 fixes the problem at its source. I feel strongly that it's the way to go, so much that I've considered unilaterally closing 7117 so this issue would progress. But I left it open to be fair to the others who have contributed here. By the way, 12287 has a test.

If there is interest in having the isComposite() function or doing something similar with interfaces, then that can go into a follow-up issue as a feature request. Let's fix this bug properly instead.

I do agree that MR 7117 is not the way to go. It's not flexible at all, as it won't work for anything added/changed by contrib.

in_array($element['#type'], ['radios', 'checkboxes']);

This is a hardcoded list of types. Not flexible.

So we want to close the 7117 then and go from there?

That's what I would do.

Alright lets go that route
Rebase the MR

And if needed open a follow up

Rebased and updated the IS.

Before

After

I also checked Require email verification when a visitor creates an account which should have aria-described

Ran the test-only job here https://git.drupalcode.org/issue/drupal-2839344/-/jobs/6981702 which shows the coverage

Believe this one is good to go.

Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!

I can't wait to remove the workaround from my theme!