Form labeling within D7 is inconsistent, inflexible and bad for accessibility.
Inconsistent: Most form elements have their label generated by theme_form_element(), while others like radio and checkbox provide their own labels.
Inflexible: There is no easy way for a form developer to tell FAPI where to put the label, or if a label should be generated at all.
Accessibility: When developers don't want labels to appear visually for elements, they cannot easily indicate that the element's #title property should be used as the title attribute for the element, and not as a label. This results in many instances of developers not setting #title so that a label isn't generated, and the element has no title associated with it for assistive technology users. Also, element containers like date, radios and checkboxes are currently being themed with a div, and not the more accessible fieldset.
Related issues:
#504962: Provide a compound form element with accessible labels
#501444: Administer > Modules pages make improper use of form labels
#451980: Select boxes still not using labels
#522772: Improve radio and checkbox title and labeling features for accessibility
#318165: wrong HTML on admin/user/user filter
#368759: All labels should require a for attributepointing to a unique id
Comment | File | Size | Author |
---|---|---|---|
#179 | form-label-cleanup-v3.patch | 3.94 KB | bowersox |
#169 | form-attribute-fix_2.patch | 789 bytes | mgifford |
#165 | form-attribute-fix.patch | 447 bytes | mgifford |
#161 | form-label-cleanup-v2.patch | 4.56 KB | bowersox |
#159 | form-label-cleanup-v2.patch | 4.56 KB | bowersox |
Comments
Comment #1
Everett Zufelt CreditAttribution: Everett Zufelt commentedThe 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.
Comment #3
Everett Zufelt CreditAttribution: Everett Zufelt commentedThe 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.
Comment #4
varunity CreditAttribution: varunity commentedyea definitely would be good for form labeling to be more consistent in D7
+1
Comment #5
mgifford1) 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
Comment #6
Brigadier CreditAttribution: Brigadier commentedHaven't had a chance to try the patch yet but I'm in favour of some more consistency in form labels.
Comment #7
annmcmeekin CreditAttribution: annmcmeekin commented+1 for making form element labelling better. It's such an important issue for so many users.
Comment #8
ronald_istos CreditAttribution: ronald_istos commentedAll 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
Comment #9
Everett Zufelt CreditAttribution: Everett Zufelt commented@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.
Comment #10
ronald_istos CreditAttribution: ronald_istos commentedHi 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:
Comment #11
Everett Zufelt CreditAttribution: Everett Zufelt commented@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.
Comment #12
mgiffordDrat. 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.
Comment #13
mgiffordThought I'd toss this in for a review swap. I'm willing to test others code in exchange for good reviews of this patch.
Comment #15
mgiffordSetting this back to needs review. Think Head's bust.
Comment #16
ronald_istos CreditAttribution: ronald_istos commentedHi, 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!
Comment #17
mgiffordSuppose 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.
Comment #18
ronald_istos CreditAttribution: ronald_istos commentedTo 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
Comment #20
cburschkaWell, 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.
Comment #21
Everett Zufelt CreditAttribution: Everett Zufelt commentedSome 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.
Comment #22
mgifford@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?
Comment #23
mgiffordThis 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.
Comment #25
bowersox CreditAttribution: bowersox commented@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:
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.
Comment #27
mgiffordOk, so working from Patch 3 I think I may have tracked down the outstanding simpletest errors.
thanks for your support on this everyone.
Mike
Comment #28
mgiffordDidn'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:
Comment #29
mattyoung CreditAttribution: mattyoung commentedsubscribe
Comment #30
Everett Zufelt CreditAttribution: Everett Zufelt commentedSetting to needs review to test most recent patch.
Comment #32
Everett Zufelt CreditAttribution: Everett Zufelt commentedLooks like we're getting closer with this patch, we're down to only 2 fails.
Comment #33
mgiffordHopefully this fixes it.
Comment #34
TheRec CreditAttribution: TheRec commentedCode 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.
Comment #35
TheRec CreditAttribution: TheRec commentedSorry, removed this tag by mistake.
Comment #36
Everett Zufelt CreditAttribution: Everett Zufelt commented@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.
Comment #37
TheRec CreditAttribution: TheRec commentedHmm 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 :
Comment #38
ronald_istos CreditAttribution: ronald_istos commentedWould 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.
Comment #39
Everett Zufelt CreditAttribution: Everett Zufelt commented@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.
Comment #40
mgiffordJust 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.
Comment #41
Everett Zufelt CreditAttribution: Everett Zufelt commentedThis 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.
Comment #42
mgiffordCan 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...
Comment #44
mgiffordThis 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.
Comment #45
Owen Barton CreditAttribution: Owen Barton commentedThis wraps the 7 calls to:
up in a simple helper function that reduces code duplication.
Comment #46
Everett Zufelt CreditAttribution: Everett Zufelt commentedIf 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.
Comment #47
mgiffordOk, here's another patch.
Btw, for all Mac/Linux users I'd heavily suggest looking at http://meld.sourceforge.net for reviewing these diffs.
Comment #48
Everett Zufelt CreditAttribution: Everett Zufelt commented@mgifford
Can you please explain the changes in the patch for comment #47?
Comment #49
bowersox CreditAttribution: bowersox commentedPlease review an alternative approach in the updated patch. There are 2 changes from #47:
@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.
Comment #51
mgifford@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?".
Comment #52
mgiffordIdentical to the previous patch, except that it's got the date issue fixed for simpletest. profile.module busted it again:
Comment #53
bowersox CreditAttribution: bowersox commentedHere'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!
Comment #54
dmitrig01 CreditAttribution: dmitrig01 commentedWhere 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
Comment #55
mgiffordIt 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.
Comment #56
Everett Zufelt CreditAttribution: Everett Zufelt commented@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.
Comment #57
bowersox CreditAttribution: bowersox commentedThis 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.
Comment #58
mgifford+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?
Comment #59
TheRec CreditAttribution: TheRec commentedI 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.
Comment #60
mgiffordSo 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.
Comment #61
dmitrig01 CreditAttribution: dmitrig01 commentedGenerally, 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.
Comment #62
mgifford@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.
Comment #63
Everett Zufelt CreditAttribution: Everett Zufelt commented* 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.
Comment #65
Owen Barton CreditAttribution: Owen Barton commentedHere 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.
Comment #66
mgiffordOk, 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.
Comment #67
Everett Zufelt CreditAttribution: Everett Zufelt commented@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.
Comment #68
Everett Zufelt CreditAttribution: Everett Zufelt commentedApplied 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.
Comment #69
Cliff CreditAttribution: Cliff commentedTrying 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.
Comment #70
Owen Barton CreditAttribution: Owen Barton commentedI 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.
Comment #71
Everett Zufelt CreditAttribution: Everett Zufelt commentedI 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).
Comment #72
Everett Zufelt CreditAttribution: Everett Zufelt commentedReview 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()?
Comment #73
mgiffordI'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
Comment #74
catchWe 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 we can remove the @todo for what to do when it's empty, since it'll be specifically declared as false then. This would lessen the API change as well since it'd be an optional attribute.
And a few nitpicks:
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.
Comment #75
mgiffordWanted 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?
Comment #76
Owen Barton CreditAttribution: Owen Barton commentedI am only 3/4 of the way through adding some tests, but uploading here as I am changing computers.
Comment #77
mgiffordThat'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.
Comment #78
Owen Barton CreditAttribution: Owen Barton commentedFinally 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:
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.
Comment #80
mgiffordThere 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
Comment #82
Owen Barton CreditAttribution: Owen Barton commentedTaking a look at these failures now
Comment #83
Owen Barton CreditAttribution: Owen Barton commentedHere is a patch that I hope should pass the tests now...
The fixes were:
Comment #84
mgiffordHey 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
Comment #85
Everett Zufelt CreditAttribution: Everett Zufelt commentedCode 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?
Comment #86
Owen Barton CreditAttribution: Owen Barton commentedThe 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.
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.
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.
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).
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.
This was explained in my previous comment.
Comment #87
TravisCarden CreditAttribution: TravisCarden commentedComment #88
Everett Zufelt CreditAttribution: Everett Zufelt commented@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.
Comment #89
Owen Barton CreditAttribution: Owen Barton commentedWhy 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?
I'll try and improve the comment around this statement for the next iteration to make this clearer.
Comment #90
Everett Zufelt CreditAttribution: Everett Zufelt commented@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.
Comment #91
Owen Barton CreditAttribution: Owen Barton commentedThis 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.
Comment #92
Owen Barton CreditAttribution: Owen Barton commentedEliminate some fuzz...
Comment #93
Cliff CreditAttribution: Cliff commented@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."
Comment #94
Owen Barton CreditAttribution: Owen Barton commentedI 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.
Comment #95
mgiffordI 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.
Comment #96
Everett Zufelt CreditAttribution: Everett Zufelt commentedCode 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.
Comment #97
bowersox CreditAttribution: bowersox commentedWe'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?
Comment #98
donquixote CreditAttribution: donquixote commentedNice 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
- 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..
Comment #99
donquixote CreditAttribution: donquixote commented(#98)
Can this be achieved by modulename_preprocess_form_element() in a custom module ?
Comment #100
mgiffordThis 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.
Comment #101
bowersox CreditAttribution: bowersox commented@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:
if (SHOW_TITLE_AFTER) ['class'] = 'option';
because the AFTER labels are precisely the ones we want to style as option.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.
Comment #102
mgiffordThis does sound like a good approach. Thanks for reviewing this very long & detailed thread and for working on a new patch!
Comment #103
bowersox CreditAttribution: bowersox commentedPlease 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:
Comment #105
mgiffordThis 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.
Comment #106
mgiffordJust 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.
Comment #109
bowersox CreditAttribution: bowersox commentedPlease review this re-rolled patch.
Changes are:
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.
Comment #110
mgiffordI'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.
Comment #111
bowersox CreditAttribution: bowersox commented@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.
Comment #112
mgiffordI 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.
Comment #113
mgiffordI'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.
Comment #114
Dries CreditAttribution: Dries commentedCode style issues:
else
andelseif
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.
Comment #115
bowersox CreditAttribution: bowersox commented@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.
Comment #116
bowersox CreditAttribution: bowersox commentedPlease review this re-rolled patch. It addresses coding standards pointed out by @Dries, and it:
Let me know if there's anything else I can do to perfect this and get it back to RTBC.
Comment #117
mgiffordThis looks great! I'm marking it as RTBC as I think all outstanding issues have now been addressed.
Comment #118
sunStrange 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.
Comment #119
bowersox CreditAttribution: bowersox commented@sun: Thanks for the feedback. Below are a few responses and plans for re-rolling this today.
What is the coding standard for if-conditions longer than 80 chars?
I'll re-roll with #title_display unless anyone has a better suggestion.
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.
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.
Great! It will be shorter and simpler to just use the plain strings. Thanks!
Comment #120
sun1) None. Just continue.
The other points don't seem to raise questions.
Comment #121
bowersox CreditAttribution: bowersox commentedPlease 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.
Comment #122
mgiffordI'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!
Comment #123
sunThis still awaits my review.
Comment #124
mgiffordGreat 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).
Comment #125
sunYears 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.
Comment #126
sunTo clarify in advance:
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.
Comment #127
bowersox CreditAttribution: bowersox commented@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.
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.
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.
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.
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'.
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.
Comment #128
sunNope, sorry, that's scope creep and doesn't belong into this issue. My fault of mentioning it.
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())
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.
Comment #129
bowersox CreditAttribution: bowersox commentedPlease 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.
Comment #131
sunI 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().
Comment #132
bowersox CreditAttribution: bowersox commentedHere'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.
Comment #133
Owen Barton CreditAttribution: Owen Barton commentedJust 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.
Comment #134
chx CreditAttribution: chx commentedAfter 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?
Comment #135
bowersox CreditAttribution: bowersox commentedPlease review this updated patch.
Here are the changes:
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.
Comment #136
bowersox CreditAttribution: bowersox commentedPlease 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.Comment #137
chx CreditAttribution: chx commentedWhat 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.
Comment #138
mgifford@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.
Comment #139
bowersox CreditAttribution: bowersox commented@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.
Comment #140
Owen Barton CreditAttribution: Owen Barton commentedI think this is what chx was getting at (see patch and interdiff) - we can use #title being unset as an implicit 'none', which would work the same way in terms of output - suppressing the label completely - but would simplify the API a bit for most use cases. I am not sure which one I prefer - it doesn't seem too critical either way.
Reviewed the updated comments and I think they all look good.
Comment #141
bowersox CreditAttribution: bowersox commented+1. I'm fine with this. Either way I think either recent patch accomplishes the basic goal. With this newest patch, it appears that contrib authors could still set #title_display='none' explicitly if they want to. They could also unset their #title (or never set a #title) which would do the same thing. Namely, it would display no title and no required marker.
Correct me if I've misunderstood.
The one thing some people might find confusing about this is that setting #title='' (empty string) has different behavior than leaving #title unset. I know we already have lots of confusion between !isset() and empty(). But that is something we can all talk about solving in D8 fapi.
Comment #142
Dries CreditAttribution: Dries commentedLet's ask chx to sign of on this, or to provide additional direction. :)
Comment #143
chx CreditAttribution: chx commentedI am OK with this patch . It does not hurt to have none if noone needs to use it :)
Comment #144
sunIf I skimmed previous comments correctly, this now only applies to certain #types. The inline comment should clarify this.
s/div/DIV/
We always write HTML tags uppercase.
Wrong indentation, see http://drupal.org/node/1354 for proper indentation of Doxygen lists.
This note still confuses more than it clarifies.
(and elsewhere) Trailing white-space here.
Grammar needs work here.
Can we move #title_display after #title, please?
Strange indentation, and trailing white-space.
Please move the comment above the condition.
I've read this twice, but still have troubles to understand it. Needs to be reworded.
huh? That comment needs work.
Ditto, please move the comment above (both) conditions.
Do we need the leading space here? If we do, then please add an inline comment that states why.
This review is powered by Dreditor.
Comment #145
Dries CreditAttribution: Dries commentedLet's do another reroll of this to incorporate sun's comments.
Comment #146
donquixote CreditAttribution: donquixote commentedAbout html processing (in theme_form_element()).
Instead of this
I much prefer this:
Makes it much easier to see how opening and closing tag belong together.
Even nicer with heredoc syntax.
I don't know if that's a documented coding standard anywhere, so it's up to you.
Comment #147
sunHTML processing in (all) those theme functions in form.inc is a different issue, not to be changed in here.
Comment #148
Owen Barton CreditAttribution: Owen Barton commentedThe doxygen for that function 2 lines above this seems quite clear that this is only called for checkboxes and radios, and the API docs in theme_form_element make it clear that 'attribute' only applies to checkboxes and radios. I am not sure if we could make this clearer without introducing duplication - ideas welcome.
Done.
Done.
Agreed - the individual descriptions of each value make it clear that the required marker is included, so this is redundant.
Removed.
Rewritten - see below.
Done.
Fixed.
Done.
Not sure how hot my grammar is, but here is what I came up with:
Cleaned this up. I also removed the redundant check from theme_form_required_marker() I mentioned in #133.
Done - I also combined the conditions and moved them to the top of the function as a more obvious "early return".
This is consistent enough through the original code that I am pretty sure it is needed - I am pretty certain it just adds a little whitespace between inline labels, such as with checkboxes. It might be preferable to do this with CSS (since that allows you to more easily remove the space if you want), but I think that is out of scope for this issue.
Comment #149
sunI'm 99% happy now, but will leave further decisions to core maintainers.
Comment #150
Dries CreditAttribution: Dries commentedI'm happy with it. It is an accessibility improvement. Committed to CVS HEAD! Thanks for all the work.
Comment #151
mgiffordI noticed two additional places of concern that weren't caught in this patch. I think they are two special cases that we didn't investigate enough.
I've got a demo here and running it through WebAIM highlights 4 errors that are caused by two problems.
1) The label & form element aren't matched in the file upload field so a screen reader has no way of tying the two together:
This is a pretty significant issue but not sure where responsibility lies for this now that the upload module has been removed from core. Is it just a cck type now?
Within the "More information about text formats" section however there are empty labels:
These are easy to remove so have added a patch.
Mike
Comment #153
mgiffordI'm adding to this issue as I assumed that it would have been picked up in the mass integration of issues that were being done in this massive patch.
Turns out we missed the ability to make labels invisible (so they don't disrupt visual flows) but are presented to screen readers. I hadn't had to test this before, but ran into this when working on a patch for:
http://drupal.org/node/448292#comment-2467302
This brings in the previous patch where I noticed that missing fields were being output. It still doesn't address the file upload field issue mind you.
This is a critical issue as we are still in the position where if you don't want to display the text of a form field as it is now the only option is not to have a label associated with it.
References can be found here:
http://www.w3.org/TR/2008/NOTE-WCAG20-TECHS-20081211/H44
http://www.usability.com.au/resources/wcag2/
Comment #154
Owen Barton CreditAttribution: Owen Barton commentedThis looks good - though my sense is that if this is critical it would be good to add a test for it, so we can ensure we don't have future regressions.
Comment #155
mgiffordAgreed that we should ideally have tests for all optional #title_display properties. Looking in Simple Test's form.test & the function testFormLabels() I think the documentation for how the options is presently being tested could probably be better.
I don't know how to add the verification of the invisible option. I'm assuming it would be something like:
In grepping for the code I didn't see any other refrences to #title_display in simpletest. I'm not sure if it's even possible to do testing for these options.
Mike
Comment #156
mgiffordThere seem to be some confusion about the optional #title_display properties and the additional invisible property that I have created a path for above. I do hope this clarifies the problem and outlines the option.
0) Default without title (label-notitle.png)
There is no title so no label will be displayed. This works fine.
1) Before or missing #title_display (label-before.png) - default displays label before the field
The default for any form with a title is to display it with it's associated label.
or
2) After (label-after.png) - label after the field
Being able to put the label after the form adds more flexibility for module developers and themers.
3) None/Attribute (label-none.png) - No label displayed
I am not sure what conditions none or attribute would be required. Previous discussions have focused on the password confirmation field. Attribute adds a tooltip & none doesn't but neither involve labels. Readme on attribute is here:
or
4) Invisible (label-invisible.png) - label invisible to all but screen reader
The proposed new option is that of invisible, which produces a label that has been hidden. This is visible only to screen readers. This option is required for Drupal forms to become WCAG 2.0 compliant. For screen readers users to be able to understand the function of form elements, having labels is critical. Links in previous posts provide more information about the importance of labels.
Comment #157
chx CreditAttribution: chx commentedAnd what's with CSS 2.1 attribute selectors? Like label[for=whatever] { visibility: hidden } . Yes IE6 does not support this and so what.
Comment #158
bowersox CreditAttribution: bowersox commented+1 for the adding 'invisible' form labeling in this clean-up patch. I think this gives a much improved tool for hiding form labels in a way that is still accessible.
Comment #159
bowersox CreditAttribution: bowersox commentedI have re-rolled with an automated test for #title_display=invisible. Otherwise the patch looks good to me. I reviewed and tested and found the following:
Comment #160
mgiffordWaiting for the bot to review this. Looks good to me, but looking for that green light.
Comment #161
bowersox CreditAttribution: bowersox commentedAnyone know why the bot has waited 3 days and still no automated test results? I've re-attached the same patch as #159 to this comment here.
Comment #162
Owen Barton CreditAttribution: Owen Barton commentedLooks like all the test clients are failing http://qa.drupal.org/pifr/status - hopefully they will be back soon!
Comment #163
mgiffordLooks like it's nearly fixed from the reports in IRC from moments ago.
Comment #164
mgiffordLike the the tests that have been added for this. I'm marking this as RTBC.
Comment #165
mgiffordOk, think we can simplify this a whole lot and get this patch through.
After talking with Everett it was clear that either a label or a title would work the same way for form elements for screen readers.
The attribute tag was supposed to provide a title attribute to the html so that form elements had that attribute, but it wasn't working.
This patch resolves that issue. However, I think the simple tests should be revisited for it and the description of it clarified so that we're all clear what attribute is supposed to do.
We shouldn't need to add the invisible tag, we just need to get attribute to work as "advertised".
Comment #167
Everett Zufelt CreditAttribution: Everett Zufelt commented@mgifford
I haven't tested your patch. But, if there is currently no way to tell FAPI to place the $element['#title'] as the title attribute of the form field in the generated markup then this does need to be solved. It was our original intent to ensure that there was a way for developers to include information about the title (label) for a form field without having to have a label generated in markup.
Thanks for working on this.
Comment #168
bowersox CreditAttribution: bowersox commented@mgifford,
Can you give a quick explanation of what was wrong with the past approach? I thought we had this done back in February ;)
Comment #169
mgiffordSo, this issue is linked to this patch (although I'll be needing to revise it based on this) http://drupal.org/node/448292#comment-2714302
But is using existing attribute tag rather than the proposed invisible one:
After providing the patch above, and below applying the patch that's attached to FAPI you get:
Notice that there is no label & no title provided. Now with the attached patch the declared title is displayed:
I'm not sure if this was intended or not, but this is a rather restrictive definition of the attribute tag. This may have been by design it's called in form_pre_render_conditional_form_element() but this is only used by radios & checboxes, but should it?
Looks like either we need to extend the attribute's definition & application, and approve this patch or go back to the invisible patch I proposed earlier.
Comment #170
bowersox CreditAttribution: bowersox commented@mgifford,
Regarding #title_display=>'attribute', see #131. @sun requested that we not add this in form_builder(), but instead in form_pre_render_conditional_form_element(). That imposed the limitation that #type->'attribute' only applies to radios & checkboxes, but that was a limitation for Drupal 7 and we all want to make this better in Drupal 8.
The prior clean-up patch (#161) was all about adding support for #title_display=>'invisible'. That was RTBC for the last 6+ weeks. It seems like that clean-up is probably still a good thing, and probably separate from the 'attribute' question you've raised.
Comment #171
sunbrandonojc is right here. However, I'm not sure whether I understand the previous patch, as it mixes 'invisible' into 'after', but afterwards doesn't process 'invisible' in the 'after' switch case. Either that was a bug, or the code needs more comments.
Comment #172
bowersox CreditAttribution: bowersox commented@sun
Regarding 'invisible' versus 'after', they are the same switch case but a few lines below in theme_form_element() we add the class to make it invisible:
Do you think we should add a comment up near the switch
case 'invisible'
? For example, we could add the comment// Class 'element-invisible' is applied below to show only to screen readers.
Let me know if that would help and I'm glad to re-roll #161. Thanks for your feedback and help.
Comment #173
sunTrailing white-space.
ok. The diff context is quite small here (adjusted?), so I was mistaken about the actually altered function here.
Can we move the inline comment above the "elseif" here? Same adjustment also for the preceding condition? Would help to understand the code flow.
Also note the trailing white-space introduced here.
I do not understand this change. If I'm not mistaken, then we need the label to have a element to select via JS/CSS to hide. If that needs to be removed for any reason, then we a) need a inline comment stating why, and b) adjustments in filter.js/filter.css, and possibly core themes.
Powered by Dreditor.
Comment #174
mgiffordIt's critical that there is some way to pass a title to a form element. I don't care if it's done with a label or a title element. One of them needs to get into D7 however.
The main advantage to using the title attribute as I did in #169 would be that it wouldn't really require a adding another option 'invisible' to the API. Adding (and then hiding) different label texts is also more complicated than just using the title tag.
However, I'm happy with the labels. Because it was sitting RTBC for a while I thought I'd kick the tires with another approach.
@brandon are you able to clean up the changes to patch in #161 as suggested by @sun?
@sun in trying to respond to this:
"I do not understand this change. If I'm not mistaken, then we need the label to have a element to select via JS/CSS to hide. If that needs to be removed for any reason, then we a) need a inline comment stating why, and b) adjustments in filter.js/filter.css, and possibly core themes."
With the label present, it will be only visible to screen readers. The css will hide any text off screen so it isn't visible to anyone else. The CSS class element-invisible will hide it away so that it appears to most of us like nothing has changed. It's related to http://drupal.org/node/448292#comment-2714302 - but this patch won't need JS & has nothing to do with selecting the items.
Comment #175
Everett Zufelt CreditAttribution: Everett Zufelt commented@brandonojc
I am not clear on the reason why title can only be used on checkboxes and radios in D7. Not arguing here, but can you point me to the comment where a clear explanation for the reason for this is stated?
Comment #176
bowersox CreditAttribution: bowersox commented@sun, can you give a quick description of why it is important to keep logic for #title_display out form_builder(), and why it is better in form_pre_render_conditional_form_element() (basically the reasoning for comment #131)?
@everett, I don't remember the reasoning but hopefully @sun can fill us in. I'm considering making form labeling and FAPI clean-up the issue I propose for the upcoming core developer summit so we can make this all better in D8.
@mgifford, yes, I'd be happy to re-roll #161 with @sun's good feedback.
Comment #177
sunform_builder() does not contain logic for particular element #types. Any attempt to add such logic won't fix.
Comment #178
mgiffordOk, so Brandon will re-roll the patch in #161 with @sun's changes and then we'll test that & return the label patch as RTBC.
I'm glad we had this deviation to explore using title, but glad we've got a solution that doesn't require it.
Comment #179
bowersox CreditAttribution: bowersox commentedPlease review and test this clean-up of #161. Maybe we can get this back to RTBC as discussed.
The one significant change in this patch is that I removed the change to filter.module. @sun was right that it would require changes to filter.js/filter.css. The javascript code on the text format dropdown on /node/add uses the label tag, so the change was breaking that dropdown functionality. Since this is a bigger deal I propose to open a new issue for it.
The patch attached includes the changes suggested by @sun in #173:
Comment #180
mgiffordI've compared form-label-cleanup-v3.patch with the previous form-label-cleanup-v2.patch (which was RTBC). Brandon brought over the improvements from @sun.
I applied this in an updated CVS version of Drupal and verified that the labels were produced as expected with the block code. For those who want to test this with the label solution see:
Hopefully this can now get into core quickly as it's had this additional review.
Comment #181
Everett Zufelt CreditAttribution: Everett Zufelt commentedI am still unclear as to why the #title property cannot be used as the title attribute on form fields other than radio and checkbox.
Comment #182
bowersox CreditAttribution: bowersox commented@Everett, here are a few links to relevant posts: #131, #132, and @sun's explanation in #177.
The #title_display property should ultimately support the 'attribute' setting for any form field type. Because of that, it seems like the logic really should go into form_builder() or the equivalent. This is something I'd like to talk with people about at DrupalCon and in planning for Form API improvements for Drupal 8.
Comment #183
sunLooks good.
Comment #184
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #185
chx CreditAttribution: chx commentedRoll this back. I said this before: and what's with CSS 2.1 attribute selectors? Like label[for=whatever] { visibility: hidden } . I am fighting this patch bloating the already bloated form.inc and now it came back to life like some zombie and got in more code when that was not necessary. Or explain what the latest zombie can do which my simple CSS selector can't?
Comment #186
Dries CreditAttribution: Dries commentedchx, good question. I overlooked your small comment in the sea of follow-ups. I guess it all depends on whether we care about IE6 or not. At the very least, we should add a TODO to remove this code once we drop support for IE6 (if not today).
Comment #187
mgifford@chx, I've certainly tried to see how it might be possible to use CSS 2.1 attribute selectors to hide the labels. I know we talked about this ages ago on IRC and you suggested it then. I'm not a CSS guy, but really don't see how it would be possible to do what you're asking from some simple CSS changes. There are many places where it is important to add labels to forms but to hide them from the browser. The Drag/Drop patch is just one. I appreciate you wanting to reduce the size of the form.inc file. I certainly hope that we were able to streamline it a bit, although perhaps not.
I'm making my comments below from http://www.456bereastreet.com/archive/200510/css_21_selectors_part_2/
I don't think it's going to work if we use something like label[for=edit-user-new-weight] we're going to have hundreds of unique ones in CSS and no way to deal with blocks that have been added by the user. It would also require having to hardcode some CSS somewhere rather than allowing a FAPI based solution to produce invisible labels.
It's possible that we could use label[for|=edit-user] to pull up , or Weight for Recent content but that's pretty dangerous as I'm guessing that forms with ID's "edit-node" are going to be very common. Now we could re-write how the ID's are created to reduce the risk, so that it's in the form "edit-node-weight-recent", but that's going to cause no end to troubles.
I don't think that practically we can expect to be able to address this with CSS (even CSS3). However, I'm totally willing to be proved wrong here. However I need to see some solid examples with existing problems (like the Drag/Drop pages). Using title as I suggested in #169 would have some advantages. However, I don't see a simple CSS solution will work here.
Comment #188
sunYou'll notice that you used a dynamic CSS selector here. Turn that example snippet into a CSS selector that applies globally and you end up in the patch that has been committed.
Unless there is a more elaborative statement of what the actual problem with the committed patch is, back to fixed.
Comment #190
yched CreditAttribution: yched commentedTurns out the original patch (#148) broke Field API's display of multiple, required fields :
Patch in #980144: Issues with "required, multiple" fields in forms