Realized that most form elements are now using labels, but select boxes aren't at the moment.

The right way to do this is described here:
http://www.webaim.org/techniques/forms/controls.php#select

Which roughly works it's way into:

function theme_select($element) {
  $select = '';
  $size = $element['#size'] ? ' size="' . $element['#size'] . '"' : '';
  _form_set_class($element, array('form-select'));
  $multiple = $element['#multiple'];
  if (!is_null($element['#title'])) {
    $output = '<label class="option" for="' . $element['#id'] . '">' . $element['#title'] . '</label>';
  }
  $output .= '<select name="' . $element['#name'] . '' . ($multiple ? '[]' : '') . '"' . ($multiple ? ' multiple="multiple" ' : '') . drupal_attributes($element['#attributes']) . ' id="' . $element['#id'] . '" ' . $size . '>' . form_select_options($element) . '</select>';
  return $output;
}

Which doesn't really improve the menu dropdown as there don't seem to be any title tags to those elements.

Anyways, we should be able to provide labels (even human understandable labels) for all form elements.

Mike

CommentFileSizeAuthor
#12 issue-451980.patch2.26 KBlilou
#10 issue-451980.patch2.24 KBlilou
#9 issue-451980.patch2.24 KBlilou
#1 drupal_select_label.patch1.75 KBmgifford

Comments

mgifford’s picture

mgifford
he/him
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new drupal_select_label.patch1.75 KB

So I wrote this patch & provided another because selection boxes haven't been written with titles before. I used for an example admin/content/node and the content filtering defined inside the "Show only items where" grouping. It will need to be applied to all of these files too, but I wanted this to be a space to get some feedback on this patch first:

includes/locale.inc
modules/aggregator/aggregator.admin.inc
modules/aggregator/aggregator.admin.inc
modules/aggregator/aggregator.module
modules/aggregator/aggregator.processor.inc
modules/block/block.admin.inc
modules/block/block.api.php
modules/book/book.module
modules/color/color.module
modules/comment/comment.admin.inc
modules/comment/comment.module
modules/contact/contact.admin.inc
modules/contact/contact.pages.inc
modules/dblog/dblog.admin.inc
modules/field/modules/options/options.module
modules/filter/filter.module
modules/forum/forum.admin.inc
modules/forum/forum.module
modules/locale/locale.module
modules/menu/menu.admin.inc
modules/menu/menu.module
modules/node/content_types.inc
modules/node/node.admin.inc
modules/node/node.module
modules/poll/poll.module
modules/profile/profile.admin.inc
modules/profile/profile.module
modules/search/search.admin.inc
modules/search/search.api.php
modules/statistics/statistics.admin.inc
modules/statistics/statistics.module
modules/system/system.admin.inc
modules/system/system.module
modules/taxonomy/taxonomy.module
modules/trigger/trigger.admin.inc
modules/upload/upload.admin.inc
modules/user/user.module

This patch gives the following output:

<dd class="b"><div class="form-item" id="edit-status-1-wrapper">
 <label for="edit-status-1">Filter by status: </label>

 <select name="status" class="form-select" id="edit-status-1" ><option value="status-1">published</option><option value="status-0">not published</option><option value="promote-1">promoted</option><option value="promote-0">not promoted</option><option value="sticky-1">sticky</option><option value="sticky-0">not sticky</option></select>
</div>
<div class="form-item" id="edit-type-wrapper">
 <label for="edit-type">Filter by type: </label>
 <select name="type" class="form-select" id="edit-type" ><option value="article">Article</option><option value="page">Page</option></select>

</div>
<div class="form-item" id="edit-term-wrapper">
 <label for="edit-term">Filter by term: </label>
 <select name="term" class="form-select" id="edit-term" ><optgroup label="Tags"><option value="2">1234</option><option value="3">listing 123</option><option value="1">test</option></optgroup></select>
</div>
</dd>

But the labels for these pages need to be hidden off-screen so that they don't disrupt the page. This isn't a huge deal, but we do need to make sure to NOT use display:none here. So let's get some movement on http://drupal.org/node/58941 so that this page doesn't get more visually complicated.

Mike

Status: Needs review » Needs work

The last submitted patch failed testing.

lilou’s picture

lilou commented
Category: bug » task
Status: Needs work » Needs review

Lower priority.

mgifford’s picture

mgifford
he/him
Primary language English
commented

Can I get a confirmation about the difference between "Testbed results" & "The last submitted patch failed testing."

Also, lilou, lower priority for who and why? Labels are pretty darn important if accessibility is going to be met without hacking core.

lilou’s picture

lilou commented

@mgifford : I misspoke (i'm not an english speaker) -- it was rather a change of category (your issue is not really a bug).

mgifford’s picture

mgifford
he/him
Primary language English
commented

Thanks for the clarification @lilou. I'm not going to switch it back, but I still think this is a bug and not a todo item. If a blind person cannot understand a form that they have to fill in (say to register for a site) because the select boxes aren't using labels then I'd call it a bug. If it doesn't fit with WC3 standards I'd call it a bug. But whatever. I'd just like to see it fixed & in core.

lilou’s picture

lilou commented
Category: task » bug
Status: Needs review » Reviewed & tested by the community

OK, same approach as http://api.drupal.org/api/function/theme_radio/7

lilou’s picture

lilou commented
Status: Reviewed & tested by the community » Needs review

$output isn't returned by theme_select()

lilou’s picture

lilou commented
StatusFileSize
new issue-451980.patch2.24 KB
lilou’s picture

lilou commented
StatusFileSize
new issue-451980.patch2.24 KB
mgifford’s picture

mgifford
he/him
Primary language English
commented

Yup, Brilliant! Thanks lilou..

lilou’s picture

lilou commented
StatusFileSize
new issue-451980.patch2.26 KB

Add a placeholder in t() :

    $form['filters']['status'][$key] = array('#type' => 'select', '#options' => $filter['options'], '#title' => t('Filter by @name', array ('@name' => $filter['title'])));

Status: Needs review » Needs work

The last submitted patch failed testing.

Everett Zufelt’s picture

Everett Zufelt commented
Status: Needs work » Needs review

Setting to needs review to see if head will install.

Personally I would prefer fapi be retooled to handle form labels across all input field types. However, this is a good interim measure.

Everett Zufelt’s picture

Everett Zufelt commented

+1 on this patch.

Looks like the patch passed testing this time around. Can we get some people to review this so that we can get it into core?

Status: Needs review » Needs work

The last submitted patch failed testing.

Everett Zufelt’s picture

Everett Zufelt commented
Status: Needs work » Needs review

Let's try testing this patch one more time.

bowersox’s picture

bowersox commented

+1 on this patch

Everett Zufelt’s picture

Everett Zufelt commented

Revisiting this issue as part of a review of how form elements are labeled throughout core.

Someone can correct me if I am wrong, but it is my understanding that:

1. If #title is set for a select element then a label will be generated by theme_element().

2. Developers can choose to do with the label what they like.

3. It may be useful to add a new property to all form fields, like #show-label, to allow developers to hide unwanted labels off-screen with the system class .element-invisible.

bowersox’s picture

bowersox commented

I suggest we start by adding these labels as 'element-invisible' for now so that we don't break the layout of all those fields. mgifford listed dozens of select fields that might need attention. Adding a visual label on-screen does break the layout of some. One example is the 'Find Content' form discussed in #1, where this patch causes label text to overlap other fields. After we have invisible labels in place, then we can review the fields individually and make the labels visible when appropriate.

The patch in 12 applies and apparently passes the testing bot. I suggest we start there and move ahead to an incremental improvement that we can get done in time for D7.

@Everett

1. Yes, the patch in 12 uses #title for the label. If there is no #title then there is no label element either. Where the patch uses !is_null($element['#title']) I suggest we use !empty($element['#title']) instead so that we don't end up with empty labels.

3. I agree. Developers would love this. As mentioned above, I suggest we start with 'element-invisible' for all these labels and then improve from there. Maybe the attribute can be called #invisible-label. In many cases we may ultimately find that the label may not need to be visible if it is entirely understandable for sighted users based on context.

@mgifford

You had a long list of all the select labels we would need to review. In a brief review I found just a few without a #title already: includes/form.inc, modules/comment/comment.admin.inc, modules/image/image.admin.inc, modules/field_ui/field_ui.admin.inc, modules/system/system.module, modules/trigger/trigger.admin.inc, modules/user/user.admin.inc.

mgifford’s picture

mgifford
he/him
Primary language English
commented

Thanks Brandon. I just grepped for a relevant string and pulled out a list of files to review. I didn't go into looking at which were missing the #title.

Look forward to having a new patch rolled to review.

Mike

Everett Zufelt’s picture

Everett Zufelt commented

@brandon @mgifford

Why don't we add the property #show-label to the select type, default to false. If not empty(#title) and #show-label then output the label,
else if not empty(#title) and not #show-label set the title attribute of the select element to #title

Everett Zufelt’s picture

Everett Zufelt commented

@brandon @mgifford

Why don't we add the property #show-label to the select type, default to false. If not empty(#title) and #show-label then output the label,
else if not empty(#title) and not #show-label set the title attribute of the select element to #title

mgifford’s picture

mgifford
he/him
Primary language English
commented

Makes sense to me.

0) The default isn't to spit out a label and this is the behavior folks expect
1) If there's a title and you explicitly want to display the label then do so
2) If there's a title and the show-attribute element isn't set (or is false) then put the title of the select into the #title
3) If there isn't a title then don't spit out a label or insert a blank title

It's the end of a long week though, so perhaps I'm missing something.

Everett Zufelt’s picture

Everett Zufelt commented
Status: Needs review » Closed (works as designed)

The current behavior of select box labeling is by design.

Someone can correct me if I am wrong on this, but select form elements are currently themed by theme_select() and theme wrapped by theme_form_element()

If there is a #title attribute set for a select element when it goes through theme_form_element a label will be generated. As form labeling is currently inflexible (see #558928: Form element labeling is inconsistent, inflexible and bad for accessibility), and does not allow developers to set the #title property without outputting a label some developers do not set the #title, or set it and then unset it before theming..

mgifford’s picture

mgifford
he/him
Primary language English
commented

So when we get http://drupal.org/node/558928 into core then select boxes will be able to have labels generated by the form array including a title element.

The challenge will then be to identify which forms in core do not have titles in them because the developers did not want to have them displayed.

mgifford’s picture

mgifford
he/him
Primary language English
commented

This needs to be tested to verify that it works as described in D7 before closing this issue.