Allow a custom HTML element to be selected for a grouping field

Created on 24 December 2011, almost 13 years ago
Updated 31 May 2024, 6 months ago

Problem/Motivation

As you can see from below the H3 tag which is generated from using grouping (@ Format: Unformatted list | Settings:Grouping field) has no divs around it like the regular fields. In my case I would like to wrap some HTML around it but I can't see where. On the fields it's easy, just override the output with what you want but on this generated group title I haven't found how to do this so any help will be much appreciated.

  <div class="view-content">
    <h2><a href="/blogs/83" typeof="skos:Concept" property="rdfs:label skos:prefLabel">Internet Department</a></h2>
    <div class="views-row views-row-1 views-row-odd views-row-first views-row-last">
      <div class="views-field views-field-picture">
        <div class="field-content">

Steps to reproduce

  1. Create a view, either list or table format
  2. Choose Settings on format
  3. Choose a grouping field to create a grouped display

The output sets the grouping label at a static <h3>, which can cause heading structure inaccessibility by outputting grouping as incorrect heading levels. This creates possible WCAG violations with no way to remedy as a site builder.

Proposed resolution

Expose a grouping label tag for each grouping field where the user can specify a wrapping label element by which to group the records via the Views UI.

Remaining tasks

<!-- See https://www.drupal.org/community/contributor-guide/task/triage-novice-issues-and-tasks for a step-by-step triage guide. Delete or add "Novice" from the Novice? column in the table below as appropriate. Uncomment tasks as the issue advances. Update the Complete? column to indicate when they are done, and maybe reference the comment number where they were done. -->

User interface changes

Adding a grouping label tag to the style options dialog allows for adjusting the tag per grouping field

API changes

A new grouping label tag element is being added to the existing view templates where a static <h3> was used to wrap a grouping label.

  
  {% if title %}
    {% if grouping_label_element is empty %}
      {{ title }}
    {% else %}
      <{{ grouping_label_element }}>{{ title }}</{{ grouping_label_element }}>
    {% endif %}
  {% endif %}
  

This code is added to the following files:

  • views-view-grouping.html.twig
  • views-view-grid-responsive.html.twig
  • views-view-grid.html.twig
  • views-view-list.html.twig
  • views-view-unformatted.html.twig

The {{ grouping_label_element }} value comes from StylePluginBase.php, which adds a new grouping_label_element array key in the form grouping array and applies if usesGroupingLabelElement = true. The following PHP style templates have that value set to TRUE:

  • DefaultStyle.php
  • Grid.php
  • GridResponsive.php
  • HtmlList.php

By default the value of this key is an <h3> element, which should prevent breakage of views that don't override the template.

Data model changes

No changes affecting the data model.

Release notes snippet

To be determined.

✨ Feature request
Status

Needs work

Version

11.0 πŸ”₯

Component
Views UIΒ  β†’

Last updated 6 days ago

Created by

πŸ‡ΊπŸ‡ΈUnited States Reg

Live updates comments and jobs are added and updated live.
  • Accessibility

    It affects the ability of people with disabilities or special needs (such as blindness or color-blindness) to use Drupal.

  • Needs manual testing

    The change/bugfix cannot be fully demonstrated by automated testing, and thus requires manual testing in a variety of environments.

  • Needs subsystem maintainer review

    It is used to alert the maintainer(s) of a particular core subsystem that an issue significantly impacts their subsystem, and their signoff is needed (see the governance policy draft for more information). Also, if you use this tag, make sure the issue component is set to the correct subsystem. If an issue significantly impacts more than one subsystem, use needs framework manager review instead.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • πŸ‡ΊπŸ‡ΈUnited States CarlyGerard

    This patch file re-rolls previous MRs from Gitlab workflow as a patch file for 11.x Work still needs to be done in figuring out caching so the wrapper tag option is stored properly. A new patch is needed once that's addressed.

  • Open in Jenkins β†’ Open on Drupal.org β†’
    Environment: PHP 8.1 & MariaDB 10.3.22
    last update about 1 year ago
    Custom Commands Failed
  • πŸ‡ΊπŸ‡ΈUnited States CarlyGerard

    Should fix Drupal command fails in 11.x re-roll patch from #80.

  • πŸ‡ΊπŸ‡ΈUnited States theMusician

    theMusician β†’ changed the visibility of the branch 1383696-how-to-change to hidden.

  • Pipeline finished with Failed
    8 months ago
    Total: 168s
    #139180
  • Pipeline finished with Failed
    8 months ago
    Total: 176s
    #139184
  • Pipeline finished with Failed
    8 months ago
    Total: 695s
    #139186
  • πŸ‡ΊπŸ‡ΈUnited States theMusician

    It appears the patch was applied cleanly. There is what seems to be an unrelated error in a unit test when testing the merge request.

    https://git.drupalcode.org/issue/drupal-1383696/-/jobs/1255878

    Drupal\Tests\editor\Functional\EditorSecurityTest 0 passes 1 exceptions
    FATAL Drupal\Tests\editor\Functional\EditorSecurityTest: test runner returned a non-zero error code (2).
    Drupal\Tests\editor\Functional\EditorSecurityTest 0 passes 1 fails

    ---- Drupal\Tests\editor\Functional\EditorSecurityTest ----
    Status    Group      Filename          Line Function                            
    --------------------------------------------------------------------------------
    Exception Other      phpunit-93.xml       0 Drupal\Tests\editor\Functional\Edit
        PHPUnit Test failed to complete; Error: PHPUnit 9.6.15 by Sebastian
        Bergmann and contributors.
        
        Testing Drupal\Tests\editor\Functional\EditorSecurityTest
        ..E                                                                 3 / 3
        (100%)
        
        Time: 00:23.789, Memory: 4.00 MB
        
        There was 1 error:
        
        1)
        Drupal\Tests\editor\Functional\EditorSecurityTest::testEditorXssFilterOverride
        Behat\Mink\Exception\ExpectationException: The field "edit-body-0-value"
        value is "Hello, Dumbo Octopus!alert(0)", but "Hello, Dumbo
        Octopus!alert(0)" expected.
        
        /builds/issue/drupal-1383696/vendor/behat/mink/src/WebAssert.php:888
        /builds/issue/drupal-1383696/vendor/behat/mink/src/WebAssert.php:781
        /builds/issue/drupal-1383696/core/tests/Drupal/Tests/WebAssert.php:968
        /builds/issue/drupal-1383696/core/modules/editor/tests/src/Functional/EditorSecurityTest.php:447
        /builds/issue/drupal-1383696/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
        
        ERRORS!
        Tests: 3, Assertions: 137, Errors: 1.
  • πŸ‡ͺπŸ‡ΈSpain fjgarlin

    fjgarlin β†’ changed the visibility of the branch drupal-1383696-82 to hidden.

  • πŸ‡ͺπŸ‡ΈSpain fjgarlin

    fjgarlin β†’ changed the visibility of the branch drupal-1383696-82 to active.

  • πŸ‡¦πŸ‡ΊAustralia acbramley

    acbramley β†’ changed the visibility of the branch gerardc/1383696 to hidden.

  • πŸ‡¦πŸ‡ΊAustralia acbramley

    acbramley β†’ changed the visibility of the branch gerardc/1383696-c to hidden.

  • πŸ‡¦πŸ‡ΊAustralia acbramley

    acbramley β†’ changed the visibility of the branch packern/view-grouping-header to hidden.

  • πŸ‡¦πŸ‡ΊAustralia acbramley

    All old MRs and patches are now hidden, work should be continued on https://git.drupalcode.org/project/drupal/-/merge_requests/7351

  • Pipeline finished with Failed
    8 months ago
    Total: 706s
    #144150
  • Pipeline finished with Failed
    8 months ago
    Total: 177s
    #145073
  • First commit to issue fork.
  • Pipeline finished with Success
    8 months ago
    #145080
  • Status changed to Needs review 8 months ago
  • πŸ‡ΊπŸ‡ΈUnited States theMusician

    New code appears to be passing.

  • Status changed to Needs work 8 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Left some comments.

    Didn't check but if any of those templates are declared in a theme in core they'll have to be updated too.

    Definitely will need a CR and probably submaintainer sign off

  • Pipeline finished with Canceled
    8 months ago
    Total: 3s
    #150572
  • Pipeline finished with Canceled
    8 months ago
    #150574
  • Pipeline finished with Failed
    8 months ago
    Total: 200s
    #150575
  • Pipeline finished with Failed
    8 months ago
    Total: 177s
    #150598
  • πŸ‡ΊπŸ‡ΈUnited States CarlyGerard

    CarlyGerard β†’ changed the visibility of the branch drupal-1383696-drupal-1383696-82 to hidden.

  • Pipeline finished with Failed
    8 months ago
    Total: 550s
    #155857
  • Status changed to Needs review 8 months ago
  • πŸ‡ΊπŸ‡ΈUnited States CarlyGerard

    The MR is now passing with the latest changes as requested (with some allowed fails in the pipeline). Updating status for the next stage of review.

  • Pipeline finished with Success
    8 months ago
    Total: 583s
    #159928
  • Status changed to Needs work 8 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Moving to NW for the change record

    But question what will happen if $usesGroupingLabelElement isn't present. Lot of contrib plugins will take a while to update.

  • πŸ‡ΊπŸ‡ΈUnited States theMusician

    Thank you for the feedback.

    To answer your question about if a view does not have $usesGroupingLabelElement present, the twig templates handle that logic and print the title. https://git.drupalcode.org/project/drupal/-/merge_requests/7351/diffs?co...

    If it is present, such as after this patch is applied, but not set, the code supports the current default of the grouping label being an h3 or empty.
    https://git.drupalcode.org/project/drupal/-/merge_requests/7351/diffs?co...

    Contrib plugins would have an option of using this way of handling additional HTML options or could continue doing it however they currently make it happen.

    There are a few of the core themes that use the twig template overrides for views-view-unformatted.html.twig, views-view-list.html.twig, and a few more. You suggest updating these in this same patch. As I understand it, the twig templates would be updated where appropriate for claro, olivero, stark, and starterkit_theme but not stable9. The same approach would be for engines/twig excluding updates to stable9. Is that correct?

  • πŸ‡ΊπŸ‡ΈUnited States theMusician
  • Status changed to Needs review 7 months ago
  • πŸ‡ΊπŸ‡ΈUnited States theMusician

    Good morning,

    I have authored a proposed change record for review by others. https://www.drupal.org/node/3446094 β†’

    I have also pushed an updated branch that includes theme updates to the core themes where the views templates are included. I did not, however, update Stable 9 as my understanding is that the Stable 9 theme is locked in place. I am happy to make the changes there if required.

  • Pipeline finished with Failed
    7 months ago
    Total: 179s
    #167742
  • Status changed to Needs work 7 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Went to review but appears MR has some failures

  • Pipeline finished with Failed
    7 months ago
    Total: 5830s
    #175362
  • Pipeline finished with Success
    7 months ago
    Total: 675s
    #177409
  • Status changed to Needs review 7 months ago
  • πŸ‡ΊπŸ‡ΈUnited States theMusician

    The test had been failing due to this upstream bug, that has now been fixed.

    https://www.drupal.org/project/drupal/issues/3448036 πŸ› InstallerTranslationExistingFileTest fails on 11.x branch Active

    Tests are now passing.

  • Status changed to Needs work 7 months ago
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    Coincidentally I just needed this patch myself. I suspect that this will need to add something to the ViewsConfigUpdater since it is adding config to Views.

    I did this earlier this year and it can be quite complex, I'm working on documenting it here πŸ“Œ Document ViewsConfigUpdater Active

    I'm not sure why tests are not triggering on the changed config already, maybe cause it's a sub item under grouping, but this adds new config: grouping_label_element: h2.
    I'm also not sure why it wouldn't be limited to heading tags like the views pager update linked in the documentation issue.

    I'm happy to help guide someone on the views update config changes since I know it's a pretty significant wrench in the process.

    This may need additional tests too.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    It might be helpful to add an example of views-view-grouping.html.twig in the change record too since the starterkit theme generation is different than the example.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    I am also getting this on pages with this patch, I think on views without a grouping set:
    Undefined array key "grouping_label_element" in Drupal\views\Plugin\views\style\StylePluginBase->renderGroupingSets() (line 559 of core/modules/views/src/Plugin/views/style/StylePluginBase.php).

  • πŸ‡ΊπŸ‡ΈUnited States theMusician

    Thanks for the feedback. I agree that the undefined array key error is because the views did not get the update to set grouping_label_element to null.

    The ViewsConfigUpdater code requirement makes sense to me as being necessary. On a separate issue, https://www.drupal.org/node/2770835 β†’ , the test update never fires even though I wrote a function in ViewsConfigUpdater but I was not aware you have to also call the function in updateAll(). The documentation improvements for how to use ViewConfigUpdater is a great idea.

    Here is a rough idea of what I think is needed. @CarlyGerard also involved with this ticket walked me through what this may end up requiring. In the protected function addGroupingLabel() I can't quite figure out what the handler represents. The parameter defines it as a display handler, so I think a view style is represented here as the handler_type "style" and that is also the plugin_id? These ideas came from one of the issues you referenced, thank you for the pointer - https://git.drupalcode.org/project/drupal/-/blob/96d21a4603df23080a34051....

      /**
       * Performs all required updates.
       *
       * @param \Drupal\views\ViewEntityInterface $view
       *   The View to update.
       *
       * @return bool
       *   Whether the view was updated.
       */
      public function updateAll(ViewEntityInterface $view) {
        return $this->processDisplayHandlers($view, FALSE, function (&$handler, $handler_type, $key, $display_id) {
          $changed = FALSE;
    
          if ($this->addGroupingLabelElement($handler, $handler_type, $view)) {
            $changed = TRUE;
          }
    
          return $changed;
        });
      }
    
      /**
       * Add Grouping Label to views without one.
       *
       * @param array $handler
       *   A display handler.
       * @param string $handler_type
       *   The handler type.
       * @param \Drupal\views\ViewEntityInterface $view
       *   The View being updated.
       *
       * @return bool
       *   Whether the handler was updated.
       */
      protected function addGroupingLabelElement(array &$handler, string $handler_type, ViewEntityInterface $view): bool {
        $changed = FALSE;
    
        // Add grouping label element to existing views.
        if (($handler_type === 'style')
          && isset($handler['plugin_id'], $handler['type'])
          && $handler['plugin_id'] === 'style'
          && $handler['type'] === 'Grid' || 'HtmlList' || 'GridResponsive' || 'DefaultStyle'
          && !isset($handler['style']['grouping_label_element'])) {
          $handler['style']= ['grouping_label_element' => NULL];
          $changed = TRUE;
        }
    
        return $changed;
      }
    

    You asked, why the allowed grouping elements would not be restricted to just heading tags like the views pager update. For the grouping use cases, grid, HtmlList, GridResponsive it seemed hard to predict that one would only ever group by heading. Using labelELements, https://git.drupalcode.org/project/drupal/-/merge_requests/7351/diffs#11..., makes it so available elements do not need to be maintained in this code as well as the views.settings. I concur that the most common use case would be headings but I am not confident that there would never be a need to group by another element.

    Thanks again for the feedback, the questions, and the guidance.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    At a quick glance that looks right: also if you want to link an issue in a comment where the issue status color comes up can you use:
    [#issuenumber]

    like this ✨ Add support for tables with two headers in views table display Needs work

  • Pipeline finished with Failed
    6 months ago
    Total: 180s
    #205041
  • Pipeline finished with Failed
    5 months ago
    Total: 171s
    #222899
Production build 0.71.5 2024