Form element labeling is inconsistent, inflexible and bad for accessibility

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.

The last submitted patch failed testing.

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.

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

+1

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

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

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

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

@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.

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:

HTML After patch:

@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.

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.

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.

The last submitted patch failed testing.

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

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!

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.

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

to themes/seven/style.css after line 498

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

The last submitted patch failed testing.

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

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.

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.

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

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

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:

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

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

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.

The last submitted patch failed testing.

@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:

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.

The last submitted patch failed testing.

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

Didn't put this to needs review earlier.

Needed to apply

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:

subscribe

Setting to needs review to test most recent patch.

The last submitted patch failed testing.

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

Hopefully this fixes it.

Code review only, I hope this helps.

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

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 ?

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

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

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

This review is powered by Dreditor.

Sorry, removed this tag by mistake.

@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.

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 :

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.

@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.

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.

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.

Can take quite a while to backtrack things in simpleTest.

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

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...

The last submitted patch failed testing.

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:

That we've removed in this patch.

This wraps the 7 calls to:

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.

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.

@mgifford

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

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.

The last submitted patch failed testing.

@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:

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

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?".

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

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!

Where are those properties used?

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?

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

" " can be ' '

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

No need for 2 line breaks

This review is powered by Dreditor

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.

@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.

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.

+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?

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 :

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.

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.

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

Needs an ending " in the multiple part.

Please break into multiple lines.

Please break into multiple lines.

Please wrap this comment at 80 chars.

@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.

* 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.

The last submitted patch failed testing.

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.

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

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.

@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.

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.

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.

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.

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).

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()?

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

To function drupal_common_theme(), patch #70 added:

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

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

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:

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

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

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

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

And a few nitpicks:

Should be "element's" I think.

Missing a period, and the next comment too.

Needs a line break before @return.

This review is powered by Dreditor.

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:

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

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

includes/locale.inc's locale_translate_edit_form():

and _locale_languages_common_controls() form:

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

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

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

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

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

and menu_configure():

modules/system/system.module system_block_configure():

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

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

and user_admin_permissions():

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

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

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

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?

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

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.

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.

The last submitted patch failed testing.

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

The last submitted patch failed testing.

Taking a look at these failures now

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.

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

Code review:

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

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.

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

What does it mean to style something as an option?

Can you please explain this modification?

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.

@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.

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.

@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.

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.

Eliminate some fuzz...

@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."

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.

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.

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.

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?

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:

and something like this in theme.inc:

----

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

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

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.

@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.

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

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.

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.

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.

The last submitted patch failed testing.

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.

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.

@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.

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.

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.

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.

Isn't this a waste of resources?

Code style issue.

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

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.

- We don't usually glue words together.

- We don't capitalize each word.

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

@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.

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.

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

Strange wrapping here.

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.

(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.

I don't grok this.

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

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

We can directly access $variables here.

$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.

We removed all colons from form element labels.

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

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

This function does not exist.

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

This review is powered by Dreditor.

@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!

1) None. Just continue.

The other points don't seem to raise questions.

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.

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!

This still awaits my review.

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).

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.

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

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.

"...on the optional #title_display property"

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

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.

We can remove this comment.

Why?

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

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

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

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().

Not sure what this means.

s/If required/If the element is required/

s/in/to/

Not sure how this comment maps to the following code.

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

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).

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

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

Why is this needed?

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

I need a explanation for this change.

This review is powered by Dreditor.

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.

@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.

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.

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.

The last submitted patch failed testing.

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().

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.

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.

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.

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?

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:

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.

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.

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.

@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.

@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.