Posted by mgifford on August 13, 2010 at 9:00pm
6 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | block.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | accessibility |
Issue Summary
So if you review the html on this page admin/structure/block
It's missing a label. It's easy to add in. Might need to figure out how to best describe it but it means the page passes the WAVE evaluator.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| block_admin_region_label_v1.patch | 660 bytes | Idle | PASSED: [[SimpleTest]]: [MySQL] 22,850 pass(es). | View details |
Comments
#1
I don't think we need label for each block. It looks like we didn't have this functionality for Drupal 6 and we don't need to add it for Drupal 7. mgifford, why do you think this is a bug?
#2
@furamag
All form input controls need an accessible name. This can b either a label programmatically associated with the for attribute, or the title attribute of the form input.
This is required to conform with WCAG 2.0, but more importantly, to improve the user experience for users of assistive technology, who might otherwise not have a clear understanding of the purpose of the form control.
#3
OK. In this case we should recreate patch. Now we are using $form['blocks'][$key]['region'] instead of $form[$key]['region'] . So we can use following code for main part of new patch:
$form['blocks'][$key]['region'] = array('#type' => 'select',
+ '#title' => t('Region for block %block.', array('%block' => $block['module'])),
'#default_value' => $block['region'],
'#options' => $block_regions,
);
#4
#5
Ok, this is just a re-roll to keep up with core as per @furamag's suggestion above. Thanks!
#6
All system blocks get the label "Region for block system."
This should be hidden - it complicates the UI and should use .element-invisible
#7
Thanks Jeff. I've rerolled it and this should resolve these issues.
#8
Thanks Jeff. I've rerolled it and this should resolve these issues.
#9
Great patch.
This reads:
Region for block User login.
Region for block Footer.
I would prefer either:
1. Region for block: User login.
or
2. Region for User login block.
Option 2 seems like a better use of natural language to me, but really this is just a tweak, otherwise RTBC.
#10
Thanks Everett. That reads better. And such a simple re-roll too. Hopefully it's enough to get it marked RTBC.
#11
Looks good. Setting to RTBC.
#12
There is some inconsistency here. Just above this code we use:
<?php'#title_display' => 'invisible',
'#title' => t('Weight for @row', array('@row' => $block['info'])),
?>
In other words, we use @ instead of %, 'row' instead of 'block', and we omit the word 'block' at the end of the sentence. I suggest we clean this up a bit more so it is nice and consistent?
#13
@Dries
Agreed, thanks for pointing this out. If @mgifford doesn't reroll today I will get it done tonight.
#14
Thanks for bringing this up Dries.
Feels like we should probably find some way to put this in a coding style guide as it keeps coming up. Maybe it's already there.
I've re-rolled the patch to make the weight & region labels be consistent. They will now both end with the word 'block' and does not include a '.' at the end as it occurred to me that this too was different. So the text is formatted like 'Weight for %block block' or 'Region for %block block' with %block being populated by $block['info'].
I was looking at other instances across the board for how this is done. In the example of modules/menu/menu.admin.inc, it's populated by:
'#title' => t('Weight for @row', array('@row' => $item['title'])),I'm sure with a bit more grepping around we could pull up others that should probably be changed. Is it good enough it is consistent within the admin file?
It wouldn't be a huge change to move the rest around, but it would take a bit of time to track down/organize.
#15
@mgifford
Looks good. I agree that it would take a great deal of work to track all of these instances down and to make them consistent across the entire project. I would think that consistency within this context would suffice.
Can you please explain your choice of %block instead of @block? Block, or !block? I've never been clear on the usage between % and @, and am curious if ! might be proper here (since the block title is (likely) a sanitized string.
#16
@Everett - for information about the secure placeholders see the documentation on t() http://api.drupal.org/api/function/t
#17
Looks like we should be using !block or @block, since we do not want the string wrapped in em elements.
!block if we can trust the value to be sanitized, @block if we cannot.
More research on the placeholder.
From http://api.drupal.org/api/function/t/7
• !variable: inserted as is
• @variable: escape plain text to HTML (using check_plain())
• %variable: escape text and theme as a placeholder for user-submitted content (using check_plain () + theme_placeholder()
From http://api.drupal.org/api/function/drupal_placeholder/7
drupal_placeholder($text) returns $text wrapped in em elements
#18
Frankly, I was emulating what others had done in similar functions & hadn't evaluated the other options fully.
I'd agree that EM elements aren't particularly useful, particularly within an invisible LABEL. I don't think it's wrong to highlight the block name and make it stand out if we could see it. Although in this instance it doesn't clarify anything.
I'd run with having the HTML characters escaped:
"@variable, which indicates that the text should be run through check_plain, to escape HTML characters. Use this for any output that's displayed within a Drupal page."
However, I think we can make this another issue and run it through for consistency with other places where invisible LABELs have been used with dynamic content like this.
#19
When I apply the patch in #14 the labels are coming after the select list in the source order - that can't be right is it? Is the patch going to be rerolled to use the @ palceholder?
In the screenshot I removed the element-invisible class to visually show the label coming after the select list.
#20
Re-rolled using @block instead of %block for placeholder. I didn't test where the invisible label is being generated in the source. If indeed it is being generated in an undesirable order a separate bug should be filed against FAPI, as we are relying on FAPI to produce the invisible label.
#21
bot
#22
Looks ready to RTBC when the bot gets to it. I applied it to my sandbox and it worked well. As far as the order it's from theme_form_element():
case 'invisible':case 'after':
$output .= ' ' . $prefix . $element['#children'] . $suffix;
$output .= ' ' . theme('form_element_label', $variables) . "\n";
break;
This (the case 'invisible') could be move easily up a few lines if there is a benefit.
Mike
#23
Setting back to Needs Review (cross-post).
#24
I hadn't intended to move it to needs work, but do intend to mark it RTBC now that the bot likes it.
#25
Thanks for the clean-up. It looks nice now. Committed to CVS HEAD. :)
#26
Automatically closed -- issue fixed for 2 weeks with no activity.