- Patch #558928 by brandonojc, mgifford, Owen Barton, Everett Zufelt: improved consistency, flexibility and accessibility of form element labels.

9 files changed, 136 insertions, 29 deletions

diff --git a/modules/comment/comment-node-form.js b/modules/comment/comment-node-form.js
index 0f51884a2..471cb106c 100644
--- a/modules/comment/comment-node-form.js
+++ b/modules/comment/comment-node-form.js
@@ -5,7 +5,7 @@
 Drupal.behaviors.commentFieldsetSummaries = {
   attach: function (context) {
     $('fieldset#edit-comment-settings', context).setSummary(function (context) {
-      return Drupal.checkPlain($('input:checked', context).parent().text());
+      return Drupal.checkPlain($('input:checked', context).next('label').text());
     });
     // Provide the summary for the node type form.
     $('fieldset#edit-comment', context).setSummary(function(context) {
@@ -15,7 +15,7 @@ Drupal.behaviors.commentFieldsetSummaries = {
       vals.push($("select[name='comment'] option:selected", context).text());
 
       // Threading.
-      var threading = $("input[name='comment_default_mode']:checked", context).parent().text();
+      var threading = $("input[name='comment_default_mode']:checked", context).next('label').text();
       if (threading) {
         vals.push(threading);
       }
diff --git a/modules/node/content_types.js b/modules/node/content_types.js
index 8ff713ba9..ed4fe81d3 100644
--- a/modules/node/content_types.js
+++ b/modules/node/content_types.js
@@ -22,7 +22,7 @@ Drupal.behaviors.contentTypes = {
     });
     $('fieldset#edit-display', context).setSummary(function(context) {
       var vals = [];
-      $('input:checked', context).parent().each(function() {
+      $('input:checked', context).next('label').each(function() {
         vals.push(Drupal.checkPlain($(this).text()));
       });
       if (!$('#edit-node-submitted', context).is(':checked')) {
diff --git a/modules/shortcut/shortcut.admin.inc b/modules/shortcut/shortcut.admin.inc
index 0baa368c2..27b6a5dfd 100644
--- a/modules/shortcut/shortcut.admin.inc
+++ b/modules/shortcut/shortcut.admin.inc
@@ -133,25 +133,6 @@ function shortcut_set_switch_submit($form, &$form_state) {
 }
 
 /**
- * Theme function for the form that switches shortcut sets.
- *
- * @param $variables
- *   An associative array containing:
- *   - form: An array representing the form.
- * @return
- *   A themed HTML string representing the content of the form.
- *
- * @ingroup themeable
- * @see shortcut_set_switch()
- */
-function theme_shortcut_set_switch($variables) {
-  $form = $variables['form'];
-  // Render the textfield for adding a new set inline with the radio button.
-  $form['set']['new']['#title'] = t('New set: !textfield', array('!textfield' => drupal_render($form['new'])));
-  return drupal_render_children($form);
-}
-
-/**
  * Menu callback; Build the form for customizing shortcut sets.
  *
  * @param $form
diff --git a/modules/shortcut/shortcut.css b/modules/shortcut/shortcut.css
index b07f6a025..3cb92652b 100644
--- a/modules/shortcut/shortcut.css
+++ b/modules/shortcut/shortcut.css
@@ -82,3 +82,12 @@ div.add-or-remove-shortcuts a:hover span.text {
   -webkit-border-bottom-right-radius: 5px;
 }
 
+#shortcut-set-switch .form-type-radios {
+  padding-bottom: 0;
+  margin-bottom: 0;
+}
+
+#shortcut-set-switch .form-item-new {
+  padding-top: 0;
+  padding-left: 17px;
+}
diff --git a/modules/shortcut/shortcut.module b/modules/shortcut/shortcut.module
index 225cb8e32..5f8da1835 100644
--- a/modules/shortcut/shortcut.module
+++ b/modules/shortcut/shortcut.module
@@ -105,10 +105,6 @@ function shortcut_menu() {
  */
 function shortcut_theme() {
   return array(
-    'shortcut_set_switch' => array(
-      'render element' => 'form',
-      'file' => 'shortcut.admin.inc',
-    ),
     'shortcut_set_customize' => array(
       'render element' => 'form',
       'file' => 'shortcut.admin.inc',
diff --git a/modules/simpletest/tests/form.test b/modules/simpletest/tests/form.test
index 527b5974b..0f1769d43 100644
--- a/modules/simpletest/tests/form.test
+++ b/modules/simpletest/tests/form.test
@@ -207,6 +207,62 @@ class FormValidationTestCase extends DrupalWebTestCase {
 }
 
 /**
+ * Test form element labels, required markers and associated output.
+ */
+class FormsElementsLabelsTestCase extends DrupalWebTestCase {
+
+  public static function getInfo() {
+    return array(
+      'name' => 'Form element and label output test',
+      'description' => 'Test form element labels, required markers and associated output.',
+      'group' => 'Form API',
+    );
+  }
+
+  function setUp() {
+    parent::setUp('form_test');
+  }
+
+  /**
+   * Test form elements, labels, title attibutes and required marks output
+   * correctly and have the correct label option class if needed.
+   */
+  function testFormLabels() {
+    $this->drupalGet('form_test/form-labels');
+
+    // Check that the checkbox/radio processing is not interfering with
+    // basic placement.
+    $elements = $this->xpath('//input[@id="edit-form-checkboxes-test-third-checkbox"]/following-sibling::label[@for="edit-form-checkboxes-test-third-checkbox" and @class="option"]');
+    $this->assertTrue(isset($elements[0]), t("Label follows field and label option class correct for regular checkboxes."));
+
+    $elements = $this->xpath('//input[@id="edit-form-radios-test-second-radio"]/following-sibling::label[@for="edit-form-radios-test-second-radio" and @class="option"]');
+    $this->assertTrue(isset($elements[0]), t("Label follows field and label option class correct for regular radios."));
+
+    // Exercise various defaults for checkboxes and modifications to ensure
+    // appropriate override and correct behaviour.
+    $elements = $this->xpath('//input[@id="edit-form-checkbox-test"]/following-sibling::label[@for="edit-form-checkbox-test" and @class="option"]');
+    $this->assertTrue(isset($elements[0]), t("Label follows field and label option class correct for a checkbox by default."));
+
+    // Exercise various defaults for textboxes and modifications to ensure
+    // appropriate override and correct behaviour.
+    $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."));
+
+    $elements = $this->xpath('//input[@id="edit-form-textfield-test-no-title-required"]/preceding-sibling::label[@for="edit-form-textfield-test-no-title-required"]/span[@class="form-required"]');
+    $this->assertTrue(isset($elements[0]), t("Label tag with required marker preceeds required textfield with no title."));
+
+    $elements = $this->xpath('//input[@id="edit-form-textfield-test-title"]/preceding-sibling::span[@class="form-required"]');
+    $this->assertFalse(isset($elements[0]), t("No required marker on non-required field."));
+
+    $elements = $this->xpath('//input[@id="edit-form-textfield-test-title-after"]/following-sibling::label[@for="edit-form-textfield-test-title-after" and @class="option"]');
+    $this->assertTrue(isset($elements[0]), t("Label after field and label option class correct for text field."));
+
+    $elements = $this->xpath('//label[@for="edit-form-textfield-test-title-no-show"]');
+    $this->assertFalse(isset($elements[0]), t("No label tag when title set not to display."));
+  }
+}
+
+/**
  * Test the tableselect form element for expected behavior.
  */
 class FormsElementsTableSelectFunctionalTest extends DrupalWebTestCase {
diff --git a/modules/simpletest/tests/form_test.module b/modules/simpletest/tests/form_test.module
index 9c253e932..c9c208d53 100644
--- a/modules/simpletest/tests/form_test.module
+++ b/modules/simpletest/tests/form_test.module
@@ -94,6 +94,14 @@ function form_test_menu() {
     'type' => MENU_CALLBACK,
   );
 
+  $items['form_test/form-labels'] = array(
+    'title' => 'Form label test',
+    'page callback' => 'drupal_get_form',
+    'page arguments' => array('form_label_test_form'),
+    'access arguments' => array('access content'),
+    'type' => MENU_CALLBACK,
+  );
+
   return $items;
 }
 
@@ -474,6 +482,62 @@ function form_test_storage_form_submit($form, &$form_state) {
   drupal_set_message("Form constructions: ". $_SESSION['constructions']);
 }
 
+ /**
+ * A form for testing form labels and required marks.
+ */
+function form_label_test_form(&$form_state) {
+  $form['form_checkboxes_test'] = array(
+    '#type' => 'checkboxes',
+    '#title' => t('Checkboxes test'),
+    '#options' => array(
+      'first-checkbox' => t('First checkbox'),
+      'second-checkbox' => t('Second checkbox'),
+      'third-checkbox' => t('Third checkbox'),
+    ),
+  );
+  $form['form_radios_test'] = array(
+    '#type' => 'radios',
+    '#title' => t('Radios test'),
+    '#options' => array(
+      'first-radio' => t('First radio'),
+      'second-radio' => t('Second radio'),
+      'third-radio' => t('Third radio'),
+    ),
+  );
+  $form['form_checkbox_test'] = array(
+    '#type' => 'checkbox',
+    '#title' => t('Checkbox test'),
+  );
+  $form['form_textfield_test_title_and_required'] = array(
+    '#type' => 'textfield',
+    '#title' => t('Textfield test for required with title'),
+    '#required' => TRUE,
+  );
+  $form['form_textfield_test_no_title_required'] = array(
+    '#type' => 'textfield',
+    // We use an empty title, since not setting #title supresses the label
+    // and required marker.
+    '#title' => '',
+    '#required' => TRUE,
+  );
+  $form['form_textfield_test_title'] = array(
+    '#type' => 'textfield',
+    '#title' => t('Textfield test for title only'),
+    // Not required.
+  );
+  $form['form_textfield_test_title_after'] = array(
+    '#type' => 'textfield',
+    '#title' => t('Textfield test for title after element'),
+    '#title_display' => 'after',
+  );
+  // Textfield test for title set not to display
+  $form['form_textfield_test_title_no_show'] = array(
+    '#type' => 'textfield',
+  );
+
+  return $form;
+}
+
 /**
  * Menu callback; Invokes a form builder function with a wrapper callback.
  */
diff --git a/modules/system/system.api.php b/modules/system/system.api.php
index f85c014a1..202c08732 100644
--- a/modules/system/system.api.php
+++ b/modules/system/system.api.php
@@ -320,6 +320,8 @@ function hook_cron_queue_info() {
  *  - "#pre_render": array of callback functions taking $element and $form_state.
  *  - "#post_render": array of callback functions taking $element and $form_state.
  *  - "#submit": array of callback functions taking $form and $form_state.
+ *  - "#title_display": optional string indicating if and how #title should be
+ *    displayed, see theme_form_element() and theme_form_element_label().
  *
  * @see hook_element_info_alter()
  * @see system_element_info()
diff --git a/modules/system/system.module b/modules/system/system.module
index 0e801f802..429d45534 100644
--- a/modules/system/system.module
+++ b/modules/system/system.module
@@ -56,7 +56,6 @@ define('REGIONS_VISIBLE', 'visible');
  */
 define('REGIONS_ALL', 'all');
 
-
 /**
  * Implement hook_help().
  */
@@ -399,7 +398,7 @@ function system_element_info() {
     '#process' => array('ajax_process_form'),
     '#theme' => 'radio',
     '#theme_wrappers' => array('form_element'),
-    '#form_element_skip_title' => TRUE,
+    '#title_display' => 'after',
   );
   $types['checkboxes'] = array(
     '#input' => TRUE,
@@ -414,7 +413,7 @@ function system_element_info() {
     '#process' => array('ajax_process_form'),
     '#theme' => 'checkbox',
     '#theme_wrappers' => array('form_element'),
-    '#form_element_skip_title' => TRUE,
+    '#title_display' => 'after',
   );
   $types['select'] = array(
     '#input' => TRUE,