For WCAG validation it is important to add semantic information to data tables. They are used throughout Drupal, but is most important for Views.

Some information about the process for creating good tables is in:
http://webaim.org/techniques/tables/data

Taking from the WebAim description:

"The purpose of data tables is to present information in a grid, or matrix, and to have column or rows that show the meaning of the information in the grid. When screen readers read straight through data tables—especially large ones—it's easy for users to get lost.

In order for a data table to be accessible, it must have the proper markup in the HTML. When the proper HTML markup is in place, users of screen readers can navigate through data tables one cell at a time, and they will hear the column and row headers spoken to them."

See http://drupal.org/node/183592#comment-3245200

Files: 
CommentFileSizeAuthor
#81 drupal-864006-81.patch1.98 KBmgifford
PASSED: [[SimpleTest]]: [MySQL] 57,465 pass(es).
[ View ]
#73 Table-headers.png117.23 KBmgifford
#73 Table-headers-WAVE.png56.41 KBmgifford
#72 drupal-864006-72.patch1.91 KBAjitS
PASSED: [[SimpleTest]]: [MySQL] 56,910 pass(es).
[ View ]
#67 drupal-864006-67.patch3.41 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-864006-67.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#67 interdiff.txt807 bytesdawehner
#65 drupal-864006-65.patch3.39 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 55,935 pass(es), 0 fail(s), and 217 exception(s).
[ View ]
#62 864006-view-table-scope-62.patch2.88 KBmgifford
PASSED: [[SimpleTest]]: [MySQL] 55,662 pass(es).
[ View ]
#62 interdiff-56-62.txt539 bytesmgifford
#56 864006-view-table-scope-56.patch2.85 KBmgifford
FAILED: [[SimpleTest]]: [MySQL] 54,119 pass(es), 0 fail(s), and 98 exception(s).
[ View ]
#54 864006-view-table-scope-54.patch2.88 KBmgifford
PASSED: [[SimpleTest]]: [MySQL] 54,089 pass(es).
[ View ]
#52 864006-view-table-scope-52.patch2.88 KBmgifford
PASSED: [[SimpleTest]]: [MySQL] 53,660 pass(es).
[ View ]
#50 864006-view-table-scope-49.patch2.45 KBmgifford
PASSED: [[SimpleTest]]: [MySQL] 53,336 pass(es).
[ View ]
#38 864006-view-table-scope-38.patch1.34 KBmgifford
PASSED: [[SimpleTest]]: [MySQL] 1,603 pass(es).
[ View ]
#34 864006-view-table-scope-34.patch590 bytesmgifford
PASSED: [[SimpleTest]]: [MySQL] 1,598 pass(es).
[ View ]
#30 864006-view-table-scope-30.patch3.54 KBmgifford
PASSED: [[SimpleTest]]: [MySQL] 1,598 pass(es).
[ View ]
#28 864006-view-table-scope-28.patch1.84 KBmgifford
PASSED: [[SimpleTest]]: [MySQL] 1,555 pass(es).
[ View ]
#22 864006-view-table-scope.patch2.21 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 864006-view-table-scope.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#13 screen-capture-42.png100.99 KBmgifford
#11 views_scope_template1.patch1.3 KBmgifford
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch views_scope_template1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Status:Active» Needs work

+  $output .= theme('table', array('header' => $header, 'rows' => $rows, 'attributes' => array('id' => 'arrange', 'scope' => 'col')));

I don't get how this helps. Should this describe what the table does? t('Rearrange the items')

+  $vars['rearrange'] = l('<span>' . t('Rearrange') . '</span>', "admin/structure/views/nojs/rearrange/$view->name/$display->id/$type", array('attributes' => array('class' => array('views-button-rearrange', 'views-ajax-link'), 'title' => t('Rearrange'), 'id' => 'views-rearrange-' . $type, 'scope' => 'col'), 'html' => true));
+/
+  $vars['add'] = l('<span>' . t('Add') . '</span>', "admin/structure/views/nojs/add-item/$view->name/$display->id/$type", array('attributes' => array('class' => array('views-button-add', 'views-ajax-link'), 'title' => t('Add'), 'id' => 'views-add-' . $type, 'scope' => 'col'), 'html' => true));

This is confusing, too. You add the scope attribute to links, afaik this is not defined here

Perhaps i'm just wrong, could you clarify me?

Let me know if this clarifies anything - http://www.webaim.org/techniques/tables/data.php#scope

This is different from a table summary.

Should I start a new version for D7? This is also a problem with D6, but assume it is only likely going to get fixed in D7 at this point and possible back-ported later (this would be normal).

I was also wondering if I should create a patch from the CVS or the last release. The CVS is currently at D6.

If patches aren't going to be applied for D5 or D6 should the issues be changed to "Won't Fix"?

I think it should be fixed for 6.x, too. As long the codebase keeps similar this makes porting of patches easier

Status:Needs work» Needs review

Can you explain why the full table need's a scope?

There's a side of this argument that is just about compliance to a W3C standard:
http://www.w3.org/TR/WCAG-TECHS/H63.html

That part is pretty clear.

The better reason is that for screen readers it can be used to provide more points to better navigate the stream of data that can be contained in a data table. Tables need to be properly formatted because a lack of row and column headings and specific attributes could prevent users with screen readers from navigating tables effectively.

The problem with this is that data tables haven't been consistently built with scope, caption & summary descriptions defined in them so many blind users don't look for this data.

Adding it is a best practice however and part of getting a website through an accessibility audit. Hopefully with a consistent application of scope, caption & summary descriptions it will be easier for new users of all abilities to learn to use Drupal.

@dereine - I'm totally up for backporting this to 6 if we can find a good solution for 7. I'm just not clear what the best approach is for Views & if I should be using HEAD (which I think is still 6) for providing a patch.

In views the current practise is to patch the drupal6 version , and then if the patch applies merlinofchaos commits in to d7 and if it does not commit, i port the patch.

You should write a patch against DRUPAL-6--2 or DRUPAL-6--3 or DRUPAL-7--3 afaik HEAD is outdated

Ahh, so like this:

$ cvs -z6 -d:pserver:anonymous:anonymous@cvs.drupal.org:/cvs/drupal-contrib checkout -r DRUPAL-6--3 -d views contributions/modules/views

I've got more D6 versions with views kicking around that I can play with. I can try to incorporate some ideas from:
http://www.isolani.co.uk/articles/structuredTables.html

Good to know how things are different here in Views than core. Does make sense considering the size/importance of the code base. Is there any documentation on this?

No i don't think so :) it's a silent rule.

StatusFileSize
new1.3 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch views_scope_template1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Ok, so this should improve some of the output of the view.

I noticed that there doesn't seem to be a way to insert a summary into the output. I'd like to see something like this with some means for users to enter in a descriptive value about the table.

<table class="<?php print $class; ?>" summary="$summary">

To test this patch one should be able to look at the output of any view table produced. I had some trouble finding the scope="row" & scope="col" in my output though so perhaps this isn't where it should be applied.

The patch should work for some elements. This is a port of what I did in #183592: Small table accessibility improvement but need places to test this

Index: includes/admin.inc
===================================================================
RCS file: /cvs/drupal-contrib/contributions/modules/views/includes/admin.inc,v
retrieving revision 1.154.2.32
diff -u -p -r1.154.2.32 admin.inc
--- includes/admin.inc 17 Jun 2010 02:45:30 -0000 1.154.2.32
+++ includes/admin.inc 26 Jul 2010 03:11:03 -0000
@@ -1273,9 +1273,9 @@ function template_preprocess_views_ui_ed
       $rearrange_url = "admin/build/views/nojs/rearrange/$view->name/$display->id/$type";
   }
-  $vars['rearrange'] = l('<span>' . t('Rearrange') . '</span>', $rearrange_url, array('attributes' => array('class' => 'views-button-rearrange views-ajax-link', 'title' => t('Rearrange')), 'html' => true));
+  $vars['rearrange'] = l('<span>' . t('Rearrange') . '</span>', $rearrange_url, array('attributes' => array('class' => 'views-button-rearrange views-ajax-link', 'title' => t('Rearrange'), 'scope' => 'col'), 'html' => true));
-  $vars['add'] = l('<span>' . t('Add') . '</span>', "admin/build/views/nojs/add-item/$view->name/$display->id/$type", array('attributes' => array('class' => 'views-button-add views-ajax-link', 'title' => t('Add')), 'html' => true));
+  $vars['add'] = l('<span>' . t('Add') . '</span>', "admin/build/views/nojs/add-item/$view->name/$display->id/$type", array('attributes' => array('class' => 'views-button-add views-ajax-link', 'title' => t('Add'), 'scope' => 'col'), 'html' => true));
   if (!$display->handler->is_default_display()) {
     if (!$display->handler->is_defaulted($types[$type]['plural'])) {

Title:Rearrange table accessibilityImprove accessibility of views

This is strange. You changed a total different file :)

This file is not part of the admin pages at all. No wondern that you cannot find find the output of the table :)

We could say you say what's needed to be changed and where and i create the patch, i think this saves time for everyone :)

One suggestion: We could add a textfield for administrators to be able to add a summary attribute to the views table output.
What do you think about this?

StatusFileSize
new100.99 KB

Ya, sorry. My main goal was to look for appropriate places to add scope = "col" to table cells in Views. would be good to also add scope = "row", but I do think this could be more complicated, there are some good examples here mind you - http://webaim.org/techniques/tables/data

I was looking at both the admin output as well as the output of the view. I'm assuming that the theme/views-view-table.tpl.php is used at any time where there is a table view defined and that you should be able to see it in the output there. But ya, I didn't explain what I was doing very well. Just came to the end of what time I had to contribute at that point and then needed to post it somewhere so that it wasn't lost.

I do think that a textfield for administrators to add summary attributes would be excellent! Simply to provide that as an option would be great. Having a caption field along with that would also be a good improvement.

Oh, I'd so love it if you would create the patch. Views is a pretty big module & probably one of the more complicated ones in Drupal. I'd like to see more default implementations like this for public content:

    <thead>
    <tr>
              <th class="views-field views-field-sourceid" scope = "col">
          <a href="/admin/content/tw/view/map?order=sourceid&amp;sort=asc" title="sort by sourceid" class="active">source id</a>        </th>
              <th class="views-field views-field-destid" scope = "col">
          <a href="/admin/content/tw/view/map?order=destid&amp;sort=asc" title="sort by destid" class="active">destination id</a>        </th>
              <th class="views-field views-field-needs-update" scope = "col">
          <a href="/admin/content/tw/view/map?order=needs_update&amp;sort=asc" title="sort by needs_update" class="active">needs_update</a>        </th>
          </tr>
  </thead>

There are going to need to be changes to the Drag/Drop rearrange section in the admin page (which I think now is the only place where tables are used here), but we can't really fix it in six & then migrate that to seven as core's basically broken in D6 for drag/drop capacity. It will be interesting to be able to enable/disable this table in D7....

In anycase, improving the output will benefit the most people so very excited about this.

Taken from the Yourhtmlsource guide on table accessibility.

The caption tag, which will appear on the screen is used to give the title of the table.
The summary attribute gives a broader description of the table's purpose.

Views is capable of some pretty complex stuff. Would be good if there were ways of making that content more accessible to people. Some useful guides include:
http://www.usability.com.au/resources/tables.cfm
http://www.userite.com/ecampus/lesson3/lesson3c.php
http://www.evengrounds.com/developers/accessible-tables
http://webstandards.psu.edu/accessibility/tech/tables

Note that the last two are Drupal sites.

Assigned:Unassigned» dawehner

I do think that a textfield for administrators to add summary attributes would be excellent! Simply to provide that as an option would be great. Having a caption field along with that would also be a good improvement.

Sure this is possible.

Assign to me

I'm totally on board with dereine in #14. That's an easy improvement that would go a long ways.

Here is a issue to add a summary for tables: #868972: summary attribute for tables

this is exciting! Thanks for setting up the new issue about summary tables and already providing a patch!

Here are two new issue. I suggest to use this issue as a collection for other issues.

Hey Dereine,

I'm just trying to see what we can do to move this along. You've opened two new issues. Thanks! I'll look at those.

I think that this one here then will be focused on views/includes/admin.inc and adding the scope = "col" in six as I tried to do it in seven in this patch: http://drupal.org/node/183592#comment-3245200

Is that right? I don't want to loose all momentum on bringing accessibility into views.

adding scope="col" to all the headers would be good. currently i'm doing this through the theming layer..
as far as i know tables generated by views have headers on top and data below.. which is perfect for scope.

it might be good to have an option for this.. but the cases where you wouldn't want scope="col" are only if you make views generate a table with headers on the left or more complex header structure.. in this case i think doing the work in the theming layer will make sense (probably because you will be doing some heavy theming to make the tables look so different anyways) so there should be an option 'remove scope from table' because 99.99% of the time this is wanted.

This is needed for anyone working with the wcag standards.. (a lot of government sites require this as well as sites which need to be accessible)

So thanks for putting in the caption!! brings us a step closer!!

Status:Needs review» Needs work

Due to changes in from the semantic views patch, this patch will no longer apply.

I think I understand the point of the scope variable now, and certainly in the table style I think this is a good idea. So +1 to the effort here. We might also consider doing it for the grid style, but I'm not certain if scope is appropriate on the grid which is a table but without headers.

However, instead of using a counter it's pretty easy to calculate the first field using something along the lines of reset($array); $first_key = key($array) and then doing an if to compare.

I also would rather see a scope variable created in the preprocess than actually embedded harshly into the template like this. This fails my sniff test of "too much PHP code in the template."

Status:Needs work» Needs review
StatusFileSize
new2.21 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 864006-view-table-scope.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

What about this?

drupal_attributes can be replaced in d7 with a process function.

Ok, so how do we test this again?

Is the latest patch against the latest dev release of D7? Or is this against the CVS of views in D6?

Just want to help if I can.

This patch appears to be against latest CVS of DRUPAL-6--3

Assigned:dawehner» bojanz

Assigning to bojanz for testing.

Is there a Drupal 7 solution for this?

Status:Needs review» Needs work

The last submitted patch, 864006-view-table-scope.patch, failed testing.

Title:Improve accessibility of viewsImprove table semantics by adding scope element
Version:6.x-3.x-dev» 7.x-3.x-dev
Status:Needs work» Needs review
StatusFileSize
new1.84 KB
PASSED: [[SimpleTest]]: [MySQL] 1,555 pass(es).
[ View ]

Changing the title to be more specific, bumping this to D7 & upgrading the patch.

Status:Needs review» Needs work

scope="row" is there, but what about scope="col"? Is this taken care of elsewhere?

Status:Needs work» Needs review
StatusFileSize
new3.54 KB
PASSED: [[SimpleTest]]: [MySQL] 1,598 pass(es).
[ View ]

Try this.

Status:Needs review» Postponed (maintainer needs more info)

I'm actually going to mark this as postponed. I'm not sure what the future of the scope attribute is after seeing:
http://www.developphp.com/view_lesson.php?v=545

Certainly doesn't appear here:
http://www.w3.org/TR/html5/global-attributes.html#global-attributes

If it's depreciated in HTML5, is this really the right time to be adding it into Views (even though D7 is going to be HTML4 by default).

Yet there are still lots of docs that suggest it's a best practice:
http://developers.whatwg.org/tabular-data.html#attr-th-scope

And no alternate example from WCAG:
http://www.w3.org/TR/WCAG20-TECHS/H63

I think we need another opinion before this can go much further.

Status:Postponed (maintainer needs more info)» Needs review

I'm not sure what the future of the scope attribute is after seeing ...

All that's noteworthy about scope in that list of HTML5 deprecated elements/attributes is that we can no longer use the scope attribute on a TD element. Scope's valid on a TH element. Makes sense, semantically.

Certainly doesn't appear here ...

That's a list of global attributes. Scope is just used on TH elements.

Yet there are still lots of docs that suggest it's a best practice ...

It's in the HTML5 spec.

And no alternate example from WCAG ...

H63 is one WCAG 'technical solution' for associating a table cell with its row or column header(s). Another's H43, which describes how to do it using id/headers attributes.

To sum up, scope is solidly in the spec. At the moment, the only problem with scope is dodgy support in some user agents, as mentioned at the top of H63 in 'User Agent and Assistive Technology Support Notes.'

Thanks @Jeffdo I've been engaging with others on Twitter about this. When bringing something into a module as important as Views I'd like to make sure that we're right about this being a best practice.

As you correctly stated, with table headers scope is still supported:
http://dev.w3.org/html5/alt-techniques/#hag21

Looks like either TH/scope or id/header approaches will work:
http://webaim.org/techniques/tables/data/#id
http://www.evengrounds.com/developers/accessible-tables
http://www.jimthatcher.com/webcourse9.htm
http://www.w3.org/TR/WCAG20-TECHS/H43
http://www.evengrounds.com/developers/accessible-tables

For Section 508 folks:
http://www.hhs.gov/web/policies/webstandards/htmltable.html

Both add semantics to the tables that is presently missing. I'm not sure much is being added semantically by simply adding a scope attribute with a direction to the TH tag, but it certainly seems easier.

Thanks for re-opening it.

StatusFileSize
new590 bytes
PASSED: [[SimpleTest]]: [MySQL] 1,598 pass(es).
[ View ]

Ok, let me just simplify this a bunch. Views at this stage doesn't allow users to customize the TH element. In a row, you can't make a TD into a TH because it makes more semantic sense. So if we're going with scope we might as well just add it to the table header in the views-view-table.tpl.php's template like:

-          <th <?php if ($header_classes[$field]) { print 'class="'. $header_classes[$field] . '" '; } ?>>
+          <th <?php if ($header_classes[$field]) { print 'class="'. $header_classes[$field] . '" '; } ?> scope="col">

This would provide some semantic improvements and would output a header row like:

      <tr>
                  <th class="views-field views-field-title"  scope="col">
            Title          </th>
                  <th  scope="col">
            Overview          </th>
                  <th  scope="col">
            Listing          </th>
                  <th  scope="col">
            Options          </th>
              </tr>
    </thead>

Views table handling would need to much more robust (as I understand it), to make use of any of the other complexities that often are needed in producing accessible tables.

Status:Needs review» Needs work

Now the "row" is missing.

You can't add the row. Scope is only valid on TH's not TD's. TD's are only available as the rows. The only option available to us (that I can see) is the patch I've added. I'm all for Views being able to create more complex views, but don't think that is possible.

Status:Needs work» Needs review

Gotcha. Let's get some more eyeballs on this.

Title:Improve table semantics by adding scope elementImprove table semantics by adding scope or headers/id attributes
StatusFileSize
new1.34 KB
PASSED: [[SimpleTest]]: [MySQL] 1,603 pass(es).
[ View ]

Ok, here's a better solution that will give semantics that AT can use for all cells. This approach is pretty simple, but I can't see a use case where it doesn't work. It's more useful than #34 because it gives details for the individual cells basically following the ID method proposed here rather than scope.

http://webaim.org/techniques/tables/data/#id

Also the related issue for core's defaults is - #1645308: Add Scope or Headers/id to associate the data cells with the appropriate headers in theme_table()

Status:Needs review» Needs work

+++ b/theme/views-view-table.tpl.phpundefined
@@ -26,7 +26,7 @@
+          <th <?php if ($header_classes[$field]) { print 'class="'. $header_classes[$field] . '" '; } ?> id="<?php print $field; ?>">

Wouldn't this potential introduce duplicated ids? I'm really not sure whether we should add unexpected IDs to the html output. What about view-$name-$display_id-table-field as ID? This should be hopefully unique enough, but to ensure that, we can use drupal_html_id in the preprocess function.

Beside from that, isn't it actually also important to add these kind of help to the admin UI?

@dawehner I can see how that would be possible. We'd just have to expose the $name & $display_id variables in
theme/theme.inc to theme/views-view-table.tpl.php though as I'm pretty sure they aren't right now.

There's definitely a related issue from Core that I've also been working on.

To be clear though it will actually bring the admin UI closer to meeting WCAG 2.0 AA guidelines (which is quite important for many institutions), but I'm not certain at this point how much of an actual benefit it will provide to screen reader users.

The disadvantage of the id/header approach in #38 is just adding additional text to the html output. It's much bigger than the scope method suggested in #34 (although the patch there is incomplete).

Component:User interface» Code
Assigned:bojanz» Unassigned
Status:Needs work» Reviewed & tested by the community

The patch in #34 is the correct approach and should be committed. This was the approach we took on EPA.gov to make Views-generated tables accessible.

Related issue for Views in Drupal 8.x: #1831162: Tables generated by Views need better semantics.

I trust mgifford for accessiblity reasons so i don't think #34 is rtbc, see http://drupal.org/node/864006#comment-6463332

Really it's either the scope method or header/id. There are advantages & disadvantages to either.

The main problem with the header/id approach is bloat. I can't tell you how much larger it makes the table, but when we're building D8 for a mobile environment, you gotta count the characters.

The method I proposed in 38 was simpler, but it had not ability to match row headers. This is partly because there is no way to define row headers in Drupal at this point.

I think it's fair to say that many blind users don't really need either scope or header/id definitions if it is a simple table. Most Drupal tables are just dead simple to understand and there is very little benefit for most screen reader users to have this for Drupal tables. Least that's what I understand.

The guidelines however specify that one of the two approaches should be used in a data table.

If it's good enough for the EPA.gov at this point, I think it's a model that the Drupal community can follow. I don't know of another agency that has implemented a different approach. It's a light weight approach that addresses the basic use case in Drupal without much bloat.

Thanks for your confidence @dawehner - I think the scope="col" approach may be the best we can do.

The header/id approach is only needed for more complicated data tables.

Agreed. And although I'd like to see Views able to create more complex tables, I don't think we're there yet.

#38: 864006-view-table-scope-38.patch queued for re-testing.

So I'm still worried that my comment in #864006-39: Improve table semantics by adding scope or headers/id attributes is still TRUE. You can for sure workaround that using some preprocess logic and call drupal_html_id() which would solve that. Isn't that a possibility?

It was a good point. I didn't see it as being very likely, but very likely isn't the same as producing consistently valid HTML. Running view-$name-$display_id-table-field through drupal_html_id() would be useful to ensure it's unique.

Project:Views» Drupal core
Version:7.x-3.x-dev» 8.x-dev
Component:Code» views.module
Status:Reviewed & tested by the community» Needs review

I applied this to D8.. Feel free to revert back to D7 Views module.

StatusFileSize
new2.45 KB
PASSED: [[SimpleTest]]: [MySQL] 53,336 pass(es).
[ View ]

oops

This looks great, thank you. (We can get this into 7.x-3.x for sure.).

The only thing i'm wondering is whether we should document this new variable in the template and whether adding a test would be helpful.

StatusFileSize
new2.88 KB
PASSED: [[SimpleTest]]: [MySQL] 53,660 pass(es).
[ View ]

Ok, here's some template documentation. This can be backported to D7, but lets see if we can bring it into D8.

Tests are always good, but I don't write them.

This looks great beside this one small thing:

+++ b/core/modules/views/views.theme.incundefined
@@ -480,6 +480,7 @@ function template_preprocess_views_view_table(&$vars) {
+$vars['id'] = array();

Sorry ... two spaces are needed here.

I guess it would be great if someone could write an issue summary, so other people understand this change.

Issue tags:+Needs issue summary update
StatusFileSize
new2.88 KB
PASSED: [[SimpleTest]]: [MySQL] 54,089 pass(es).
[ View ]

Spaces added. I also added a tag to help with the summary.

Essentially it's a change from just using the column to using the full header/id approach to the Views tables. It's a more comprehensive approach to adding semantics to the tables.

The code is looking great, but I have no idea about the accessibility aspect.

Does the core gates tell that you should have manual testing for that?

StatusFileSize
new2.85 KB
FAILED: [[SimpleTest]]: [MySQL] 54,119 pass(es), 0 fail(s), and 98 exception(s).
[ View ]

Realized that I needed to only use drupal_html_id() once.

I will do some manual testing on this, but as far as the core gates go, I'm pretty sure it's fine to go with the best practices.

Status:Needs review» Needs work
Issue tags:-Needs issue summary update, -accessibility

The last submitted patch, 864006-view-table-scope-56.patch, failed testing.

Status:Needs work» Needs review

#56: 864006-view-table-scope-56.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 864006-view-table-scope-56.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Needs issue summary update, +accessibility

#54: 864006-view-table-scope-54.patch queued for re-testing.

StatusFileSize
new539 bytes
new2.88 KB
PASSED: [[SimpleTest]]: [MySQL] 55,662 pass(es).
[ View ]

addressing php notice.

+++ b/core/modules/views/templates/views-view-table.tpl.phpundefined
@@ -38,7 +40,7 @@
+          <td <?php print $field_classes[$field][$row_count]; ?> headers="<?php print $headers[$field]; ?>">
+++ b/core/modules/views/views.theme.incundefined
@@ -570,6 +573,9 @@ function template_preprocess_views_view_table(&$vars) {
+      // Improves accessibility of complex tables.
+      $vars['headers'][$field] = (isset($vars['id'][$field])) ? $vars['id'][$field] : '';

It would be cool, if the variable could be named a bit different. There are already two similar variables "header" and "header_attributes". Why not use $vars['field_classes'] directly, because this isalready an Attribute object, so you can add new attributes.

@dawehner - as it's defined right now this patch has nothing to do with CSS classes so not sure to add it to "An array of classes to apply to each field". It's a different attribute, so not sure it would make sense to just add it to "field_classes". I'm open to suggestions though.

I couldn't find a "header_attributes" variable when grepping through d8.

StatusFileSize
new3.39 KB
FAILED: [[SimpleTest]]: [MySQL] 55,935 pass(es), 0 fail(s), and 217 exception(s).
[ View ]

Let's do it properly and add a todo.

Status:Needs review» Needs work

The last submitted patch, drupal-864006-65.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new807 bytes
new3.41 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-864006-67.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Let's do it different.

Status:Needs review» Needs work
Issue tags:-Needs issue summary update, -accessibility

The last submitted patch, drupal-864006-67.patch, failed testing.

Status:Needs work» Needs review

#67: drupal-864006-67.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Needs issue summary update, +accessibility

The last submitted patch, drupal-864006-67.patch, failed testing.

Issue tags:+Needs reroll

Well, even it is a reroll, it is probably more like a full rewrite as now twig is in core.

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new1.91 KB
PASSED: [[SimpleTest]]: [MySQL] 56,910 pass(es).
[ View ]

Re-rolling the patch from #67

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new56.41 KB
new117.23 KB

I'm going to mark this RTBC. Looks good to me!

Table-headers.pngTable-headers-WAVE.png

Issue tags:+RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/views/views.theme.incundefined
@@ -542,6 +542,7 @@ function template_preprocess_views_view_table(&$vars) {
+  // @todo Remove field_classes to field_attributes.
@@ -606,6 +607,7 @@ function template_preprocess_views_view_table(&$vars) {
+      // @todo Rename to header_attributes.

Is there a follow up to address these todos?

Also the issue summary could do with an update.

@dawehner these were added in #65 based on feedback in #63. Can you create the follow-up issues for these todos? Also verify that they are still relevant.

@alexpott agreed on the summary. I've bumped it up a bit.

Status:Needs work» Needs review

#72: drupal-864006-72.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

I believe that @alexpott's concerns have been addressed now so moving this back to RTBC.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Needs a reroll

git ac https://drupal.org/files/drupal-864006-72.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  1954  100  1954    0     0   3088      0 --:--:-- --:--:-- --:--:--  4800
error: patch failed: core/modules/views/views.theme.inc:542
error: core/modules/views/views.theme.inc: patch does not apply

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new1.98 KB
PASSED: [[SimpleTest]]: [MySQL] 57,465 pass(es).
[ View ]

re-roll...

Issue tags:+TwinCities

Looks like this just needs a re-roll and review to be RTBC again.

#81: drupal-864006-81.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

Let's get it in again.

Status:Reviewed & tested by the community» Fixed

Committed 41f6e35 and pushed to 8.x. Thanks!

Status:Fixed» Closed (fixed)
Issue tags:-Needs issue summary update, -accessibility, -RTBC July 1, -TwinCities

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

Issue summary:View changes

Updated issue summary.