views-view-table.html.twig template directly uses details without render array and polyfills

Added in #843708: Add option to set caption & remove summary in the html table (Accessibility).

This changes it for all Themes, including Stable, I never remember what we can and cannot change there.

Simplifies the twig, and looks a lot better too :)

Before

After

The test for this passes locally without any changes so all good from that end too it seems.

Edit: So this would be Option A from the IS

@dww nice, and thanks @bnjmnm!

Since we are marking this as a bug, let's add some test coverage. Also this needs a cache rebuild to pick up the twig template changes.

Let's target 8.9 for now and see if we can backport later?

Added the CR.

While writing the CR I realised we shouldn't backport this to 8.8.x since it will break any existing theme that extends views-view-table.html.twig, it will render the new summary inside the hardcoded summary, causing the theme to show a nested collapsible field.

Would this mean the current patch in #8 is the wrong way to go here? Should we do this in a BC way and not reuse the existing 'summary' variable? Maybe provide a new 'summary_element' (or something) variable and leave the summary variable as it is, and use the new variable in the core templates?

We have BC, we have an update, we have test coverage. Seems ready to me.

Updated the CR to match the new direction

+++ b/core/modules/views/views.post_update.php
@@ -436,3 +436,10 @@ function views_post_update_remove_core_key(&$sandbox = NULL) {
+/**
+ * Add proper accessibility to views table summaries.
+ */
+function views_post_update_add_accessibility_for_table_summary(&$sandbox) {
+  // Empty update to cause a cache rebuild to pickup changes to twig templates.
+}

This is not necessary - the twig cache key includes the Drupal version. - turns out my memory is not right - see \Drupal\Core\Template\TwigEnvironment::__construct().

Ah but still this is not necessary :)

Here's why. Our container is cached by the drupal version and on container rebuild we're going to run \Drupal\Core\DependencyInjection\Compiler\TwigExtensionPass() and the mtimes of these files will have changed so the container property twig_extension_hash will change and twig will be rebuilt.

Not having this function means the patch applies to and should be tested against 9.x

Feedback has been addressed. The trigger for me to add the update was that I had to drush cr after applying the patch, but the version number explanation in #14 does cover that nicely, thanks @alexpott!

1. +++ b/core/modules/views/views.theme.inc
   @@ -660,8 +660,17 @@ function template_preprocess_views_view_table(&$variables) {
      $variables['description'] = $handler->options['description'];
   ...
   +      '#markup' => $variables['description'],
   

   To follow the previous behavior precisely, we should use inline template here. We could potentially use #plain_text here too, with the down side that it doesn't take into account if the string has been already escaped.

2. +++ b/core/themes/seven/templates/classy/views/views-view-table.html.twig
   index 1f4910a..dddfd00 100644
   --- a/core/themes/stable/templates/views/views-view-table.html.twig
   
   --- a/core/themes/stable/templates/views/views-view-table.html.twig
   +++ b/core/themes/stable/templates/views/views-view-table.html.twig
   

   This change should be applied to stable9 too.

3. +++ b/core/modules/views/views.theme.inc
   @@ -660,8 +660,17 @@ function template_preprocess_views_view_table(&$variables) {
      $variables['summary'] = $handler->options['summary'];
      $variables['description'] = $handler->options['description'];
   

   Should we open a follow-up for discussing how to deprecate template variables?

itty bitty nit picks

1. +++ b/core/modules/views/views.theme.inc
   @@ -660,8 +660,23 @@ function template_preprocess_views_view_table(&$variables) {
   +  // For backwards compatibility, initialize the 'summary' variable, although
   +  // core templates now all use 'summary_element' instead.
   

   this goes for the 'description' variable too, right?

2. +++ b/core/modules/views/views.theme.inc
   @@ -660,8 +660,23 @@ function template_preprocess_views_view_table(&$variables) {
   +    // To ensure that the summary description is properly escaped during rendering,
   +    // use an 'inline_template' to let Twig do its magic, instead of 'markup'.
   

   > 80 characters

3. +++ b/core/modules/views/views.theme.inc
   @@ -660,8 +660,23 @@ function template_preprocess_views_view_table(&$variables) {
   +        'description' => $variables['description'],
   

   lets use $handler->options['description'] so that we don't have to update this when we remove the BC variables

Looks great! All feedback has been addressed.

Committed and pushed e43c919056 to 9.1.x and 9059fe71a4 to 9.0.x. Committed e3c98f5 and pushed to 8.9.x. Thanks!

Backported to 8.9.x and 9.0.x because accessibility bugs are allowed to be fixed in the rc stage and changing templates prior to a release is better than in a bug fix release.

Just seen the change record via RSS. I think the terminology in the change record is confusing, especially the title.

Current CR title: "Views table summary now uses a render array instead of hardcoded summary tags"

The CR title talks about a "table summary", which may mislead people into thinking it's about the old-fashioned <table summary=""> attribute and/or aria-describedby. But it's not about either of those things. The words "table" and "summary" are used together in a way that conflates concepts from HTML, WCAG, and Drupal Views. (In fairness, the W3C docs also conflate them, and the settings UI for Views table display aren't clear about how they will be used.)

The title of this issue is closer to what the issue was actually about: a <details> element. It just happens to be nested inside a table's <caption> element. I'm going to update the CR.

Aside: cramming a <details> inside a table <caption> is a bit elaborate; putting it there in addition to the Twig caption variable ("caption for the table" in Views settings) is downright messy. In practice I think you'd use approach one or the other, not both. It certainly isn't clear to from Views UI what will be used as the accessible name of the table. I'll file a new issue about that; I can imagine it has BC implications.

Updated the CR. https://www.drupal.org/node/3119928/revisions/view/11856607/11859162

Follow up for something we missed #3149930: Views table settings exposes "Details" field even when empty