Posted by mgifford on August 13, 2010 at 8:39pm
11 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | filter.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | a11ySprint, accessibility, html5 |
Issue Summary
In reviewing this page as a logged in user:
http://drupal7.dev.openconcept.ca/node/2
I get labels used as text with no associated form:
<label>Filtered HTML:</label>This is a problem for screen readers and is just bad use of HTML. Seems like a simple enough patch.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| screen-capture-58.png | 164.68 KB | Ignored: Check issue status. | None | None |
| filter_label_use_v1.patch | 952 bytes | Idle | PASSED: [[SimpleTest]]: [MySQL] 22,848 pass(es). | View details |
Comments
#1
This isn't really a problem for screen-raeder users, as far as I am aware. But, you are correct, it is poor markup.
#2
good to know it doesn't bug screen readers. WebAIM's WAVE didn't like it and ya it should be fixed.
#3
Can someone fix the issue title? No idea what it tries to say.
+++ modules/filter/filter.module 13 Aug 2010 20:39:01 -0000@@ -1075,7 +1075,7 @@ function theme_filter_tips_more_info() {
+ $name = isset($format->name) ? '<h3>' . check_plain($format->name) . ':</h3>' : '';
What about the trailing semicolon? Sucks, no?
Powered by Dreditor.
#4
Thanks @sun - friggin trailing colon. Sorry. The title should be clearer too I hope.
#5
+++ modules/filter/filter.module 13 Aug 2010 20:39:01 -0000@@ -1075,7 +1075,7 @@ function theme_filter_tips_more_info() {
- $name = isset($format->name) ? '<label>' . check_plain($format->name) . ':</label>' : '';
+ $name = isset($format->name) ? '<h3>' . check_plain($format->name) . '</h3>' : '';
...
return '<div' . drupal_attributes($attributes) . '>' . $name . theme('filter_tips', array('tips' => _filter_tips($format->format, FALSE))) . '</div>';
Sorry, I just realized that this entire surrounding logic is needless... so while being here:
- $format->name is required, i.e., always non-empty.
- We can therefore skip the $name variable assignment and move that heading directly into the returned markup.
- That said, themers would highly welcome it if we'd make the generated markup more understandable:
$output = '<div ...';$output .= '<h3>' ...;
$output .= theme('filter_tips'...;
$output .= '</div>';
return $output;
Powered by Dreditor.
#6
Good suggestion. That's much more straight forward.
#7
Awesome!
+++ modules/filter/filter.module 16 Aug 2010 18:06:54 -0000@@ -1074,11 +1074,13 @@ function theme_filter_tips_more_info() {
+ $output .= '<h3>' . check_plain($format->name) . '</h3>'; ¶
...except for the trailing white-space ;)
Powered by Dreditor.
#8
S#!t, like where are all of these spaces coming from anyways.. cut & paste errors suck..
Thanks sun.
#9
@Everett, via twitter @webaxe said "Besides poor code, orphaned labels may mislead user; expected functionality broken such as clicking to activate a form field", although he followed up by saying "It ain't a show stopper."
So would be good to fix it, but not critical.
#10
Something odd happened to the patch in #8... very odd. Saw the proper file contents yesterday.
Anyway, there was still trailing white-space contained, and I hoped you'd recognize it yourself...
#11
Ok, I visually reviewed this patch in another editor. Not sure how that space was missed using vi......
#12
Thank you! :)
#13
Committed to CVS HEAD. Thanks.
#14
Automatically closed -- issue fixed for 2 weeks with no activity.
#15
This turned into a regression, #890212: text format help text uses wrong headings
LABELs do not follow and cannot break any hierarchical document structure. H3 does.
As mentioned in #884846: Orphaned Labels Being Generated for Radios, Checkboxes & Items, we should be using a non-hierarchical element, such as DIV.form-label or similar.
#16
For accessibility headings help to break of the navigation of a site & allow screen readers to have an alternate way to skip through the content looking for chunks of relevant text.
Generally navigation is done either by reading out the text of the site chronologically or either skipping through the links or through the headings. Although DIVs won't break the hierarchical document structure they don't add anything either.
It's better to use a DIV than a LABEL in this instance, as a LABEL is just bad HTML, however headings provide a structure that can be navigated. I think that these two 456 Berea Street articles Heading & Document Structure & Headings & Heading Hierarchy sum it up pretty nicely.
There isn't right now a nice way to deal with this. Hopefully this will change with HTML5.
#17
Thanks for the additional insight. Since we can't use HTML5's section yet, we need to change it into a DIV, since headings receive different styling depending on their level. All of those semi-labels that may appear in forms or with or without an actual form element should visually look like a form element label, but not like a heading of an arbitrary level.
In this case, it seems like the semi-label is sometimes appearing in forms and sometimes not. I guess it would make sense if we'd change the other patch to use DIV.label instead of DIV.form-label then, and also use DIV.label here.
Should be a job of a minute.
#18
If it was just this patch it would be quick. However there are a few places where I know this has been done. Not sure how to best find them though.
There's is a great deal of disagreement about how to use heading tags in HTML4. This doesn't need to wait till HTML5 or for that matter adoption of WAI-ARIA.
I'll see if I can get some second opinions about this.
#19
Ok, I'm going to concede on the on heading issue and run with the div after discussing this this with @BrandonOJC.
We're going to also need to re-roll to be either a .label or a div.label:
.form-label {display: block;
font-weight: bold;
}
from http://drupal.org/node/884846#comment-3347986
Hope this helps move this issue forward.
EDIT: Ultimately I suppose it comes down to what users expect. Headings within forms isn't standard even if a group of form elements is essentially the same as a paragraph or two.
#20
@sun can we get this RTBC'd so that we can then build against it when modifying #884846: Orphaned Labels Being Generated for Radios, Checkboxes & Items to use:
<div class="label"></div>#21
As mentioned over there, the first patch that introduces DIV.label also has to introduce the corresponding stylesheet changes, which are currently contained in the patch in http://drupal.org/node/884846#comment-3347986, but are still using .form-label, not .label.
#22
sub
#23
Ok, I've re-rolled that patch & submitted it.
#24
#19: filter_label_use_v6.patch queued for re-testing.
#25
Is this the CSS change we need to make?:
-.form-item label {+.form-item label, div.label {
display: block;
font-weight: bold;
}
Or can we get rid of
.form-item labeland just make it onlydiv.label?This corresponding change would be made in system.css, bartik/css/style.css, and seven/style.css.
#26
Hey Brandon, It's in this patch here http://drupal.org/node/884846#comment-3499784
But yes, I don't think we can get rid of the .form-item label as we want the labels to be defined to look the same. The patch I rolled didn't include a change to the system.css
mike@dev:/home/dm7$ grep -ir form-item modules/system/* | grep label
modules/system/system.admin.css:.exposed-filters .form-item label {
modules/system/system.admin-rtl.css:.exposed-filters .form-item label {
This is the relevant code:
.exposed-filters .form-item label {float: left; /* LTR */
font-weight: normal;
width: 10em;
}
It looks like
div.labelshould be applied there too.#27
#19: filter_label_use_v6.patch queued for re-testing.
#28
Bumping to D8. This is important to resolve, since it is poor practice to use label in this way, but it has no real impact on users, only on testing tools, so it can wait for D8 to be resolved.
#29
Ok, so how do we get these in for consideration in D8. They are pretty trivial accessibility enhancements.
#30
This isn't an accessibility issue as such, but it does cause confusion as it isn't actually a label.
Would be a lost opportunity too to remove this as we're going through & verifying HTML5 coding standards.
A grep for '' in D8 just produced:
core/modules/field/field.form.inc: 'data' => '<label>' . t('!title: !required', array('!title' => $element['#title'], '!required' => $required)) . "</label>",core/modules/field/modules/text/text.js: $fullLabel = $('<label></label>').prependTo($full);
core/modules/locale/locale.admin.inc: $title = '<label>' . $form[$type]['#title'] . '</label>';
The instance has since been removed from:
/core/modules/filter/filter.module
#31
Patch is from CVS days.
#32
Good point.. Hopefully this moves it ahead a bit.
#33
Oh ya, I also hit up the following files.
filter/filter.module field/field.module locale/locale.admin.inc system/system.admin.css
#34
Needs before/after screenshot and UX review.
#35
Waiting for screens.
#36
#32: filter_label_use_v7.patch queued for re-testing.
#37
The last submitted patch, filter_label_use_v7.patch, failed testing.
#38
Patch didn't apply when I tried it...
error: core/modules/locale/locale.admin.inc: No such file or directory#39
Sorry, status field was playing games with me :-)
#40
Okay, I re-rolled patch. It turns out that a some of locale.module is now in language.module...
Language negotiation system now part of language module
#41
Screenshots before and after the patch in comment #40.
I haven't done a thorough test across browsers yet, but the new DIV is less prominent than the LABEL in my Firefox 13 test.
#42
I hope I find this issue before.
Here is same problem:
Point #5
http://drupal.org/node/1452188#comment-6115332
#43
I had forgotten why we had removed
<h3>until stumbling down to comment #15.Any suggestions on dealing with the DIV? Should we just make it larger in the CSS?
#44
Nothing to review, other than a markup bug?
#45
#40: filter_label_use_v8-882666-40.patch queued for re-testing.
#46
The last submitted patch, filter_label_use_v8-882666-40.patch, failed testing.
#47
@Bojhan, in the screenshots in #41 div.label looks less bold than the
<label>tag. I can't see why that is, but think that's why it needed a usability review.What was the markup bug? Would like to get this marked RTBC.
#48
Well I imagine we don't want to change anything visually, heh :D that's the markup bug.
#49
Is it standardize all help texts headings to div.class=label ?? why not use h2~h6 tag or dfn
+++ b/core/modules/language/language.admin.incundefined@@ -567,7 +567,7 @@ function theme_language_negotiation_configure_form($variables) {
+ $title = '<div class="label">' . $form[$type]['#title'] . '</div>';
#50
#51
@droplet see @sun's comment here - http://drupal.org/node/882666#comment-3402358
#52
It's worth also noting that this doesn't fix the same problem with radios & checkboxes.
This is just on the page creating new users.
Edit: I don't think #884846: Orphaned Labels Being Generated for Radios, Checkboxes & Items has a solution that we can work with easily to get rid of these headings.
#53
#51: filter_label_use_v8-882666-51b.patch queued for re-testing.
#54
This is a simple fix.
#55
@droplet it might be more semantic to change this to a heading, but essentially this issue was opened because it was just a markup bug for an inappropriate use of a label element.
I'm fine with someone else verifying that indeed it is appropriate for it to be a heading & making it so, but I'd really like that to either be another issue or to to have someone else champion it.
This addresses the original concern I raised in 2010 and I'm marking it RTBC as it's really a trivial markup change that shouldn't take 2 years to solve.
#56
Note that I'm currently working on #1829202: Make #type 'item' work outside of a form context to render a compound label + content
Essentially, all of the cases in this patch try to output a container that is compound of a label + inner content, but they are not necessarily in a form context, or there is no corresponding input element for the label (which really means it is just a heading).
#57
I'm going to drop this down to needs work until I understand how your other patch is going to interact here.
In your patch does it become just a heading or is it a label that is styled to look like a heading?
#58
Yeah, #1829202: Make #type 'item' work outside of a form context to render a compound label + content, #type 'item' essentially becomes just a heading (H4) + arbitrary content markup.
However, I just checked this patch here once again, and it does not really overlap with that issue. It would merely make sense to harmonize the two in terms of markup + CSS.
Attached patch does so. Tested manually, and still works as expected. So I'd consider this back as RTBC.
#59
Here's a screenshot from FF in Ubuntu.
Thanks @sun.
#60
Looks fine. Committed/pushed to 8.x, thanks!
#61
That's great, thanks!
#62
Automatically closed -- issue fixed for 2 weeks with no activity.