Tables should take an optional footer variable and produce <tfoot>

Bump. This needs to be added to D6 and D7 as well.

One immediate bug is that tfoot should come BEFORE tbody in the rendered HTML.

From the HTML 4 specifications:

TFOOT must appear before TBODY within a TABLE definition so that user agents can render the foot before receiving all of the (potentially numerous) rows of data. The following summarizes which tags are required and which may be omitted:

There's also a syntax error in the patch here:

+    $output .= ' <tfoot><tr>');

That trailing bracket shouldn't be there.

#D8 /core upgrade, address #4 and #6

Getting the following notice:
Notice: Undefined index: footer in theme_table() (line 1771 of \core\include\theme.inc).
from the line declaring: $footer = $variables['footer'];

You should also document the new $variables['footer'] by adding explanation in the function doxygen part. See http://drupal.org/node/1354 for details.

Untested, but documentation added and warning in #8 fixed. Not how this patch needs to relate to theme_tablesort, in general more testing is required.

I may just be missing the obvious. In the following lines in the patch:

+  if (count($footer)) {
+    // HTML requires that the tfoot tag has tr tags in it followed by tbody
+    // tags. Using ternary operator to check and see if we have any rows.
+    $output .= (count($rows) ? ' <tfoot><tr>' : ' <tr>');
+    $i = 0;
+    foreach ($header as $cell) {
+      $cell = tablesort_cell($cell, $header, $ts, $i++);
+      $output .= _theme_table_cell($cell);
+    }
+    // Using ternary operator to close the tags based on whether or not there are rows
+    $output .= (count($rows) ? " </tr></tfoot>\n" : "</tr>\n");
+  }

I presume the reference to $header is meant to be $footer?

Yes, of course. "Untested, ..." Good catch.

Very good!!
Is it possible to add to D7 future upgrades?

Atthached the patch corrected changing $header => $footer

#14: Yes. After 8.x is committed, a backport is possible.

Ok thanks,

Attached patch for D7.18

Is there any pre-test tool?

I've started a separate issue for a D7 backport: #1892654: D7 Backport: theme_table() should take an optional footer variable and produce <tfoot>

Rerolled patch from #15 against current 8.x branch.

Thanks, @Floydm, for rerolling it. Now, onto details:

• What impact, if any, would there be when using tablesort_cell() on a footer cell?
• Now that D8 tables are responsive, the patch should be updated to inherit the responsive priorities from header. Does anyone have experience with showing/hiding columns where one or more rows (e.g. footer) has cells where colspan > 1?

In general, I think the previous patch (prior to reroll) might be a bit out of date, considering that theme_table() in D8 has changed considerably since this issue originally started. The D7 backport issues (i.e. being non-responsive) can be dealt with separately, especially given the patch in #24 is pretty much D7-ready.

It would be great if someone with expertise in the D8 responsive table implementation could chime in (from #1276908: Administrative tables are too wide for smaller screens).

#24: theme_table_tfoot-806982-24.patch queued for re-testing.

#24: theme_table_tfoot-806982-24.patch queued for re-testing.

Just a start. There is more to do. Basically, I directly copied header.

With zero knowledge of this, "- $ts = array();" should not be removed or it would probably trigger PHP notices latter (without looking at the code). All references to table sort should probably be removed from the footer section imho, unless the intent is to allow footer based sorting.

Fixed a missing bracket.

Very quick review:

1. +++ b/core/includes/theme.inc
   @@ -1549,7 +1549,6 @@ function template_preprocess_table(&$variables) {
   -  $ts = array();
   

   Agreed. Do not remove this.

2. +++ b/core/includes/theme.inc
   @@ -1598,6 +1597,55 @@ function template_preprocess_table(&$variables) {
   +    $ts = tablesort_init($variables['footer']);
   ...
   +        tablesort_footer($cell_content, $cell, $variables['footer'], $ts);
   

   This function doesn't even exist and is not needed.

3. +++ b/core/modules/system/templates/table.html.twig
   @@ -78,4 +78,16 @@
   +
   

   Unnecessary space.

4. +++ b/core/modules/system/templates/table.html.twig
   @@ -78,4 +78,16 @@
   +  {% if header %}
   ...
   +        {% for cell in header %}
   

   Should be footer.

Adding back in the footer parameter documentation. Leaving out the example: doesn't seem like the right place for it.

1. +++ b/core/includes/theme.inc
   +++ b/core/includes/theme.inc
   @@ -1460,6 +1460,12 @@ function drupal_pre_render_table(array $element) {
   
   @@ -1460,6 +1460,12 @@ function drupal_pre_render_table(array $element) {
   + *   - footer: An array containing the table footers. Each element of the array
   + *     can be either a localized string or an associative array with the
   + *     following keys:
   + *     - "data": The localized summary information of the table column.
   + *     - Any HTML attributes, such as "colspan", to apply to the column footer
   + *       cell.
   

   The template itself should also have documentation for footer, not just this PHP function.

2. +++ b/core/includes/theme.inc
   @@ -1549,7 +1555,7 @@ function template_preprocess_table(&$variables) {
   -  $ts = array();
   +  $ts =array();
   

   This chunk shouldn't exist (missing space).

3. +++ b/core/modules/system/templates/table.html.twig
   @@ -64,7 +62,17 @@
   +  {% if footer %}
   ...
      {% if rows %}
        <tbody>
   

   tfoot should come after <tbody>.

I will update the template.

I thought we wanted tfoot before tbody according to comment #4.

Minor updates. Changed to hardcoded td tags in the footer, since a footer won't have th tags.

That is the old HTML 4.01 spec. From HTML5 (which can be before or after <tbody> now):

As a child of a table element, after any caption, colgroup, and thead elements and before any tbody and tr elements, but only if there are no other tfoot elements that are children of the table element.
As a child of a table element, after any caption, colgroup, thead, tbody, and tr elements, but only if there are no other tfoot elements that are children of the table element.

Also, please don't hard code the tags. Just use the same format as the header.

+++ b/core/modules/system/templates/table.html.twig
@@ -17,6 +17,10 @@
  *   - content: A localized string for the title of the column.
  *   - field: Field name (required for column sorting).
  *   - sort: Default sort order for this column ("asc" or "desc").
+ * - header: Table header cells. Each cell contains the following properties:
+ *   - attributes: HTML attributes to apply to the tag.
+ *   - content: A localized string for the title of the column.
+ *   - field: Field name (required for column sorting).
  * - sticky: A flag indicating whether to use a "sticky" table header.
  * - rows: Table rows. Each row contains the following properties:
  *   - attributes: HTML attributes to apply to the <tr> tag.

• This needs to be properly indented and changed to footer.
• It doesn't have "field" since there's no sorting on footers.
• Should probably also be moved down below rows since that is where the markup should be located.

Implemented the above review.

1. +++ b/core/includes/theme.inc
   @@ -1598,6 +1605,49 @@ function template_preprocess_table(&$variables) {
   +        $is_header = isset($cell['header']) ? $cell['header'] : TRUE;
   

   We should probably default to FALSE here. You'll see the test fail now because it's outputting th by default. Speaking of tests, we should probably add a test for this "header" configuration too.

2. +++ b/core/modules/system/templates/table.html.twig
   @@ -28,6 +28,11 @@
     *     - attributes: Any HTML attributes, such as "colspan", to apply to the
     *       table cell.
     *     - content: The string to display in the table cell.
   + * - footer: Table footer cells. Each cell contains the following properties:
   + *   - tag: The HTML tag name to use; either TH or TD.
   + *   - attributes: HTML attributes to apply to the tag.
   + *   - content: A localized string for the title of the column.
   + *   - field: Field name (required for column sorting).
     * - empty: The message to display in an extra row if table does not have
     *   any rows.
   

   Again, needs proper indenting and no field.

I'm not seeing the indentation problem.

Ah, nm about the indention. The context lines before made it look a bit off.

+++ b/core/modules/system/templates/table.html.twig
@@ -28,6 +28,11 @@
+ *   - field: Field name (required for column sorting).

Remove this.

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TableTest.php
@@ -87,7 +87,10 @@ function testThemeTableWithEmptyMessage() {
     $footer = array(
-      'Footer 1',
+      array(
+        'data' => 'Footer 1',
+        'header' => TRUE,
+      ),
       array(

By changing this to an array, we've lost the test case that a simple string works. I would add a simple string: 'Footer 3' at the bottom.

Changes from #53.

Awesome! This looks good to me. I'd like to get some manual testing with some screenshots in before I RTBC it. Thanks @lokapujya for all your work! This went by quickly. I'm also going to see if I can't get some others to review.

1. +++ b/core/includes/theme.inc
   @@ -1598,6 +1605,50 @@ function template_preprocess_table(&$variables) {
   +  if (!empty($variables['footer'])) {
   

   Given Mark's reference to the HTML5 spec above, the proposed implementation does not actually respect the standard.

   In case we intentionally do not support the full standard, we not only need to document that we do not, we also need to document why we do not.

   2) All of the code that follows this condition has been copied from the 'rows' code path → duplicate code.

   Pending a clarification on why we don't support the full HTML5 standard, at minimum we should look into ways to de-duplicate the code in this function.

2. +++ b/core/includes/theme.inc
   @@ -1598,6 +1605,50 @@ function template_preprocess_table(&$variables) {
   +        // Track responsive classes for each column as needed. Only the footer
   +        // cells for a column are marked up with the responsive classes by a
   +        // module developer or themer. The responsive classes on the footer cells
   +        // must be transferred to the content cells.
   +        if (!empty($cell['class']) && is_array($cell['class'])) {
   +          if (in_array(RESPONSIVE_PRIORITY_MEDIUM, $cell['class'])) {
   +            $responsive_classes[$col_key] = RESPONSIVE_PRIORITY_MEDIUM;
   +          }
   

   Actually, I was wrong:

   The code was copied from 'header', not 'rows'.

   In particular the quoted lines here are reversed — instead of consuming the data from the table header columns, they are re-setting the responsive classes for each column (without applying the expected classes to the table footer columns).

   → Most likely the best possible demonstration for why code duplication is bad. ;-)

3. +++ b/core/includes/theme.inc
   @@ -2553,7 +2604,7 @@ function drupal_common_theme() {
   -      'variables' => array('header' => NULL, 'rows' => NULL, 'attributes' => array(), 'caption' => NULL, 'colgroups' => array(), 'sticky' => FALSE, 'responsive' => TRUE, 'empty' => ''),
   +      'variables' => array('header' => NULL, 'footer' => NULL, 'rows' => NULL, 'attributes' => array(), 'caption' => NULL, 'colgroups' => array(), 'sticky' => FALSE, 'responsive' => TRUE, 'empty' => ''),
   

   Similar to Mark's documentation remarks, let's also place the new 'footer' variable after 'rows' here.

4. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TableTest.php
   @@ -86,15 +86,28 @@ function testThemeTableWithEmptyMessage() {
   +    $footer = array(
   

   This test class is a "semi-unit" test — the test method you've added to is named testThemeTableWithEmptyMessage()

   If you want to cover the case of an empty table with header + footer, that's fine — but you're additionally changing the table column layout here: the footer contains one more column than the header + the empty row.

   Varying amount of columns in header + rows + footer is a separate test.

   In any case, we need one (or more) new test methods in this class to cover the actual expectations for table footers.

5. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TableTest.php
   @@ -86,15 +86,28 @@ function testThemeTableWithEmptyMessage() {
   +    $this->assertRaw('<tfoot><tr><th>Footer 1</th><td colspan="2">Footer 2</td><td>Footer 3</td></tr>', 'Table footer found.');
        $this->assertRaw('<tr class="odd"><td colspan="3" class="empty message">Empty row.</td>', 'Colspan on #empty row found.');
   

   Speaking of expectations...

   Is it sane to expect a table footer to appear where no table rows are?

   Outputting the header makes (some leve of) sense to me, but why the footer?

   We need to cover all permutations/expectations in the test, and next to comments in the functional code, the test is also a good place to document why we do expect what we expect. :)

6. +++ b/core/modules/system/templates/table.html.twig
   @@ -28,6 +28,10 @@
   + * - footer: Table footer cells. Each cell contains the following properties:
   + *   - tag: The HTML tag name to use; either TH or TD.
   

   Can TFOOT contain a TH?

7. Lastly, why are we doing this? Can we think of use-case for a table footer in core + actually use this feature?

   In case we cannot, what's the use-case? :)

2. Removed Duplication.
4. Partially implemented: Separated the unit test.
6. Yes, see core/assets/vendor/normalize-css/test.html for an example.

Alright.

I just attached the interdiff between #54 and #57 just in case.

Also, i think tfoot can contain a TH
accord with the definition:

The tfoot element represents the block of rows that consist of the column summaries (footers) for the parent table element, if the tfoot element has a parent and it is a table.

(http://www.w3.org/TR/html5/tabular-data.html#the-tfoot-element)

there is not any clarification about if there is any special restriction related with what kind of rows can contain the tfoot tag

+1 to @gnuget

There is another proof for that in https://developer.mozilla.org/en-US/docs/Web/HTML/Element/tfoot.

Usage note: Do not use this attribute, as it is non-standard and only implemented some versions of Microsoft Internet Explorer: the <tfoot> element should be styled using CSS. To give a similar effect to the bgcolor attribute, use the CSS property background-color, on the relevant <td> or <th> elements.

Here are my responses/questions to @sun's review in #56:

1. This is vague. What isn't met exactly?
2. Agreed. I didn't look at that portion too closely, as evident by it's copied nature. I disagree that we should go the route of the above patch and "merge" the code. Headers have extra logic to handle/display sorting, footers do not.
3. Agreed and great catch. This still wasn't done though.
4. Originally, I didn't think it was that big a deal, but in retrospect we really should create a completely separate test for footers. They really have nothing to do with existing tests or implementations. We also run the risk of breaking the existing tests while implementing new functionality and thus potentially creating new bugs. This also wasn't done.
5. Because footers are often used for things like "Totals" of the rows above. I have had some clients ask to leave the totals visible (even with empty rows) and others to hide it. Regardless, the tables #empty doesn't have anything to do with headers or footers, it simply adds empty text when #rows is... empty. Implementation logic must be left to code actually constructing the render array (ie: add/don't add footer elements if rows are empty, etc).
6. Yes. Regardless of whether it is rare or not, and technically speaking, the <th> element is only a child of <tr>. It has no context or relationship to <thead>, <tbody>, or <tfoot>. http://www.w3.org/TR/html5/tabular-data.html#the-th-element. FWIW, I have used header cells in the footer before, granted it's extremely rare, but it does happen (especially when dealing with complex tables/data).

   Also, should we really be asking "where do we use it in core?". This element has existed since HTML 4.01. I'd like to stop asking these types of questions when it's simply just part of the table spec. Core may not be using it, but clients do and contrib could. Instead, we should be following the spec and providing a native way for anyone to implement this in Drupal. As stated above, this element has more semantic use for data based tables (totaling). It also prevents class-itis on body rows to determine which is a "footer" element.

I will work on this.

Ok.

I've been working on this but i have some questions/thoughts.

While i was working on this patch, i read again the HTML5 spec and i found something to we miss in the previous patches.

Accord with the HTML5 spec: tbody and tfoot have the same structure:
http://www.w3.org/TR/html5/tabular-data.html#the-tbody-element
http://www.w3.org/TR/html5/tabular-data.html#the-tfoot-element

That means to tfoot element actually can have more than one row just like tbody.

So... instead to put the logic of the footer apart, i did a little change in the rows because accord with the spec tbody and tfoot work in the same way.

But while there is not an important change in the code, this changes the way we want to test the footer and the way how we will use it.

So, if we really want to follow the spec, i think this is the way the footer should work.

If you are agree with this new approach i have some questions:

• The documentation should almost repeat all the same documentation as #rows? or we can just say something like: "The footer works in a similar way as rows"?
• One odd thing is: now the rows into the tfoot have the classes odd/even, while exists the option no_striping i think the value of no_striping in the footer should be TRUE by default. What do you think?
• With this new approach still we need to do a new set of tests for the footer? i think these tests will be pretty similar with the rows tests.

I attached the patch, i didn't add an interdiff because i didn't start from #57 because with this new approach i think a lot of things could change.

I will continue working on this, i just want a little feedback.

Regarding #62, seems to me that we should just have $variables['rows'] and $variables['footer_rows'] and they effectively work the same.

Seems to me that we can just leave $no_striping as FALSE for consistency's sake with tbody rows, and that the odd/even count should persist across both -- people can override this in CSS if they please.

Test-wise, having something identical for the rows test that just makes sure $footer_rows appears in a <footer> seems fine to me.

Please for the love of all things lets just keep it $variables['footer'], not $variables['footer_rows'].

I'm not at my computer right now, but I'll take a look at #62 tomorrow.

Re: $footer vs $footer_rows, seems fine, though seems like behavior should be relatively identical to $rows.

I think I changed my mind about multiple rows :) I checked out the spec for thead and it can also take multiple rows.

Given that we've built $header to only support one row, it seems like keeping $footer to one row is also okay.

Personally I haven't really used tfoot much in my web career.

1. +++ b/core/includes/theme.inc
   @@ -1601,72 +1601,76 @@ function template_preprocess_table(&$variables) {
   +  $table_parts = array('rows' , 'footer');
   +  foreach ($table_parts as $table_part) {
   

   I'd rather just use $sections and $section. I don't think it's necessary to prefix with "table" as we're already in the table's preprocess function and know what context we're in. This is really just a personal preference and very trivial though ;)

2. +++ b/core/includes/theme.inc
   @@ -1601,72 +1601,76 @@ function template_preprocess_table(&$variables) {
   +      $flip = array('even' => 'odd', 'odd' => 'even');
   +      $class = 'even';
   ...
   +          $no_striping = isset($row['no_striping']) ? $row['no_striping'] : FALSE;
   

   We shouldn't be adding any striping anyway since #1649780: Remove first/last/odd/even classes in favor of CSS3 pseudo selectors landed. This should be a separate issue though, for now use:
   $no_striping = isset($row['no_striping']) ? $row['no_striping'] : ($table_part === 'rows' ? FALSE : TRUE);

Re: $footer vs $footer_rows, seems fine, though seems like behavior should be relatively identical to $rows.

footer is simpler and cleaner. Yes, it should behave identically to rows.

I think I changed my mind about multiple rows :) I checked out the spec for thead and it can also take multiple rows.

Yes, thead can also technically contain multiple rows, however changing this to be spec would require much more work and should be a separate issue as well.

Given that we've built $header to only support one row, it seems like keeping $footer to one row is also okay.

header already exists and was constructed based on simple use, not per spec. We're adding in a new variable here to complete the table spec, we shouldn't constrict our addition with legacy cruft.

Personally I haven't really used tfoot much in my web career.

Understood. It's a rare element for sure, but it is part of the spec. That is kind of the whole point of this issue is to flush out the table theme hook and be more in-line with the spec instead of assuming what people need. Not many will use it, but the last client I worked for needed it extensively (and I also utilized multiple tfoot rows). Having tfoot can be extremely helpful when dealing with tons of tabular data and constructing interactive tables that utilize lots of JS/AJAX.

This is why I put #2224487: Support "footer" in tables in Bootstrap. Themes shouldn't have to do this, it should just be provided by core naturally.

I attached a patch with the suggested changes of #67

Also i added a test for check the footer and the documentation based in $rows.

Reroll because of this #2247991: [May 27] Move all module code from …/lib/Drupal/… to …/src/… for PSR-4.

I just remove an extra space

I did a little modification into the test.

The idea is make sure the order of the rows is respected in the footer.

1. +++ b/core/includes/theme.inc
   @@ -1463,6 +1463,32 @@ function drupal_pre_render_table(array $element) {
   + *     - "data": an array of cells
   

   Needs ..

2. +++ b/core/includes/theme.inc
   @@ -1463,6 +1463,32 @@ function drupal_pre_render_table(array $element) {
   + *     - "no_striping": a boolean indicating that the row should receive no
   

   Minor nitpick. Really shouldn't have quotes around no_striping.

3. +++ b/core/includes/theme.inc
   @@ -1463,6 +1463,32 @@ function drupal_pre_render_table(array $element) {
   + *         'data' => array('Cell 1', array('data' => 'Cell 2', 'colspan' => 2)), 'class' => array('funky')
   

   This should be expanded into multiple lines.

4. +++ b/core/includes/theme.inc
   @@ -1601,72 +1627,75 @@ function template_preprocess_table(&$variables) {
   +        $no_striping = ($section === 'rows') ? FALSE : TRUE;
   

   This can be simplified to:
   $no_striping = $section === 'footer'

Hi Mark.

Thanks for your review.

The documentation of footer comes from rows so, the problems in 1,2,3 are also presented in rows. Should i change that in the documentation of rows too?

About 4 i will change that.

We might as well since we're in there. They're just minor documentation/coding standards nitpicks. I don't think that really qualifies as scope creep.

no_striping and data don't need quotes around them as @Mark Carver mentioned. @gnuget looking good, thanks for the minor test change too to foo.

Ok.

Here a new version of the patch with the suggestions of #72

Thank you for your reviews guys!

This looks great! In retrospect, I would actually like to get @jhodgdon's feedback on the documentation bits. We are essentially duplicating and I wonder if there is a way to consolidate this so we only have to update it once and not in both sections.

I'll leave her a tell to review.

I think the docs look mostly great too!

My suggestion for the footer area would be something like this:

 * - footer: An array of table rows which will be printed within a <tfoot>
 *     tag, in the same format as the rows element (see above).

I agree that duplication is not desirable.

There are also a couple of minor formatting glitches:

b)

+ *     - sort: A default sort order for this column ("asc" or "desc"). Only
  *        one column should be given a default sort order because table sorting
  *        only applies to one column at a time.

Text in the 2nd and 3rd line should line up directly under the word sort (2 space indentation). It is indented an extra space. This is true of some, but not all, of the rest of the list.

c)

+ *     - data: an array of cells.
 

After : in a list item, the next word should be capitalized.

d)

 *     - no_striping: a boolean indicating that the row should receive no

Boolean should be capitalized. It is a proper noun.

e) In this code section:

 *         'data' => array(
+ *           'Cell 1', array(
+ *             'data' => 'Cell 2', 'colspan' => 2
+ *           ),
+ *         ),
+ *         'class' => array('funky')
+ *       ),
+ *     );
+ *     @endcode

You've gone to the trouble to partially split out the one-line array into multiple lines... but it's not fully split. Can we format it the way we would in actual code? I think it would be clearer?

f) Why are we repeating this documentation in the Twig file and in the preprocess function? That doesn't make any sense to me. I think it should just be in the Twig file, which is where most people would look for it. Let's just take it out of the preprocess function and make sure that function links to the twig template for docs on the variable input?

HI jhodgdon!

Thanks for your time reviewing this patch.

I fixed a,b,c,d.

In the e i just restore the code as it was before, which is the way i would do it, i hope that's is ok for you.

and for f i just want to be sure about what i have to do, because while the documentation is almost the same, is not exactly the same.

The documentation in the preprocess function is focused in the format what type => table expect the variables, the documentation in table.html.twig file is focused in all the available variables in that twig template.

If you are sure about this, should i remove the documentation from the preprocess and overwrite the documentation of the twig template and link the preprocess to the twig template?

I guess we can leave the duplicated documentation. Probably out of scope for this issue anyway.

Thanks for making the other clean-ups; the documentation looks good to me now. I'm not going to mark the patch RTBC though since I did not look at the code or tests.

I manually tested this and it works wonderfully. Here's a screenshot of the output (render array used at the bottom).

In hindsight, there could be some whitespace cleanup done, but it's very... very trivial:

<table>
  
  
      <thead>
      <tr>
                  <th>Header 1</th>
                  <th>Header 2</th>
                  <th>Header 3</th>
              </tr>
    </thead>
  
      <tbody>
              <tr class="row-class odd">
                      <td>Row Cell 1</td>
                      <td colspan="2">Row Cell 2</td>
                  </tr>
          </tbody>
        <tfoot>
              <tr class="footer-class">
                      <td>Footer Cell 1</td>
                      <td colspan="2">Footer Cell 2</td>
                  </tr>
          </tfoot>
  </table>

    return array(
      '#theme' => 'table',
      '#header' => array(
        'Header 1',
        'Header 2',
        'Header 3',
      ),
      '#rows' => array(
        array(
          'class' => array('row-class'),
          'data' => array(
            'Row Cell 1',
            array(
              'data' => 'Row Cell 2',
              'colspan' => 2,
            ),
          ),
        ),
      ),
      '#footer' => array(
        array(
          'class' => array('footer-class'),
          'data' => array(
            'Footer Cell 1',
            array(
              'data' => 'Footer Cell 2',
              'colspan' => 2,
            ),
          ),
        ),
      ),
    );

Committed 580fd45 and pushed to 8.x. Thanks!

This needs to be backported to 7.x (not sure why there wasn't a label).

Created a draft change notice at https://drupal.org/node/2282663

It will need a review before it can be published.

Critical? regression. Responsive tables are no longer responsive.

Since we don't have any simple tests that check that the responsive table classes get applied correctly, this was bound to happen eventually. :-p

As you can see from the screenshot only the header is properly hiding columns on small and medium sized screens. The table data is not hidden and causes a horizontal scrollbar.

Committed f3a0ffa and pushed to 8.x to revert.

The patch is not critical so lets rollback to fix.

Blocked on #2282683: Responsive tables do not have tests

@JohnAlbin, turns out this was actually happening before this patch and is still passes tests after applying the patch after #2282683: Responsive tables do not have tests got in.

It turns out that this is actually a regression from #1939008: Convert theme_table() to Twig.

I've created #2296859: [regression] Responsive classes not always added to table rows to address this (which this issue is now/still postponed on).

@Mark Carver, where in theme_table conversion did the regression happen? If I recall we moved the responsive stuff to type table preprocess outside of that conversion issue, if my memory serves right it was to do with the removal of theme() calls or drupal_add_js() removals.

I could be wrong but this could be your regression issue you are looking for? #2171071: Rename drupal_add_library() to _drupal_add_library() and remove its uses

From the patch in #1939008-133: Convert theme_table() to Twig. It was previously using an incremental $i variable as the index, instead it was changed to use the associative array key, which can differ greatly between some table headers/rows:

+++ b/core/includes/theme.inc
@@ -1495,83 +1497,66 @@ function drupal_pre_render_table(array $element) {
-    $ts = tablesort_init($header);
-    // HTML requires that the thead tag has tr tags in it followed by tbody
-    // tags. Using ternary operator to check and see if we have any rows.
-    $output .= (count($rows) ? ' <thead><tr>' : ' <tr>');
-    $i = 0;
-    foreach ($header as $cell) {
-      $i++;
+  $ts = array();
+  if (!empty($variables['header'])) {
+    $ts = tablesort_init($variables['header']);
+
+    foreach ($variables['header'] as $col_key => $cell) {

@@ -1590,10 +1575,10 @@ function theme_table($variables) {
         // must be transferred to the content cells.
         if (!empty($cell['class']) && is_array($cell['class'])) {
           if (in_array(RESPONSIVE_PRIORITY_MEDIUM, $cell['class'])) {
-            $responsive[$i] = RESPONSIVE_PRIORITY_MEDIUM;
+            $responsive_classes[$col_key] = RESPONSIVE_PRIORITY_MEDIUM;
           }
           elseif (in_array(RESPONSIVE_PRIORITY_LOW, $cell['class'])) {
-            $responsive[$i] = RESPONSIVE_PRIORITY_LOW;
+            $responsive_classes[$col_key] = RESPONSIVE_PRIORITY_LOW;
           }
         }

@@ -1601,54 +1586,50 @@ function theme_table($variables) {
-        $i = 0;
-        foreach ($cells as $cell) {
-          $i++;
...
+        foreach ($cells as $col_key => $cell) {

@@ -1671,26 +1652,22 @@ function theme_table($variables) {
-          if (isset($responsive[$i])) {
-            $cell_attributes['class'][] = $responsive[$i];
+          if (isset($responsive_classes[$col_key])) {
+            $cell_attributes['class'][] = $responsive_classes[$col_key];

Well I can see where you would see that, though I still fail to see where those keys won't match... I'm probably missing something still, sorry.

Now that #2296859: [regression] Responsive classes not always added to table rows is in, this will need a reroll.

A updated version attached.

Setting this back to RTBC since the regression wasn't caused by this issue.

Committed 1c7cf17 and pushed to 8.x. Thanks!

Hey all,

Thanks for the work on this.

Hate to post on a Fixed issue, but I just (successfully) used the patch from #1 of #1892654: D7 Backport: theme_table() should take an optional footer variable and produce <tfoot> for D7 (came from #23 here). I noticed multiple mentions and tags to backport to D7 after committing to 8.x. Are there plans to address that? Is this the place to bring that up? FWIW, the linked backport issue was closed as a dupe in favor of this one.

Thanks again,
Gus

Yep, good catch! Normally we just re-open this issue and change the version when we see a "needs backport to D7" tag. Unfortunately, there was never such a tag on this issue and it was forgotten. I would love to help move this forwards, however I do not have time to write the conversion myself. If someone wants to start working on the 7.x version of this patch I, would be happy to review and get it to an RTBC state.

Attached is a backport to D7.

It follows the changes made in the 8.x commit (1c7cf17) rather than the previous 7.x approach all the way back from #23.

Could you review this, Mark Carver? Thanks!

From first glance it looks ok. I really haven't reviewed it manually yet though. The test should also be backported: http://cgit.drupalcode.org/drupal/tree/modules/simpletest/tests/theme.te...

While this is not fixed yet in D7 core one can use FooBar Table module to render table with footer.

Adding the test.

retest

Let try to get this thing done.

Retriggering the tests.

Re-uploading patch from #105 because I'm unable to trigger tests on that one.

Is this issue meant to supersede https://www.drupal.org/node/1892654?

If so, maybe 1892654 should be closed and we point people to this one, since that issue seems dormant.

:facepalm:

Here's an updated patch with fixed tests.

Updating patch to solve last failing test.

Any update on this ?

There already is a D7 backport issue open for this.
#1892654: D7 Backport: theme_table() should take an optional footer variable and produce <tfoot>
Let's copy the Pol patch from here to there and mark this issue as fixed.