Posted by samuelsov on July 2, 2010 at 2:37pm
14 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | views.module |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | a11ySprint, accessibility, Needs tests, VDC |
Issue Summary
For accessibilty, it can be useful to add a summary attributes in html table.
Example from http://www.w3.org/TR/html401/struct/tables.html#adef-summary
<TABLE summary="This table charts the number of cups
of coffee consumed by each senator, the type
of coffee (decaf or regular), and whether
taken with sugar.">
<CAPTION>Cups of coffee consumed by each senator</CAPTION>
<TR> ...A header row...
<TR> ...First row of data...
<TR> ...Second row of data...
...the rest of the table...
</TABLE>
Comments
#1
Here is a patch that i think solve this problem. It should be easy to do the same for views 6.x-3.x
Please review and comment.
Thanks !
#2
Oups, here is a good version...
#3
+++ theme/views-view-table.tpl.php 2 Jul 2010 14:42:36 -0000@@ -15,7 +15,7 @@
+<table class="<?php print $class; ?>" summary="<?php print $options['summary']; ?>">
We should check plain this value.
#4
tagging
#5
+1 for the concept, haven't reviewed the patch
Not sure about inline help with views, but if it is available it would be useful to provide an explanation about what a summary attribute is used for.
#6
I hope this helps to understand summary:
A quote form http://www.w3.org/TR/html401/struct/tables.html
Each table may have an associated caption (see the CAPTION element) that provides a short description of the table's purpose. A longer description may also be provided (via the summary attribute) for the benefit of people using speech or Braille-based user agents.#7
@dereine
I don't think that description is sufficiently clear to let developers know when to use a summary and not. I also doesn't think that it is very clear about what type of content to include in the summary.
#8
I think Everett means we should provide this explanation in the #description of the summary attribute:
+ '#title' => 'Summary attribute in the html table for accessibility',So a shorter #title and a longer #description.
+<table class="<?php print $class; ?>" <?php if (!empty($summary)) : ?>summary="<?php print $summary ?>"<?php endif; ?>>That's a lot of open and close PHP tags on one line. How about this instead:
+<table class="<?php print $class; ?>" <?php if (!empty($summary)) { print 'summary="' . $summary . '"' } ?>>#9
#10
I'd agree with that modification. Thanks for the summary & code.
There is still some confusion on when to use caption & summary and what the difference is. Is it worth while putting in a link to a page to help explain what the summary attribute is supposed to contain in the description?
#11
Can you provide a good link/explaination?
#12
A good link for more info is http://webaim.org/techniques/tables/data#caption
Captions are essentially titles. Summaries provide brief summaries of complex data. Summaries aren't always required, but can be helpful in providing a snapshot.
Another useful link http://jimthatcher.com/webcourse9.htm
#13
Setting to minor. Would it be nice for developers to be able to add a summary to a table, yes. Is it rare that this would be useful, yes. Are there other parts of Views that need more accessibility love, yes.
#14
Everett, please open up bugs you find with Views. It's not core, but it's so very close to core for most of us.
Views also has D6 as being the dev version & then things are brought forward to D7. I don't know when this will change.
I'm setting this back to Normal though. Whether or not people using screen readers bother with summary tables is important, but it's not the only factor.
Not being able to effectively set summary tables generated in Views could be used as a reason why Drupal isn't accessible enough. Especially for the output of the page, it is critical for adoption by many government agencies that it can be added. See Canada's CLF:
http://www.tbs-sct.gc.ca/clf2-nsi2/index-eng.asp
#15
This patch didn't apply since #868972: summary attribute for tables went in.
I re-rolled it with an attempt to improve the titles/descriptions for both summaries and captions.
Caption element:
Title: Short description of table
Description: This value will be displayed as the caption for the table. Set this for better accessiblity of all tables.
Summary element:
Title: Table summary:
Description: This value will be displayed as table-summary attribute in the html. Use this to give a summary of complex tables.
I think this gives better guidance for site builders on when to use each one. I put the Caption input box above the Summary box since it seems Caption should be used more often.
I added this to both table and grid plugins/tpl.phps.
#16
Applies to latest 6.x-2.x, btw.
#17
Sorry for yet another comment, but I realized the focus has changed here.
#18
Just a quick review of the text/labeling
Ideally we'd be able to provide example summary and caption, maybe a link to one of the established sites, but it's not crucial.
#19
This will be very exciting to have this in Views! @greggles thanks for the patch!
#20
I think that's somewhat incorrect. The default HTML styles show the caption tag contents to sighted users.
#21
@greggles, oops, you're right.
How about "Include a caption to provide an overview of the table contents."
#22
Rather, "Include a caption for better accessibility of your table."
#23
Re-rolled for lisa's suggested text.
#24
Sadly this patch doesn't apply to 6.x-3.x anymore
Some rerole would be definitive helpful here.
#25
Re-rolling this patch for Drupal 7.
#26
The previous patch didn't work because the option wasn't being set in the preprocess functions. Try this one instead.
#27
#28
Uploading the same file again so that the test bot uses the correct branch this time. (I can't find another way to do this.)
#29
The last submitted patch, add_captions_to_views_tables_and_grids-843708-28.patch, failed testing.
#30
#31
Sorry, the previous patches were adding it to the table attributes instead of making it a separate tag.
#32
Here's a screenshot & example of the current output:
<table class="views-table cols-5" summary="This isn't a complex table"><caption>This is a list of facilities</caption>
Now that's just fine for HTML4 code, but we're going to have to adjust this for HTML5 in D8. That being said, this is a good improvement that will fill a useful need.
I'll put in some examples for recent table best practices (all HTML5) and the depreciated summary attribute will not be a problem and the added caption will be a big improvement in the accessibility of Views.
http://www.developerfusion.com/article/136530/making-tables-more-accessi...
https://developer.mozilla.org/en-US/docs/HTML/Element/table
http://www.w3.org/html/wg/wiki/SummaryForTABLE
http://www.pdprogrammeur.com/tables-and-html5-table/
#33
+++ b/theme/theme.incundefined
@@ -649,8 +649,14 @@ function template_preprocess_views_view_table(&$vars) {
+ if (!empty($handler->options['caption'])) {
+ $vars['caption'] = filter_xss_admin($handler->options['caption']);
}
+++ b/theme/views-view-table.tpl.phpundefined
@@ -19,8 +20,8 @@
+ <?php if (!empty($title) || !empty($caption)) : ?>
+ <caption><?php print $caption . $title; ?></caption>
Just in case title is not empty and caption is empty, this would probably lead to a notice of an undefined variable, which is indeed a regression because it will happen for all people
#34
Good catch. This patch just returns a null result.
+ // Add the caption to the list if set.+ if (!empty($handler->options['caption'])) {
+ $vars['caption'] = filter_xss_admin($handler->options['caption']);
+ } else {
+ $vars['caption'] = '';
}
#35
+++ b/theme/theme.incundefined@@ -762,8 +770,15 @@ function template_preprocess_views_view_grid(&$vars) {
+ if (!empty($handler->options['caption'])) {
+ $vars['caption'] = filter_xss_admin($handler->options['caption']);
There are two places using that code, sorry.
#36
Thank you for pointing that out. No need to apologize, you're making this patch better. I also realized that I didn't use the proper formatting for the else statements I was adding. Cleaned that up too.
#37
Committed the patch finally to d7, let's first discuss how a proper solution would look like for d8.
Wow this one is hard to decide who gets commit credit, but i choosed mgifford, especially because you made the final push and the port.
Fixed some whitespace errors before the commit.
#38
According to the first link you would probably need both summary and details for the caption. Do you agree with that...
These two should be hidden for the normal user, does core already provide some css for that?
#39
Thanks @dawehner - was definitely a community effort & I'll get @colan a beer next time I see him. :)
Definitely own @greggles a beer for tonnes of other stuff, but I'm less likely to run into him around town.
#40
#41
#36: add_captions_to_views_tables_and_grids-843708-36.patch queued for re-testing.
#42
tagging.
#43
The last submitted patch, add_captions_to_views_tables_and_grids-843708-36.patch, failed testing.
#44
Re-rolling here.
#45
Talking to Bojhan he suggested the "patch probably needs a style in Bartik as followup" and I expect he's right.
We definitely need a screenshot or two. Very likely styling is going to be required.
#46
updating patch.
#47
The last submitted patch, add_captions_to_views_tables_and_grids-843708-46.patch, failed testing.
#48
Annoying.. Missed a } in copy/paste.
#49
The last submitted patch, add_captions_to_views_tables_and_grids-843708-48.patch, failed testing.
#50
Arg, rushing around too many issues today. Missed another } for the class.
#51
The last submitted patch, add_captions_to_views_tables_and_grids-843708-50.patch, failed testing.
#52
Found some other issues. It's still not showing up though.
#53
--- a/core/modules/views/lib/Drupal/views/Plugin/views/style/Grid.php+++ b/core/modules/views/lib/Drupal/views/Plugin/views/style/Grid.phpundefined
Does it really makes sense to provide captions for grids, because if it does you might want to add those to all of the style plugins?
+++ b/core/modules/views/views.theme.incundefined@@ -773,13 +773,24 @@ function template_preprocess_views_view_grid(&$vars) {
+ } ¶
Wow, a whitespace, didn't had that for a long time
+++ b/core/modules/views/views.theme.incundefined@@ -773,13 +773,24 @@ function template_preprocess_views_view_grid(&$vars) {
+ if (isset($vars['header']) && count($vars['header']) && $responsive) {
Why should be the header not set?
#54
On captions for grids, all depends on #1903746: Replace the views grid table template with one using divs I think. If it's remains a table, we should be able to add caption & summary elements through Views.
I wish I could say I added that whitespace for nostalgia purposes, but think I'm just tired.
I was getting PHP Notice errors, so added the check. The Caption/Summary aren't being spit out properly, but perhaps when they are this will not be needed.
#55
I still don't have the summary element working, but think that was busted in bringing it over to Core.
Not sure how this:
$vars['attributes_array'] = array('summary' => filter_xss_admin($handler->options['summary']));Is supposed to become available to:
<table <?php print $attributes; ?>>#56
#55: add_captions_to_views_tables_and_grids-843708-55.patch queued for re-testing.
#57
This has been marked RTBC several times since it was open in 2010. I'm marking it RTBC Feb 18 as I do think it is an important feature for some types of views tables (which presently includes grids).
#58
Sorry, Summary's not being displayed. I'm hoping this can be considered a task though rather than a feature.
#59
The only thing I'm not sure about, is it common to show the actual caption.
Are we sure, that it makes sense to have a caption for grids as well? They will be probably converted to a div based approach, but then it seems to be not needed anymore?
<?php if (isset($caption) && !empty($caption)) : ?>Wouldn't !empty($caption) be enough?
#60
I was receiving notice errors, which is why I added that in. Would be better to fix it elsewhere though.
The summary didn't work in Views (already in Views) because it was formatted with:
if (!empty($handler->options['summary'])) {$vars['attributes_array'] = array('summary' => $handler->options['summary']);
}
Rather than:
if (!empty($handler->options['summary'])) {$vars['attributes']['summary'] = $handler->options['summary'];
}
With this change we should be good. @dawehner I nixed the isset() as requested as I can't see how that would show up at this point. Should be a null value or a caption.
#61
--- a/core/modules/views/lib/Drupal/views/Plugin/views/style/Grid.php+++ b/core/modules/views/lib/Drupal/views/Plugin/views/style/Grid.phpundefined
Do you have an oppinion about grids?
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/style/Table.phpundefined
@@ -220,12 +221,20 @@ public function buildOptionsForm(&$form, &$form_state) {
+ $form['caption'] = array(
+ '#type' => 'textfield',
+ '#title' => t('Title of the table'),
+ '#description' => t('A title which is semantically associated to your table for increased accessibility.'),
+ '#default_value' => $this->options['caption'],
+ '#maxlength' => 255,
Just wondering whether we really want to have first the summary and then the caption in the UI?
+++ b/core/modules/views/templates/views-view-table.tpl.phpundefined@@ -20,8 +21,8 @@
+ <?php if (!empty($title) || (isset($caption) && !empty($caption))) : ?>
Yeah no reason for isset and !empty as well.
+++ b/core/modules/views/templates/views-view-table.tpl.phpundefined@@ -20,8 +21,8 @@
+ <caption><?php print (!empty($caption)) ? $caption : $title; ?></caption>
We could simplify that quite a bit by splitting the if for title and if for caption. In general I feel like there should be the title in h3 and the caption, but i'm not sure.
+++ b/core/modules/views/views.theme.incundefined@@ -773,13 +777,19 @@ function template_preprocess_views_view_grid(&$vars) {
+ $vars['attributes_array'] = array('summary' => filter_xss_admin($handler->options['summary']));
we should maybe open a follow up to add test coverage for that.
#62
1) Grids - I like the idea of CSS only grids. Would be much more mobile friendly. However, with caption/summary there's no accessibility problem I know of with exposing grids in a table.
2) I'm not sure it matters whether the caption or summary is first. I suppose the caption is more like a heading so might make sense to go first. I've swapped it.
3) Removed isset()
4) If captions are selected but not filled in, then the title can be used in it's place. I think that's a useful default.
5) Maybe, but this seems to be a basic bug that can be fixed here. I'd like to have tests for both caption & settings and maybe put that in a follow-up issue. It just wasn't possible to use a summary element.
#63
forgot to attach it.
#64
The last submitted patch, add_captions_to_views_tables_and_grids-843708-62.patch, failed testing.
#65
#63: add_captions_to_views_tables_and_grids-843708-62.patch queued for re-testing.
#66
Just trying a re-roll as this didn't make sense to me.
exception 'Symfony\Component\DependencyInjection\Exception\InvalidArgumentException' with message 'The service definition "module_handler" does not exist.' in /var/lib/drupaltestbot/sites/default/files/checkout/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php:799Stack trace:
#0 /var/lib/drupaltestbot/sites/default/files/checkout/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php(423): Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition('module_handler')
#1 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/bootstrap.inc(2481): Symfony\Component\DependencyInjection\ContainerBuilder->get('module_handler')
#2 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/bootstrap.inc(1783): module_implements('watchdog')
#3 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/file.inc(972): watchdog('file', 'The file %path ...', Array, 5)
#4 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/file.inc(1023): file_unmanaged_delete('sites/default/f...')
#5 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php(1016): file_unmanaged_delete_recursive('sites/default/f...', Array)
#6 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/user/lib/Drupal/user/Tests/TempStoreDatabaseTest.php(78): Drupal\simpletest\TestBase->tearDown()
#7 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php(732): Drupal\user\Tests\TempStoreDatabaseTest->tearDown()
#8 /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh(404): Drupal\simpletest\TestBase->run()
#9 /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh(22): simpletest_script_run_one_test('593', 'Drupal\user\Tes...')
#10 {main}PHP Fatal error: Exception thrown without a stack frame in Unknown on line 0
#67
Works with tables in views, however not with grids. Summary attribute does not show up when display is set to grid.
#68
This is re-rolled with the grids and should be working. It was broken previously with the summary in the table too. I fixed it there, but didn't check to see if it was also broken with the Grid.
#69
#68: add_captions_to_views_tables_and_grids-843708-68.patch queued for re-testing.
#70
#68: add_captions_to_views_tables_and_grids-843708-68.patch queued for re-testing.
#71
Interdiff between my two patches. Should include changes for grid as specified.
#72
Patch works great. However I cannot mark it RTBC. This is my first code review, I hope I did it properly. :-)
In the table summary description:
maybe "table attribute" should be "table summary" or "table summary attribute". From a usability point of view I would prefer the first option.
<?php+++ b/core/modules/views/templates/views-view-grid.tpl.php
- <tr <?php print $row_classes[$row_number];
?>
+
why do we need this?
<?php+ // Add the summary to the list if set.
?>
I am not sure about this, but maybe we don't need this comment (wherever it is used).
If the table summary has a test somewhere in Views, this patch misses also test coverage.
#73
Restoring proper issue values. Not sure what happened, but I didn't change them, except for status :-)
#74
I changed the text to "table summary"
I changed this because I must have hit a PHP notice and decided to fix it. I can remove it from this patch for clarity.
- <tr <?php print $row_classes[$row_number]; ?>>+ <tr <?php print (isset($row_classes[$row_number])) ? $row_classes[$row_number] : ''; ?>>
The comment just follows in the example of the caption below it so I think it's fine.
options['caption'])) ? filter_xss_admin($handler->options['caption']) : '';Good review
#75
Changing the status on the bot.
#76
+++ b/core/modules/views/views.theme.incundefined@@ -773,13 +777,19 @@ function template_preprocess_views_view_grid(&$vars) {
+ if (isset($vars['header']) && count($vars['header']) && $responsive) {
I still think this can't happen, and if it does, maybe we need more test coverage.
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/style/Grid.phpundefined@@ -80,12 +81,19 @@ public function buildOptionsForm(&$form, &$form_state) {
\ No newline at end of file
Please don't remove the newline at the end :)
#77
So if there are no headers,
count($vars['header'])would return 0 & therefore that if statement would return false. Right? Is there a better way to check that $vars['header'] contains at least one header?Totally fine with more test coverage, but I'm not the one to write it.
Sorry about removing the newline..
#78
Adding back in the line at the end of the file.
#79
#78: add_captions_to_views_tables_and_grids-843708-78.patch queued for re-testing.
#80
I still think we should describe better when to use the summary and when to use the caption. If you have no clue about accessible tables
you won't be able to find that out.
#81
From http://webaim.org/blog/10-easy-accessibility-tips/ - "Most data tables have short descriptive text immediately before the data table that describes that table."
I agree that summary/caption is confusing for folks. That's why I tried to clarify with this text:
Caption:
+ '#description' => t('A title which is semantically associated to your table for increased accessibility.'),Summary:
+ '#description' => t('The table table summary should be used to increase accessibility by providing a summary of complex tables to describe how they are laid out structurally.'),Maybe the Caption could be simply:
"A short descriptive title for a table that describes the table and improves accessibility"
And the Summary:
"For complex tables consider providing a summary of how it is laid out structurally to improve accessibility"
The description text is pretty easy to change though. Up until the release.
#82
Well I don't see why we should not start with a proper help text from the beginning.
I asked Bojhan for help.
#83
Can this get some screens or something, I have no clue where to look :)
#84
I created a new View (test table) that had a page with a table layout:
admin/structure/views/view/table_test
Then went to Format: Settings and this is the popup of the latest patch:
#85
Maybe this is clearer.
Caption:
A title which is logically associated to the table to improve accessibility.
ie. The List of Registered Users
Summary:
a description of the structure for complex tables to describe how they are organized.
ie. This table charts the registered users of the site by username, email address, occupation and favorite hockey team.
They are sometimes they overlap, but not always. The summary is optional.
#86
Ok, Just adding to the confusion on this issue. Apparently SUMMARY is obsolete in HTML5.
HTML5 Candidate Recommendation - Section 4.9.1.1 Techniques for describing tables
Which leaves us in a situation where Core has legacy code already in it (the SUMMARY element), but CAPTION is still supported, but isn't in Core.
So this issue is still valid, but we will probably need to remove SUMMARY.