Wanted to update the HTML structure used in this issue. Summary has been depreciated as it was originally expressed in this issue.

 <caption>
  <strong>Characteristics with positive and negative sides.</strong>
  <details>
   <summary>Help</summary>
   <p>Characteristics are given in the second column, with the
   negative side in the left column and the positive side in the right
   column.</p>
  </details>
</caption>

---- Original ----

For accessibility, 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>
CommentFileSizeAuthor
#101 drupal-843708-101.patch9.24 KBdawehner
#101 interdiff.txt1.02 KBdawehner
#96 drupal-843708-96.patch9.26 KBdawehner
#93 Before_Title_Entered.png37.55 KBmgifford
#93 After_Title_Entered.png62.04 KBmgifford
#92 interdiff-89-91.txt11.47 KBmgifford
#91 drupal-843708-91.patch9.79 KBdawehner
#90 CollapsedDetailsFields.png130.56 KBmgifford
#90 ExpandedDetailsFields.png99.62 KBmgifford
#90 PublicPages-Caption-Details.png111.43 KBmgifford
#89 add_caption_remove_summary_views_tables_and_grids-843708-89.patch8.87 KBmgifford
#87 add_caption_remove_summary_views_tables_and_grids-843708-87.patch8.87 KBmgifford
#84 CaptionAndSummaryInTable.png258.01 KBmgifford
#78 add_captions_to_views_tables_and_grids-843708-78.patch7.25 KBmgifford
#74 add_captions_to_views_tables_and_grids-843708-74.patch7.28 KBmgifford
#71 interdiff-62-68.txt1.24 KBmgifford
#68 add_captions_to_views_tables_and_grids-843708-68.patch7.54 KBmgifford
#67 patch-with-tables.png121.71 KBOaryx
#67 patch-wth-grids.png100.19 KBOaryx
#63 add_captions_to_views_tables_and_grids-843708-62.patch7.43 KBmgifford
#60 add_captions_to_views_tables_and_grids-843708-60.patch7.46 KBmgifford
#60 Table-With-Caption-Summary.png188.42 KBmgifford
#58 CaptionSummaryAdmin.png88.54 KBmgifford
#58 Caption-NO-SUMMARY-output.png168.51 KBmgifford
#55 add_captions_to_views_tables_and_grids-843708-55.patch7.34 KBmgifford
#52 add_captions_to_views_tables_and_grids-843708-52.patch6.7 KBmgifford
#50 add_captions_to_views_tables_and_grids-843708-50.patch5.5 KBmgifford
#48 add_captions_to_views_tables_and_grids-843708-48.patch5.5 KBmgifford
#46 add_captions_to_views_tables_and_grids-843708-46.patch5.27 KBmgifford
#44 add_captions_to_views_tables_and_grids-843708-44.patch5.82 KBmgifford
#36 add_captions_to_views_tables_and_grids-843708-36.patch5.97 KBmgifford
#34 add_captions_to_views_tables_and_grids-843708-34.patch5.93 KBmgifford
#32 AddingCaption2ViewsTables.png101.78 KBmgifford
#31 add_captions_to_views_tables_and_grids-843708-31.patch6.48 KBcolan
#30 add_captions_to_views_tables_and_grids-843708-30.patch6.54 KBcolan
#28 add_captions_to_views_tables_and_grids-843708-28.patch6.6 KBcolan
#26 add_captions_to_views_tables_and_grids-843708-26.patch6.6 KBcolan
#25 843708_views_caption6x_25.patch4.63 KBmgifford
#23 843708_views_caption6x_23.patch5.35 KBgreggles
#15 843708_views_caption6x.patch5.45 KBgreggles
#6 views-summary-attribute-843708.patch2.08 KBdawehner
#2 views-summary-attribute-843708-2.patch2.15 KBsamuelsov
#1 views-summary-attribute-843708-1.patch2.16 KBsamuelsov
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

samuelsov’s picture

Status: Active » Needs review
FileSize
2.16 KB

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 !

samuelsov’s picture

Oups, here is a good version...

dawehner’s picture

Status: Needs review » Needs work
+++ 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.

mgifford’s picture

Issue tags: +Accessibility

tagging

Everett Zufelt’s picture

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.08 KB

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.
Everett Zufelt’s picture

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

merlinofchaos’s picture

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 . '"' } ?>>
merlinofchaos’s picture

Status: Needs review » Needs work
mgifford’s picture

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?

dawehner’s picture

Can you provide a good link/explaination?

mgifford’s picture

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

Everett Zufelt’s picture

Priority: Normal » Minor

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.

mgifford’s picture

Priority: Minor » Normal

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

greggles’s picture

Status: Needs work » Needs review
FileSize
5.45 KB

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.

greggles’s picture

Version: 6.x-2.8 » 6.x-2.x-dev

Applies to latest 6.x-2.x, btw.

greggles’s picture

Title: Add option to set summary attributes in the html table (Accessibility) » Add option to set caption in the html table (Accessibility)

Sorry for yet another comment, but I realized the focus has changed here.

lisarex’s picture

Just a quick review of the text/labeling

Title: Short description of table contents
(added word)
Description: This value will be displayed as the caption for the table. Include a caption to provide screenreader users an overview of the table contents.
(fixed spelling/expanded description)

Description: This value will be displayed as table-summary attribute in the html. Include a summary for complex tables.
(modified description slightly)

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.

mgifford’s picture

This will be very exciting to have this in Views! @greggles thanks for the patch!

greggles’s picture

Include a caption to provide screenreader users an overview of the table contents.

I think that's somewhat incorrect. The default HTML styles show the caption tag contents to sighted users.

lisarex’s picture

@greggles, oops, you're right.

How about "Include a caption to provide an overview of the table contents."

lisarex’s picture

Rather, "Include a caption for better accessibility of your table."

greggles’s picture

Re-rolled for lisa's suggested text.

dawehner’s picture

Status: Needs review » Needs work

Sadly this patch doesn't apply to 6.x-3.x anymore
Some rerole would be definitive helpful here.

mgifford’s picture

Version: 6.x-2.x-dev » 7.x-3.3
Status: Needs work » Needs review
FileSize
4.63 KB

Re-rolling this patch for Drupal 7.

colan’s picture

The previous patch didn't work because the option wasn't being set in the preprocess functions. Try this one instead.

colan’s picture

Version: 7.x-3.3 » 7.x-3.x-dev
colan’s picture

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

Status: Needs review » Needs work

The last submitted patch, add_captions_to_views_tables_and_grids-843708-28.patch, failed testing.

colan’s picture

Status: Needs work » Needs review
FileSize
6.54 KB
  • The option was being incorrectly set in the summary functions. I removed these lines.
  • Added some filter_xss_admin() action.
colan’s picture

Sorry, the previous patches were adding it to the table attributes instead of making it a separate tag.

mgifford’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
101.78 KB

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/

dawehner’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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

mgifford’s picture

Status: Needs work » Needs review
FileSize
5.93 KB

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'] = '';
   }
dawehner’s picture

Status: Needs review » Needs work
+++ 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.

mgifford’s picture

Status: Needs work » Needs review
FileSize
5.97 KB

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.

dawehner’s picture

Version: 7.x-3.x-dev » 8.x-3.x-dev
Status: Needs review » Patch (to be ported)
Issue tags: +VDC

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.

dawehner’s picture

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?

mgifford’s picture

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.

xjm’s picture

Project: Views (for Drupal 7) » Drupal core
Version: 8.x-3.x-dev » 8.x-dev
Component: table style » views.module
mgifford’s picture

Status: Patch (to be ported) » Needs review
mgifford’s picture

Issue tags: +a11ySprint

tagging.

Status: Needs review » Needs work

The last submitted patch, add_captions_to_views_tables_and_grids-843708-36.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
5.82 KB

Re-rolling here.

mgifford’s picture

Status: Needs review » Needs work
Issue tags: +Needs screenshots

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.

mgifford’s picture

Status: Needs work » Needs review
FileSize
5.27 KB

updating patch.

Status: Needs review » Needs work

The last submitted patch, add_captions_to_views_tables_and_grids-843708-46.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
5.5 KB

Annoying.. Missed a } in copy/paste.

Status: Needs review » Needs work

The last submitted patch, add_captions_to_views_tables_and_grids-843708-48.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
5.5 KB

Arg, rushing around too many issues today. Missed another } for the class.

Status: Needs review » Needs work

The last submitted patch, add_captions_to_views_tables_and_grids-843708-50.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
6.7 KB

Found some other issues. It's still not showing up though.

dawehner’s picture

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

mgifford’s picture

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.

mgifford’s picture

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

mgifford’s picture

mgifford’s picture

Issue tags: +RTBC Feb 18

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

mgifford’s picture

Category: feature » task
Issue tags: -Needs screenshots, -RTBC Feb 18
FileSize
168.51 KB
88.54 KB

Sorry, Summary's not being displayed. I'm hoping this can be considered a task though rather than a feature.

dawehner’s picture

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?

mgifford’s picture

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.

dawehner’s picture

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

mgifford’s picture

Issue tags: +Needs tests

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.

mgifford’s picture

forgot to attach it.

Status: Needs review » Needs work
Issue tags: -Needs tests, -Accessibility, -VDC, -a11ySprint

The last submitted patch, add_captions_to_views_tables_and_grids-843708-62.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests, +Accessibility, +VDC, +a11ySprint
mgifford’s picture

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:799
Stack 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
Oaryx’s picture

Status: Needs review » Needs work
FileSize
100.19 KB
121.71 KB

Works with tables in views, however not with grids. Summary attribute does not show up when display is set to grid.

mgifford’s picture

Status: Needs work » Needs review
FileSize
7.54 KB

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.

mgifford’s picture

mgifford’s picture

mgifford’s picture

FileSize
1.24 KB

Interdiff between my two patches. Should include changes for grid as specified.

falcon03’s picture

Version: 8.x-dev » 7.x-dev
Component: views.module » views_ui.module
Status: Needs review » Needs work

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:

The table attribute should be used to increase accessibility by providing a summary of complex tables to describe how they are laid out structurally.

maybe "table attribute" should be "table summary" or "table summary attribute". From a usability point of view I would prefer the first option.

+++ b/core/modules/views/templates/views-view-grid.tpl.php
-      <tr <?php print $row_classes[$row_number]; 

>
+

print (isset($row_classes[$row_number])) ? $row_classes[$row_number] : ''; >
?>
why do we need this?
+  // 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.

falcon03’s picture

Version: 7.x-dev » 8.x-dev
Component: views_ui.module » views.module

Restoring proper issue values. Not sure what happened, but I didn't change them, except for status :-)

mgifford’s picture

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

mgifford’s picture

Status: Needs work » Needs review

Changing the status on the bot.

dawehner’s picture

+++ 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 :)

mgifford’s picture

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

mgifford’s picture

Adding back in the line at the end of the file.

mgifford’s picture

dawehner’s picture

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.

mgifford’s picture

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.

dawehner’s picture

Well I don't see why we should not start with a proper help text from the beginning.
I asked Bojhan for help.

Bojhan’s picture

Can this get some screens or something, I have no clue where to look :)

mgifford’s picture

FileSize
258.01 KB

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:

Caption And Summary In Table

mgifford’s picture

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.

mgifford’s picture

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.

mgifford’s picture

Title: Add option to set caption in the html table (Accessibility) » Add option to set caption & remove summary in the html table (Accessibility)
FileSize
8.87 KB

I am a bit sleep deprived (and DrupalCon hasn't even started yet).

But figured it would be useful to post this patch to help move this ahead.

Need to be able to produce output like:

 <caption>
  <strong>Characteristics with positive and negative sides.</strong>
  <details>
   <summary>Help</summary>
   <p>Characteristics are given in the second column, with the
   negative side in the left column and the positive side in the right
   column.</p>
  </details>
 </caption>

Status: Needs review » Needs work
mgifford’s picture

I'm going to have to figure out why my local install doesn't work on the train to Portland tomorrow...

mgifford’s picture

Status: Needs work » Needs review
FileSize
111.43 KB
99.62 KB
130.56 KB

Here's what this patch looks like with expanded details (default is attached, but not embedded below):
Expanded Details Fields

This is what the details look like when added to the captions:
Public Pages Caption & Details.png

I don't know why the details are outside of the table.

dawehner’s picture

Issue tags: -Needs tests
FileSize
9.79 KB

I'm sorry I had opened the tab quite long without reloading it.

Personally I don't think we should make this for grids, because grids will get responsive based upon DIVs, so we probably don't need this functionality anymore. Wrote the patch so it actually works as expected and added test coverage.

mgifford’s picture

FileSize
11.47 KB

Agreed about moving grids to div's. Definitely a better way to go. I'm just disappointed that we've made so little progress in this area since January -> #1903746: Replace the views grid table template with one using divs

I've attached an interdiff. You've added testing from the looks of it. That's great!

Good call adding $caption_needed too, but on the resulting view I don't see either the caption or the details I added.

You making it to DrupalCon? Would be good to push this through.

mgifford’s picture

Oh ya, I grabbed screen shots. I think the UI for this is good. We're probably going to need to add alerts to it, but I do think this is the way to go.

When you expand the detail element you see just the title field:
Only local images are allowed.

After the title is entered the description appears:
After Title Entered

dawehner’s picture

Opened an issue to connection states js and announce js: #2000324: Let state.js uses announce.js

mgifford’s picture

@dawehner - great meeting with you briefly. We're pretty close to marking this RTBC. If we use state.js to announce the change in the UI via Drupal.announce, then I think we're just left with my note in #92 "I don't see either the caption or the details I added."

Can you confirm that with the latest patch that the Caption & Details are now added to the table when it is saved? I didn't see the results when I did my testing.

dawehner’s picture

FileSize
9.26 KB

Rerolled the patch as it is.

dawehner’s picture

Manual testing let the summary appear.

Status: Needs review » Needs work
Issue tags: -Accessibility, -VDC, -a11ySprint

The last submitted patch, drupal-843708-96.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

#96: drupal-843708-96.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Accessibility, +VDC, +a11ySprint

The last submitted patch, drupal-843708-96.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.02 KB
9.24 KB

I totally failed on the template.

Status: Needs review » Needs work
Issue tags: -Accessibility, -VDC, -a11ySprint

The last submitted patch, drupal-843708-101.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
Issue tags: +Accessibility, +VDC, +a11ySprint

#101: drupal-843708-101.patch queued for re-testing.

mgifford’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me. Tested it locally and the front-end/back-end looked as expected. Will be great to have this in Core!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 2a17837 and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -Accessibility, -VDC, -a11ySprint

Automatically closed -- issue fixed for 2 weeks with no activity.

mgifford’s picture

Issue summary: View changes

update of description of summary.

mgifford’s picture

Issue summary: View changes

Just wanting to add that there is now an inconsistency in that CKEditor still uses the old approach to using caption/summary as per:

http://dev.ckeditor.com/ticket/12238