Missing caption, if view (format table) is grouped by a field

.

Something like this adds the caption.

Still needs test coverage though.

Added test.

Fix and test looks go to me. Bit of nitpicking:

1. +++ b/core/modules/views/src/Tests/Plugin/StyleTableTest.php
   @@ -109,4 +109,36 @@ public function testFieldInColumns() {
   +    // Clear the caption text, the rendered job field will be used a caption.
   

   'will be used as a' (missing as)

2. +++ b/core/modules/views/src/Tests/Plugin/StyleTableTest.php
   @@ -109,4 +109,36 @@ public function testFieldInColumns() {
   +    // Ensure tht all expected captions are found.
   

   tht => that

Also needs a beta evaluation added.

Fixed nitpicks

+++ b/core/modules/views/src/Tests/Plugin/StyleTableTest.php
@@ -109,4 +109,36 @@ public function testFieldInColumns() {
+    $display['display_options']['style']['options']['caption'] = '';
+    $display['display_options']['style']['options']['summary'] = '';
+    $display['display_options']['style']['options']['description'] = '';

How does the views ui let you know that you're still going to get a caption?

Assigning to @dawehner to get an opinion.

First I tried to understand how this actually works in Drupal 7:

This is the table template code we talk about.

   <?php if (!empty($title) || !empty($caption)) : ?>
     <caption><?php print $caption . $title; ?></caption>
  <?php endif; ?>

So even if $caption is empty, it $title is set, we use it. This is set, when we group by a specific field, see http://cgit.drupalcode.org/views/tree/plugins/views_plugin_style.inc#n344

Alright, let's have a look at the situation in Drupal 8.

The grouping code itself looks still pretty much the same, see http://cgit.drupalcode.org/drupal/tree/core/modules/views/src/Plugin/vie...
I'm curious as it seems to be that the fix is not the right thing to do. The template still prints out the {{ title }} in case {{ caption_needed }},
but we simply set it up wrong. This is some code from HEAD:

  // Add the caption to the list if set.
  if (!empty($handler->options['caption'])) {
    $variables['caption'] = Xss::filterAdmin($handler->options['caption']);
    $variables['caption_needed'] = TRUE;
  }
  else {
    $variables['caption'] = '';
    $variables['caption_needed'] = FALSE;
  }

  $variables['caption_needed'] |= !empty($variables['summary']) || !empty($variables['description']);

so all we need is $variables['caption_needed'] = !empty($variables['summary']) || !empty($variables['description']) || !empty($variables['title']);.

So let's clarify things a bit. The option in the UI is for manual hand-crafted captions, which might be useful for additional accessibility on the site. In case you
group the view, the title is coming directly from the grouping. As additional accessibility features we support to add a summary and a description, but we don't take
into account {{ caption }} or {{ title }} so I guess #843708: Add option to set caption & remove summary in the html table (Accessibility) was simply not entirely correct.

1. +++ b/core/modules/views/views.theme.inc
   @@ -651,6 +651,10 @@ function template_preprocess_views_view_table(&$variables) {
   +    $variables['title'] = Xss::filterAdmin($variables['title']);
   

   Do we really need the escaping here? Don't we autoescape now? In case we have to, why can't we do that in the template itself? Is there something like Xss::filterAdmin() in twig?

2. +++ b/core/modules/views/views.theme.inc
   @@ -651,6 +651,10 @@ function template_preprocess_views_view_table(&$variables) {
   +    $variables['caption_needed'] = TRUE;
   

   So yeah as written above I think we better encapsulate this into the line coming below.

1. It seems to be already escaped, we do need to set it as SafeMarkup though.
2. Done

We could also use the raw filter in the template, not sure what's best.

No clue why the testbot keeps failing on this one. Seems unrelated to the patch.

It seems like all feedback from #16 is processed.
It looks like this is RTBC.

+++ b/core/modules/views/views.theme.inc
@@ -656,9 +656,13 @@ function template_preprocess_views_view_table(&$variables) {
+  if (!empty($variables['title'])) {
+    $variables['title'] = SafeMarkup::set($variables['title']);
+  }

We shouldn't be using SafeMarkup::set(). Why are we?

Changed SafeMarkup::set() to SafeMarkup::format() and added some extra test coverage.

Yup, missed that.
I changed Xss::filterAdmin() for SafeMarkup::escape()
I also changed the xpath select for a assertRaw because I had trouble finding the escaped markup with the xpath selector.

+++ b/core/modules/views/views.theme.inc
@@ -658,9 +658,13 @@ function template_preprocess_views_view_table(&$variables) {
+  if (!empty($variables['title'])) {
+    $variables['title'] = SafeMarkup::format($variables['title']);
+  }

This is wrong what is this trying to do?

+++ b/core/modules/views/src/Tests/Plugin/StyleTableTest.php
@@ -109,4 +111,56 @@ public function testFieldInColumns() {
+      $filtered_caption = SafeMarkup::escape($raw_caption);
+      $this->assertRaw($filtered_caption, 'Found caption containing "' . $filtered_caption . '"');

$this->assertEscaped() instead I think.

1. Just trying to avoid double escaping, changed this to a #markup array now
2. $this->assertEscaped() fixes that nicely indeed

bump

Overall looks good, but there's this in the test.

+  public function testGrouping() {
+    $view = \Drupal::entityManager()->getStorage('view')->load('test_table');
+    $display = &$view->getDisplay('default');
+    // Set job as the grouping field.

The test goes on to modify display and use the passage by reference to modify the view object without using the normal setters. This is unusual, and I'd discourage such in production code but sometimes tests have to do things in an unorthodox manner. Some commentary as to why a pass by reference here is being used instead of using the setters would be good I think.

Note: If $display is an ArrayObject then the pass by reference is redundant - objects are always passed by reference in PHP.

Had to reroll this.

+++ b/core/modules/views/views.theme.inc
@@ -658,9 +658,13 @@ function template_preprocess_views_view_table(&$variables) {
+    $variables['title'] = ['#markup' => $variables['title']];

This was also not working anymore due to the recent SafeMarkup changes I guess. I changed it to use ViewsRenderPipelineSafeString.

The test goes on to modify display and use the passage by reference to modify the view object without using the normal setters.

getDisplay() doesn't return an object there, it returns an array reference to the display configuration (so I need the &).
Including /** @var \Drupal\views\ViewEntityInterface $view */ already explains this better I think but I also added an extra comment to be clear on this matter.

Did some manual and automated tests.
Looks good to me.

+++ b/core/modules/views/views.theme.inc
@@ -660,9 +661,13 @@ function template_preprocess_views_view_table(&$variables) {
+  if (!empty($variables['title'])) {
+    $variables['title'] = ViewsRenderPipelineSafeString::create($variables['title']);
+  }

This does look right - how is $variables['title'] being set? This looks like it is user input and therefore this would be very unsafe.

After looking into this again I noticed $variables['title'] actually turned out to be a SafeString object so ViewsRenderPipelineSafeString::create() was not necessary there anymore.
While I was testing this I did bump into another issue in StylePluginBase::renderGrouping() which was using that same SafeString object as an array key which caused a fatal error.
I'm not completely sure why this wasn't picked up by our tests though.

For now, this patch fixes that but maybe we need to expand our test coverage there.

The fatal error was not appearing because the grouping field in our has a label.
StylePluginBase::renderGrouping() is doing this:

if ($this->view->field[$field]->options['label']) {
  $group_content = $this->view->field[$field]->options['label'] . ': ' . $group_content;
}

So the string cast is happening there.

The reason that the test is failing is because the grouping string is not marked as safe in combination with the label.

Setting this to Major since this is a Fatal Error that I can reproduce on HEAD

This should demonstrate the problem.
The complete patch should fail on the unsafe markup with a label.

Error from #46 is actually fixed in #2546924: Views fatal error when grouping on a field without a label set

Looked into this issue a bit more.

The reason the group label is being double escaped is because of the string casting happening in StylePluginBase::renderGrouping():

if ($this->view->field[$field]->options['label']) {
  $group_content = $this->view->field[$field]->options['label'] . ': ' . $group_content;
}

I would have thought the label would have been added in theme_views_view_field() but it's actually being added in theme_views_view_fields().

So either we need to move the caption label logic to a preprocess function or template, which I don't want to do since then we're just copying logic from theme_views_view_fields() or just mark the string safe somehow.

Manually tested this, and this is still an issue.

Since the fix for the fatal error was fixed in #2546924: Views fatal error when grouping on a field without a label set, we can take that bit out.
Also a lot of other safe markup stuff and theming has changed since this was last rolled. Setting the caption to markup with the escaped title in it does the trick to pass the tests.

Since the fatal error has been fixed elsewhere, I think we can move this back to normal.

+++ b/core/modules/views/src/Tests/Plugin/StyleTableTest.php
@@ -136,4 +136,80 @@ public function testNumericFieldVisible() {
+    $view = \Drupal::entityManager()->getStorage('view')->load('test_table');

let's use the entity type manager directly

@dawehner Like this?

Yeah, thanks for that!

reroll, didn't apply anymore.

Confirmed the re-rolled patch applied correctly against 8.0.x-DEV and correctly displays the caption after being applied.

Before and after screen shots attached.

Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!