Problem/Motivation

Node edit page "Menu Settings" hidden (as with other closed details elements) because it has classes added ClaroPreRender::verticalTabs reaches too many "groups" that are not "vertical tabs" when an a vertical tab is added to the form. AKA "overreaching code"

Steps to reproduce

  1. Using Claro admin theme
  2. Add an image_widget_crop widget to a node (or any thing that adds vertical tab)
  3. Edit node and add a photo (to add a vertical tab to the page) and save
  4. Visit node edit form again and notice all the collapsed details like Menu Settings are missing because they have vertical tab classes attached.

Proposed resolution

Only target groups that the element belongs to instead of all #groups.

Remaining tasks

User interface changes

Unbroken details elements

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joelpittet created an issue. See original summary.

jplana’s picture

Here is a simple patch for this issue.

joelpittet’s picture

Issue summary: View changes
joelpittet’s picture

Issue summary: View changes
joelpittet’s picture

Issue summary: View changes
Status: Active » Needs review

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

joelpittet’s picture

Priority: Normal » Major
Status: Needs review » Reviewed & tested by the community

I think this is a great fix because it prevents the classes from being applied to all other details elements in every group and only targets the group that the vertical tab element belongs to.

It could use a comment describing that, but should fix a major UI bug. Moving to Major because it hides all the details summaries in the node form side bar ("advanced") group

joelpittet’s picture

Title: Vertical Tabs applied to more details groups than it should » Vertical Tabs CSS classes applying to non-vertical tab detail element groups

re-title

lauriii’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Tested manually for regression and everything seemed to work as expected. However, I think we should add test coverage for the bug just to make sure that we don't break this again in future.

jplana’s picture

I had a second look at the original patch code posted and worked a little on refactoring it. The idea behind the refactor is to include all parents, and avoid the overreach of all the details elements.

I'm posting here my work in progress as a patch.

ramil g’s picture

joelpittet’s picture

While writing unit tests, jplana, @ramil g, and I found that in Unit tests the classes from core themes weren't loading. @larowlan and @berdir helped pinpoint the issue in Drupal Slack and created an issue #3209048: Core themes are not added to the test autoloader

jplana’s picture

I'm posting here a potential test, in a work in progress state. I ran out of time today, please feel free to jump and fix/complete it :)

core/tests/Drupal/KernelTests/Core/Theme/ClaroVerticalTabsTest.php

<?php

namespace Drupal\KernelTests\Core\Theme;

use Drupal\KernelTests\KernelTestBase;
use Drupal\Core\Form\FormState;

/**
 * Confirms that Claro can render vertical tabs correctly.
 *
 * @group Theme
 */
class ClaroVerticalTabsTest extends KernelTestBase {

  /**
   * {@inheritdoc}
   */
  protected static $modules = ['system'];

  /**
   * Confirms that Claro can render vertical tabs correctly.
   */
  public function testVerticalTabs() {
    // Enable the Claro theme.
    \Drupal::service('theme_installer')->install(['claro']);
    $this->config('system.theme')->set('default', 'claro')->save();

    $form = [
      '#parents' => [],
      '#array_parents' => [],
      '#tree' => FALSE,
    ];
    $form['container'] = [
      '#type' => 'container',
      '#title' => 'Container',
    ];
    $form['container_details'] = [
      '#type' => 'details',
      '#title' => 'Details',
      '#group' => 'container',
    ];
    $form['vertical_tabs'] = [
      '#type' => 'vertical_tabs',
      '#title' => 'Vertical Tabs',
    ];
    $form['vertical_tabs_details'] = [
      '#type' => 'details',
      '#title' => 'Details',
      '#group' => 'vertical_tabs',
    ];

    /** @var \Drupal\Core\Form\FormBuilderInterface $form_builder */
    $form_builder = $this->container->get('form_builder');
    $form_state = new FormState();
    $form_builder->doBuildForm('form', $form, $form_state);

    /** @var \Drupal\Core\Render\RendererInterface $renderer */
    $renderer = $this->container->get('renderer');
    $output = $renderer->renderRoot($form);

    // todo: Missing assertions (only one of the details elements should have vertical-tabs__item class).
  }
}

ramil g’s picture

This is an iteration on jplana's code (#13).
I've added the assertions which tries to find the classes in the elements.
The output markup is missing the bug, but the structure of the form element has the incorrect class.

Gauravvvv’s picture

Status: Needs work » Needs review

kishor_kolekar’s picture

added patch as#14 custom command failed.
please review the patch.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

joelpittet’s picture

Finally tricked it out, the order matters of the rendering for this bug to show... apparently.

One test should fail (tests only) and the other should pass!

joelpittet’s picture

joelpittet’s picture

I should mention I RTBCd the patch because though I had a hand in it, it was only a small tweak to the order of elements that made the patch test act accordingly. Feel free to knock it back if you need another set of 👀

Rinku Jacob 13’s picture

verified and tested patch #19 and applied successfully thanks @joelpittet.

larowlan’s picture

Issue summary: View changes

Hi @Rinku Jacob 13 thanks for uploading a screenshot, however a screenshot showing a patch applies is not something we give issue credit for.
We have automation to tell if a patch applies or not.

  • lauriii committed c5820b9 on 9.3.x
    Issue #3177415 by ramil g, joelpittet, jplana, kishor_kolekar: Vertical...
lauriii’s picture

Version: 9.3.x-dev » 9.2.x-dev

I was concerned that the #parents couldn't be relied for this but it seems like vertical tabs is documented to only work in forms and Drupal\Core\Render\Element\VerticalTabs::preRenderVerticalTabs already depends on it.

Committed c5820b9 and pushed to 9.3.x. Thanks!

Since this is fixing a major bug, I'm moving this to 9.2.x for a potential backport.

  • lauriii committed 7de0623 on 9.2.x
    Issue #3177415 by ramil g, joelpittet, jplana, kishor_kolekar: Vertical...
lauriii’s picture

Status: Reviewed & tested by the community » Fixed

Backported this to 9.2.x because @catch +1'd on Slack.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.