Problem/Motivation

Somehow the <tfoot> element of a table never got into core. While not commonly used, it still is a necessary component on some sites. Themes have to add this functionality when it's optional use should be provided by core.

Proposed resolution

Add optional #footer variable in theme hook and corresponding <tfoot> markup in table template.

Remaining tasks

None

User interface changes

None

API changes

None

Original report by @sreynen

Files: 

Comments

sreynen’s picture

Status: Active » Needs review
FileSize
991 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch tfoot.patch. View

This patch is against HEAD, which is currently D7. I hope that's the right way to do this.

The new tfoot code is basically just a copy of the existing thead code. Ideally both thead and tfoot would take rows rather than cells, as both can contain multiple rows in HTML, but that's probably a separate issue. I also took out the check if body rows exist. I'm not really sure why that's in thead (shouldn't we be semantically identifying the table head whether or not there are body rows?), but it seems to be thead-specific in any case.

Jaypan’s picture

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

Status: Needs review » Needs work

The last submitted patch, tfoot.patch, failed testing.

bfroehle’s picture

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:

sreynen’s picture

Well shoot, I've been doing tfoot after tbody all these years.

minorOffense’s picture

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

+    $output .= ' ');

That trailing bracket shouldn't be there.

kathyh’s picture

Status: Needs work » Needs review
FileSize
859 bytes
FAILED: [[SimpleTest]]: [MySQL] 33,661 pass(es), 1 fail(s), and 1,778 exception(es). View

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

kathyh’s picture

Status: Needs review » Needs work

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'];

claudiu.cristea’s picture

Issue tags: +Needs Documentation

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

bfroehle’s picture

FileSize
2.98 KB
PASSED: [[SimpleTest]]: [MySQL] 34,002 pass(es). View

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.

bfroehle’s picture

Status: Needs work » Needs review
zhangtaihao’s picture

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?

bfroehle’s picture

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

miqmago’s picture

Version: 8.x-dev » 7.x-dev

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

miqmago’s picture

FileSize
2.96 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch theme_table_tfoot-806982-14.patch. Unable to apply patch. See the log in the details link for more information. View

Atthached the patch corrected changing $header => $footer

Status: Needs review » Needs work

The last submitted patch, theme_table_tfoot-806982-14.patch, failed testing.

mitchell’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs work » Needs review

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

miqmago’s picture

FileSize
2.42 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 806982-d7-core-theme_table-tfoot-18.patch. Unable to apply patch. See the log in the details link for more information. View

Ok thanks,

Attached patch for D7.18

Status: Needs review » Needs work

The last submitted patch, 806982-d7-core-theme_table-tfoot-18.patch, failed testing.

miqmago’s picture

FileSize
2.4 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 806982-d7-core-theme_table-tfoot-20.patch. Unable to apply patch. See the log in the details link for more information. View

Is there any pre-test tool?

bfroehle’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 806982-d7-core-theme_table-tfoot-20.patch, failed testing.

willvincent’s picture

floydm’s picture

Status: Needs work » Needs review
FileSize
2.58 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch theme_table_tfoot-806982-24.patch. Unable to apply patch. See the log in the details link for more information. View

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

zhangtaihao’s picture

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

jimmyko’s picture

Issue tags: -Needs Documentation

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

claudiu.cristea’s picture

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

Status: Needs review » Needs work

The last submitted patch, theme_table_tfoot-806982-24.patch, failed testing.

shashikant_chauhan’s picture

Issue summary: View changes
FileSize
3.53 KB
FAILED: [[SimpleTest]]: [MySQL] 59,708 pass(es), 2 fail(s), and 1 exception(s). View
shashikant_chauhan’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 29: theme_table_tfoot-806982-29.patch, failed testing.

markcarver’s picture

Title: theme_table() should take an optional footer variable and produce <tfoot> » Tables should take an optional footer variable and produce <tfoot>
Category: Feature request » Task
Priority: Minor » Normal
Issue tags: +Twig, +theme system cleanup, +sprint, +dreammarkup, +html5, +focus, +Needs reroll, +Needs issue summary update
lokapujya’s picture

Status: Needs work » Needs review
FileSize
3.65 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/includes/theme.inc. View

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

Status: Needs review » Needs work

The last submitted patch, 33: 806982-33.patch, failed testing.

Alan D.’s picture

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.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
3.59 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,510 pass(es). View

Fixed a missing bracket.

markcarver’s picture

Status: Needs review » Needs work

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.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
4.8 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,581 pass(es). View
3.97 KB
lokapujya’s picture

Issue tags: -Needs reroll
FileSize
835 bytes
5.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,510 pass(es). View

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

markcarver’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Needs issue summary update
  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>.

markcarver’s picture

Issue tags: +Needs backport to 7.x
lokapujya’s picture

Assigned: Unassigned » lokapujya

I will update the template.

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

lokapujya’s picture

Assigned: lokapujya » Unassigned
Status: Needs work » Needs review
FileSize
2.23 KB
5.84 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

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

Status: Needs review » Needs work

The last submitted patch, 43: 806982-43.patch, failed testing.

markcarver’s picture

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.

markcarver’s picture

+++ 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.
lokapujya’s picture

Status: Needs work » Needs review
FileSize
6.01 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,675 pass(es), 1 fail(s), and 0 exception(s). View
3.74 KB

Implemented the above review.

markcarver’s picture

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

lokapujya’s picture

I'm not seeing the indentation problem.

markcarver’s picture

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

The last submitted patch, 47: 806982-47.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
6.1 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,663 pass(es). View
2.45 KB
markcarver’s picture

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

lokapujya’s picture

Status: Needs work » Needs review
FileSize
1.68 KB
6.08 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,385 pass(es). View

Changes from #53.

markcarver’s picture

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.

sun’s picture

Issue summary: View changes
  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? :)

lokapujya’s picture

FileSize
11.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,673 pass(es). View

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

gnuget’s picture

Issue tags: -
FileSize
10.3 KB

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

jimmyko’s picture

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

markcarver’s picture

Status: Needs review » Needs work

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.

gnuget’s picture

Assigned: Unassigned » gnuget

I will work on this.

gnuget’s picture

Status: Needs work » Needs review
FileSize
7.8 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,182 pass(es). View

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.

c4rl’s picture

Status: Needs review » Needs work

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.

markcarver’s picture

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.

c4rl’s picture

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

c4rl’s picture

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.

markcarver’s picture

  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.

gnuget’s picture

Status: Needs work » Needs review
FileSize
10.11 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,770 pass(es). View
5.53 KB

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.

gnuget’s picture

FileSize
10.05 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,674 pass(es). View
gnuget’s picture

FileSize
10.05 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,670 pass(es). View
649 bytes

I just remove an extra space

gnuget’s picture

FileSize
10.06 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,630 pass(es). View
948 bytes

I did a little modification into the test.

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

markcarver’s picture

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

gnuget’s picture

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.

markcarver’s picture

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.

joelpittet’s picture

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.

gnuget’s picture

Status: Needs work » Needs review
FileSize
12.59 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,718 pass(es). View
5 KB

Ok.

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

Thank you for your reviews guys!

markcarver’s picture

Issue tags: +documentation

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.

jhodgdon’s picture

Status: Needs review » Needs work

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?

gnuget’s picture

Status: Needs work » Needs review
FileSize
11.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,855 pass(es). View
5.67 KB

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?

jhodgdon’s picture

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.

markcarver’s picture

Assigned: gnuget » Unassigned
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing, -Needs screenshots
FileSize
13.62 KB

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,
            ),
          ),
        ),
      ),
    );
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

  • Commit 580fd45 on 8.x by alexpott:
    Issue #806982 by lokapujya, gnuget, miqmago, shashi1028, Floydm,...
markcarver’s picture

Version: 8.x-dev » 7.x-dev
Status: Fixed » Active

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

markcarver’s picture

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

It will need a review before it can be published.

JohnAlbin’s picture

Version: 7.x-dev » 8.x-dev
Status: Active » Needs work
FileSize
38.13 KB

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

Screenshot of bug

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.

  • Commit f3a0ffa on 8.x by alexpott:
    Revert "Issue #806982 by lokapujya, gnuget, miqmago, shashi1028, Floydm...
alexpott’s picture

Committed f3a0ffa and pushed to 8.x to revert.

The patch is not critical so lets rollback to fix.

markcarver’s picture

Status: Needs work » Postponed
Related issues: +#2282683: Responsive tables do not have tests

Mark Carver queued 79: 806982-79.patch for re-testing.

markcarver’s picture

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

joelpittet’s picture

@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

markcarver’s picture

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];
joelpittet’s picture

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.

markcarver’s picture

Status: Postponed » Needs work
Issue tags: +Needs reroll
gnuget’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
12.25 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,289 pass(es). View

A updated version attached.

markcarver’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

  • alexpott committed 1c7cf17 on 8.x
    Issue #806982 by lokapujya, gnuget, miqmago, shashi1028, Floydm,...
guschilds’s picture

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

markcarver’s picture

Version: 8.x-dev » 7.x-dev
Status: Fixed » Active
Issue tags: -Twig, -sprint, -html5, -focus

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.

guschilds’s picture

Status: Active » Needs review
FileSize
6.01 KB

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!

markcarver’s picture

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

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

potop’s picture

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

lokapujya’s picture

Status: Needs work » Needs review
FileSize
6.93 KB
FAILED: [[SimpleTest]]: [MySQL] 41,093 pass(es), 15 fail(s), and 0 exception(s). View
944 bytes

Adding the test.

Status: Needs review » Needs work

The last submitted patch, 105: 806982-105.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review

retest

The last submitted patch, 102: 806982-102-7.x-do-not-test.patch, failed testing.

  • alexpott committed 580fd45 on 8.3.x
    Issue #806982 by lokapujya, gnuget, miqmago, shashi1028, Floydm,...
  • alexpott committed f3a0ffa on 8.3.x
    Revert "Issue #806982 by lokapujya, gnuget, miqmago, shashi1028, Floydm...
  • alexpott committed 1c7cf17 on 8.3.x
    Issue #806982 by lokapujya, gnuget, miqmago, shashi1028, Floydm,...

  • alexpott committed 580fd45 on 8.3.x
    Issue #806982 by lokapujya, gnuget, miqmago, shashi1028, Floydm,...
  • alexpott committed f3a0ffa on 8.3.x
    Revert "Issue #806982 by lokapujya, gnuget, miqmago, shashi1028, Floydm...
  • alexpott committed 1c7cf17 on 8.3.x
    Issue #806982 by lokapujya, gnuget, miqmago, shashi1028, Floydm,...

  • alexpott committed 580fd45 on 8.4.x
    Issue #806982 by lokapujya, gnuget, miqmago, shashi1028, Floydm,...
  • alexpott committed f3a0ffa on 8.4.x
    Revert "Issue #806982 by lokapujya, gnuget, miqmago, shashi1028, Floydm...
  • alexpott committed 1c7cf17 on 8.4.x
    Issue #806982 by lokapujya, gnuget, miqmago, shashi1028, Floydm,...

  • alexpott committed 580fd45 on 8.4.x
    Issue #806982 by lokapujya, gnuget, miqmago, shashi1028, Floydm,...
  • alexpott committed f3a0ffa on 8.4.x
    Revert "Issue #806982 by lokapujya, gnuget, miqmago, shashi1028, Floydm...
  • alexpott committed 1c7cf17 on 8.4.x
    Issue #806982 by lokapujya, gnuget, miqmago, shashi1028, Floydm,...
Pol’s picture

Let try to get this thing done.

Retriggering the tests.

Pol’s picture

FileSize
6.93 KB

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

The last submitted patch, 102: 806982-102-7.x-do-not-test.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 114: 806982-105.patch, failed testing.

apotek’s picture

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:

Pol’s picture

FileSize
7.31 KB

Here's an updated patch with fixed tests.

Pol’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 118: 806982.patch, failed testing.

Pol’s picture

Status: Needs work » Needs review
FileSize
6.05 KB

Updating patch to solve last failing test.