Add alternating even/odd classes for the fieldset depth level

Nice idea! This could certainly be beneficial to themers who are trying to extend Seven's styles. I gave this a quick review, and had a few thoughts. I'm also tagging this for front-end framework manager review because I'm not even sure this can be committed, as it might constitute a BC break. Also, a framework manager will be better equipped than I to find any problems with the approach.

1. +++ b/themes/seven/template.php
   @@ -17,11 +22,32 @@ function seven_preprocess_maintenance_page(&$vars) {
   -  drupal_add_css(path_to_theme() . '/ie.css', array('group' => CSS_THEME, 'browsers' => array('IE' => 'lte IE 8', '!IE' => FALSE), 'weight' => 999, 'preprocess' => FALSE));
   +  drupal_add_css(path_to_theme() . '/ie.css', array(
   +    'group' => CSS_THEME,
   +    'browsers' => array(
   +      'IE' => 'lte IE 8',
   +      '!IE' => FALSE),
   +    'weight' => 999,
   +    'preprocess' => FALSE)
   +  );
      // Add conditional CSS for IE7 and below.
   -  drupal_add_css(path_to_theme() . '/ie7.css', array('group' => CSS_THEME, 'browsers' => array('IE' => 'lte IE 7', '!IE' => FALSE), 'weight' => 999, 'preprocess' => FALSE));
   +  drupal_add_css(path_to_theme() . '/ie7.css', array(
   +    'group' => CSS_THEME,
   +    'browsers' => array(
   +      'IE' => 'lte IE 7',
   +      '!IE' => FALSE),
   +    'weight' => 999,
   +    'preprocess' => FALSE)
   +  );
      // Add conditional CSS for IE6.
   -  drupal_add_css(path_to_theme() . '/ie6.css', array('group' => CSS_THEME, 'browsers' => array('IE' => 'lte IE 6', '!IE' => FALSE), 'weight' => 999, 'preprocess' => FALSE));
   +  drupal_add_css(path_to_theme() . '/ie6.css', array(
   +    'group' => CSS_THEME,
   +    'browsers' => array(
   +      'IE' => 'lte IE 6',
   +      '!IE' => FALSE),
   +    'weight' => 999,
   +    'preprocess' => FALSE)
   +  );
   

   While these changes do improve readability tremendously, they do not appear to be adding or removing anything, so unfortunately I think this chunk should be reverted because it's out of scope. :(

2. +++ b/themes/seven/template.php
   @@ -116,3 +142,34 @@ function seven_css_alter(&$css) {
   + * Added odd/even classes for more fieldset theming options.
   

   This sentence should probably be reworked. Something like "Adds alternating odd/even classes to nested fieldsets."

3. +++ b/themes/seven/template.php
   @@ -116,3 +142,34 @@ function seven_css_alter(&$css) {
   +  $no_of_parents = count($element['#parents']) - 1;
   +  $additional_class = ($no_of_parents % 2 == 0) ? 'fieldset-nested-even' : 'fieldset-nested-odd';
   +  _form_set_class($element, array('form-wrapper', $additional_class));
   

   $element['#parents'] can be set arbitrarily, and we can't be sure that all of the parents are fieldsets. Therefore, I'm not sure that this code is "trustworthy". Maddeningly, I'm don't have any ideas off the top of my head about how to get around that, but I thought I'd annoyingly point it out anyway. This function would benefit from a reliable, dedicated helper function to determine the number of *fieldsets* above the element in the render array's hierarchy.

4. +++ b/themes/seven/template.php
   @@ -116,3 +142,34 @@ function seven_css_alter(&$css) {
   +  $output = '<fieldset' . drupal_attributes($element['#attributes']) . '>';
   +  if (!empty($element['#title'])) {
   +    // Always wrap fieldset legends in a SPAN for CSS positioning.
   +    $output .= '<legend><span class="fieldset-legend">' . $element['#title'] . '</span></legend>';
   +  }
   +  $output .= '<div class="fieldset-wrapper">';
   +  if (!empty($element['#description'])) {
   +    $output .= '<div class="fieldset-description">' . $element['#description'] . '</div>';
   +  }
   +  $output .= $element['#children'];
   +  if (isset($element['#value'])) {
   +    $output .= $element['#value'];
   +  }
   +  $output .= '</div>';
   +  $output .= "</fieldset>\n";
   +  return $output;
   

   I'm not intimately familiar with Seven's theme functions (especially in D7), but this seems like a copy-paste of theme_fieldset(). It might be preferable here to just call theme_fieldset() directly to reuse its code path as much as possible, rather than copy its code.

One more thing...

+++ b/themes/seven/style.css
@@ -575,11 +575,11 @@ html.js fieldset.collapsed {
+fieldset.fieldset-nested-even {
+  background-color: #f7f7f3;
 }

We probably shouldn't change the color here, since that change is probably also out of scope. Sorry! :(

Also, some before/after screenshots would help review.

Thanks for posting this patch, @aubjr_drupal!

I discussed this issue with @Fabianx (Drupal 7 framework manager) and @lauriii (Drupal 8 frontend framework manager). We probably wouldn't want to make this change in Drupal 7 at this point since it could disrupt other subthemes of the Seven theme.

We could consider the change for Drupal 8 in a minor release, since the Seven theme is considered internal API in Drupal 8. Reference: https://www.drupal.org/core/d8-bc-policy#themes (Improvements need to be made in Drupal 8 first regardless according to the backport policy: https://www.drupal.org/core/backport-policy)

So, I've moved this issue to 8.x in case we're interested in making a similar improvement in D8's version of Seven. It'd of course need a different patch and screenshots would still help reviewers. :)

Thanks again for sharing your improvement with the Drupal community!

I don't know if it makes sense to do anything here for Drupal 8. In Drupal 7, the Seven theme has this CSS in its style.css:

fieldset fieldset {
  background-color: #fff;
}
fieldset fieldset fieldset {
  background-color: #f8f8f8;
}

This patch wants to genericize that to nesting/depth levels beyond 3.

In Drupal 8, even these two levels were removed. First, #1168246: Freedom For Fieldsets! Long Live The DETAILS. converted many fieldset elements to details and therefore changed these selectors to details details and details details details. And then #1838114: Change node form’s vertical tabs into details elements removed even those selectors.

I'm curious what the use cases are for high levels of fieldset and/or details depth, both in Drupal 8 and in Drupal 7. So +1 to screenshots, especially of real cases that you've run into.

Since seven has been removed from D10 I'll move to the contrib

Don't believe any theme in D10 is currently having this issue.

The patch is for Drupal 7.

I have check the according to issue branch its properly updated the D10 version no need to reroll the patch and branch files.

If have any doubt and I am missing something please let me know.

Thanks

@anchal_gupta No commit has been done in the existing issue fork.

I agree with effulgentsia: Drupal 8+ does not require the changes proposed here. Therefore, I am closing this issue.