Labels missing from a number of locale admin forms

+++ modules/locale/locale.admin.inc	17 Aug 2010 14:00:51 -0000
@@ -550,8 +557,15 @@ function _locale_languages_configure_for
       $table_form['enabled'][$id] = array('#type' => 'checkbox', '#default_value' => $enabled);

maybe I'm misreading the patch, but shouldn't this line be removed?

Powered by Dreditor.

All tests good, and code ok

Wouldn't it be better if we consolidated the 'Enabled' and 'Description' columns in http://drupal.org/files/issues/screen-capture-10_3.png? The reason I'm suggesting this is because the description seems to be much better than the invisible text. I don't have the answer -- just bringing it up as a suggestion. Thoughts?

I think I already tried the idea of using checkboxes in a tabledrag for the new Filter format UI, but that did not work out.

or...

'#title' => t('Enable @title language detection method', array('@title' => drupal_strtolower($provider_name))),

Looks good, but I'm not comfortable enough with this module to give an RTBC.

Setting to major priority. IMO missing labels, a WCAG 2.0 level A success criteria, is major (if not critical).

Code looks good. I haven't tested it yet.

+++ modules/locale/locale.admin.inc	17 Aug 2010 14:00:51 -0000
@@ -30,6 +30,8 @@ function locale_languages_overview_form(
+      '#title' => t('Weight for @row', array('@row' => $language->name)),

@row is semantically incorrect. Use @language instead.

+++ modules/locale/locale.admin.inc	17 Aug 2010 14:00:51 -0000
@@ -550,8 +557,15 @@ function _locale_languages_configure_for
+        '#title' => t('Enable/Disable language'),

I don't see "Enable/Disable language" anywhere in the screenshots. Where is it being used? Also, checkbox titles tell users what happens if they check the checkbox, not what happens if they dont, so use t('Enable language') or t('Enable @language').

Powered by Dreditor.

@mgifford great stuff. No idea what @Xano is talking about. Here's some additional thoughts:

      $title = drupal_render($form['name'][$key]);
      $form['enabled'][$key]['#title'] = t('Enable @title', array('@title' => $title));
      $form['enabled'][$key]['#title_display'] = 'invisible';
      $form['site_default'][$key]['#title'] = t('Set @title as default', array('@title' => $title));
      $form['site_default'][$key]['#title_display'] = 'invisible';
      $rows[] = array(
        'data' => array(
          '<strong>' . $title . '</strong>',

This stuff is not theming, it's form building and so shouldn't be in a theme function. We can't put it into the form builder because the checkboxes and radios aren't expanded yet, but perhaps this should go in a #process step.

Otherwise looks great.

I think I accidentally reviewed an older patch. My bad!

#82694: System Information: information about the Drupal installation and system environment fixed this, looks like a straight duplicate to me. If that issue missed one I'd suggest opening a new issue since there's a lot of discussion here which is outdated now that other patch is in.

Applied and tested with 8.x-dev.

#20: locale_overview_labels_v4.patch queued for re-testing.

+++ modules/locale/locale.admin.inc	26 Nov 2010 19:47:15 -0000
@@ -32,6 +32,8 @@ function locale_languages_overview_form(
       '#title_display' => 'invisible',
...
+      '#title_display' => 'invisible',
+      '#title' => t('Weight for @title', array('@title' => $language->name)),

At minimum, #title_display is duplicated here. Quite possibly, #title is too (out of diff context). In any case, #title should be defined before #title_display.

+++ modules/locale/locale.admin.inc	26 Nov 2010 19:47:15 -0000
@@ -77,9 +79,14 @@ function theme_locale_languages_overview
+      $title = drupal_render($form['name'][$key]);
+      $form['enabled'][$key]['#title'] = t('Enable @title', array('@title' => $title));
...
+      $form['site_default'][$key]['#title'] = t('Set @title as default', array('@title' => $title));

Since drupal_render() is called on the element, I can only guess that the value in $title is already sanitized to be displayed in HTML. Thus, @title would run check_plain() once again on the already sanitized string, potentially leading to double-escaping.

This issue needs to be verified very carefully.

+++ modules/locale/locale.admin.inc	26 Nov 2010 19:47:15 -0000
@@ -568,20 +575,21 @@ function _locale_languages_configure_for
+      $provider_name = check_plain($provider['name']);
...
-        '#title' => t('Weight for @title', array('@title' => $provider['name'])),
+        '#title' => t('Weight for @title language detection method', array('@title' => drupal_strtolower($provider_name))),
...
-      $table_form['title'][$id] = array('#markup' => check_plain($provider['name']));
+      $table_form['title'][$id] = array('#markup' => $provider_name);
...
-        '#title' => t('@title language provider', array('@title' => $provider['name'])),
+        '#title' => t('Enable @title language detection method', array('@title' => drupal_strtolower($provider_name))),

check_plain() already ran on $provider_name, so @title leads to potential double-escaping.

In this case, I'd recommend to leave the manual check_plain()s intact and pass the original $provider['name'] as value for @title instead.

12 days to next Drupal core point release.

Now you've inconsistently changed the t() tokens - the token in the string subject must match the array key in the passed in replacement token array.

+++ b/modules/locale/locale.admin.inc
@@ -80,9 +80,14 @@ function theme_locale_languages_overview_form($variables) {
+      $title = drupal_render($form['name'][$key]);
+      $form['enabled'][$key]['#title'] = t('Enable !title', array('@title' => $title));
+      $form['enabled'][$key]['#title_display'] = 'invisible';
+      $form['site_default'][$key]['#title'] = t('Set @title as default', array('@title' => $title));
+      $form['site_default'][$key]['#title_display'] = 'invisible';

This chunk of code needs an inline comment that explains why the form element properties are manipulated in a theme function instead of already being set in the form constructor.

The reason for doing it in the theme function is that we're theming the form as a table, so the element labels are not visible and do not fit into the table cells. If we wouldn't theme the form as a table, then the regular element labels would be sufficient and perfectly okay. (Obviously, don't copypaste this 1:1, please)

12 days to next Drupal core point release.

+++ b/modules/locale/locale.admin.inc
@@ -80,9 +80,16 @@ function theme_locale_languages_overview_form($variables) {
+      // Accessibility enhancement to ensure table elements are labeled properly but not visible.

Exceeds 80 chars.

Let's change that into:

"Add invisible labels for the checkboxes and radio buttons in the table for accessibility. These changes are only required and valid when the form is themed as a table, so it would be wrong to perform them in the form constructor."

The similar other changes we applied elsewhere in core do not need this explanation, because we applied them to the form constructors. The situation is a bit special here, since such kind of form structure changes normally do not belong into a theme function. We apply them to the theme function, because the form is themed as a table. If some module would override that theming to not output the form as a table, then the form would contain extremely wonky labels for the checkboxes and radio buttons. Hope it's clear now?

12 days to next Drupal core point release.

Thanks, looks good to go for me now.

Committed to 8.x. Moving to 7.x.

+1 for this. The code looks good (both what was committed to 8.x and is now proposed for 7.x). The labels will help make this form meaningful.

While the hidden form elements are probably okay, since they fix a legitimate accessibility bug and are not user-facing the majority of the time, I don't think we can do these in D7:

-        '#title' => t('Weight for @title', array('@title' => $provider['name'])),
+        '#title' => t('Weight for !title language detection method', array('!title' => drupal_strtolower($provider_name))),

...

-        '#title' => t('@title language provider', array('@title' => $provider['name'])),
+        '#title' => t('Enable !title language detection method', array('!title' => drupal_strtolower($provider_name))),

These would break user-facing (well, admin-facing) strings, and this seems to be a small text clarification, not an actual bug fix.

@webchick: The string changes you objected in #42 are not visible anyway...? They are output as 'title' attribute of a checkbox. Only visually impaired users read them.

Applied and tested successfully against 7.x

@Sun

Just saw your comment. Although I understand that the strings are not visible, and would enhance the UI slightly for screen-reader users, I think that it is a good practice to keep strings aligned. Unless there is a strong reason I don't see the need for a string exception for accessibility as a general rule.

Great, thanks for the re-roll!

Committed to 8.x and 7.x.