Responsive tables do not have tests

Here's a few tests to check if responsive table classes are being applied correctly.

Here's a revised patch with the interdiff.

Added the additional test.

Tried a different approach to check how output is handled when responsive tables are not enabled.

Hi mdrummond.

You have done an awesome job with these tests, thank you.

Right now, the: #responsive => TRUE only add the class 'responsive-enabled' in the table tag and the core/drupal.tableresponsive library.

// If the table has headers and it should react responsively to columns hidden
// with the classes represented by the constants RESPONSIVE_PRIORITY_MEDIUM
// and RESPONSIVE_PRIORITY_LOW, add the tableresponsive behaviors.
if (count($element['#header']) && $element['#responsive']) {
    $element['#attached']['library'][] = 'core/drupal.tableresponsive';
    // Add 'responsive-enabled' class to the table to identify it for JS.
    // This is needed to target tables constructed by this function.
    $element['#attributes']['class'][] = 'responsive-enabled';
}

Which i think it is totally ok, so in the tests the #responsive => FALSE only should check if theresponsive-enabled class and the library are added, it is not necessary to make sure the priority-low and priority-medium classes are there because these are a common CSS classes, and the user must be able to add those classes even if not want a responsible table.

The problem here is in the themes we have something like this:

/**
 * Responsive tables.
 */
@media screen and (max-width: 37.5em) { /* 600px */
  th.priority-low,
  td.priority-low,
  th.priority-medium,
  td.priority-medium {
    display: none;
  }
}
@media screen and (max-width: 60em) { /* 920px */
  th.priority-low,
  td.priority-low {
    display: none;
  }
}

it doesn't matter if we have #responsive => FALSE the priority-low and priority-medium classes will still be hiding in narrow devices.

I think we should change that for something like this:

/**
 * Responsive tables.
 */
@media screen and (max-width: 37.5em) { /* 600px */
  .responsive-enabled th.priority-low,
  .responsive-enabled td.priority-low,
  .responsive-enabled th.priority-medium,
  .responsive-enabled td.priority-medium {
    display: none;
  }
}
@media screen and (max-width: 60em) { /* 920px */
  .responsive-enabled th.priority-low,
  .responsive-enabled td.priority-low {
    display: none;
  }
}

But that is out of the scope of the issue.

For now, In my opinion is only necessary change how #responsive => FALSE is tested.

@gnuget I agree it should be a bit more specific so we aren't name collision. Maybe @LewisNyman can chime in on #10?

I think it just testing the .responsive-enabled and the library is enough for this issue.

I think the problem with the CSS classes is that it is not clear what the .priority-x classes do, so there is potential for unintentional overlap and false positives.

Our CSS standards have a way of dealing with this :) I would suggest:

.responsive-priority-low
.responsive-priority-medium

Removing the elements from the selectors would also be nice.

If we have a property that turns responsiveness on tables on and off, then it doesn't make sense to enable responsiveness unless that responsive-enabled class is present. The suggestion in #10, minus the elements, would maybe be good:

/**
 * Responsive tables.
 */
@media screen and (max-width: 37.5em) { /* 600px */
  .responsive-enabled .priority-low,
  .responsive-enabled .priority-low,
  .responsive-enabled .priority-medium,
  .responsive-enabled .priority-medium {
    display: none;
  }
}
@media screen and (max-width: 60em) { /* 920px */
  .responsive-enabled .priority-low,
  .responsive-enabled .priority-low {
    display: none;
  }
}

I'm fine if we change that to:

/**
 * Responsive tables.
 */
@media screen and (max-width: 37.5em) { /* 600px */
  .responsive-enabled .responsive-priority-low,
  .responsive-enabled .responsive-priority-low,
  .responsive-enabled .responsive-priority-medium,
  .responsive-enabled .responsive-priority-medium {
    display: none;
  }
}
@media screen and (max-width: 60em) { /* 920px */
  .responsive-enabled .responsive-priority-low,
  .responsive-enabled .responsive-priority-low {
    display: none;
  }
}

That's outside the scope of this issue, but could be done in a follow-up side issue.

I'm still not clear why the latest tests I wrote failed. Any insights?

Your tests fails because the classes are added even if the '#responsive' => FALSE, is set... this is because those are normal classes and aren't stripped when the #responsive is false.

So, here you only need to tests if the table has the responsive-enabled class, and that's it.

+++ b/core/modules/system/src/Tests/Theme/TableTest.php
@@ -138,6 +138,102 @@ function testThemeTableHeaderCellOption() {
+  public function testThemeTableResponsivePriorityNotEnabled() {
+    $header = array(
+      array('data' => 1, 'class' => array(RESPONSIVE_PRIORITY_MEDIUM)),
+      array('data' => 2, 'class' => array(RESPONSIVE_PRIORITY_LOW)),
+      array('data' => 3),
+    );
+    $rows = array(array(4, 5, 6));
+    $table = array(
+      '#type' => 'table',
+      '#header' => $header,
+      '#rows' => $rows,
+      '#responsive' => FALSE,
+    );
+    $this->render($table);
+    $this->assertRaw('<th>1</th>', 'Header 1: the priority-medium class was not applied.');
+    $this->assertRaw('<th>2</th>', 'Header 2: the priority-low class was not applied.');
+    $this->assertRaw('<th>3</th>', 'Header 3: no priority classes were applied.');
+    $this->assertRaw('<td>4</td>', 'Cell 1: the priority-medium class was not applied.');
+    $this->assertRaw('<td>5</td>', 'Cell 2: the priority-low class was not applied');
+    $this->assertRaw('<td>6</td>', 'Cell 3: no priority classes were applied.');
+  }

Tests are still failing :( See #14

Awesome work! I'll sleep better at night with this patch. :)

Committed and pushed to 8.x. Thanks!