Form labeling within D7 is inconsistent, inflexible and bad for accessibility.

Inconsistent: Most form elements have their label generated by theme_form_element(), while others like radio and checkbox provide their own labels.

Inflexible: There is no easy way for a form developer to tell FAPI where to put the label, or if a label should be generated at all.

Accessibility: When developers don't want labels to appear visually for elements, they cannot easily indicate that the element's #title property should be used as the title attribute for the element, and not as a label. This results in many instances of developers not setting #title so that a label isn't generated, and the element has no title associated with it for assistive technology users. Also, element containers like date, radios and checkboxes are currently being themed with a div, and not the more accessible fieldset.

Related issues:
#504962: Provide a compound form element with accessible labels
#501444: Administer > Modules pages make improper use of form labels
#451980: Select boxes still not using labels
#522772: Improve radio and checkbox title and labeling features for accessibility
#318165: wrong HTML on admin/user/user filter
#368759: All labels should require a for attributepointing to a unique id

CommentFileSizeAuthor
#179 form-label-cleanup-v3.patch3.94 KBbowersox
#169 form-attribute-fix_2.patch789 bytesmgifford
#165 form-attribute-fix.patch447 bytesmgifford
#161 form-label-cleanup-v2.patch4.56 KBbowersox
#159 form-label-cleanup-v2.patch4.56 KBbowersox
#156 label-notitle.png212.22 KBmgifford
#156 label-before.png227.36 KBmgifford
#156 label-after.png206.67 KBmgifford
#156 label-none.png214.17 KBmgifford
#156 label-invisible.png193.21 KBmgifford
#153 form-label-cleanup-v1.patch2.51 KBmgifford
#151 forms-remove-blank-labels-1.patch828 bytesmgifford
#148 issue-558928-form-labeling-47.patch22.23 KBOwen Barton
#148 46-47.txt5.9 KBOwen Barton
#140 issue-558928-form-labeling-46.patch22.96 KBOwen Barton
#140 45-46.txt3.89 KBOwen Barton
#136 issue-558928-form-labeling-45.patch23.68 KBbowersox
#135 issue-558928-form-labeling-44.patch23.73 KBbowersox
#132 issue-558928-form-labeling-43.patch23.61 KBbowersox
#129 issue-558928-form-labeling-41.patch24.38 KBbowersox
#121 issue-558928-form-labeling-40.patch25.55 KBbowersox
#116 issue-558928-form-labeling-39.patch26.31 KBbowersox
#116 admin-shortcuts-before.gif41.32 KBbowersox
#116 admin-shortcuts-after.gif41.4 KBbowersox
#110 screen-capture-4.png66.72 KBmgifford
#110 screen-capture-5.png50.74 KBmgifford
#110 screen-capture-6.png59.28 KBmgifford
#110 screen-capture-7.png67.17 KBmgifford
#110 screen-capture-8.png62.47 KBmgifford
#109 issue-558928-form-labeling-37.patch22.84 KBbowersox
#103 issue-558928-form-labeling-30.patch21.09 KBbowersox
#95 issue-558928-form-labeling-24.patch21.92 KBmgifford
#92 issue-558928-form-labeling-23.patch21.94 KBOwen Barton
#91 issue-558928-form-labeling-22.patch21.94 KBOwen Barton
#83 issue-558928-form-labeling-21.patch21.46 KBOwen Barton
#80 issue-558928-form-labeling-20.patch18.92 KBmgifford
#78 issue-558928-form-labeling-19.patch19.11 KBOwen Barton
#76 issue-558928-form-labeling-18-shorter-approach.patch15.34 KBOwen Barton
#70 issue-558928-form-labeling-17-shorter-approach.patch11.1 KBOwen Barton
#66 issue-558928-form-labeling-16-shorter-approach.patch10.44 KBmgifford
#65 issue-558928-form-labeling-15-shorter-approach.patch7.18 KBOwen Barton
#63 issue-558928-fieldsets-non-cumulative-1.patch2.91 KBEverett Zufelt
#62 issue-558928-form-labeling-14-short-approach.patch12.9 KBmgifford
#57 issue-558928-form-labeling-13-short-approach.patch12.81 KBbowersox
#53 issue-558928-form-labeling-12-short-approach.patch12.69 KBbowersox
#52 issue-558928-form-labeling-11-short-approach.patch13.13 KBmgifford
#49 issue-558928-form-labeling-10-short-approach.patch12.44 KBbowersox
#47 issue-558928-form-labeling-10.patch20.32 KBmgifford
#45 issue-558928-form-labeling-9.patch19.06 KBOwen Barton
#44 issue-558928-form-labeling-8.patch19.82 KBmgifford
#41 issue-558928-form-labeling-7.patch44.7 KBEverett Zufelt
#33 issue-558928-form-labeling-6.patch44.85 KBmgifford
#27 issue-558928-form-labeling-5.patch18.62 KBmgifford
#25 issue-558928-form-labeling-3.patch12.55 KBbowersox
#23 issue-558928-form-labeling-2.patch12.94 KBmgifford
#12 issue-558928-form-labeling-1.patch12.47 KBmgifford
#8 beforepatch.png12.88 KBronald_istos
#8 afterpatch.png13.41 KBronald_istos
#1 issue-558928-form-labeling-0.patch11.97 KBEverett Zufelt
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Everett Zufelt’s picture

Status: Active » Needs review
FileSize
11.97 KB

The attached patch attempts to resolve some form labeling problems and should be considered preliminary.

1. Creates three static variables in system.module FORM_ELEMENT_SHOW_TITLE_BEFORE, ..._AFTER, ..._ATTRIBUTE to be used in determining if a form element #title should be rendered as a label before or after the element, or as the title attribute of the element.

2. Defines the element property #show_title for the element types textfield, password, textarea, select, radio, checkbox, file and sets the property value to the current behaviour (before for all elements except radio and checkbox, which are after).

3. Modifies theme_form_element() to test for #show_title and output labels appropriately.

4. modifies the theme functions for the elements with #show_title set to test for FORM_ELEMENT_SHOW_TITLE_ATTRIBUTE and sets the title attribute of the element accordingly.

5. Removes functionality from theme_radio() and theme_checkbox() that has them self labeling and places this in theme_form_element() to be consistent with other form elements.

What is missing is dealing with using the fieldset wrapper for date, radios and checkboxes element types, and possible options for displaying / hiding the legend (containing the #title property) associated with fieldsets.

Also, some work will need to be done to correct issues in core where titles are not being set, or are being set and then unset, for form elements where it would be more appropriate for accessibility purposes for a title to be set and #show_title to be set to attribute.

Status: Needs review » Needs work

The last submitted patch failed testing.

Everett Zufelt’s picture

The above patch failed at least one test in the following test classes, Bike shed, cancel account, content language settings, form_clean_id() test, Test date field. I will run the tests locally this afternoon to see what the exact issues are.

varunity’s picture

yea definitely would be good for form labeling to be more consistent in D7

+1

mgifford’s picture

1) Consistency helps everyone as it takes so much less documentation to figure it out.

2) I applied the patch successfully against my CVS install.

3) Seems to maintain the existing functioning of the forms

4) It also provides a lot more flexibility for everyone.

Mike

Brigadier’s picture

Haven't had a chance to try the patch yet but I'm in favour of some more consistency in form labels.

annmcmeekin’s picture

+1 for making form element labelling better. It's such an important issue for so many users.

ronald_istos’s picture

FileSize
13.41 KB
12.88 KB

All for this - caused enough headaches already. Applied patch and got some strange results on standards forms - see attached images.

Will try and figure out why and also spend some time on the logic of how this is/could be done as well.

- Ronald

Everett Zufelt’s picture

@Ronald

Thanks for the review. I'd love it if you could explain the strange results. Most people don't know, but I'm blind, and the image is not very useful to me.

ronald_istos’s picture

Hi Everett, apologies - while not new to Drupal I am new to the world of the Drupal issue queues so only just getting to know people.

The images are from the admin/people page and the effect of the patch is that text next to radio buttons is rendered partially hidden by the button, drop a line and is bold.

A cursory look around shows that this happens pretty much anywhere there is a radio button within the admin interface

Below is the html from that admin/people page - which partially explains the problem. Before the patch the label tag encloses the input type tag while after the patch the input type tag is outside and before the label tag.

I should also mention that I am on Leopard, Safari 4.03

HTML Before patch:

<label class="option" for="edit-filter-role"><input type="radio" id="edit-filter-role" name="filter" value="role"   class="form-radio" /> role</label>

HTML After patch:

<input type="radio" id="edit-filter-role" name="filter" value="role"   class="form-radio" />  <label for="edit-filter-role">role </label>
Everett Zufelt’s picture

@Ronald

Thanks for the explanation of the problem. I think that this is an issue that should be addressed with CSS. If this is only happening on admin pages I wonder if it is a quirk with the Seven theme.

Although wrapping the radio button in a label is an acceptable accessibility practice, I believe it is better for consistency to not wrap any element.

If this is a problem that exists in more than just Safari, or is not correctable with a CSS modification I can rewrite the patch to make the label for radios and checkboxes wrap their input elements.

mgifford’s picture

Status: Needs work » Needs review
FileSize
12.47 KB

Drat. Missed that. Thanks for pointing it out @Ronald.

I've tracked down the bad css, fixed the issue and re-rolled it against the latest version of the CVS.

Only substantive change is taking out the display: block; in the css.

Index: modules/system/system.css
===================================================================
RCS file: /cvs/drupal/drupal/modules/system/system.css,v
retrieving revision 1.61
diff -u -p -r1.61 system.css
--- modules/system/system.css	24 Aug 2009 03:11:34 -0000	1.61
+++ modules/system/system.css	29 Aug 2009 18:25:51 -0000
@@ -136,7 +136,6 @@ tr.merge-up, tr.merge-up td, tr.merge-up
   font-size: 0.85em;
 }
 .form-item label {
-  display: block;
   font-weight: bold;
 }
 .form-item label.option {

mgifford’s picture

Issue tags: +Review swap

Thought I'd toss this in for a review swap. I'm willing to test others code in exchange for good reviews of this patch.

Status: Needs review » Needs work

The last submitted patch failed testing.

mgifford’s picture

Status: Needs work » Needs review

Setting this back to needs review. Think Head's bust.

ronald_istos’s picture

Hi, applied the new patch against current head. It fixes the issue. Labels are next to radio button as they should be.

Minor change from before is that font-weight is bold while before it was normal. system.css says form-item labels should be bold but seven's reset.css has input items as font-weight:normal. So when input was enclosed in label it has the system.css directive overidden while now it does not. Aesthetically font-weight normal is nicer but that is something for seven to deal with I guess!

mgifford’s picture

Suppose a patch could set it in themes/seven/style.css in this patch.

Do you have a recommendation of CSS you'd like applied to make it nicer?

Send me some CSS & I can reroll the patch.

ronald_istos’s picture

To get it as it was with seven you just need to add

  font-weight:normal;
  font-size:12px;

to themes/seven/style.css after line 498

so that the end result styling for the div.form element is

div.form-item label {
  margin: 0;
  padding: 0;
  font-weight:normal;
  font-size:12px;
}

Status: Needs review » Needs work

The last submitted patch failed testing.

cburschka’s picture

Well, I reproduced at least one of the test failures locally.

Fail       Other      search.test                    228                       
    "Enter your keywords" found

It'd take about four hours for the unit test to run through completely, so I don't know about the others. It's likely they have the same cause.

Everett Zufelt’s picture

Some of these tests will fail, it's natural with a patch that changes the markup like this. Once we get finalization on the patch we can identify and modify all failing tests.

mgifford’s picture

@Arancaytar thanks. The simple tests (and correcting them) is confusing.

Ok, so from modules/search/search.test the function related is:

  function testFailedSearch() {
    $this->drupalLogin($this->searching_user);
    $this->drupalGet('search/node');
    $this->assertText(t('Enter your keywords'));

    $edit = array();
    $edit['keys'] = 'bike shed ' . $this->randomName();
    $this->drupalPost('search/node', $edit, t('Search'));
    $this->assertText(t('Consider loosening your query with OR. bike OR shed will often show more results than bike shed.'), t('Help text is displayed when search returns no results.'));
  }

Seems to be failing in bringing up this page with drupalGet. Comes up fine here http://localhost:8888/drupal-cvs/search/node

So I go take a look at the protected function drupalGet() here - modules/simpletest/drupal_web_test_case.php

Which just loads:

  /**
   * Retrieves a Drupal path or an absolute path.
   *
   * @param $path
   *   Drupal path or URL to load into internal browser
   * @param $options
   *   Options to be forwarded to url().
   * @param $headers
   *   An array containing additional HTTP request headers, each formatted as
   *   "name: value".
   * @return
   *   The retrieved HTML string, also available as $this->drupalGetContent()
   */
  protected function drupalGet($path, array $options = array(), array $headers = array()) {
    $options['absolute'] = TRUE;

    // We re-using a CURL connection here. If that connection still has certain
    // options set, it might change the GET into a POST. Make sure we clear out
    // previous options.
    $out = $this->curlExec(array(CURLOPT_HTTPGET => TRUE, CURLOPT_URL => url($path, $options), CURLOPT_NOBODY => FALSE, CURLOPT_HTTPHEADER => $headers));
    $this->refreshVariables(); // Ensure that any changes to variables in the other thread are picked up.

    // Replace original page output with new output from redirected page(s).
    if (($new = $this->checkForMetaRefresh())) {
      $out = $new;
    }
    $this->verbose('GET request to: ' . $path .
                   '<hr />Ending URL: ' . $this->getUrl() .
                   '<hr />' . $out);
    return $out;
  }

How am I supposed to tell why this test is failing?

If I can browse there, why can't simpleTest?

mgifford’s picture

Status: Needs work » Needs review
FileSize
12.94 KB

This should be the last patch for the form element labeling. It includes the modification for Seven proposed by @istos

I've tested this in FF, Safari, Chrome & Opera for the mac. It looks fine.

Status: Needs review » Needs work

The last submitted patch failed testing.

bowersox’s picture

Status: Needs work » Needs review
FileSize
12.55 KB

@mgifford

Here is an updated patch with a different suggestion for how to handle the CSS. In system.css we can simply flag individual radio and checkbox labels to display:inline and font-weight:normal:

-.form-item label.option {
+.form-item label.option,
+.form-type-checkbox label,
+.form-type-radio label {
   display: inline;
   font-weight: normal;
 }

With this approach we don't need to change Seven at all. This patch keeps everything visually unchanged in Seven, Garland/Minnelli and Stark. (The prior strategy of making all form labels display:inline broke the layout of lots of forms, such as fields within vertical tabs of /node/add/page.)

Also, there is one issue I found with the prior patch (#23) and this one. I'm assuming you might know how to fix it. It is no longer printing out labels that precede a group of checkboxes or radios. So a number of admin forms now have a missing label that makes them confusing, including:

  • 'Status' and 'Roles' labels are missing from Add Person (/admin/people/create)
  • 'Preview Before Submitting' is missing from /admin/structure/node-type/article
  • 'Apply to Content Types' is missing from /admin/structure/taxonomy/1

Everything else looks good, so once that is fixed and the tests work, I think this will be ready for RTBC. This is a great approach to getting consistent labels and accessibility in Drupal 7! Thanks for your work on this.

Status: Needs review » Needs work

The last submitted patch failed testing.

mgifford’s picture

Ok, so working from Patch 3 I think I may have tracked down the outstanding simpletest errors.

thanks for your support on this everyone.

Mike

mgifford’s picture

Status: Needs work » Needs review

Didn't put this to needs review earlier.

Needed to apply

'#show_title' => FORM_ELEMENT_SHOW_TITLE_BEFORE,

to all checkboxes and radios forms. including those in taxonomy.admin.inc, content_types.inc, search.module, profile.module, locale.module, form_test.module, user.module

Also needed to allow for form type item in form.inc:

  if ($element['#type'] == 'item') {
    $output .= '<h3>' . $element['#title'] . '</h3>';
  }
mattyoung’s picture

Status: Needs review » Needs work

subscribe

Everett Zufelt’s picture

Status: Needs work » Needs review

Setting to needs review to test most recent patch.

Status: Needs review » Needs work

The last submitted patch failed testing.

Everett Zufelt’s picture

Looks like we're getting closer with this patch, we're down to only 2 fails.

mgifford’s picture

Status: Needs work » Needs review
FileSize
44.85 KB

Hopefully this fixes it.

TheRec’s picture

Status: Needs review » Needs work
Issue tags: -Review swap

Code review only, I hope this helps.

+++ includes/form.inc	31 Aug 2009 13:08:01 -0000
@@ -1514,7 +1514,13 @@ function theme_select($element) {
+  return '<select name="' . $element['#name'] . '' . ($multiple ? '[]' : '') . '"' . ($multiple ? ' multiple="multiple"' : '') . $title . ' ' . drupal_attributes($element['#attributes']) . ' id="' . $element['#id'] . '" ' . $size . '>' . form_select_options($element) . '</select>';

Between $element['#name'] . and ($multiple ? '[]' : '') there is an empty string (two single quotes) which are of no use.

+++ includes/form.inc	31 Aug 2009 13:08:01 -0000
@@ -1654,15 +1660,19 @@ function theme_fieldset($element) {
+  if (isset($element['#show_title']) && $element['#show_title'] == FORM_ELEMENT_SHOW_TITLE_ATTRIBUTE && !empty($element['#title'])) {
@@ -1990,18 +2000,21 @@ function theme_text_format_wrapper($elem
+  if (isset($element['#show_title']) && $element['#show_title'] == FORM_ELEMENT_SHOW_TITLE_ATTRIBUTE && !empty($element['#title'])) {
@@ -2424,6 +2437,12 @@ function theme_textfield($element) {
+  if (isset($element['#show_title']) && $element['#show_title'] == FORM_ELEMENT_SHOW_TITLE_ATTRIBUTE && !empty($element['#title'])) {
@@ -2476,6 +2495,11 @@ function theme_form($element) {
+  if (isset($element['#show_title']) && $element['#show_title'] == FORM_ELEMENT_SHOW_TITLE_ATTRIBUTE && !empty($element['#title'])) {
@@ -2518,9 +2542,14 @@ function theme_markup($element) {
+  if (isset($element['#show_title']) && $element['#show_title'] == FORM_ELEMENT_SHOW_TITLE_ATTRIBUTE && !empty($element['#title'])) {
@@ -2554,7 +2583,13 @@ function form_process_weight($element) {
+  if (isset($element['#show_title']) && $element['#show_title'] == FORM_ELEMENT_SHOW_TITLE_ATTRIBUTE && !empty($element['#title'])) {

As an empty '#title' would not need to be displayed anyways, it should be more logical to put this part of the condition in the first position (so the following parts conditions are not evaluated for nothing), this is a very minor optimization. Looking how many times you do this same verification, you should maybe create a dedicated function to do it, would simplify the modifications if at some point we add another condition ?

+++ includes/form.inc	31 Aug 2009 13:08:01 -0000
@@ -2437,7 +2456,7 @@ function theme_textfield($element) {
+  $output .= '<input type="text"' . $title . $maxlength . ' name="' . $element['#name'] . '" id="' . $element['#id'] . '"' . $size . ' value="' . check_plain($element['#value']) . '"' . drupal_attributes($element['#attributes']) . ' />';
@@ -2484,7 +2508,7 @@ function theme_textarea($element) {
+  return '<textarea cols="' . $element['#cols'] . '" rows="' . $element['#rows'] . '" name="' . $element['#name'] . '" id="' . $element['#id'] . '"' . $title . ' ' . drupal_attributes($element['#attributes']) . '>' . check_plain($element['#value']) . '</textarea>';
@@ -2554,7 +2583,13 @@ function form_process_weight($element) {
+  return '<input type="file" name="' . $element['#name'] . '"' . $title . ' ' . ($element['#attributes'] ? ' ' . drupal_attributes($element['#attributes']) : '') . ' id="' . $element['#id'] . '" size="' . $element['#size'] . "\" />\n";

Not pretty readable, these could be split on multiple lines in the same fashion as the other theme functions.

+++ modules/field/modules/text/text.module	31 Aug 2009 13:08:07 -0000
@@ -157,6 +157,7 @@ function text_field_instance_settings_fo
+    '#show_title' => FORM_ELEMENT_SHOW_TITLE_BEFORE,s

What's this "s" doing at the end of the line ? Surely a typo I guess.

+++ modules/filter/filter.admin.inc	31 Aug 2009 13:08:08 -0000
@@ -36,7 +36,7 @@ function filter_admin_overview() {
+  $form['default'] = array('#type' => 'radios', '#options' => $options, '#default_value' => variable_get('filter_default_format', 1), '#show_title' => FORM_ELEMENT_SHOW_TITLE_BEFORE,);
+++ modules/user/user.admin.inc	31 Aug 2009 13:08:22 -0000
@@ -646,7 +649,7 @@ function user_admin_permissions($form_st
+    $form['checkboxes'][$rid] = array('#type' => 'checkboxes', '#options' => $options, '#default_value' => isset($status[$rid]) ? $status[$rid] : array(), '#show_title' => FORM_ELEMENT_SHOW_TITLE_BEFORE);
@@ -693,7 +696,7 @@ function theme_user_admin_permissions($f
+        $row[] = array('data' => drupal_render($form['checkboxes'][$rid][$key]), 'class' => array('checkbox'), 'title' => $roles[$rid] . ' : ' . t($key), '#show_title' => FORM_ELEMENT_SHOW_TITLE_BEFORE);

These arrays should be split into multiple lines and their elements should be indented as coding standards recommands.

This review is powered by Dreditor.

TheRec’s picture

Issue tags: +Review swap

Sorry, removed this tag by mistake.

Everett Zufelt’s picture

@TheRec

Thanks for the code review.

1. The two typos (unnecessary double '' and "s" at the end of the line) can be easily cleaned up.

2. You are right, the check for !empty($element['#title']) should be first in the logic in those if statements.

3. Can you recommend a function name / location if we were to generalize the title attribute test into one place? I think this could be useful, but not necessary, and may not make it in before code freeze.

4. I believe that the poor coding standards may mostly be there because the pre-existing poor code was poor, but we'll look at cleaning this up as well.

TheRec’s picture

Hmm not sure if there are conventions for this in FAPI, there is no similar type of function in the file, but it could be something like this :

/**
 * Indicates if the title attribute of the element should be used or not.
 * 
 * @param $element
 *   An associative array containing the properties of the element to evaluate.
 * @return
 *   A boolean, TRUE when title should be used, FALSE when title should be
 *   ignored.
 */
function form_element_must_display_title($element) {
  return !empty($element['#title']) && isset($element['#show_title']) && $element['#show_title'] == FORM_ELEMENT_SHOW_TITLE_ATTRIBUTE;
}
ronald_istos’s picture

Would it make sense to have to have '#show_title' => FORM_ELEMENT_SHOW_TITLE_BEFORE as the default rather than adding it to code in every case?

That would make it easier to get things through it seems.

Everett Zufelt’s picture

@istos

Each form element type does have a default show position. Some forms in core are not designed correctly so the positioning had to be set explicitly. Hopefully this bad code can be cleaned up after code freeze.

mgifford’s picture

Status: Needs work » Needs review

Just to clarify. the goal of this issue is to improve the output of the forms API so that it is more accessible.

Seems that there are some outstanding legacy issues with the forms API that should be cleaned up. I'd really encourage someone to open up a new issue queue and propose some code changes to ensure that this happens. It's quite possible that this type of work could be cleaned up after the feature freeze.

If there was an easy way to determine which of the existing forms performed in a non-standard way that would be great. However, I wanted to verify that the patch in #33 worked and ensure that this where the two out-standing problems were.

Everett Zufelt’s picture

This patch fixes the two typos in 36.1 and the order of comparison operators in 36.2

I do not believe that we need to address using a different function for comparison logic as in #35 / 36.3 and hopefully coding standards 36.4 and excessive use of explicit assignment of #show_title (done for testing purposes) in #37 are addressed in a subsequent patch.

mgifford’s picture

Can take quite a while to backtrack things in simpleTest.

Found that I needed to add the title attribute in the profile.module

      case 'date':
        $fields[$category][$field->name] = array('#type' => 'date',
          '#title' => check_plain($field->title),
          '#default_value' => isset($edit[$field->name]) ? $edit[$field->name] : '',
          '#description' => _profile_form_explanation($field),
          '#required' => $field->required,
          '#show_title' => FORM_ELEMENT_SHOW_TITLE_BEFORE,
        );

This is by looking at http://testing.drupal.org/pifr/file/1/issue-558928-form-labeling-5_0.patch

Matching this -> Test date field 37 1 0

To results to running tests on the profile module isn't very intuitive

I'm now just running everything in order to try to find -> Content language settings 63 1 1

The last known bug...

Status: Needs review » Needs work

The last submitted patch failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
19.82 KB

This catches the substantive changes that were made in 6 & 7, but not all of it. It's a lighter patch that should pass the tests.

Brandon & I got it down to an extra space in:

+  return '<select name="' . $element['#name'] . ($multiple ? '[]' : '') . '"' . ($multiple ? ' multiple="multiple"' : '') . $title . ' ' . drupal_attributes($element['#attributes']) . ' id="' . $element['#id'] . '" ' . $size . '>' . form_select_options($element) . '</select>';

That we've removed in this patch.

Owen Barton’s picture

This wraps the 7 calls to:

if (!empty($element['#title']) && isset($element['#show_title']) && $element['#show_title'] == FORM_ELEMENT_SHOW_TITLE_ATTRIBUTE) {

up in a simple helper function that reduces code duplication.

Everett Zufelt’s picture

If we can get some eyes on the most recent patch in #45 to make sure that it is not causing any visual abnormalities in core we can set this to RTBC and move on to wrapping radios, checkboxes and date with a fieldset. This will go a * long * way to improving D7 accessibility.

mgifford’s picture

Ok, here's another patch.

Btw, for all Mac/Linux users I'd heavily suggest looking at http://meld.sourceforge.net for reviewing these diffs.

Everett Zufelt’s picture

@mgifford

Can you please explain the changes in the patch for comment #47?

bowersox’s picture

Please review an alternative approach in the updated patch. There are 2 changes from #47:

  1. In system.module I added a default setting of FORM_ELEMENT_SHOW_TITLE_BEFORE to 'radios' and 'checkboxes' (which previously had no default).
  2. I removed all the changes to core forms with radios and checkboxes (locale.module, content_types.inc, node.module, profile.admin.inc, profile.module, form_test.module, taxonomy.admin.inc, user.admin.inc, user.module). Now that we set a default, we don't need to manually set all those. The only files we need to have in this patch are form.inc, system.css, and system.module. This patch is a lot smaller and does the same thing.

@mgifford and @Everett

I hope this makes sense. Let me know how I can help to move this to RTBC one way or another.

I tested this on a handful of forms and they all look the same as before without modifying their code: /admin/people/create, /search, /admin/structure/taxonomy/1, /admin/config/people/accounts, /admin/content, /admin/appearance/settings.

Status: Needs review » Needs work

The last submitted patch failed testing.

mgifford’s picture

Status: Needs work » Needs review

@Everett, to get back to the changes I made. I haven't reviewed the latest patch from @brandon yet.

A big chunk of this is in patch 9 which Owen rolled. I'll try to explain jumps issue-558928-form-labeling-8.patch ro issue-558928-form-labeling-10.patch though.

In form.inc many of the changes are centralizing the management of the 'title' element in the function form_element_title_attribute($element). The function is below:

 /**
+ * Return an attribute string if a form element's title should be output
+ * as a title attribute.
+ * 
+ * @param $element
+ *   An associative array containing the properties of the element.
+ *   Properties used: #title, #value, #options, #description, #extra, #multiple,
+ *   #required, #name, #attributes, #size.
+ * @return
+ *   A string " title=" and the element #title if #show_title is set to
+ *   FORM_ELEMENT_SHOW_TITLE_ATTRIBUTE, otherwise an empty string. 
+ */
+function form_element_title_attribute($element) {
+  if (!empty($element['#title']) && isset($element['#show_title']) && $element['#show_title'] == FORM_ELEMENT_SHOW_TITLE_ATTRIBUTE) {
+    return ' title="' . $element['#title'] . '"';
+  }
+  return '';
+}

The goal was to create a helper function to shorten repetitive code like:

if (!empty($element['#title']) && isset($element['#show_title']) && $element['#show_title'] == FORM_ELEMENT_SHOW_TITLE_ATTRIBUTE) {

I've added a call to modules/node/node.module & modules/user/user.admin.inc to fix bugs identified by Brandon to ensure that the 'Advanced Search' area at /search's label called "Only of the type(s)" was displayed above the checkboxes and also also /admin/config/people/accounts was missing the label "Who can register accounts?".

mgifford’s picture

Identical to the previous patch, except that it's got the date issue fixed for simpletest. profile.module busted it again:

     case 'date':
       $fields[$category][$field->name] = array('#type' => 'date',
         '#title' => check_plain($field->title),
         '#default_value' => isset($edit[$field->name]) ? $edit[$field->name] : '',
         '#description' => _profile_form_explanation($field),
         '#required' => $field->required,
         '#show_title' => FORM_ELEMENT_SHOW_TITLE_BEFORE,
bowersox’s picture

Here's another update with only a change to how the 'Test date field' is fixed. This patch fixes it in system.module by giving all date fields a default FORM_ELEMENT_SHOW_TITLE_BEFORE--that matches the prior behavior so we pass the test without needing to change profile.module.

@mgifford

I'll be offline (sleeping) but back online around 8:30am Central time. Thanks for all your continued work to push this ahead!

dmitrig01’s picture

+++ includes/form.inc	1 Sep 2009 03:47:52 -0000
@@ -1494,6 +1494,25 @@ function form_options_flatten($array, $r
+ * 
+ * @param $element
+ *   An associative array containing the properties of the element.
+ *   Properties used: #title, #value, #options, #description, #extra, #multiple,
+ *   #required, #name, #attributes, #size.

Where are those properties used?

+++ includes/form.inc	1 Sep 2009 03:47:52 -0000
@@ -1514,7 +1533,7 @@ function theme_select($element) {
-  return '<select name="' . $element['#name'] . '' . ($multiple ? '[]' : '') . '"' . ($multiple ? ' multiple="multiple" ' : '') . drupal_attributes($element['#attributes']) . ' id="' . $element['#id'] . '" ' . $size . '>' . form_select_options($element) . '</select>';
+  return '<select name="' . $element['#name'] . ($multiple ? '[]' : '') . '"' . ($multiple ? ' multiple="multiple"' : '') . form_element_title_attribute($element) . drupal_attributes($element['#attributes']) . ' id="' . $element['#id'] . '" ' . $size . '>' . form_select_options($element) . '</select>';

Boo for making a long line longer. also the multiple part can be

($multiple ? '[]" multiple="multiple' : '"')

What if someone sets the attribute "title" in drupal_attributes? Then you'll get two 'title' attributes.
Howa bout making form_element_title attribute accept &$element and just adding $element['#attributes']['title'] if it doesn't already exist?

+++ includes/form.inc	1 Sep 2009 03:47:52 -0000
@@ -2584,17 +2596,31 @@ function theme_form_element($element) {
+  if ($element['#type'] == 'item') {
+    $output .= '<h3>' . $element['#title'] . '</h3>';
+  }

Why special case for item? shouldn't this go in theme_item?

+++ includes/form.inc	1 Sep 2009 03:47:52 -0000
@@ -2584,17 +2596,31 @@ function theme_form_element($element) {
+  if (isset($element['#show_title']) && $element['#show_title'] == FORM_ELEMENT_SHOW_TITLE_BEFORE) {
+    $output .= $label . " " . $element['#children'] . "\n";
+  }
+  else if (isset($element['#show_title']) && $element['#show_title'] == FORM_ELEMENT_SHOW_TITLE_AFTER) {
+    $output .= $element['#children'] . " " . $label . "\n";
+  }
+  else {
+    $output .= " " . $element['#children'] . "\n";

" " can be ' '

+++ modules/system/system.module	1 Sep 2009 03:47:52 -0000
@@ -88,6 +88,25 @@ define('REGIONS_ALL', 'all');
 /**
+ *
+ * Output form element titles as labels before form elements. @see system_elements().
+ */
+define('FORM_ELEMENT_SHOW_TITLE_BEFORE', 'before');
+
+/**
+ *
+ * Output form element titles as labels after form elements. @see system_elements().
+ */
+define('FORM_ELEMENT_SHOW_TITLE_AFTER', 'after');
+
+/**
+ *
+ * Output form element titles as the title attribute of form elements. @see system_elements().
+ */
+define('FORM_ELEMENT_SHOW_TITLE_ATTRIBUTE', 'attribute');

What if I want to make the title show before and in an attribute? Also please wrap comments at 80 chars.

+++ modules/system/system.module	1 Sep 2009 03:47:52 -0000
@@ -88,6 +88,25 @@ define('REGIONS_ALL', 'all');
+define('FORM_ELEMENT_SHOW_TITLE_ATTRIBUTE', 'attribute');
+
+
+/**

No need for 2 line breaks

This review is powered by Dreditor

mgifford’s picture

It applies well. I haven't discovered any visual gotcha's. It will need an accessibility review though as it's had a few round of enhancements.

Everett Zufelt’s picture

@dmitrig01

I will only address a couple of your comments here, hopeuflly someone else can address the rest.

1. Can you please provide a use case for having a label and title attribute both for a form element?

2. theme_item() doesn't exist in d7. I suppose we could recreate this.

bowersox’s picture

This patch has no functionality changes, just code clean up as suggested by @dmitrig01 in #54.1 (code comment correction), #2 including $multiple, #5 (' ') and #7 (extra line break).

I didn't address #54.3 about the title being handled by drupal_attributes, or #54.4 theme_item() or #54.6 which @Everett responded to in #56.

mgifford’s picture

+1

I think we may have it here! Think this is a much lighter implementation that previous patches. It applies nicely to the CVS. We've addressed the outstanding issues that have been raised. This patch improves consistency of form labeling. By adding labels by default we are improving accessibility!

What else do we need to do to switch this to RTBC?

TheRec’s picture

Status: Needs review » Needs work

I don't think we should address the possibility of coders using title attribute through #title AND drupal_attribues(), otherwise we should address it for "name", "multiple" and "id" (not to mention all the other places where drupal_attributes is used in core ;)). It's the responsability of the coders to know what the functions (in this case FAPI) they use can output...

A thing that was not completely addressed in the previous patches :

+++ includes/form.inc	1 Sep 2009 14:03:40 -0000
@@ -2437,7 +2456,7 @@ function theme_textfield($element) {
+  $output .= '<input type="text"' . form_element_title_attribute($element) . $maxlength . ' name="' . $element['#name'] . '" id="' . $element['#id'] . '"' . $size . ' value="' . check_plain($element['#value']) . '"' . drupal_attributes($element['#attributes']) . ' />';
@@ -2484,7 +2502,7 @@ function theme_textarea($element) {
+  return '<textarea cols="' . $element['#cols'] . '" rows="' . $element['#rows'] . '" name="' . $element['#name'] . '" id="' . $element['#id'] . '"' . form_element_title_attribute($element) . ' ' . drupal_attributes($element['#attributes']) . '>' . check_plain($element['#value']) . '</textarea>';
@@ -2518,9 +2536,8 @@ function theme_markup($element) {
+  $output = '<input type="password" name="' . $element['#name'] . '" id="' . $element['#id'] . '" ' . form_element_title_attribute($element) . $maxlength . $size . drupal_attributes($element['#attributes']) . ' />';
@@ -2554,7 +2571,7 @@ function form_process_weight($element) {
+  return '<input type="file" name="' . $element['#name'] . '"' . form_element_title_attribute($element) . ' ' . ($element['#attributes'] ? ' ' . drupal_attributes($element['#attributes']) : '') . ' id="' . $element['#id'] . '" size="' . $element['#size'] . "\" />\n";

I still think those would be way more readable on multiple lines, as it is done in other theme functions. (For the ones with return, we should use an $output variable)

This review is powered by Dreditor.

mgifford’s picture

So you'll approve the patch with the changes you've mentioned to code line length & the use of $output variable rather than strict return?

Can you re-roll the patch for this one? I've done it a bunch on this issue.

dmitrig01’s picture

Generally, you're not allowed to review your own patch (or one you've contributed to).

+++ includes/form.inc	1 Sep 2009 14:03:40 -0000
@@ -1510,11 +1528,17 @@ function form_options_flatten($array, $r
+  $select = '<select name="' . $element['#name'];
+  $select .= ($multiple ? '[]" multiple="multiple' : '"');

Needs an ending " in the multiple part.

+++ includes/form.inc	1 Sep 2009 14:03:40 -0000
@@ -2437,7 +2456,7 @@ function theme_textfield($element) {
-  $output .= '<input type="text"' . $maxlength . ' name="' . $element['#name'] . '" id="' . $element['#id'] . '"' . $size . ' value="' . check_plain($element['#value']) . '"' . drupal_attributes($element['#attributes']) . ' />';
+  $output .= '<input type="text"' . form_element_title_attribute($element) . $maxlength . ' name="' . $element['#name'] . '" id="' . $element['#id'] . '"' . $size . ' value="' . check_plain($element['#value']) . '"' . drupal_attributes($element['#attributes']) . ' />';

Please break into multiple lines.

+++ includes/form.inc	1 Sep 2009 14:03:40 -0000
@@ -2484,7 +2502,7 @@ function theme_textarea($element) {
-  return '<textarea cols="' . $element['#cols'] . '" rows="' . $element['#rows'] . '" name="' . $element['#name'] . '" id="' . $element['#id'] . '" ' . drupal_attributes($element['#attributes']) . '>' . check_plain($element['#value']) . '</textarea>';
+  return '<textarea cols="' . $element['#cols'] . '" rows="' . $element['#rows'] . '" name="' . $element['#name'] . '" id="' . $element['#id'] . '"' . form_element_title_attribute($element) . ' ' . drupal_attributes($element['#attributes']) . '>' . check_plain($element['#value']) . '</textarea>';

Please break into multiple lines.

+++ modules/system/system.module	1 Sep 2009 14:03:41 -0000
@@ -86,6 +86,24 @@ define('REGIONS_VISIBLE', 'visible');
+/**
+ *
+ * Output form element titles as the title attribute of form elements. @see system_elements().

Please wrap this comment at 80 chars.

mgifford’s picture

Status: Needs work » Needs review
FileSize
12.9 KB

@dmitrig01 - accessibility is an area of expertise where there really aren't more than a handful of people who can contribute a substantive review. From a purely coding perspective, there aren't enough developers who understand why this is important enough to bother reviewing. This team's been pushing very hard on this for the last six months and have gotten very little response from the community, despite the fact that these changes can help so very many people be able to use & administer Drupal sites.

I'm sure that they will all improve the readability of the PHP and as a general code cleanup for D7 I can see this as relevant. Most of these suggestions are geared at cleaning up legacy code (that we're improving by ensuring that we have more consistent form output). Thanks for pointing out the missing quote!

I'm hoping this might be it and we'll be ready to roll with this.

Everett Zufelt’s picture

* I have not tested, but for the moment this non-cumulative patch needs to be applied separately from the other patch. If you attempt to apply both to head at the same time one will likely not apply correctly.

This is a preliminary non-cumulative patch to attempt to wrap the date, radios, and checkboxes form element types in the theme_fieldset wrapper.

1. Adds #theme_wrapper 'fieldset' for date, radios and checkboxes element types.

2. Adds a #show_legend property to radios, checkboxes and date (defaulting to true.

3. Adds logic to theme_fieldset to hide the legend with class="element-invisible" if #show_legend is FALSE.

Some markup and CSS changes will certainly be necessary.

Also, theme_fieldset currently outputs $element['#value'] just prior to the closing fieldset tag. I don't believe hat this is actually used anywhere, as the #value property is, to the best of my knowledge, never intended for visual output to users. If someone can confirm this we can get rid of this, else we can place a logic test and only output $element['#value'] if the element type is not date, radios or checkboxes.

Status: Needs review » Needs work

The last submitted patch failed testing.

Owen Barton’s picture

Here is an even shorter approach to #62, inserting the title as an #attribute before it even hits the theme functions. I think this approach is nicer, although I don't think it is quite working correctly with radios/checkboxes (you get doubled labels) - should be easy enough to figure out though.

mgifford’s picture

Ok, so you've added the following code to the function form_builder() so that the title attribute is automatically appended to forms at the root if there is a title and it is identified as being shown

  // Set the elements title attribute to the elements #title, if set to do so
  // by #show_title.
  if (!empty($element['#title']) && isset($element['#show_title']) && $element['#show_title'] == FORM_ELEMENT_SHOW_TITLE_ATTRIBUTE) {
    $element['#attributes']['title'] = $element['#title'];
  }

Because you've approached it this way, you don't need the form_element_title_attribute() function. However right now radio buttons & checkboxes are failing pretty badly.

As an aside is "$select = '';" needed at the top of the function theme_select()?

I've put form_element_title_attribute() to keep the functionality working until there's a better replacement.

I've also put back some of the spacing so that it's easier to see the changes from the prior version. I don't see any reason this can't work for most form fields.

Everett Zufelt’s picture

Assigned: Everett Zufelt » Unassigned
Status: Needs work » Needs review

@Owen

Nice idea on using the title attribute of the #attributes property. I wish it had of worked better.

@Mike

Thanks for undoing the change that didn't seem to quite work. You say "I've also put back some of the spacing so that it's easier to see the changes from the prior version. I don't see any reason this can't work for most form
fields.". Can you please clarify, you don't see why "what" can't work, and for what form fields does it not work?

Although I see the benefit of the proposed improvements in #65, let's try to get #66 RTBC without adding any extra functionality or improvements. We can worry about cleanup after code freeze.

Everett Zufelt’s picture

Applied the patch in #66 to clean head. Applied properly and seems to have no problems properly associating labels with form fields. even fixes #501444: Administer > Modules pages make improper use of form labels. Unless someone else notices a bug I think this is ready to be RTBC.

We then need to document, and clean up any legacy code that doesn't conform to coding standards.

Hopefully after this we can work on refining the patch in #63 to wrap radios, checkboxes and date form element types in fieldsets.

Cliff’s picture

Trying to help with little time in a discussion that is way over my head. I just now reread Drupal's guidance for helping evaluate patches, and one thing jumps out at me: Is this a hack that papers over problems or an actual solution? It seems to me that this is the first attempt at a global solution to remove, if not "hacks," previous local solutions to a broader problem. Even if imperfect, isn't it a vast improvement in consistency?

I am not set up to evaluate code, but I can vouch for the fact that fixing this problem will vastly improve the accessibility of Drupal to the visually disabled. In doing so, it will make Drupal a viable option for any entities that are required to produce accessible sites. In North America, this includes virtually all levels of government. I work for a state governmental agency and participate in a forum for Web content managers at all levels of government in the United States. Many are awaiting the release of Drupal 7 to see if its output will be accessible enough for them to use it in their sites. This patch will systematically improve the labeling of form elements, and in my opinion the inability to label form elements properly is the most significant barrier to accessibility remaining in D6. I don't see how D7 can be an effective release without fixing it.

@dmitrig01: Mike (mgifford) has been working in this area for at least a year with little support. To that extent, he is one of the unsung heroes of Drupal's move toward world domination. ;-) If it's impossible to move this issue to RTBC as it now stands, what can be done to get Mike and Everett the help they need to get it there in time for D7 to include it?

FWIW, this experience has led me to resolve to try to find someone local who can help me set up to evaluate patches and thus participate more effectively in this process in the future.

Owen Barton’s picture

I took some time tonight to really dig into this and I think I have found an approach that I like a lot.

Summary of changes from previous patch:
- There is a new #label_option element attribute that is used to flag an element label as an option such that it is displayed (normally) inline. This fixes the checkbox/radio problem, and also means we can remove the label adding code from these theme functions (woohoo, consistency!) also and also the css code we had in previously.
- There is now a new "theme_form_element_label" function. This abstracts out this code to increase readability, and also means that we don't theme a label if we don't use it.
- Fixes an issue (I think) with previous patches in that if FORM_ELEMENT_SHOW_TITLE_ATTRIBUTE was used there was NO indication of required fields at all, which is both a usability and accessibility issue. WIth this patch you get the on it's own (no label). If you don't specify anything for #show_title there is still potentially an issue here, although it's hard to know what the expected behavior should be here so I didn't do anything for now (return '';).
- The label generation code is cleaned up, but (above bug excepted) the overall logic and output should be the same as with previous patches.
- Quite a bit of work on inline documentation, also making sure the "properties used" list in theme functions is correct, and documenting the new element properties in what is (I hope!) the right place in the API docs.
- Also removes the "$select = '';" dead code that Mike noticed.

Everett Zufelt’s picture

I really believe that the most recent patches have complicated the code for this issue. I am not saying that the concept and implementation aren't good. But practically speaking we need to go back to the patch in #62 and refine / RTBC from there, it is to late in the game to add features (and yes I believe that generlizing functionality that does not * need * to be generalized is a feature).

Everett Zufelt’s picture

Review of patch in #62

1. form_element_title_attribute should be themeable and handle required fields.

2. This patch is not dealing with the item element type consistently. All other form element types get a label for their #title attribute, but item is getting an h3. I'm not sure what the best approach is here, but to stay consistent it is worth pointing out that item is themed by theme_markup (see system.module:system_elements()).

Item: Generate a display-only form element allowing for an optional title and description. (http://api.drupal.org/api/drupal/developer--topics--forms_api_reference....).

Perhaps we can add the #show_title property to the item element type and get rid of the custom code in theme_form_element()?

mgifford’s picture

I'm agreed that for getting RTBC the focus should be on patch #62.

Wanted to compare the two though as there are some interesting improvements here to the forum code that are worth considering, either after this functionality gets into core or in a separate issue.

#62 touches these files
- includes/form.inc
- modules/system/system.css
- modules/system/system.module

#70 touches these files
- includes/common.inc
- includes/form.inc
- modules/system/system.api.php
- modules/system/system.module

The user filter /admin/people displays properly, which is I believe the reason for adding this to the system.css

.form-item label.option,
.form-type-checkbox label,
.form-type-radio label {
  display: inline;
  font-weight: normal;
}

To function drupal_common_theme(), patch #70 added:

    'form_element_label' => array(
      'arguments' => array('element' => NULL),
    ),

You've added inline docs to the function hook_elements() with #70:

 *  - "#show_title": optionally one of the FORM_ELEMENT_SHOW_TITLE_* constants
 *    indicating if and how the #title should be displayed.
 *  - "#label_option": add a class to style the element label as an option.

To modules/system/system.module you've added this new class to style the element:

  $type['radio'] = array(
    '#input' => TRUE,
    '#default_value' => NULL,
    '#process' => array('ajax_process_form'),
    '#theme' => 'radio',
    '#theme_wrappers' => array('form_element'),
    '#show_title' => FORM_ELEMENT_SHOW_TITLE_AFTER,
    '#label_option' => TRUE,
  );

...

  $type['checkbox'] = array(
    '#input' => TRUE,
    '#return_value' => 1,
    '#process' => array('ajax_process_form'),
    '#theme' => 'checkbox',
    '#theme_wrappers' => array('form_element'),
    '#show_title' => FORM_ELEMENT_SHOW_TITLE_AFTER,
    '#label_option' => TRUE,
  );

Then there's the form.inc which where #70 inserted the element title into form_builder() as I described in comment #66

#70 seems to work properly now without form_element_title_attribute()

Concerns about the code lines extending over 80 characters would need to be addressed again. Still don't see why that would be required to get into core, but....

Theme theme_checkbox() & theme_radio() are now having titles inserted by changes made to modules/system/system.module

Modifications to theme_form_element() are big. This is taken out of patch #62:

  $required = !empty($element['#required']) ? '<span class="form-required" title="' . $t('This field is required.') . '">*</span>' : '';

  $label = '';

  if (!empty($element['#title'])) {
    $title = $element['#title'];
    if (!empty($element['#id'])) {
      $label = ' <label for="' . $element['#id'] . '">' . $t('!title !required', array('!title' => filter_xss_admin($title), '!required' => $required)) . "</label>\n";
    }
    else {
      $label= ' <label>' . $t('!title !required', array('!title' => filter_xss_admin($title), '!required' => $required)) . "</label>\n";
    }
  }

and the functionality is largely replaced with the new function theme_form_element_label() which is well documented and seems to be working.

/**
 * Theme a form element label and required mark.
 *
 * @param element
 *   An associative array containing the properties of the element.
 *   Properties used: #required, #show_title, #title, #label_option, #id
 * @return
 *   A string representing the form element label.
 *
 * @ingroup themeable
 */
function theme_form_element_label($element) {
  // This is also used in the installer, pre-database setup.
  $t = get_t();

  $required = !empty($element['#required']) ? '<span class="form-required" title="' . $t('This field is required.') . '">*</span>' : '';
  // If the title is in an attribute we simply return any required mark here.
  if (isset($element['#show_title']) && $element['#show_title'] == FORM_ELEMENT_SHOW_TITLE_ATTRIBUTE) {
    return $required;
  }

  $title = $element['#title'];
  // We don't want to return an empty tag.
  if (!empty($title) || !empty($required)) {
    $attributes = array();
    if (isset($element['#label_option']) && $element['#label_option']) {
      // This class styles the label as an option, inline with the element.
      $attributes['class'] = 'option';
    }
    if (!empty($element['#id'])) {
      $attributes['for'] = $element['#id'];
    }
    return ' <label' . drupal_attributes($attributes) . '>' . $t('!title !required', array('!title' => filter_xss_admin($title), '!required' => $required)) . "</label>\n";
  }
  return '';
}

This is now largely used by this piece of code within theme_form_element():

  if (isset($element['#show_title'])) {
    // Insert label in correct position, depending on #show_title
    if ($element['#show_title'] == FORM_ELEMENT_SHOW_TITLE_AFTER) {
      $output .= $element['#children'] . ' ' . theme('form_element_label', $element) . "\n";
    }
    else {
      // FORM_ELEMENT_SHOW_TITLE_AFTER or FORM_ELEMENT_SHOW_TITLE_ATTRIBUTE
      $output .= theme('form_element_label', $element) . ' ' . $element['#children'] . "\n";
    }
  }
  else {
    // No #show_title is set, so we continue with no label.
    $output .= ' ' . $element['#children'] . "\n";
  }

These look like good changes. I do think that it does make for a better implementation. Just hard to have so many changes introduced so close to what we think of as our deadline.

Mike

catch’s picture

We have a 5 week code slush which this ought to come under, and accessibility improvements can be made until November some time, so I think it's worth continuing with grugnog's patch, especially since we're looking at < 12k patch.

A couple of questions:

Seems like we should add #show_title to element_basic_defaults() (probably defaulting to SHOW_TITLE_BEFORE)? Since most of the lines in the patch just seem to be adding that in.

Then it's a case of

-  if (isset($element['#show_title'])) {
+  if (!empty($element['#show_title'])) {

And we can remove the @todo for what to do when it's empty, since it'll be specifically declared as false then. This would lessen the API change as well since it'd be an optional attribute.

And a few nitpicks:

+++ includes/form.inc	2 Sep 2009 08:32:04 -0000
@@ -1082,6 +1082,11 @@ function form_builder($form_id, $element
+  // Set the elements title attribute to the elements #title, if needed.

Should be "element's" I think.

+++ includes/form.inc	2 Sep 2009 08:32:04 -0000
@@ -2582,19 +2579,25 @@ function theme_form_element($element) {
+    // Insert label in correct position, depending on #show_title

Missing a period, and the next comment too.

+++ includes/form.inc	2 Sep 2009 08:32:04 -0000
@@ -2606,6 +2609,43 @@ function theme_form_element($element) {
+ *   Properties used: #required, #show_title, #title, #label_option, #id
+ * @return

Needs a line break before @return.

This review is powered by Dreditor.

mgifford’s picture

Wanted to note here that there still don't seem to be labels or titles applied to the file upload form. I do think we should have some way to better highlight this.

@Everett's post #72 still applies to patch #70 as the H3 is still being used for the form's item element type.

Not sure if more logic could be put into the theme_markup() function:
http://api.drupal.org/api/function/theme_markup/7

I applied the H3 to the patch in #27 to the theme_form_element() function and it stuck. Now the form that called it was probably incorrectly, but it was one of the simpletest fails. and was not being expressed in the new code:

  "Enter your keywords" found	Other	search.test	228	SearchBikeShed->testFailedSearch()

The problem I ran into was in the search_form() module that the Bike shed test tripped over:

  $form['basic'] = array('#type' => 'item', '#title' => $prompt, '#id' => 'edit-keys');

Now, there probably just weren't tests to find the text in the following:

includes/locale.inc's locale_translate_edit_form():

  if (!empty($source->context)) {
    $form['context'] = array(
      '#type' => 'item',
      '#title' => t('Context'),
      '#markup' => check_plain($source->context),
    );
  }

and _locale_languages_common_controls() form:

    $form['langcode_view'] = array(
      '#type' => 'item',
      '#title' => t('Language code'),
      '#markup' => $language->language
    );

modules/comment/comment.module in comment_form():

      $form['_author'] = array(
        '#type' => 'item',
        '#title' => t('Your name'),
        '#markup' => theme('username', $user),
      );

modules/contact/contact.pages.inc in contact_personal_form():

  $form['from'] = array('#type' => 'item',
    '#title' => t('From'),
    '#markup' => theme('username', $user) . ' &lt;' . check_plain($user->mail) . '&gt;',
  );
  $form['to'] = array('#type' => 'item',
    '#title' => t('To'),
    '#markup' => theme('username', $recipient),
  );

modules/field/modules/list/list.module in list_field_settings_form():

  $form['allowed_values_function_display'] = array(
    '#type' => 'item',
    '#title' => t('Allowed values list'),
    '#markup' => t('The value of this field is being determined by the %function function and may not be changed.', array('%function' => $settings['allowed_values_function'])),
    '#access' => !empty($settings['allowed_values_function']),
  );

modules/image/image.admin.inc in image_style_form():

  $form['preview'] = array(
    '#type' => 'item',
    '#title' => t('Preview'),
    '#markup' => theme('image_style_preview', $style),
  );

modules/menu/menu.admin.inc in menu_edit_item():

    $form['menu']['_path'] = array(
      '#type' => 'item',
      '#title' => t('Path'),
      '#description' => l($item['link_title'], $item['href'], $item['options']),
    );

and menu_configure():

  $form['intro'] = array(
    '#type' => 'item',
    '#markup' => t('The menu module allows on-the-fly creation of menu links in the content authoring forms. The following option sets the default menu in which a new link will be added.'),
  );

modules/system/system.module system_block_configure():

    $form['wrapper']['preview'] = array(
      '#type' => 'item',
      '#title' => 'Preview',
      '#markup' => theme('image', $image_path, t('Powered by Drupal, an open source content management system'), t('Powered by Drupal, an open source content management system'), array('class' => array('powered-by-preview')), FALSE),
    );

modules/upload/upload.admin.inc in upload_admin_settings():

  $form['settings_general']['upload_max_resolution'] = array(
    '#type' => 'item',
    '#title' => t('Maximum resolution for uploaded images'),
    '#description' => t('The maximum allowed image size (e.g. 640x480). Set to 0x0 for no restriction. If an <a href="!image-toolkit-link">image toolkit</a> is installed, files exceeding this value will be scaled down to fit.', array('!image-toolkit-link' => url('admin/config/media/image-toolkit'))),
    '#prefix' => '<div class="form-item-wrapper form-item-resolution">',
    '#suffix' => '</div>',
  );

modules/user/user.admin.inc in user_admin_settings():

  $form['registration_cancellation']['user_cancel_method'] = array(
    '#type' => 'item',
    '#title' => t('When cancelling a user account'),
    '#description' => t('Users with the %select-cancel-method or %administer-users <a href="@permissions-url">permissions</a> can override this default method.', array('%select-cancel-method' => t('Select method for cancelling account'), '%administer-users' => t('Administer users'), '@permissions-url' => url('admin/config/people/permissions'))),
  );

... 

  $form['email_title'] = array(
    '#type' => 'item',
    '#title' => t('E-mails'),
  );

and user_admin_permissions():

        $form['permission'][$perm] = array(
          '#type' => 'item',
          '#markup' => $perm_item['title'],
          '#description' => $hide_descriptions ? $perm_item['description'] : NULL,
        );

modules/user/user.module in user_multiple_cancel_confirm():

  $form['user_cancel_method'] = array(
    '#type' => 'item',
    '#title' => t('When cancelling these accounts'),
  );

think this contains a spelling mistake as cancelling should be spelled canceling, right?

modules/user/user.pages.inc in user_cancel_confirm_form():

  $form['user_cancel_method'] = array(
    '#type' => 'item',
    '#title' => ($account->uid == $user->uid ? t('When cancelling your account') : t('When cancelling the account')),
    '#access' => $can_select_method,
  );

So, how should this content be displayed? H3 probably isn't appropriate as there are a number of different messages that get expressed here.

I think that the #show_title property is redundant on this element type. If you didn't want to show it, you wouldn't have the function. Do we need to get rid of the custom code in theme_form_element() or just improve it? Should we allow a property to default to a different type of html tag?

Owen Barton’s picture

Status: Needs review » Needs work
FileSize
15.34 KB

I am only 3/4 of the way through adding some tests, but uploading here as I am changing computers.

mgifford’s picture

That's great Owen! Just wanted to let you know I've installed the code (which went well), then browsed about through the edit pages (which went well) and finally ran the tests (which went well).

Still need more review, but wanted to pass along the positive news.

Owen Barton’s picture

Status: Needs work » Needs review
FileSize
19.11 KB

Finally found some time to polish this off. Here is a summary of the changes from #70 (patch 17), most of which have been discussed with other contributors to this issue:

  • If #show_title is not set by the time it gets to us we default to not show the title at all - it was felt that this was a more natural API than unset meaning "before" yet FALSE meaning "don't display". However please note that all the elements that currently use theme_form_element do each set it to before or after as appropriate.
  • The #item type h3 insertion was removed - we couldn't find any equivalent function in the original code, if tests are broken by this we will investigate further.
  • The nasty logic around displaying the title in the right place (if at all) and at the same time ensuring that a required mark is output if needed (i.e. "if there is empty title, or there is empty #show_title, or it is both set and is set to attribute") has been greatly simplified by simply unsetting #show_title once we have added the #title as an attribute - from that point on it follows the same execution path as if the title is set not to display.
  • There are various documentation cleanups and improvements.
  • There are now fairly exhaustive tests making sure that things come out in the right order and place.

The only potential limitation I know of is around the multiple checkboxes/radios elements, because there is no way to semantically indicate if you want these properties to apply to the "parent" element (which they do naturally, or the child elements. This is a fairly intrinsic problem with these kind of multiplying elements however, and one that existed previously, and exists for other FAPI properties too - so I don't think it need hold up this patch. Possibly in a different issue we could introduce "#child_element_defaults" that child elements would each inherit.

Status: Needs review » Needs work

The last submitted patch failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
18.92 KB

There were two rejected patches when I applied this.

Think I've rolled them up in this patch. These were the ones to watch out for though.

1 out of 1 hunk FAILED -- saving rejects to file modules/system/system.api.php.rej
patching file modules/system/system.module
Hunk #3 FAILED at 373.
1 out of 11 hunks FAILED -- saving rejects to file modules/system/system.module.rej

Status: Needs review » Needs work

The last submitted patch failed testing.

Owen Barton’s picture

Taking a look at these failures now

Owen Barton’s picture

Status: Needs work » Needs review
FileSize
21.46 KB

Here is a patch that I hope should pass the tests now...

The fixes were:

  • '#show_title' => FORM_ELEMENT_SHOW_TITLE_BEFORE got dropped from textfield elements in the reroll causing most of the fails.
  • I looked into the the item #type (2 or 3 fails were due to this) and realized that the current output of a #title on these elements is simply a label, which in all the cases I checked is actually correct, because is is the label for a series of radio or checkboxe elements (assembled "manually", rather than with the radios or checkboxes elements for various reasons). Hence the fix is simply to add '#show_title' => FORM_ELEMENT_SHOW_TITLE_BEFORE to the item type, so that it is treated the same as it is now. Note that in a some cases listed above the item element is used with #markup, which is just raw HTML, and no label is output (also fine).
  • We are now treating #required more consistently than before, meaning that if you do something that doesn't make sense, such as set each of a series of radio buttons as required it will actually mark them as required. I found a case of this in user.pages.inc and removed the #required, since it clearly doesn't make sense (I checked the original issue #8: Let users cancel their accounts in case there is some obscure reason for this but found nothing). There is potentially a bug here because the fact that the radios were set as required was ignored by FAPI (both before and after patch) - something to investigate in another issue, however.
  • Added one more test (textfield title as attribute) that I missed.
  • Tidied up yet more of the properties listings in the docinfos.
mgifford’s picture

Hey Owen, I've applied this to http://drupal7.dev.openconcept.ca

Worked just fine. Just need to do an accessibility review now I think. Thanks for adding more tests.

+1

Everett Zufelt’s picture

Status: Needs review » Needs work

Code review:

+  if (!empty($element['#title']) && isset($element['#show_title']) && $element['#show_title'] == FORM_ELEMENT_SHOW_TITLE_ATTRIBUTE) {
+    $element['#attributes']['title'] = $element['#title'];
+    // Unset #show_title to simplify later theming, we have done all we need.
+    unset($element['#show_title']);

Don't really like unsetting this property. Is there a clear reason why we * need * to modify the property that the developer has set?

+  // Place label & required mark in correct position, depending on #show_title.
+  if (isset($element['#show_title']) && $element['#show_title'] == FORM_ELEMENT_SHOW_TITLE_BEFORE) {
+    $output .= theme('form_element_label', $element) . ' ' . $element['#children'] . "\n";
+  }
+  else {
+    // In all other cases we follow the form element.
+    $output .= $element['#children'] . ' ' . theme('form_element_label', $element) . "\n";
   }

Not sure why we aren't testing for !empty($element['#title']) in this if statement.
The current logic has the label output after the form element in all cases but that where #show_title == ..._BEFORE. This is unclear logic to me, and I'm familiar with the issue.

+  // If there is no title,  or the title is not set to display we simply
+  // return any required mark here.
+  if (empty($element['#title']) || empty($element['#show_title'])) {
+    return $required;
+  }

Would this logic not mean that an astrisk would be floating without an element title? Is this possible in head (unpatched)?

+ *  - "#label_option": add a class to style the element label as an option.

What does it mean to style something as an option?

+++ modules/user/user.pages.inc	10 Sep 2009 16:52:24 -0000
@@ -460,7 +460,6 @@ function user_cancel_methods() {
       '#return_value' => $name,
       '#default_value' => $default_method,
       '#parents' => array('user_cancel_method'),
-      '#required' => TRUE,
     );
   }
   return $form;

Can you please explain this modification?

Owen Barton’s picture

unset($element['#show_title']);

The reason to unset it is it makes the logic later on much much simpler, and there is no reason for us *not* not unset it - the developer has no control by this point and shouldn't care what happens to their element (there are many other modifications to the element structure around this point) and we have already done everything we need to handle this property.

Not sure why we aren't testing for !empty($element['#title']) in this if statement.

This is handled later on in theme_form_element_label - the required mark processing needs to be integrated with the label processing, because the required mark needs to go inside the label if there is one.

The current logic has the label output after the form element in all cases but that where #show_title == ..._BEFORE. This is unclear logic to me, and I'm familiar with the issue.

Well, the other cases are ..._AFTER, or an empty(#show_title). The latter still needs to be processed by theme_form_element_label to handle the required mark.

Would this logic not mean that an astrisk would be floating without an element title? Is this possible in head (unpatched)?

Yes, as I described in my comment above we now add a required mark even if there is no label - previously the #required property was ignored if the #title happened to be empty. I have been musing about possibly putting the required mark inside an (otherwise empty) label, which is in some ways better - we would probably need to add the option class to have it display reasonably (an asterisk on a line by itself looks very odd). Either way, if developers are following best practices this should be a rare situation (it doesn't occur in core at all, for example).

What does it mean to style something as an option?

It adds a class="option" - the default CSS makes this cause the element and it's label to appear inline, however individual themes could do something else if they want.

Can you please explain this modification?

This was explained in my previous comment.

TravisCarden’s picture

Issue tags: +#d7ux
Everett Zufelt’s picture

@Owen

Thanks for all of your work on this patch.

1. I don't think that the required mark should appear on its own without a title, if it doesn't happen in core currently, I really don't see the need to add this functionality.

2. I think it would be more clear to themers investigating theme_form_element() for possible overrides if the logic for outputting labels was more clear, even if that clarity is provided in the form of a description.

a. if _BEFORE ...
b. elseif _AFTER ...
c. else ...

With an explanation of where to find the themable function for _ATTRIBUTE

Thanks again.

Owen Barton’s picture

Status: Needs work » Needs review

1. I don't think that the required mark should appear on its own without a title, if it doesn't happen in core currently, I really don't see the need to add this functionality.

Why not? I think it the current behavior is inconsistent, inflexible (the title of this issue), not to mention confusing for developers and a usability and accessibility issue because you are no longer able to see that a field is required until you submit the form and get an error. This was previously an edge case because it would be unusual/incorrect to not supply a #title with a field - however, with the new functionality we have added of allowing titles to be displayed as attributes (or disabling their display completely) I think this has become critical to fix.

The main question, to me, is should we add them "as is" (in the current patch), or wrap them in a label tag with an option class. The latter is nicer in that the mark is clearly associated with the field (via for/id), but a little odd in that it not actually a sane label itself. Also, what happens with AT when a field has both a label and a title attribute - are they both accessible, or does the software preference the label?

2. I think it would be more clear to themers investigating theme_form_element() for possible overrides if the logic for outputting labels was more clear, even if that clarity is provided in the form of a description.

I'll try and improve the comment around this statement for the next iteration to make this clearer.

Everett Zufelt’s picture

@Owen

Placing the required mark near the form element is fine, but it shouldn't be a label if the field is already "labeled" with a title attribute. The required mark should also be part of the title attribute for the form field if #show_title == _ATTRIBUTE.

Owen Barton’s picture

This patch adds " (This field is required.)" to the title attribute when #show_title == _ATTRIBUTE and the field is required.

It also (hopefully) cleans up the inline documentation around the if-then-else logic to add the label and required mark.

Owen Barton’s picture

Eliminate some fuzz...

Cliff’s picture

@Owen, I'd make the wording as concise as possible. People who use screen readers don't like to have to listen to extra words any more than those of us who can see like having to read them. I wonder if Everett will agree that it would be better for the title attribute to be simply either "Required" or, at most, "Required field."

Owen Barton’s picture

I used the same wording that is used for the title of the span around the "*" - I have no problem changing it to something shorter though if that is preferred.

mgifford’s picture

I tested this and it worked fine. Had to repackage the code though because of some added text in the CVS that was messing up my import.

Everett Zufelt’s picture

Status: Needs review » Needs work

Code review:

+ $element['#attributes']['title'] .= ' (' . $t('This field is required.') . ')';
+ }
+ // Unset #show_title to simplify later theming, we have done all we need.
+ unset($element['#show_title']);

1. Why are we using text to represent the required property of an element and not the commonly used "*".

2. Once again I have strong objections to unsetting a property of an element for the sole purpose of reducing an if clause later on.

+ // Place label & required mark in correct position, depending on #show_title.
+ if (isset($element['#show_title']) && $element['#show_title'] == FORM_ELEMENT_SHOW_TITLE_BEFORE) {
+ $output .= theme('form_element_label', $element) . ' ' . $element['#children'] . "\n";
+ }
+ else {
+ // In all other cases (#show_title is FORM_ELEMENT_SHOW_TITLE_BEFORE,
+ // FORM_ELEMENT_SHOW_TITLE_ATTRIBUTE or is not set) we add the label and
+ // required mark after the element. In the last 2 cases this ensures the
+ // required mark is still present to indicate the field is required even
+ // through there is no label added.
+ $output .= $element['#children'] . ' ' . theme('form_element_label', $element) . "\n";
}

3. I don't think that a required mark should be shown if there is no title set. In my mind the required mark is part of the label.

4. I would prefer to see more than two branches in this if statements that are more clear about their purpose. E.g. before, after, none (if necessary).

+function theme_form_element_label($element) {
+ // This is also used in the installer, pre-database setup.
+ $t = get_t();
+
+ $required = !empty($element['#required']) ? '*' : '';

5. setting a title on a span element does nothing for accessibility, we can omit the title / span and just leave the *.

+ if (isset($element['#label_option']) && $element['#label_option']) {

6. label_option is still not meaningful to me, we need better naming for this property if it is needed.

bowersox’s picture

We've all let this one go a few weeks. Does anyone want to join forces to pick up and get this one over the finish line?

donquixote’s picture

Nice to see people working on this!

A typical use case I have is that I want to have labels and form elements in separate table cells, with a custom theme function. This means:
- I don't want any useless wrapper divs around the form elements
- I want the table cell and row to have class attributes depending on the form element type and name, so the

replaces the wrapper div (). Right now this is done with my custom theme function (for the entire form), and theme_table(), which is an ok solution.
- I want a "naked" form element, with no decorations, to be put in the table cell.
- All of this happens at module level, and should be independent of themes.

In other situations I might want to have a custom wrapper instead of the wrapper div or table cell.

Would the patch allow this?

What i imagine is something like this:

<?php
$element['#wrapper'] = false;  // no wrapper
$element['#wrapper']['tagName'] = 'td';  // custom wrapper tag name
$element['#wrapper']['theme'] = 'mymodule_field_wrapper';  // this will use theme_mymodule_field_wrapper
?>

and something like this in theme.inc:

<?php
function theme_checkbox($element) {
  ...
  $html = '<input type="checkbox" ... />';
  return theme($element['#wrapper']['theme'], $html, $element);
}
?>

----

I tried to apply the patch to my local copy of D7 to review it, but this doesn't work..

donquixote’s picture

(#98)
Can this be achieved by modulename_preprocess_form_element() in a custom module ?

mgifford’s picture

This issue is a culmination of about 4-5 different form accessibility issues. This is an attempt to produce a consistent, flexible & accessible approach to form element labeling and it very much needs to be reviewed and brought into core.

bowersox’s picture

@Owen Barton, @mgifford, @Everett Zufelt:

I just re-read this entire thread -- wow, we were really close to a final solution! I would like to re-roll what we've got and bring this to a conclusion as follows:

  • Use @Owen's work as the starting point. Basically this looks really good to me so let's get it in! The patches in #92 and #95 are both the same patch, just re-rolled.
  • As @Cliff suggested in #93, I am okay shortening the wording to 'Required'.
  • Take one of @Everett's suggestions, namely that instead of unset() we can simply add the ATTRIBUTE option as a branch in the if statement and put a comment to explain why.
  • Otherwise I agree with @Owen about using a required mark even if there is no label.
  • Instead of introducing a new #label_option, I am considering just doing if (SHOW_TITLE_AFTER) ['class'] = 'option'; because the AFTER labels are precisely the ones we want to style as option.
  • Finally, as a separate patch later after this is committed, I'd like to return to @Everett's ideas in #63 about wrapping radios and checkboxes properly in a fieldset-legend. But let's keep that separate and out of this patch for now.

Let me know if you have any suggestions or advice. We all did a lot of work so let's get it into D7. I'm going to post a proposed patch in the next day.

mgifford’s picture

This does sound like a good approach. Thanks for reviewing this very long & detailed thread and for working on a new patch!

bowersox’s picture

Status: Needs work » Needs review
FileSize
21.09 KB

Please review this patch critically. Tell me how I can further simplify, clarify, and make this clean, robust code.

This patch essentially does what is proposed in #101 above. As you review it please note:

  • The default #show_title position is now _BEFORE. This applies to every kind of field wrapped by theme_form_element().
  • Radios and checkboxes are not wrapped by theme_form_element() yet. I'd appreciate advice on how to set #theme and #theme_wrappers in system_element_info() to accomplish that.
  • #form_element_skip_title has been eliminated and replaced by #show_title. It appears it was never a documented part of the API anyway.
  • Updates to vertical tab summary javascript in comment-node-form.js and content_types.js were needed so we didn't break a few of the tab summaries when we altered the HTML.
  • Known bug: password fields, such as on /admin/people/create, get an extra required mark from the logic in theme_form_element_label().

verot requested that failed test be re-tested.

mgifford’s picture

This patch applies nicely. Looks like you cleaned up a lot of the documentation as well. The theme_form_element_label() looks good. Will be nice to have this centralized.

I will try to find time to do a more thorough review over the weekend. So far it looks good and I haven't seen any new anomalies.

mgifford’s picture

Just wanted to raise this up to the top of the issue queue again.

This patch is a combination of about 4-5 other accessibility issues that have been raised about online forms.

* Use fieldset and legend for date field
* Administer > Modules pages make improper use of form labels
* Select boxes still not using labels
* Improve radio and checkbox title and labeling features for accessibility
* wrong HTML on admin/user/user filter
* All labels should require a for attributepointing to a unique id

Not only are these serious accessibility challenges for anyone trying to use drupal's forms api from an accessibility point of view, but it also presents problems for everyone.

Everyone needs consistent & flexible forms and the latest patch is the best route to get there.

brandonojc requested that failed test be re-tested.

Status: Needs review » Needs work
Issue tags: +Usability, +Accessibility, +#d7ux, +Review swap

The last submitted patch failed testing.

bowersox’s picture

Status: Needs work » Needs review
FileSize
22.84 KB

Please review this re-rolled patch.

Changes are:

  • Re-rolled because the form.test files moved.
  • For required fields with no #title, we now wrap the required marker inside a label element so at least the fact that it is required is somehow associated with the particular field.
  • For required fields with no #title, set the default #show_title to be after so the required mark appears inline after the field.
  • Updated tests to correspond with that.
  • Simplified logic in theme_form_element_label by not calling it if we don't need a label (so it doesn't have to handle all the cases of #show_title and all that logic can live in theme_form_element().)
  • Fixed the known bug mentioned in #103 by setting the password_confirm field's #show_title to NONE, so it explicitly will not get a label or required marker.
  • Cleaned up 2 extra spaces in form.inc.
  • Added more comments.

The only known issue with this patch is 1 form (shortcut.admin.inc) that puts a whole textfield inside a #title. This patch uses filter_xss_admin() so the textfield gets lost. But this should never have been allowed in the first place, and it was only because theme_radio was doing its own labeling without filter_xss_admin() that it ever worked.

I'd appreciate critical review so we can get this patch good and get it in.

mgifford’s picture

Status: Needs review » Needs work
FileSize
62.47 KB
67.17 KB
59.28 KB
50.74 KB
66.72 KB

I've tested this on the Mac in FireFox 3.5.5, Opera, Safari & Chrome. I haven't run into any issues yet with it.

This is a very important patch to get in because it is a big issue that touches on a lot of code.

Unfortunately it's a big one to review that touches on almost all components of Drupal's admin interface.

Jumping back to the top of this issue. Let's make sure that D7 isn't inconsistent, all form elements need to have their label generated by theme_form_element(), rather than providing their own labels. D7 also needs to be more flexible, providing an easier way for form developer to tell FAPI where to put the label, or if a label should be generated at all. Developers shouldn't have to hack the FAPI to not display a title (which is presently what needs to happen without this patch).

Here are some screenshots with the patch.

bowersox’s picture

@mgifford: Thanks for taking a look. In all your screenshots, everything looks correct -- am I right in assuming that you haven't found any forms that this patch breaks*? If you do find any problems with this patch, let me know because I want to make it clean and robust.

*besides the shortcut.admin.inc form mentioned in #109 that puts HTML inside a #title.

mgifford’s picture

I didn't see any errors other than the one you noted. I didn't have a chance to do as thorough a job as I'd like but did test it in a few browsers to see if I could find any problems.

Only issue I noticed really was with Opera (which isn't supported) and the new admin toolbar. Other than that it seemed just fine.

Since this is such a large patch touching on so many items I think it would be good to have one more review. I think at that time we should mark it RTBC and bring it into core.

I'm not sure that it's critical to have another review, but do know that @webchick & @Dries are rather busy too so would like to keep it to a minimum.

mgifford’s picture

Status: Needs work » Reviewed & tested by the community

I've gone through a fresh site now with this patch applied. I viewed it all in Chrome on the Mac and it all looked just fine.

The code has been reviewed & rehashed by many people at this point, but #109 seems to capture the consensus of this list.

I'm happy to recommend it to core.

Dries’s picture

Status: Reviewed & tested by the community » Needs work
+++ includes/form.inc	23 Nov 2009 22:23:14 -0000
@@ -2811,20 +2810,37 @@ function theme_form_element($variables) 
+  if (isset($element['#show_title'])) {
+    // If an explicit show_title position is set, always use it.
+    $show_title = $element['#show_title'];
+  } elseif (empty($element['#title'])) {
+    // If show_title is not set and title is empty, default position for the
+    // label is after. It will contain the required marker, if needed.
+    $show_title = FORM_ELEMENT_SHOW_TITLE_AFTER;
+  } else {
+    // Otherwise, the default position for elements with a title is before.
+    $show_title = FORM_ELEMENT_SHOW_TITLE_BEFORE;
+  }

Code style issues: else and elseif need to be on a new line.

I think I support this patch, but I found it a little odd that the position of the title is passed in. Shouldn't be this a designer's call? It would be good to understand why we need to control this through a property.

+++ includes/form.inc	23 Nov 2009 22:23:14 -0000
@@ -2837,6 +2853,10 @@ function theme_form_element($variables) 
+ * Note: This function always returns a required marker, whether or not the
+ * element passed in $variables is required. It depends on the calling logic
+ * to only output a required marker for required fields.

Isn't this a waste of resources?

+++ includes/form.inc	23 Nov 2009 22:23:14 -0000
@@ -2856,6 +2876,50 @@ function theme_form_required_marker($var
+  } else {

Code style issue.

+++ includes/form.inc	23 Nov 2009 22:23:14 -0000
@@ -2856,6 +2876,50 @@ function theme_form_required_marker($var
+    // Style the label as an option inline with the element.

What do you mean with 'an option inline with the element'? That wasn't immediately clear to me.

+++ modules/simpletest/tests/form.test	23 Nov 2009 22:23:14 -0000
@@ -153,6 +153,74 @@ class FormsTestCase extends DrupalWebTes
+ * Test the form elements, labels and associated output for correctness.

When reading this I wondered what 'correctness' means in the case of labels. I think it would be good to document what correct behavior is, and why it is important. Maybe that should be documented in theme_form_element_label(), and not in the test.

+++ modules/simpletest/tests/form_test.module	23 Nov 2009 22:23:14 -0000
@@ -351,6 +359,71 @@ function form_storage_test_form_submit($
+      'firstcheckbox' => t('First Checkbox'),
+      'secondcheckbox' => t('Second Checkbox'),
+      'thirdcheckbox' => t('Third Checkbox'),

- We don't usually glue words together.

- We don't capitalize each word.

+++ modules/simpletest/tests/form_test.module	23 Nov 2009 22:23:14 -0000
@@ -351,6 +359,71 @@ function form_storage_test_form_submit($
+      'firstradio' => t('First Radio'),
+      'secondradio' => t('Second Radio'),
+      'thirdradio' => t('Third Radio'),

- We don't usually glue words together.
- We don't capitalize each word.

bowersox’s picture

@Dries: Thanks for the review and feedback! I'll roll an updated version.

The reason #show_title is a property is so that we output the HTML code in the correct order (putting the label before or after title in different cases). The HTML order is something designers couldn't change in CSS after the fact, so it must be done in theme_form_element(). Prior to this, some element types were self-labeling and did things in different orders. This change would put labeling logic in one place but use the #show_title setting to handle different types correctly and make it customizable.

bowersox’s picture

Status: Needs work » Needs review
FileSize
41.4 KB
41.32 KB
26.31 KB

Please review this re-rolled patch. It addresses coding standards pointed out by @Dries, and it:

  • Prevents wasted resources in theme_form_required_marker() by return'ing early for non-required fields.
  • Adds and clarifies documentation for theme_form_element() and theme_form_element_label(), so we document correct behavior in the code and not just the test, as suggested.
  • Fixes the shortcut admin chooser form (shortcut.admin.inc form at /admin/config/system/shortcut). Prior versions of this patch had rendered the form unusable because the form was squeezing a whole textfield inside a radio #title, which is no longer allowed because we call filter_xss_admin(). This patch removes that logic from theme_shortcut_set_switch() and instead uses a few lines of targeted CSS to put the textfield near the radio on that form. Screenshots of the old and new form are attached.

Let me know if there's anything else I can do to perfect this and get it back to RTBC.

mgifford’s picture

Status: Needs review » Reviewed & tested by the community

This looks great! I'm marking it as RTBC as I think all outstanding issues have now been addressed.

sun’s picture

Status: Reviewed & tested by the community » Needs work
+++ includes/form.inc	28 Nov 2009 18:20:39 -0000
@@ -1166,6 +1166,16 @@ function form_builder($form_id, $element
+  if (isset($element['#title']) && isset($element['#show_title']) &&
+      $element['#show_title'] == FORM_ELEMENT_SHOW_TITLE_ATTRIBUTE) {
@@ -2875,6 +2904,58 @@ function theme_form_required_marker($var
+  if (isset($element['#show_title']) &&
+      $element['#show_title'] == FORM_ELEMENT_SHOW_TITLE_AFTER) {

Strange wrapping here.

+++ includes/form.inc	28 Nov 2009 18:20:39 -0000
@@ -2805,10 +2803,16 @@ function theme_file($variables) {
+ * Each form element is wrapped in a div with #type and #name classes. In
+ * addition to the element itself, the div contains a label before or after
+ * the element based on #show_title. The label includes a required marker for
+ * required fields. Lastly, the optional #description is added.

This needs much more explanation. It doesn't explain #show_title at all.

Also, #show_title is a poor name. Please find a better one. It should at least start with "#title_", perhaps #title_display or similar.

+++ includes/form.inc	28 Nov 2009 18:20:39 -0000
@@ -2805,10 +2803,16 @@ function theme_file($variables) {
- *     Properties used: #title, #description, #id, #required, #children
+ *     Properties used: #type, #name, #title, #show_title, #children,
+ *     #description.

(and elsewhere) I don't get why #type and #name have been added here.

Also not sure why #required and #id was removed? They are passed on to a sub-theme function, so the properties are still used.

+++ includes/form.inc	28 Nov 2009 18:20:39 -0000
@@ -2830,19 +2834,38 @@ function theme_form_element($variables) 
+  elseif (empty($element['#title'])) {
+    // If show_title is not set and title is empty, default position for the
+    // label is after. It will contain the required marker, if needed.
+    $show_title = FORM_ELEMENT_SHOW_TITLE_AFTER;
+  }

I don't grok this.

If there is no label, then we still apply a default position for a label? (?)

+++ includes/form.inc	28 Nov 2009 18:20:39 -0000
@@ -2830,19 +2834,38 @@ function theme_form_element($variables) 
+    case FORM_ELEMENT_SHOW_TITLE_BEFORE:
...
+    case FORM_ELEMENT_SHOW_TITLE_AFTER:
...
+    case FORM_ELEMENT_SHOW_TITLE_NONE:
+    case FORM_ELEMENT_SHOW_TITLE_ATTRIBUTE:

I also don't like these constants. Please find a way without constants.

+++ includes/form.inc	28 Nov 2009 18:20:39 -0000
@@ -2865,8 +2888,14 @@ function theme_form_element($variables) 
 function theme_form_required_marker($variables) {
+  $element = $variables['element'];
...
+  if (empty($element['#required'])) {
+    return '';
+  }

We can directly access $variables here.

+++ includes/form.inc	28 Nov 2009 18:20:39 -0000
@@ -2875,6 +2904,58 @@ function theme_form_required_marker($var
+  $title = '';
+  if (empty($element['#title'])) {
+    if (empty($required)) {
+      // If title and required marker are both empty, output no label.
+      return '';
+    }
+  }
+  else {
+    $title = filter_xss_admin($element['#title']);
+  }
...
+  return ' <label' . drupal_attributes($attributes) . '>' . $t('!title !required', array('!title' => $title, '!required' => $required)) . "</label>\n";

$title can be empty here. So you can get a label that contains no real label, but only a required marker. Is that on purpose? Looks like a bug to me.

+++ modules/shortcut/shortcut.admin.inc	28 Nov 2009 18:20:39 -0000
@@ -52,7 +52,7 @@ function shortcut_set_switch($form, &$fo
-    $options['new'] = t('New set');
+    $options['new'] = t('New set') . ':';

We removed all colons from form element labels.

+++ modules/shortcut/shortcut.css	28 Nov 2009 18:20:39 -0000
@@ -83,3 +83,12 @@ div.add-or-remove-shortcuts a:hover span
+#shortcut-set-switch .form-type-radios.form-item-set {

This selector will probably not work in IE6. I think you can remove .form-type-radios

+++ modules/simpletest/tests/form_test.module	28 Nov 2009 18:20:40 -0000
@@ -441,6 +449,71 @@ function form_storage_test_form_submit($
+  $form['form_checkbox_test_attribute'] = array(
+    '#type' => 'checkbox',
+    '#title' => t('Checkbox test with label as attribute'),
+    '#show_title' => FORM_ELEMENT_SHOW_TITLE_ATTRIBUTE,
+  );

I really wonder how much sense this makes. A checkbox without a visible label?

+++ modules/system/system.module	28 Nov 2009 18:20:40 -0000
@@ -56,6 +56,34 @@ define('REGIONS_VISIBLE', 'visible');
+ * @see system_elements().
...
+ * @see system_elements().
...
+ * @see system_elements().
...
+ * @see system_elements().

This function does not exist.

+++ modules/system/system.module	28 Nov 2009 18:20:40 -0000
@@ -56,6 +56,34 @@ define('REGIONS_VISIBLE', 'visible');
+define('FORM_ELEMENT_SHOW_TITLE_BEFORE', 'before');
...
+define('FORM_ELEMENT_SHOW_TITLE_AFTER', 'after');
...
+define('FORM_ELEMENT_SHOW_TITLE_ATTRIBUTE', 'attribute');
...
+define('FORM_ELEMENT_SHOW_TITLE_NONE', 'none');

Your constants are plain strings. Hence, no need for ugly-long constants.

This review is powered by Dreditor.

bowersox’s picture

@sun: Thanks for the feedback. Below are a few responses and plans for re-rolling this today.

1. Strange wrapping here.

What is the coding standard for if-conditions longer than 80 chars?

2. #show_title is a poor name.

I'll re-roll with #title_display unless anyone has a better suggestion.

4/7. If there is no label, then we still apply a default position for a label? $title can be empty here. So you can get a label that contains no real label, but only a required marker. Is that on purpose? Looks like a bug to me.

That is actually on purpose. There can be required fields that have no #title but should still get a required marker. The required marker should still appear inside the label element, just like it does if there is a title. That helps associate the fact that it is required with the particular field that is required. I'll add more explanatory comments; let me know if you suggest a different behavior.

10. I really wonder how much sense this makes. A checkbox without a visible label?

Drupal has lots of checkboxes with no visible label, such as on the Admin Permissions page which is a wall of unlabeled checkboxes. They have tooltips and table headers to help users know the context. The code here is a simpletest for such a checkbox.

12. Your constants are plain strings. Hence, no need for ugly-long constants.

Great! It will be shorter and simpler to just use the plain strings. Thanks!

sun’s picture

1) None. Just continue.

The other points don't seem to raise questions.

bowersox’s picture

Status: Needs work » Needs review
FileSize
25.55 KB

Please review this re-rolled patch. It has no changes in functionality or appearance from the prior one, just documentation and code clean-up as suggested by @sun.

A few notes:

#3. You're right, I didn't want to get rid of the 'Properties Used' because other theme functions end up using them.

For #9 it turns out the best selector is simply #shortcut-set-switch .form-type-radios since we only want to adjust the padding and margin on the one outer div.form-type-radios.

Hopefully we're ready for RTBC again.

mgifford’s picture

Status: Needs review » Reviewed & tested by the community

I've implemented this patch and tested it out thoroughly on FF. The changes shouldn't have had any impact on the output of the forms but I wanted to test it again to see if I'd missed anything again. There are so many forms that come bundled with Drupal!

Seems much easier to read without the constants. Sun's suggestions with the comments make it much easier to understand as well. I used http://meld.sf.net to compare versions patches 39 & 40.

Think we've addressed these concerns. Thanks everyone!

sun’s picture

Status: Reviewed & tested by the community » Needs review

This still awaits my review.

mgifford’s picture

Great that you're going to go through it again. Please let us know if there's anything else we can do. This is a central enhancement to forms in D7 so I'm glad you're going through it again.

Looking forward to seeing this marked RTBC by you after your review (or the next patch if it's required).

sun’s picture

+++ includes/form.inc	30 Nov 2009 22:01:10 -0000
@@ -1164,6 +1164,15 @@ function form_builder($form_id, $element
+  // Set the element's title attribute to show #title as a tooltip, if needed.
+  if (isset($element['#title']) && isset($element['#title_display']) && $element['#title_display'] == 'attribute') {
+    $element['#attributes']['title'] = $element['#title'];
+    if (!empty($element['#required'])) {
+      // Append an indication that this field is required.
+      $element['#attributes']['title'] .= ' (' . $t('Required') . ')';
+    }
+  }

Years of work went into Form API to free up form_builder() from any element- and property-specific stuff like this.

Why can't this live in theme_form_element() ?

theme_form_element() is invoked for all form elements that have a #title or #description. See http://api.drupal.org/api/function/form_pre_render_conditional_form_elem...

If necessary, then we need to extend the condition to also account for #required, but inlining this code into form_builder() is an absolute no-go.

+++ includes/form.inc	30 Nov 2009 22:01:10 -0000
@@ -1645,7 +1654,7 @@ function form_options_flatten($array, $r
- *     #multiple, #required, #name, #attributes, #size.
+ *     #multiple, #required, #name, #attributes, #size, #id.

@@ -1798,7 +1806,7 @@ function theme_fieldset($variables) {
- *     #description
+ *     #description, #id, #name.

@@ -2146,7 +2150,7 @@ function theme_text_format_wrapper($vari
- *     #attributes.
+ *     #attributes, #name, #id.

These changes don't really have anything to do with this patch, so we better remove them.

+++ includes/form.inc	30 Nov 2009 22:01:10 -0000
@@ -2168,11 +2172,6 @@ function theme_checkbox($variables) {
-  if (isset($element['#title'])) {
-    $required = !empty($element['#required']) ? ' <span class="form-required" title="' . $t('This field is required.') . '">*</span>' : '';
-    $checkbox = '<label class="option" for="' . $element['#id'] . '">' . $checkbox . ' ' . $element['#title'] . $required . '</label>';
-  }

I'm not sure where this special case for checkboxes is handled, where the label wraps the the checkbox and title.

Sorry, if this may be one of the main purposes of this issue and patch, but I didn't read anything from this issue, and just trying to make sense of the changes like any other Drupal developer will without the context of this issue.

+++ includes/form.inc	30 Nov 2009 22:01:10 -0000
@@ -2803,10 +2802,22 @@ function theme_file($variables) {
+ * the element based on #title_display:

"...on the optional #title_display property"

+++ includes/form.inc	30 Nov 2009 22:01:10 -0000
@@ -2803,10 +2802,22 @@ function theme_file($variables) {
+ * - 'before' outputs the label before the element.
+ * - 'after' outputs the label after the element.
+ * - 'attribute' sets the title attribute on the element to create a tooltip
+ *   but outputs no label element.
+ * - 'none' supresses output of any label or required marker.

1) What's the default?

2) We want to add use-cases here. Especially for values other than 'before'.

3) The keys shouldn't be wrapped in quotes, just use

- before: The label is output before the element. This is the default.
+++ includes/form.inc	30 Nov 2009 22:01:10 -0000
@@ -2803,10 +2802,22 @@ function theme_file($variables) {
+ * The label or attribute includes a required marker for required fields.

This is a bit confusing. Probably should be stated for each option separately, where appropriate.

Especially, because I don't see this mentioning the special logic of #required for the 'attribute' value.

+++ includes/form.inc	30 Nov 2009 22:01:10 -0000
@@ -2828,19 +2839,40 @@ function theme_form_element($variables) 
+    // If an explicit title_display position is set, always use it.

We can remove this comment.

+++ includes/form.inc	30 Nov 2009 22:01:10 -0000
@@ -2828,19 +2839,40 @@ function theme_form_element($variables) 
+  elseif (empty($element['#title'])) {
+    // If title is empty and title_display is not set, we still want a label to
+    // include the required marker for required fields. The label element will
+    // associate the required marker with the field that is required. The
+    // default position for a required marker with no title is after the field.
+    $title_display = 'after';
+  }

Why?

Especially this 'after' scenario needs more precise and clear docs, because it literally maps to "nothing" in my mind.

+++ includes/form.inc	30 Nov 2009 22:01:10 -0000
@@ -2828,19 +2839,40 @@ function theme_form_element($variables) 
+      break;
+    case 'after':
...
+      break;
+    case 'none':

There should a newline between switch cases, unless a case falls-through to the next case.

+++ includes/form.inc	30 Nov 2009 22:01:10 -0000
@@ -2873,6 +2910,59 @@ function theme_form_required_marker($var
+ * Form element labels include the #title and a required marker. The label is

If we state #title, then we also want to state #required here.

+++ includes/form.inc	30 Nov 2009 22:01:10 -0000
@@ -2873,6 +2910,59 @@ function theme_form_required_marker($var
+ * #title_display. For elements that have an empty #title and are not required,
+ * this function will output no label (''). For required elements that have an
+ * empty #title, this will output the required marker alone within the label.

AFAICS, this function is not invoked for element that have no #title and are not #required -- since it is invoked by theme_form_element() and because of aforementioned conditional invocation of theme_form_element().

+++ includes/form.inc	30 Nov 2009 22:01:10 -0000
@@ -2873,6 +2910,59 @@ function theme_form_required_marker($var
+ * The label will associate the marker with the field that is required.

Not sure what this means.

+++ includes/form.inc	30 Nov 2009 22:01:10 -0000
@@ -2873,6 +2910,59 @@ function theme_form_required_marker($var
+  // If required, append a required marker in the label.

s/If required/If the element is required/

s/in/to/

+++ includes/form.inc	30 Nov 2009 22:01:10 -0000
@@ -2873,6 +2910,59 @@ function theme_form_required_marker($var
+  // Output the title and required mark in the label.

Not sure how this comment maps to the following code.

+++ includes/form.inc	30 Nov 2009 22:01:10 -0000
@@ -2873,6 +2910,59 @@ function theme_form_required_marker($var
+  return ' <label' . drupal_attributes($attributes) . '>' . $t('!title !required', array('!title' => $title, '!required' => $required)) . "</label>\n";

While being here: Did you test whether we can remove the space between !title and !required?

+++ modules/shortcut/shortcut.css	30 Nov 2009 22:01:11 -0000
@@ -83,3 +83,12 @@ div.add-or-remove-shortcuts a:hover span
+#shortcut-set-switch .form-item-new {
+  padding-top: 0;
+  padding-left: 17px;
+}

This is highly theme-dependent.

17px? Will break.

Not sure how we do the new form element descriptions for checkboxes in HEAD now, but we better use the same approach here.

Additionally, we want to make sure that there is no margin, because I now understand the purpose of this CSS (without looking at it live).

+++ modules/simpletest/tests/form.test	30 Nov 2009 22:01:11 -0000
@@ -208,6 +208,74 @@ class FormValidationTestCase extends Dru
+    $elements = $this->xpath('//label[@for="edit-form-textfield-test-title-and-required"]/child::span[@class="form-required"]/parent::*/following-sibling::input[@id="edit-form-textfield-test-title-and-required"]');
+    $this->assertTrue(isset($elements[0]), t("Label preceeds textfield, with required marker inside label."));

These are some hardcore xpath-based tests. I hope I can trust you that you got them right. :P

+++ modules/system/system.api.php	30 Nov 2009 22:01:11 -0000
@@ -274,6 +274,8 @@ function hook_cron_queue_info() {
+ *  - "#title_display": optional string indicating if and how the #title
+ *    should be displayed: 'before', 'after', 'attribute', or 'none'.

Instead of listing the possible values without context here directly, we want to point to theme_form_element_label().

+++ modules/system/system.module	30 Nov 2009 22:01:12 -0000
@@ -369,6 +368,9 @@ function system_element_info() {
+    // Do not output a title or required marker for password_confirm.
+    // The children elements will output their own titles as usual.
+    '#title_display' => 'none',

Why is this needed?

If it really is, then the inline comment should state why.

+++ modules/user/user.pages.inc	30 Nov 2009 22:01:12 -0000
@@ -452,7 +452,6 @@ function user_cancel_methods() {
-      '#required' => TRUE,

I need a explanation for this change.

This review is powered by Dreditor.

sun’s picture

To clarify in advance:

While being here: Did you test whether we can remove the space between !title and !required?

This space requires a lot of JavaScript for fieldset summaries to use additional trim() logic, because $label.text() returns trailing white-space. Thus, removing this space would simplify a lot of other stuff.

bowersox’s picture

@sun: Thanks again for helping make this patch solid. Here are some quick responses before I re-roll:

1. Cool. You were noting that theme_form_element() was only invoked if there is a title or description, but I believe it's also invoked automatically on many field #types because of lines like this in modules/system/system.module:
'#theme_wrappers' => array('form_element'),. That applies for #type = textfield, password, password_confirm, textarea, radio, checkbox, select, date, file, or item. In the past (without this patch) there was not any support for attribute titles on field types not on that list. So if we end up supporting attribute titles for only the field types on that list, that's fine. In short, I'll try to move the logic out of form_builder into theme_form_element.

2. Okay, good, I'll remove the changes that have nothing to do with this patch.

3. #type=Checkbox no longer has a special case that wraps a label around the element; after discussion above around #11 we decided it was not needed to have that special case.

4. I will change as suggested.

5. Ditto. Thanks for giving examples.

6. I will add comments as suggested.

7. I will remove the comment as suggested.

Why?

8. Why? This situation (a required field without a title) does not happen in core. Except for password_confirm which sets title_display to 'none'. But I still wanted a good default for this situation in case contrib has required fields with no title. In this situation it looks really odd to have a required mark ("*") on its own line about the field, and it looks much more natural to have the required mark after the label on the same line. That's why the default is 'after' for this case. I'll add comments about that.

9. I will add newlines in the case statements.

10. I'll update the comment as suggested.

11. As mentioned above, theme_form_element is still invoked for many #type's of fields even if they have no #title but they are #required. Dries might be right when he tweeted yesterday, "It is obvious that the Form API is going to need some tough love in Drupal 8."

12-14. I'll add a comment and update the comments as suggested.

Did you test whether we can remove the space between !title and !required?

15. Unfortunately, we cannot remove that space without negative consequences. If that space is removed, then if you browse without CSS you get a field label like this: "Username*" with no space. I guess we could take the space out and instead put a space inside the $required span if you prefer.

17px is highly theme-dependent

16. You're correct that this one line of CSS somewhat depends on theme. For Seven the 17px lines things up best. It still looks good for Garland, although not lined up quite the same. If you have a better suggestion, please advise. The trouble is trying to style this textfield that used to be crammed inside a #title, but now we call filter_xss_admin() so that won't fly any more. I'll post a screenshot of Garland so you can see.

17. Xpath tests work now. Owen Barton wrote them first and I revised and updated. I can confirm that when pieces of the code are taken out, the tests fail as expected.

18. I'll update the comment. Good point.

Why is this needed?

19. For password_confirm, we need to use #title_display='none' to explicitly block the title and required mark. The children elements (the 2 password fields) get a visible title and required mark, but the parent container should not. If we don't set it to 'none', then the default behavior would be to actually display the required marker. I tested to confirm it actually happens--visually it adds a floating redundant required marker above the 2 password fields. We definitely don't want that floating required marker. So we set it to 'none'.

I need an explanation for this change.

20. See Owen Barton's comment #83. He felt from looking at your work on #8: Let users cancel their accounts that each radio was not actually intended to be required. Let us know if that was a misunderstanding.

sun’s picture

I guess we could take the space out and instead put a space inside the $required span if you prefer.

Nope, sorry, that's scope creep and doesn't belong into this issue. My fault of mentioning it.

19. For password_confirm, we need to use #title_display='none' to explicitly block the title and required mark. The children elements (the 2 password fields) get a visible title and required mark, but the parent container should not. If we don't set it to 'none', then the default behavior would be to actually display the required marker. I tested to confirm it actually happens--visually it adds a floating redundant required marker above the 2 password fields. We definitely don't want that floating required marker. So we set it to 'none'.

This means we need to clarify this behavior in docs, because there are many contributed modules that expand form elements into multiple, which work similarly to #type password_confirm. (That will also affect one of my patches, #414424: Introduce Form API #type 'text_format', so that's very important and should really live in in-code docs -- but I think you already found the right place to document, i.e. hook_element_info())

He felt from looking at your work on #8: Let users cancel their accounts that each radio was not actually intended to be required. Let us know if that was a misunderstanding.

This means that #required needs to live in http://api.drupal.org/api/function/user_cancel_confirm_form/7, which may not be as easy as it sounds. The user account cancellation method is required, otherwise the process doesn't know what to do.

bowersox’s picture

Please review this re-rolled patch in progress. It has a few changes and lots of comment and documentation updates as suggested above. Let me know if you see anything else. It still has 2 known bugs where some of our new tests fail that I'm sorting out now.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review

I told Brandon that we're good to go if we manage to move the logic out of form_builder(), potentially into form_pre_render_conditional_form_element().

bowersox’s picture

Here's that update. We moved that logic out of form_builder into form_pre_render_conditional_form_element(). I added documentation to clarify that $title_display='attribute' is only supported for checkboxes and radios now. I also removed a few of our new automated tests that we testing this functionality on other field #types. In D8 we can revisit and make 'attribute' supported for other types.

Owen Barton’s picture

Just read through the patch and comments and I think this is looking really good. You have addressed several niggling issues I had with my original patch and managed to simplify several areas in the process.

The comments were all clear to me, but I am sure a read through by someone unfamiliar with this issue may flag a few things. In general I thought that it might be good to break up some of the longer paragraphs with whitespace (this is fine - see http://drupal.org/node/1354) to make them more digestable.

We might also want to add a tad more detail to theme_form_element about the accessibility implications of attribute vs. none - i.e. that the attributes on the checkboxes make them possible to use even when the visual context is missing (this is a major point of the patch, but is not really mentioned in the comment) and that "none" can make things inaccessible if used inappropriately.

There are a couple of whitespace blips in batch_process that we can drop.

+  // If the element is required, append a required marker to the label.
+  $required = !empty($element['#required']) ? theme('form_required_marker', array('element' => $element)) : '';
...and then in them_form_required_marker
+  if (empty($variables['element']['#required'])) {
+    return '';
+  }

This seems redundant, I don't think an extra empty() check to save a single function call really is worth the extra code - at the very least it seems like it might use a comment to explain why this is needed.

Some potential follow up issues while I am looking at this (these don't block this issue - some may be Drupal 8...)
* Search form - can we drop the specific theming for this?
* Documentation - we will want to explain the new property, as well as give folks an idea of how to respect this property in their custom fapi fields.
* Refactoring the FAPI element theme process (D8!) - perhaps there could be a preprocess layer that breaks down a simple form element into separate label, required, field and description elements in the appropriate order, adding the appropriate theme functions to each - and then the whole thing can be passed into drupal_render. Of course, you could also then pass in the "broken down" elements originally (or in an alter), which would allow all sorts of interesting things, such as adding a description both above and below a form field, as well as a potentially simpler workflow for other multiplex style elements, such as radios. It's hard to see if this additional abstraction would make things simpler or more complex, but it would be interesting to try.

chx’s picture

Status: Needs review » Needs work

After a lengty discusison with brandonojc here is the summary for the issue:

Lots of modules get rid of #title because they don't want a label to appear visually. So lots of fields end up non-accessible because there is no title available to a screenreader.

That's it. Now on to the implementation: as we always want to use #title as a title attribute for screenreaders but not always to print as visible text for browsers and this mandates a new attribute. In current code, labels are added to the output in three functions (theme_radio, theme_checkbox and theme_form_element) so it seems to make sense to centralize this and if we centralize we could make it a theme function. That's all good, after all.

I am marking this as CNW because the code could use a tad bit of simplification by moving #title_display => before into form_builder where the generic defaults are loaded (like attributes => array()). Then you cna switch on $element['#title_display'] directly. Also, I am wondering whether the switch is necessary, as you are calling the theme label twice this way... not sure though whether it could be made better with a bunch of ifs but imo worths a try. Speaking of this why do have none? I am thinking that #title_display => none can be replaced by not setting #title simply. where does it make sense to ste #title and then not display it at all?

bowersox’s picture

Status: Needs work » Needs review
FileSize
23.73 KB

Please review this updated patch.

Here are the changes:

  • Added comments per @Owen's feedback in #133. Broke up long paragraphs. Explained when 'none' could be an accessibility problem. Explained the extra empty() checks that save an unnecessary call to theme().
  • Moved the #title_display default into form_builder() as suggested by @chx in #134. Simplified it so we just always default to 'before'. Updated the corresponding test. For D8 we can add more complexity to default to 'after' for certain situations; that was really a new feature that didn't need to be part of this.

Please make sure the form_builder() default is set correctly. It appears to work with one new line of code:

@@ -1066,6 +1066,7 @@ function form_builder($form_id, $element
   $element += array(
     '#required' => FALSE,
     '#attributes' => array(),
+    '#title_display' => 'before',
   );

On IRC, @chx and I talked through why 'none' is needed in situations such as password_confirm. And we both agreed a lot of this can be better for D8.

bowersox’s picture

Please review this minor change. As suggested by @chx, I removed 2 unnecessary isset($element['#title_display']) checks from form_pre_render_conditional_form_element() and theme_form_element_label(). Now that #title_display is always set in form_builder(), let's not waste time checking.

chx’s picture

What we agreed on was that we will try setting #required none in form_process_password_confirm and thus getting rid of none. I still can't believe none is a sensible choice.

mgifford’s picture

@chx, thanks for your input on this issue (way late into the night with Brandon). Wanted to say that although the confirm password option is a pretty special case, I can imagine that there may be other instances where a similar confirmation may be needed by a custom module. I think it would be a pretty rare case. Suppose it could just be hidden by css. There are very few cases where it would make any sense to not display a #title at all.

Uninstall now displays labels - admin/config/modules/uninstall - as per http://drupal.org/node/501444

Select boxes now display labels - admin/structure/types/manage/forum - as per http://drupal.org/node/451980

Checkboxes & radio boxes still display labels here - node/add/page - however this isn't the best instance of the issue - http://drupal.org/node/522772

The patch removes the blank label listed here - http://drupal.org/node/368759

Seems to display great. I don't see anything to stop this from getting into core.

bowersox’s picture

@chx and @sun: Getting rid of the 'none' option might break things for contrib developers who sometimes need a way to suppress a label. @sun pointed out another case where 'none' is needed in #128.

Also, if we get rid of 'none', then how can we still give a default value of 'before'? Currently if #title_display is not set, we default to 'before'. So we need the API to allow a setting like 'none' to explicitly override that default.

Owen Barton’s picture

I think this is what chx was getting at (see patch and interdiff) - we can use #title being unset as an implicit 'none', which would work the same way in terms of output - suppressing the label completely - but would simplify the API a bit for most use cases. I am not sure which one I prefer - it doesn't seem too critical either way.

Reviewed the updated comments and I think they all look good.

bowersox’s picture

+1. I'm fine with this. Either way I think either recent patch accomplishes the basic goal. With this newest patch, it appears that contrib authors could still set #title_display='none' explicitly if they want to. They could also unset their #title (or never set a #title) which would do the same thing. Namely, it would display no title and no required marker.

Correct me if I've misunderstood.

The one thing some people might find confusing about this is that setting #title='' (empty string) has different behavior than leaving #title unset. I know we already have lots of confusion between !isset() and empty(). But that is something we can all talk about solving in D8 fapi.

Dries’s picture

Let's ask chx to sign of on this, or to provide additional direction. :)

chx’s picture

Status: Needs review » Reviewed & tested by the community

I am OK with this patch . It does not hurt to have none if noone needs to use it :)

sun’s picture

+++ includes/form.inc	1 Dec 2009 17:12:02 -0000
@@ -2236,6 +2227,15 @@ function theme_checkboxes($variables) {
 function form_pre_render_conditional_form_element($element) {
+  // Set the element's title attribute to show #title as a tooltip, if needed.
+  if (isset($element['#title']) && $element['#title_display'] == 'attribute') {

If I skimmed previous comments correctly, this now only applies to certain #types. The inline comment should clarify this.

+++ includes/form.inc	1 Dec 2009 17:12:02 -0000
@@ -2833,10 +2833,38 @@ function theme_file($variables) {
+ * Each form element is wrapped in a div with #type and #name classes. In
+ * addition to the element itself, the div contains a label before or after

s/div/DIV/

We always write HTML tags uppercase.

+++ includes/form.inc	1 Dec 2009 17:12:02 -0000
@@ -2833,10 +2833,38 @@ function theme_file($variables) {
+ * The optional #title_display property can have these values:
+ * - before: The label is output before the element. This is the default.
+ *     The label includes the #title and the required marker, if #required.
+ * - after: The label is output after the element. For example, this is used
+ *     for radio and checkbox #type elements as set in system_element_info().
+ *     If the #title is empty but the field is #required, the label will
+ *     contain only the required marker.
+ * - attribute: Set the title attribute on the element to create a tooltip
+ *     but output no label element. This is supported only for checkboxes
+ *     and radios in form_pre_render_conditional_form_element(). It is used
+ *     where a visual label is not needed, such as a table of checkboxes where
+ *     the row and column provide the context. The tooltip will include the
+ *     title and required marker.

Wrong indentation, see http://drupal.org/node/1354 for proper indentation of Doxygen lists.

+++ includes/form.inc	1 Dec 2009 17:12:02 -0000
@@ -2833,10 +2833,38 @@ function theme_file($variables) {
+ * The label or attribute includes a required marker for required fields.

This note still confuses more than it clarifies.

+++ includes/form.inc	1 Dec 2009 17:12:02 -0000
@@ -2833,10 +2833,38 @@ function theme_file($variables) {
+ * 
+ * If the #title property is not set the label and any required marker will 

(and elsewhere) Trailing white-space here.

+++ includes/form.inc	1 Dec 2009 17:12:02 -0000
@@ -2833,10 +2833,38 @@ function theme_file($variables) {
+ * If the #title property is not set the label and any required marker will 
+ * not be displayed. This is used by the password_confirm element #type.

Grammar needs work here.

+++ includes/form.inc	1 Dec 2009 17:12:02 -0000
@@ -2833,10 +2833,38 @@ function theme_file($variables) {
- *     Properties used: #title, #description, #id, #required, #children
+ *     Properties used: #title, #description, #id, #required, #children, #type,
+ *     #name, #title_display.

Can we move #title_display after #title, please?

+++ includes/form.inc	1 Dec 2009 17:12:02 -0000
@@ -2856,22 +2884,32 @@ function theme_form_element($variables) 
+  
+  // If #title is not present, we don't display any label or required marker
+  if (!isset($element['#title'])) {
+  	$element['#title_display'] = 'none';
+  }

Strange indentation, and trailing white-space.

+++ includes/form.inc	1 Dec 2009 17:12:02 -0000
@@ -2888,13 +2926,20 @@ function theme_form_element($variables) 
+  if (empty($variables['element']['#required'])) {
+    // If this element is not required, it gets no required marker.
+    return '';
+  }

Please move the comment above the condition.

+++ includes/form.inc	1 Dec 2009 17:12:02 -0000
@@ -2903,6 +2948,62 @@ function theme_form_required_marker($var
+ * This function will not be called for elements with no labels, depending on
+ * #title_display. For elements that have an empty #title and are not required,
+ * this function will output no label (''). For required elements that have an
+ * empty #title, this will output the required marker alone within the label.
+ * The label will use the #id to associate the marker with the field that is
+ * required. That is especially important for screenreader users to know
+ * which field is required.

I've read this twice, but still have troubles to understand it. Needs to be reworded.

+++ includes/form.inc	1 Dec 2009 17:12:02 -0000
@@ -2903,6 +2948,62 @@ function theme_form_required_marker($var
+  // If the element is required, append a required marker to the label.
+  // If the element is not required, skip the unnecessary funtion call.
+  $required = !empty($element['#required']) ? theme('form_required_marker', array('element' => $element)) : '';

huh? That comment needs work.

+++ includes/form.inc	1 Dec 2009 17:12:02 -0000
@@ -2903,6 +2948,62 @@ function theme_form_required_marker($var
+  if (empty($element['#title'])) {
+    if (empty($required)) {
+      // If title and required marker are both empty, output no label.
+      return '';

Ditto, please move the comment above (both) conditions.

+++ includes/form.inc	1 Dec 2009 17:12:02 -0000
@@ -2903,6 +2948,62 @@ function theme_form_required_marker($var
+  return ' <label' . drupal_attributes($attributes) . '>' . $t('!title !required', array('!title' => $title, '!required' => $required)) . "</label>\n";

Do we need the leading space here? If we do, then please add an inline comment that states why.

This review is powered by Dreditor.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

Let's do another reroll of this to incorporate sun's comments.

donquixote’s picture

About html processing (in theme_form_element()).

Instead of this

<?php
  $output = '<div class="' . implode(' ', $class) . '">' . "\n";
  [..]
  $output .= 'something';
  [..]
  $output .= "</div>\n";
?>

I much prefer this:

<?php
  [..]
  $output = 'something';
  [..]
  $output = '<div class="' . implode(' ', $class) . '">' . "\n" . $output . "</div>\n";
?>

Makes it much easier to see how opening and closing tag belong together.
Even nicer with heredoc syntax.
I don't know if that's a documented coding standard anywhere, so it's up to you.

sun’s picture

HTML processing in (all) those theme functions in form.inc is a different issue, not to be changed in here.

Owen Barton’s picture

Status: Needs work » Needs review
FileSize
5.9 KB
22.23 KB

If I skimmed previous comments correctly, this now only applies to certain #types. The inline comment should clarify this.

The doxygen for that function 2 lines above this seems quite clear that this is only called for checkboxes and radios, and the API docs in theme_form_element make it clear that 'attribute' only applies to checkboxes and radios. I am not sure if we could make this clearer without introducing duplication - ideas welcome.

s/div/DIV/

Done.

Wrong indentation, see http://drupal.org/node/1354 for proper indentation of Doxygen lists.

Done.

This note still confuses more than it clarifies.

Agreed - the individual descriptions of each value make it clear that the required marker is included, so this is redundant.

(and elsewhere) Trailing white-space here.

Removed.

Grammar needs work here.

Rewritten - see below.

Can we move #title_display after #title, please?

Done.

Strange indentation, and trailing white-space.

Fixed.

Please move the comment above the condition.

Done.

+++ includes/form.inc 1 Dec 2009 17:12:02 -0000
@@ -2903,6 +2948,62 @@ function theme_form_required_marker($var
+ * This function will not be called for elements with no labels, depending on
+ * #title_display. For elements that have an empty #title and are not required,
+ * this function will output no label (''). For required elements that have an
+ * empty #title, this will output the required marker alone within the label.
+ * The label will use the #id to associate the marker with the field that is
+ * required. That is especially important for screenreader users to know
+ * which field is required.

I've read this twice, but still have troubles to understand it. Needs to be reworded.

Not sure how hot my grammar is, but here is what I came up with:

 * If the #title property is not set, then the label and any required marker
 * will not be output, regardless of the value of #title_display or #required.
 * This can be useful in cases such as the password_confirm element, which
 * creates children elements that have their own labels and required markers,
 * but the parent element should have neither. Use this carefully because a
 * field without an associated label can cause accessibility challenges.
huh? That comment needs work.

Cleaned this up. I also removed the redundant check from theme_form_required_marker() I mentioned in #133.

Ditto, please move the comment above (both) conditions.

Done - I also combined the conditions and moved them to the top of the function as a more obvious "early return".

Do we need the leading space here? If we do, then please add an inline comment that states why.

This is consistent enough through the original code that I am pretty sure it is needed - I am pretty certain it just adds a little whitespace between inline labels, such as with checkboxes. It might be preferable to do this with CSS (since that allows you to more easily remove the space if you want), but I think that is out of scope for this issue.

sun’s picture

Status: Needs review » Reviewed & tested by the community

I'm 99% happy now, but will leave further decisions to core maintainers.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

I'm happy with it. It is an accessibility improvement. Committed to CVS HEAD! Thanks for all the work.

mgifford’s picture

Status: Fixed » Needs review
FileSize
828 bytes

I noticed two additional places of concern that weren't caught in this patch. I think they are two special cases that we didn't investigate enough.

I've got a demo here and running it through WebAIM highlights 4 errors that are caused by two problems.

1) The label & form element aren't matched in the file upload field so a screen reader has no way of tying the two together:

<label for="edit-field-image-zxx-0">Image </label>

...

<input type="file" name="files[field_image_zxx_0]" accept="png,gif,jpg,jpeg" class="form-file" id="edit-field-image-zxx-0-upload" size="22" />

This is a pretty significant issue but not sure where responsibility lies for this now that the upload module has been removed from core. Is it just a cck type now?

Within the "More information about text formats" section however there are empty labels:

<label>Filtered HTML:</label><ul class="tips"><li>Web page addresses and e-mail addresses turn into links automatically.</li><li>Allowed HTML tags: &lt;a&gt; &lt;em&gt; &lt;strong&gt; &lt;cite&gt; &lt;blockquote&gt; &lt;code&gt; &lt;ul&gt; &lt;ol&gt; &lt;li&gt; &lt;dl&gt; &lt;dt&gt; &lt;dd&gt;</li><li>Lines and paragraphs break automatically.</li></ul></div><div id="filter-guidelines-3" class="filter-guidelines-item"><label>Plain text:</label><ul class="tips"><li>No HTML tags allowed.</li><li>Lines and paragraphs break automatically.</li></ul>

These are easy to remove so have added a patch.

Mike

Re-test of forms-remove-blank-labels-1.patch from comment #151 was requested by mgifford.

mgifford’s picture

FileSize
2.51 KB

I'm adding to this issue as I assumed that it would have been picked up in the mass integration of issues that were being done in this massive patch.

Turns out we missed the ability to make labels invisible (so they don't disrupt visual flows) but are presented to screen readers. I hadn't had to test this before, but ran into this when working on a patch for:
http://drupal.org/node/448292#comment-2467302

This brings in the previous patch where I noticed that missing fields were being output. It still doesn't address the file upload field issue mind you.

This is a critical issue as we are still in the position where if you don't want to display the text of a form field as it is now the only option is not to have a label associated with it.

References can be found here:
http://www.w3.org/TR/2008/NOTE-WCAG20-TECHS-20081211/H44
http://www.usability.com.au/resources/wcag2/

Owen Barton’s picture

This looks good - though my sense is that if this is critical it would be good to add a test for it, so we can ensure we don't have future regressions.

mgifford’s picture

Agreed that we should ideally have tests for all optional #title_display properties. Looking in Simple Test's form.test & the function testFormLabels() I think the documentation for how the options is presently being tested could probably be better.

I don't know how to add the verification of the invisible option. I'm assuming it would be something like:

    $elements = $this->xpath('//label[@for="edit-form-textfield-test-title-and-required"]/child::span[@class="element-invisible"]/parent::*/following-sibling::input[@id="edit-form-textfield-test-title-and-required"]');
    $this->assertTrue(isset($elements[0]), t("Invisible label for textfield."));

In grepping for the code I didn't see any other refrences to #title_display in simpletest. I'm not sure if it's even possible to do testing for these options.

Mike

mgifford’s picture

FileSize
193.21 KB
214.17 KB
206.67 KB
227.36 KB
212.22 KB

There seem to be some confusion about the optional #title_display properties and the additional invisible property that I have created a path for above. I do hope this clarifies the problem and outlines the option.

0) Default without title (label-notitle.png)

There is no title so no label will be displayed. This works fine.

 79     $form[$key]['weight'] = array(
 80       '#type' => 'weight',
 81       '#default_value' => $block['weight'],
 82       '#delta' => $weight_delta,
 83     );
<select id="edit-search-form-weight" class="block-weight block-weight-sidebar_first form-select" name="search_form[weight]" gtbfieldid="2">

1) Before or missing #title_display (label-before.png) - default displays label before the field

The default for any form with a title is to display it with it's associated label.

 79     $form[$key]['weight'] = array(
 80       '#type' => 'weight',
 81       '#default_value' => $block['weight'],
 82       '#delta' => $weight_delta,
 83       '#title_display' => 'before',
 84       '#title' => t('Weight for') . ' ' . check_plain($block['info']),
 85     );

or

 79     $form[$key]['weight'] = array(
 80       '#type' => 'weight',
 81       '#default_value' => $block['weight'],
 82       '#delta' => $weight_delta,
 83       '#title' => t('Weight for') . ' ' . check_plain($block['info']),
 84     );
<label for="edit-search-form-weight">Weight for Search form </label>
<select id="edit-search-form-weight" class="block-weight block-weight-sidebar_first form-select" name="search_form[weight]" gtbfieldid="149">

2) After (label-after.png) - label after the field

Being able to put the label after the form adds more flexibility for module developers and themers.

 79     $form[$key]['weight'] = array(
 80       '#type' => 'weight',
 81       '#default_value' => $block['weight'],
 82       '#delta' => $weight_delta,
 83       '#title_display' => 'after',
 84       '#title' => t('Weight for') . ' ' . check_plain($block['info']),
 85     );
<select id="edit-search-form-weight" class="block-weight block-weight-sidebar_first form-select" name="search_form[weight]" gtbfieldid="198">
</select>
<label class="option" for="edit-search-form-weight">Weight for Search form </label>

3) None/Attribute (label-none.png) - No label displayed

I am not sure what conditions none or attribute would be required. Previous discussions have focused on the password confirmation field. Attribute adds a tooltip & none doesn't but neither involve labels. Readme on attribute is here:

  * - attribute: Set the title attribute on the element to create a tooltip
  *   but output no label element. This is supported only for checkboxes
  *   and radios in form_pre_render_conditional_form_element(). It is used
  *   where a visual label is not needed, such as a table of checkboxes where
  *   the row and column provide the context. The tooltip will include the
  *   title and required marker.
 79     $form[$key]['weight'] = array(
 80       '#type' => 'weight',
 81       '#default_value' => $block['weight'],
 82       '#delta' => $weight_delta,
 83       '#title_display' => 'attribute',
 84       '#title' => t('Weight for') . ' ' . check_plain($block['info']),
 85     );

or

 79     $form[$key]['weight'] = array(
 80       '#type' => 'weight',
 81       '#default_value' => $block['weight'],
 82       '#delta' => $weight_delta,
 83       '#title_display' => 'none',
 84       '#title' => t('Weight for') . ' ' . check_plain($block['info']),
 85     );
<select id="edit-search-form-weight" class="block-weight block-weight-sidebar_first form-select" name="search_form[weight]" gtbfieldid="247">

4) Invisible (label-invisible.png) - label invisible to all but screen reader

The proposed new option is that of invisible, which produces a label that has been hidden. This is visible only to screen readers. This option is required for Drupal forms to become WCAG 2.0 compliant. For screen readers users to be able to understand the function of form elements, having labels is critical. Links in previous posts provide more information about the importance of labels.

 79     $form[$key]['weight'] = array(
 80       '#type' => 'weight',
 81       '#default_value' => $block['weight'],
 82       '#delta' => $weight_delta,
 83       '#title_display' => 'invisible',
 84       '#title' => t('Weight for') . ' ' . check_plain($block['info']),
 85     );
<div class="form-item form-type-select form-item-search-form-weight">
<select id="edit-search-form-weight" class="block-weight block-weight-sidebar_first form-select" name="search_form[weight]" gtbfieldid="82">
</select>
<label class="element-invisible" for="edit-search-form-weight">Weight for Search form </label>
</div>
chx’s picture

And what's with CSS 2.1 attribute selectors? Like label[for=whatever] { visibility: hidden } . Yes IE6 does not support this and so what.

bowersox’s picture

+1 for the adding 'invisible' form labeling in this clean-up patch. I think this gives a much improved tool for hiding form labels in a way that is still accessible.

bowersox’s picture

FileSize
4.56 KB

I have re-rolled with an automated test for #title_display=invisible. Otherwise the patch looks good to me. I reviewed and tested and found the following:

  • The filter.module fix (explained in #151) works fine and fixes the markup without changing the visual appearance of the page (/filter/tips).
  • I made my own form that used a #title_display=invisible field and visually it worked as desired and hid the field (while leaving it as "element-invisible" for screenreaders).
mgifford’s picture

Waiting for the bot to review this. Looks good to me, but looking for that green light.

bowersox’s picture

FileSize
4.56 KB

Anyone know why the bot has waited 3 days and still no automated test results? I've re-attached the same patch as #159 to this comment here.

Owen Barton’s picture

Looks like all the test clients are failing http://qa.drupal.org/pifr/status - hopefully they will be back soon!

mgifford’s picture

Looks like it's nearly fixed from the reports in IRC from moments ago.

mgifford’s picture

Status: Needs review » Reviewed & tested by the community

Like the the tests that have been added for this. I'm marking this as RTBC.

mgifford’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
447 bytes

Ok, think we can simplify this a whole lot and get this patch through.

After talking with Everett it was clear that either a label or a title would work the same way for form elements for screen readers.

The attribute tag was supposed to provide a title attribute to the html so that form elements had that attribute, but it wasn't working.

This patch resolves that issue. However, I think the simple tests should be revisited for it and the description of it clarified so that we're all clear what attribute is supposed to do.

We shouldn't need to add the invisible tag, we just need to get attribute to work as "advertised".

Status: Needs review » Needs work

The last submitted patch, form-attribute-fix.patch, failed testing.

Everett Zufelt’s picture

@mgifford

I haven't tested your patch. But, if there is currently no way to tell FAPI to place the $element['#title'] as the title attribute of the form field in the generated markup then this does need to be solved. It was our original intent to ensure that there was a way for developers to include information about the title (label) for a form field without having to have a label generated in markup.

Thanks for working on this.

bowersox’s picture

@mgifford,

Can you give a quick explanation of what was wrong with the past approach? I thought we had this done back in February ;)

mgifford’s picture

Status: Needs work » Needs review
FileSize
789 bytes

So, this issue is linked to this patch (although I'll be needing to revise it based on this) http://drupal.org/node/448292#comment-2714302

But is using existing attribute tag rather than the proposed invisible one:

Index: modules/block/block.admin.inc
===================================================================
RCS file: /cvs/drupal/drupal/modules/block/block.admin.inc,v
retrieving revision 1.75
diff -u -p -r1.75 block.admin.inc
--- modules/block/block.admin.inc	9 Mar 2010 12:09:52 -0000	1.75
+++ modules/block/block.admin.inc	13 Mar 2010 05:27:14 -0000
@@ -80,6 +80,8 @@ function block_admin_display_form($form,
       '#type' => 'weight',
       '#default_value' => $block['weight'],
       '#delta' => $weight_delta,
+      '#title_display' => 'attribute',
+      '#title' => t('Weight for') . ' ' . check_plain($block['info']),
     );
     $form[$key]['region'] = array(
       '#type' => 'select',

After providing the patch above, and below applying the patch that's attached to FAPI you get:

<select name="system_main-menu[weight]" class="block-weight block-weight--1 form-select" id="edit-system-main-menu-weight" ><option value="-14">-14</option><option value="-13">-13</option><option value="-12">-12</option><option value="-11">-11</option><option value="-10">-10</option><option value="-9" selected="selected">-9</option><option value="-8">-8</option><option value="-7">-7</option><option value="-6">-6</option><option value="-5">-5</option><option value="-4">-4</option><option value="-3">-3</option><option value="-2">-2</option><option value="-1">-1</option><option value="0">0</option><option value="1">1</option><option value="2">2</option><option value="3">3</option><option value="4">4</option><option value="5">5</option><option value="6">6</option><option value="7">7</option><option value="8">8</option><option value="9">9</option><option value="10">10</option><option value="11">11</option><option value="12">12</option><option value="13">13</option><option value="14">14</option></select>

Notice that there is no label & no title provided. Now with the attached patch the declared title is displayed:

<select name="system_main-menu[weight]" title="Weight for Main menu" class="block-weight block-weight--1 form-select" id="edit-system-main-menu-weight" ><option value="-14">-14</option><option value="-13">-13</option><option value="-12">-12</option><option value="-11">-11</option><option value="-10">-10</option><option value="-9" selected="selected">-9</option><option value="-8">-8</option><option value="-7">-7</option><option value="-6">-6</option><option value="-5">-5</option><option value="-4">-4</option><option value="-3">-3</option><option value="-2">-2</option><option value="-1">-1</option><option value="0">0</option><option value="1">1</option><option value="2">2</option><option value="3">3</option><option value="4">4</option><option value="5">5</option><option value="6">6</option><option value="7">7</option><option value="8">8</option><option value="9">9</option><option value="10">10</option><option value="11">11</option><option value="12">12</option><option value="13">13</option><option value="14">14</option></select>

I'm not sure if this was intended or not, but this is a rather restrictive definition of the attribute tag. This may have been by design it's called in form_pre_render_conditional_form_element() but this is only used by radios & checboxes, but should it?

 * - attribute: Set the title attribute on the element to create a tooltip
 *   but output no label element. This is supported only for checkboxes
 *   and radios in form_pre_render_conditional_form_element(). It is used
 *   where a visual label is not needed, such as a table of checkboxes where
 *   the row and column provide the context. The tooltip will include the
 *   title and required marker.

Looks like either we need to extend the attribute's definition & application, and approve this patch or go back to the invisible patch I proposed earlier.

bowersox’s picture

@mgifford,

Regarding #title_display=>'attribute', see #131. @sun requested that we not add this in form_builder(), but instead in form_pre_render_conditional_form_element(). That imposed the limitation that #type->'attribute' only applies to radios & checkboxes, but that was a limitation for Drupal 7 and we all want to make this better in Drupal 8.

The prior clean-up patch (#161) was all about adding support for #title_display=>'invisible'. That was RTBC for the last 6+ weeks. It seems like that clean-up is probably still a good thing, and probably separate from the 'attribute' question you've raised.

sun’s picture

Status: Needs review » Needs work

brandonojc is right here. However, I'm not sure whether I understand the previous patch, as it mixes 'invisible' into 'after', but afterwards doesn't process 'invisible' in the 'after' switch case. Either that was a bug, or the code needs more comments.

bowersox’s picture

@sun

Regarding 'invisible' versus 'after', they are the same switch case but a few lines below in theme_form_element() we add the class to make it invisible:

+  elseif ($element['#title_display'] == 'invisible') {
+    // Show label only to screen readers to avoid disruption in visual flows.
+    $attributes['class'] = 'element-invisible';
+  }

Do you think we should add a comment up near the switch case 'invisible'? For example, we could add the comment // Class 'element-invisible' is applied below to show only to screen readers.

Let me know if that would help and I'm glad to re-roll #161. Thanks for your feedback and help.

sun’s picture

+++ includes/form.inc	31 Jan 2010 22:17:54 -0000
@@ -2989,6 +2989,9 @@ function theme_file($variables) {
+ * - invisible: Labels are critical for screen readers to enable them to ¶

Trailing white-space.

+++ includes/form.inc	31 Jan 2010 22:17:54 -0000
@@ -3127,6 +3131,11 @@ function theme_form_element_label($varia
     // Style the label as class option to display inline with the element.
     $attributes['class'] = 'option';
   }
+  elseif ($element['#title_display'] == 'invisible') {
+    // Show label only to screen readers to avoid disruption in visual flows.
+    $attributes['class'] = 'element-invisible';
+  }
+  ¶

ok. The diff context is quite small here (adjusted?), so I was mistaken about the actually altered function here.

Can we move the inline comment above the "elseif" here? Same adjustment also for the preceding condition? Would help to understand the code flow.

Also note the trailing white-space introduced here.

+++ modules/filter/filter.module	31 Jan 2010 22:17:54 -0000
@@ -846,7 +846,7 @@ function theme_filter_tips_more_info() {
 function theme_filter_guidelines($variables) {
...
-  $name = isset($format->name) ? '<label>' . $format->name . ':</label>' : '';
+  $name = isset($format->name) ? $format->name : '';
   return '<div id="filter-guidelines-' . $format->format . '" class="filter-guidelines-item">' . $name . theme('filter_tips', array('tips' => _filter_tips($format->format, FALSE))) . '</div>';

I do not understand this change. If I'm not mistaken, then we need the label to have a element to select via JS/CSS to hide. If that needs to be removed for any reason, then we a) need a inline comment stating why, and b) adjustments in filter.js/filter.css, and possibly core themes.

Powered by Dreditor.

mgifford’s picture

It's critical that there is some way to pass a title to a form element. I don't care if it's done with a label or a title element. One of them needs to get into D7 however.

The main advantage to using the title attribute as I did in #169 would be that it wouldn't really require a adding another option 'invisible' to the API. Adding (and then hiding) different label texts is also more complicated than just using the title tag.

However, I'm happy with the labels. Because it was sitting RTBC for a while I thought I'd kick the tires with another approach.

@brandon are you able to clean up the changes to patch in #161 as suggested by @sun?

@sun in trying to respond to this:
"I do not understand this change. If I'm not mistaken, then we need the label to have a element to select via JS/CSS to hide. If that needs to be removed for any reason, then we a) need a inline comment stating why, and b) adjustments in filter.js/filter.css, and possibly core themes."

With the label present, it will be only visible to screen readers. The css will hide any text off screen so it isn't visible to anyone else. The CSS class element-invisible will hide it away so that it appears to most of us like nothing has changed. It's related to http://drupal.org/node/448292#comment-2714302 - but this patch won't need JS & has nothing to do with selecting the items.

Everett Zufelt’s picture

@brandonojc

I am not clear on the reason why title can only be used on checkboxes and radios in D7. Not arguing here, but can you point me to the comment where a clear explanation for the reason for this is stated?

bowersox’s picture

@sun, can you give a quick description of why it is important to keep logic for #title_display out form_builder(), and why it is better in form_pre_render_conditional_form_element() (basically the reasoning for comment #131)?

@everett, I don't remember the reasoning but hopefully @sun can fill us in. I'm considering making form labeling and FAPI clean-up the issue I propose for the upcoming core developer summit so we can make this all better in D8.

@mgifford, yes, I'd be happy to re-roll #161 with @sun's good feedback.

sun’s picture

form_builder() does not contain logic for particular element #types. Any attempt to add such logic won't fix.

mgifford’s picture

Ok, so Brandon will re-roll the patch in #161 with @sun's changes and then we'll test that & return the label patch as RTBC.

I'm glad we had this deviation to explore using title, but glad we've got a solution that doesn't require it.

bowersox’s picture

Status: Needs work » Needs review
FileSize
3.94 KB

Please review and test this clean-up of #161. Maybe we can get this back to RTBC as discussed.

The one significant change in this patch is that I removed the change to filter.module. @sun was right that it would require changes to filter.js/filter.css. The javascript code on the text format dropdown on /node/add uses the label tag, so the change was breaking that dropdown functionality. Since this is a bigger deal I propose to open a new issue for it.

The patch attached includes the changes suggested by @sun in #173:

  • Removed trailing white space in 2 places
  • Moved two comments to preceed the elseif statements
mgifford’s picture

Status: Needs review » Reviewed & tested by the community

I've compared form-label-cleanup-v3.patch with the previous form-label-cleanup-v2.patch (which was RTBC). Brandon brought over the improvements from @sun.

I applied this in an updated CVS version of Drupal and verified that the labels were produced as expected with the block code. For those who want to test this with the label solution see:

Index: modules/block/block.admin.inc
===================================================================
RCS file: /cvs/drupal/drupal/modules/block/block.admin.inc,v
retrieving revision 1.75
diff -u -p -r1.75 block.admin.inc
--- modules/block/block.admin.inc 9 Mar 2010 12:09:52 -0000 1.75
+++ modules/block/block.admin.inc 13 Mar 2010 05:27:14 -0000
@@ -80,6 +80,8 @@ function block_admin_display_form($form,
       '#type' => 'weight',
       '#default_value' => $block['weight'],
       '#delta' => $weight_delta,
+      '#title_display' => 'invisible',
+      '#title' => t('Weight for') . ' ' . check_plain($block['info']),
     );
     $form[$key]['region'] = array(
       '#type' => 'select',

Hopefully this can now get into core quickly as it's had this additional review.

Everett Zufelt’s picture

I am still unclear as to why the #title property cannot be used as the title attribute on form fields other than radio and checkbox.

bowersox’s picture

@Everett, here are a few links to relevant posts: #131, #132, and @sun's explanation in #177.

The #title_display property should ultimately support the 'attribute' setting for any form field type. Because of that, it seems like the logic really should go into form_builder() or the equivalent. This is something I'd like to talk with people about at DrupalCon and in planning for Form API improvements for Drupal 8.

sun’s picture

Looks good.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

chx’s picture

Status: Fixed » Reviewed & tested by the community

Roll this back. I said this before: and what's with CSS 2.1 attribute selectors? Like label[for=whatever] { visibility: hidden } . I am fighting this patch bloating the already bloated form.inc and now it came back to life like some zombie and got in more code when that was not necessary. Or explain what the latest zombie can do which my simple CSS selector can't?

Dries’s picture

chx, good question. I overlooked your small comment in the sea of follow-ups. I guess it all depends on whether we care about IE6 or not. At the very least, we should add a TODO to remove this code once we drop support for IE6 (if not today).

mgifford’s picture

@chx, I've certainly tried to see how it might be possible to use CSS 2.1 attribute selectors to hide the labels. I know we talked about this ages ago on IRC and you suggested it then. I'm not a CSS guy, but really don't see how it would be possible to do what you're asking from some simple CSS changes. There are many places where it is important to add labels to forms but to hide them from the browser. The Drag/Drop patch is just one. I appreciate you wanting to reduce the size of the form.inc file. I certainly hope that we were able to streamline it a bit, although perhaps not.

I'm making my comments below from http://www.456bereastreet.com/archive/200510/css_21_selectors_part_2/

I don't think it's going to work if we use something like label[for=edit-user-new-weight] we're going to have hundreds of unique ones in CSS and no way to deal with blocks that have been added by the user. It would also require having to hardcode some CSS somewhere rather than allowing a FAPI based solution to produce invisible labels.

It's possible that we could use label[for|=edit-user] to pull up , or Weight for Recent content but that's pretty dangerous as I'm guessing that forms with ID's "edit-node" are going to be very common. Now we could re-write how the ID's are created to reduce the risk, so that it's in the form "edit-node-weight-recent", but that's going to cause no end to troubles.

I don't think that practically we can expect to be able to address this with CSS (even CSS3). However, I'm totally willing to be proved wrong here. However I need to see some solid examples with existing problems (like the Drag/Drop pages). Using title as I suggested in #169 would have some advantages. However, I don't see a simple CSS solution will work here.

sun’s picture

Status: Reviewed & tested by the community » Fixed

And what's with CSS 2.1 attribute selectors? Like label[for=whatever] { visibility: hidden } . Yes IE6 does not support this and so what.

You'll notice that you used a dynamic CSS selector here. Turn that example snippet into a CSS selector that applies globally and you end up in the patch that has been committed.

Unless there is a more elaborative statement of what the actual problem with the committed patch is, back to fixed.

Status: Fixed » Closed (fixed)

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

yched’s picture

Turns out the original patch (#148) broke Field API's display of multiple, required fields :

multiple_required_field.png

Patch in #980144: Issues with "required, multiple" fields in forms