[DX] Provide an easy way to specify attributes for the wrapper div in the output of theme_item_list()

[klonos]

Consider this code:

  // Prepare the various items for the list.
  $things_to_create_a_list_from = array(
    ...
  );
  $list_items = array();
  foreach ($things_to_create_a_list_from as $thing => $thing_info) {
    $item_class = do something with $thing_info['abc'];
    $list_items[] = array(
      'data' => do something with $thing_info['xyz'],
      // Add CSS classes to each individual <li> item.
      'class' => $item_class,
    );
  }
  $my_list_of_items = theme('item_list', array(
    'items' => $list_items,
    // Add CSS id and classes to the <ul> element.
    'attributes' => array('id' => 'ul-css-id', 'class' => array('some-css-class', 'another-css-class')),
  ));

The output of the above implementation of theme('item_list', ...) looks like this:

  <div class="item-list">
    <ul id="ul-css-id" class="some-css-class another-css-class">
      <li class="class-for-item-1 odd first">...</li>
      <li class="class-for-item-2 even">...</li>
      <li class="class-for-item-3 odd">...</li>
      <li class="class-for-item-3 even last">...</li>
    </ul>
  </div>

I was going through the comments in https://api.drupal.org/api/drupal/includes%21theme.inc/function/theme_item_list/7.x and there are many comments to ask how to add a class, id, or other property to the wrapper div, because CSS doesn't have a way to target parents - see: https://stackoverflow.com/questions/1014861/is-there-a-css-parent-selector

As of October 2023, there is no way to select the parent of an element in CSS in a way that works across all major browsers.

The Selectors Level 4 Working Draft specifies a :has() pseudo-class that provides this capability, among others:

li:has(> a.active) { /* styles to apply to the li tag */ }

According to https://caniuse.com/css-has, most major browsers added support for this pseudo-class in 2022; however, as of October 2023, it is not yet enabled by default in Firefox.

So if you want a solution that supports Firefox, minor browsers, and/or old browser versions, then for now, you'll have to resort to JavaScript.

So, each time that someone asks how to add an attribute to the wrapper <div> of the list, the answer is that you have to write a custom MYTHEME_item_list() function. DX 👎🏼

Instead of having the output of that wrapper <div> be hard-coded, I would like to propose that we introduce a new 'wrapper_attributes' property in theme_item_list(), to allow for this to happen:

  $my_list_of_items = theme('item_list', array(
    'items' => $list_items,
    // Add CSS id and classes to the <ul> element.
    'attributes' => array('id' => 'ul-css-id', 'class' => array('some-css-class', 'another-css-class')),
    // Add CSS id and classes to the wrapper <div> element.
    'wrapper_attributes' => array('id' =>'div-css-id', 'class' => array('some-css-classes', 'for-the-div')),
  ));

...so that people can achieve the following output without having to write custom theme functions:

  <div id="div-css-id" class="some-css-classes for-the-div item-list">
    <ul id="ul-css-id" class="some-css-class another-css-class">
      <li class="class-for-item-1 odd first">...</li>
      <li class="class-for-item-2 even">...</li>
      <li class="class-for-item-3 odd">...</li>
      <li class="class-for-item-3 even last">...</li>
    </ul>
  </div>

[klonos]

I have come across this need again while working on #6290 and other related issues. Instead of being able to simply target the list with something like this and be done with it for the entire theme:

.task-list.item-list {
  width: 100%;
}

(where task-list is a custom class added specifically to the task list everywhere it is rendered), I have to do things like this:

.installer-browser-install-tasks .item-list,
.maintenance-page .tasks .item-list,
...
.add .more-classes .tasks .item.list,
...
.in-every .page-you-want .tasks .item.list {
  width: 100%;
}

...and sometimes, there is no container element available in order to target the .item-list div, which becomes an even bigger nightmare of having to create new templates for various things.

[bugfolder]

I think I've come across this situation, too, so I'd support this.

And I see that FAPI item already supports #wrapper_attributes.

[bugfolder]

Reviewing the code, I see that <...> was added to <tbody> and <tfoot> tags in comment text; should div be <div> similarly in docblock text? Though I could also see that "div" is so common that everyone knows what it means without angles, while "tbody" and "tfoot" are a little obscure and so might warrant extra clarification.

[bugfolder]

Tested, WFM. Code is tentatively LGTM, just wondering about the point above. If it's a Task, then that sounds like a bugfix milestone.

[klonos]

Thanks @bugfolder 🙏🏼

... If it's a Task, then that sounds like a bugfix milestone.

For now, I've updated the @since in the docblock to mention the addition for 1.26.2, but we can change that if we decide to push this to 1.27.0. ...although I really hope to get this in sooner, since it's either soft- or hard-blocking other PRs with UX improvements I'm working on.

Reviewing the code, I see that <...> was added to <tbody> and <tfoot> tags in comment text; should div be <div> similarly in docblock text?

I only changed that inline comment because PHPCS was complaining about the // tbody and tfoot have the same structure, and are built using the same line starting with a lowercase letter. We don't have any standard specified for this, but I know that things may break in api.b.org if such unclosed HTML tags are present in docblocks. I've changed those to TBODY and TFOOT instead, and I think I'm going to start a trend in doing that (using uppercase) when mentioning HTML tags within docblocks and inline comments.

In the meantime, I have rebased, and fixed the tests - only the LayoutUpgradePathTest gremlin remains (see #6293 for that).

[bugfolder]

Jiminy, I hate to quibble over text, but since our standard for HTML is to lower-case tags, upper-casing them when we talk about them seems wrong.

I think you have the right solution to the "Don't start a comment with a lower-case" rule, though; saying The tbody and tfoot HTML tags have the same structure avoids that problem and makes it clear that we're talking about tags (so I wouldn't require the <...> either).

(And if CSpell complains about tbody and tfoot, those should be added to the dictionary; all valid HTML tags should be acceptable.)

[bugfolder]

I've made my above suggestion a comment on the PR. Looking through the failing tests, they're irrelevant, so @klonos, if you're willing to go back to LC on tbody and tfoot by accepting the suggestion, I can mark this WFM and RTBC.

[bugfolder]

Code re-reviewed. LGTM! RTBC. (Only failing tests are our old friend testLayoutUpgrade and unrelated CSpell.)

[quicksketch]

I think this looks good too! Only problem is this isn't a UX or bug fix so it should probably be a 1.27.0 issue rather than 1.26.2. I updated the @since line and now this is ready for 1.x.

[quicksketch]

I merged backdrop/backdrop#4458 into 1.x for 1.27.0. Thanks @klonos and @bugfolder!

[klonos]

Thanks @quicksketch 🙏🏼

@bugfolder how do we update https://docs.backdropcms.org/form_api with this change again?

[klonos]

...scratch that. We didn't add that to any FAPI element - just a theme_* function. All good 👍🏼