backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 40 forks source link

[A11Y] FAPI: Labels should not have a "for" attribute for non labelable elements #6178

Open klonos opened 1 year ago

klonos commented 1 year ago

I accidentally came across this while working on #6177, and I initially thought that it was specific to the "When cancelling a user account" element of the "Account settings" form, but it is generic to all form elements of '#type' => 'item' after all.

Consider the following form item declaration:

  $form['my_item'] = array(
    '#type' => 'item',
    '#title' => t('Test item'),
    '#description' => t('Test item description.'),
  );

The rendered HTML output of the above code would be as follows:

<div id="edit-my-item" class="form-item form-type-item">
  <label for="edit-my-item">Test item </label>
  <div class="description">Test item description.</div>
</div>

The above is invalid though, since the for attribute must always be the HTML id attribute of a labelable element, which div elements are not. See:

The for attribute may be specified to indicate a form control with which the caption is to be associated. If the attribute is specified, the attribute's value must be the ID of a labelable element in the same tree as the label element. If the attribute is specified and there is an element in the tree whose ID is equal to the value of the for attribute, and the first such element in tree order is a labelable element, then that element is the label element's labeled control.

Some elements, not all of them form-associated, are categorized as labelable elements. These are elements that can be associated with a label element. button, input (if the type attribute is not in the Hidden state), meter, output, progress, select, textarea, form-associated custom elements

This is the respective issue for Drupal core: https://www.drupal.org/project/drupal/issues/2279111

klonos commented 1 year ago

...it seems that using a <label> without the for attribute would throw a11y errors. The best suggestion that I could find to sort this was to use the aria-describedby attribute instead: https://stackoverflow.com/questions/52882497/how-to-semantically-add-labeling-to-non-input-elements-that-are-used-as-inputs

The aria-describedby attribute is not limited to form controls. It can also be used to associate static text with widgets, groups of elements, regions that have a heading, definitions, and more. The aria-describedby attribute can be used with semantic HTML elements and with elements that have an ARIA role.

klonos commented 1 year ago

...and I found this relevant/respective issue for Drupal core: https://www.drupal.org/project/drupal/issues/2279111

(added the link in the issue summary as well)

Repeating my comment from that issue:

So this for example, is NOT proper use of the <label> and for attribute:

<div id="edit-my-item" >
  <label for="edit-my-item">Test item </label>
  <div class="description">Test item description.</div>
</div>

After some quick research, the best suggestion I have found is to use a combination of a <span> and the aria-describedby attribute. Something like this for instance:

<div aria-describedby="label-for-div" >
  <span id="label-for-div">Test item </span>
  <div class="description">Test item description.</div>
</div>

Noting the following from https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-labelledby :

  1. the aria-labelledby attribute is not appropriate, because it should only be used for interactive elements (which divs are NOT)

    The purpose of aria-labelledby is the same as that of aria-label . It provides the user with a recognizable, accessible name for an interactive element.

  2. the aria-describedby is not ideal either, since it should be used for a description (longer text) rather than a label (short text)

    The aria-labelledby and aria-describedby attributes both reference other elements to calculate text alternatives. aria-labelledby should reference brief text that provides the element with an accessible name. aria-describedby is used to reference longer content that provides a description.

klonos commented 1 year ago

I've retitled this issue, in order to not suggest a specific solution since none of the potential solutions I found seems to be ideal, and I've also set the "needs more feedback" label in order to get more ideas and figure this problem out.

As it is, using a <span> with the aria-describedby property added to the wrapper <div> would be better than using the <label> with the for attribute, which is wrong/inappropriate. However it is still not ideal, as the aria-describedby property is a good fit for descriptions (longer text) - not for labels (short text) ...and we cannot use aria-labelledby either, because it is meant to be used on interactive elements only.

At the same time, changing the <label> to a <span> would mean that we'd be changing the markup, which we don't do so not to break existing themes. The d.org issue discussed the idea of simply removing the for attribute, and leaving the <label> element, however that was deemed equally wrong or worse:

...is pointing screen readers nowhere better than pointing them to a div? Seems like it might actually be worse.

So, do we fix the wrong/inappropriate markup and potentially break themes, or do we leave things broken from a standards and a11y point of view? It's a pickle 🤔

PS: Also worth noting that a new '#label_for' element property was introduced and is taken into account in theme_form_element_label() in D7.92 as part of https://www.drupal.org/project/drupal/issues/2594955 (change record - commit). That change will eventually be cross-ported to Backdrop, and it will help us solve things, but it leaves things to the developer to fix manually and on a case-by-case basis - it won't automagically be outputting proper and accessible HTML for every label that points to non-labelable elements.