Vertical tabs header and container rendered even when no vertical tabs

Simple fix

Thanks for providing the test case Everett

Patch attached

This one adds tests

#4: vtabs-empty-1742438-4.patch queued for re-testing.

#6: vtabs-empty-1742438-6-pass.patch queued for re-testing.

Tests are failing because book doesn't set #access on it's form elements, updating logic.
New patches to follow.

Looking over the patch, looks like it's a useful addition.

I do wonder if there's another place to check if there's visual elements in the element_children array so that we don't have to run through the foreach check at the end. It's unlikely to have a performance hit.

Is there a place in core where it is easy to test that that vertical tabs are no longer present after this patch?

Hi Mike
Testing the patch:

• Allow anonymous users permissions to create a basic page
• As anonymous user go to node/add/page
• With the patch no vertical tabs output
• without the patch, wrapper (with nested hidden input) and h2 element (class element-invisible) are in the markup.

Additional testing:

• Enable book module and grant anonymous users access to basic book functionality (placing books, creating books)
• Head to node/add/book - should see the vertical tab

Book is a special case as it does not set ['#access'] on it's tab - same story with translation tab.

Patch includes tests that use xpath to automate this checking.

Patch applies nicely. After following the steps in #14 verifies that the header isn't there if there are no elements.

I grabbed some screenshots, but there is no change. HTML change is:

<h2 class="element-invisible">Vertical Tabs</h2><div class="vertical-tabs-panes"><input class="vertical-tabs-active-tab" type="hidden" name="additional_settings__active_tab" value="" />
</div><div class="form-actions form-wrapper" id="edit-actions"><input type="submit" id="edit-submit" name="op" value="Save" class="form-submit" /><input type="submit" id="edit-preview" name="op" value="Preview" class="form-submit" /></div></div></form>  </div>
</div>

<div class="form-actions form-wrapper" id="edit-actions"><input type="submit" id="edit-submit" name="op" value="Save" class="form-submit" /><input type="submit" id="edit-preview" name="op" value="Preview" class="form-submit" /></div></div></form> </div>

+++ b/core/includes/form.inc
@@ -3796,11 +3796,26 @@ function form_process_vertical_tabs($element, &$form_state) {
+  $name = implode('__', $element['#parents']);

Unless I'm mistaken, this $name variable isn't ever used anywhere? So can we remove this (rather scary looking) line of code?

+++ b/core/includes/form.inc
@@ -3796,11 +3796,26 @@ function form_process_vertical_tabs($element, &$form_state) {
+  foreach (element_children($element['group']) as $tab_index) {
+    if (!isset($element['group'][$tab_index]['#access']) ||
+        !empty($element['group'][$tab_index]['#access'])) {
+      $visible_tab = TRUE;
+      break;
+    }
+  }

This actually looks like a nice thing to split into a general helper function in case there are other places we need to do such a check. Can be a separate issue/patch.

21 days to next Drupal core point release.

+++ b/core/modules/system/lib/Drupal/system/Tests/Form/ElementsVerticalTabsTest.php
@@ -40,4 +48,21 @@ class ElementsVerticalTabsTest extends WebTestBase {
+   * Ensures that the vertical-tabs wrapper markup is not shown if the user does
+   * not have access to any of the tabs.

(nitpick) Can we shorten this so it fits into 80 characters and one line?

Re-roll of #12 with changes applied from #16

Kim

@webchick asked for a general helper function to be made out of foreach (element_children($element['group']) as $tab_index) but other than that the issues have been addressed so setting it back to RTBC.

@larowlan can you set up a new issue for that general helper function so that idea doesn't get lost?

@mgifford: added #1800434: Create helper function to determine if a user has access to any tabs in a vertical tab element as follow up.

Yeah, except for the 80 chars thing. I manually shortened that comment to:

  /**
   * Ensures that vertical tab markup is not shown if user has no tab access.
   */

Let's make sure the D7 version has that text too, ok?

Committed and pushed to 8.x. Marking to 7.x for backport.

Backported #17 to D7.

That patch works as it did in D8. Only problem that I can see is that the tests seem to be dropped. Don't we need those in D7 too?

Sorry, I didn't mean to keep that at RTBC. I can contribute a screenshot however.

@mgifford, I don't understand. The tests were added to form.test in #22.

Woops.. Must have been comparing with an earlier patch.

Marking it RTBC.

Edit: I should add that there is no markup change as such. The vertical tabs just don't appear because they aren't there.

Thanks for the backport!

Committed and pushed to 7.x. (and manually fixed that comment again ;))

Thanks.. Should have looked for that.

Yeah, I'm sorry about forgetting to fix that, webchick.

Can we get a change notice on this? It breaks forms that don't set #access.

Thanks.

In that case, reverted this commit from 7.x while we figure this out. Also moving back to 8.x.

Mike, can you give any more details? Preferably in some form that we could translate to a test case.

We recently added vertical tab support to WordPress Migrate (#1797148: Vertical tab-ify the import page), and after pulling the latest core the settings section of the form disappeared completely (#1814524: Vertical tabs broken with post-7.15 drupal) - nothing in between Blog Password and Import WordPress Blog. I briefly restored the vertical tabs by adding #access in one place, but then added it elsewhere and everything disappeared again, still figuring out exactly what it needs...

Here's a bit of the original code, which does not render:

  // Import settings vertical tabs.
  $form['settings'] = array(
    '#type' => 'vertical_tabs',
    '#title' => t('Import settings'),
    '#collapsible' => TRUE,
    '#collapsed' => FALSE,
    '#pre_render' => array('vertical_tabs_form_pre_render'),
    '#attached' => array(
      'js' => array(
        'vertical-tabs' => drupal_get_path('module', 'wordpress_migrate') . '/wordpress_migrate.js',
      ),
    ),
  );
...

  $form['settings']['wordpress_migrate_content_mapping'] = array(
    '#type' => 'fieldset',
    '#title' => t('Map content'),
  );
  $form['settings']['wordpress_migrate_content_mapping']['wordpress_migrate_page_type'] = array(
    '#type' => 'select',
    '#title' => t('Convert WordPress pages to'),
    '#default_value' => variable_get('wordpress_migrate_page_type', $default),
    '#options' => $options,
    '#description' => t(''),
    '#group' => 'settings',
  );
...

  // Select default text format for bodies etc.
  $form['settings']['wordpress_migrate_text_formats'] = array(
    '#type' => 'fieldset',
    '#title' => t('Text formats'),
  );

  $options = array();
  foreach (filter_formats() as $format_id => $format) {
    $options[$format_id] = $format->name;
  }
  $form['settings']['wordpress_migrate_text_formats']['wordpress_migrate_text_format'] = array(
    '#type' => 'select',
    '#title' => t('Default format for text fields'),
    '#default_value' => variable_get('wordpress_migrate_text_format', 'filtered_html'),
    '#options' => $options,
    '#description' => t(''),
  );

Full code at http://drupalcode.org/project/wordpress_migrate.git/blob/refs/heads/7.x-....

Thanks.

So, in test case terms, _form_test_vertical_tabs_form should try adding a tab without #access, let me give that a try...

Well, #access I think was a red herring. I have confirmed that our vertical tabs work with the old theme_vertical_tabs() and break with the new one. There was actually a bug in our code - it only set #group on the first element within the first fieldset (tab), it should have been setting it on the fieldsets (and only the fieldsets). But, that doesn't fix it - if I set #group on all the fieldsets, none of them is rendered. However... If I set #group on one or more (but not all) of the fieldsets, all those which do NOT have #group set do render. So, I think this patch is correct in and of itself, but it's exposing another problem - previously all the children were automatically tabified, whether or not #group was set, now it's insisting on seeing at least one valid $element['group']. Could it be that the population of ['group'] is backwards? I'm trying to work out the logic among ['group']/['#group']/['#groups']... Hmm, but first let me dig in and see why, say, the node edit form vertical tabs do work...

  // Import settings vertical tabs.
  $form['settings'] = array(
    '#type' => 'vertical_tabs',
 ...
  );
  $form['settings']['wordpress_migrate_content_mapping'] = array(
    '#type' => 'fieldset',
    '#group' => 'settings',
...

I've worked out what the buggy behavior is, although not how to fix it. In the above snippet, removing ['settings'] from the fieldset definition makes it work. So, let's take a step back and look at the documentation:

Formats all child fieldsets and all non-child fieldsets whose #group is assigned this element's name as vertical tabs.

The actual behavior with this patch is:

1. Non-child fieldset with #group assigned: renders.
2. Child fieldset with #group assigned: does not render.
3. Child fieldset without #group assigned: renders only if there is at least one other fieldset with #group assigned.

Next step is probably writing tests to fail in the latter two cases.

Here's a D7 version of the original patch plus tests designed to break in the broken scenarios...

This snippet from form_pre_render_fieldset() is highly suspicious:

    // Otherwise, this element belongs to a group and the group exists, so we do
    // not render it.
    elseif (element_children($element['#groups'][$group])) {
      $element['#printed'] = TRUE;
    }

So, why not render it? Is this assuming that some other piece of code (i.e., the old theme_vertical_tabs()) is rendering it?

Removing the whole chunk of code around this does allow the "Child with #group found" test to pass, and my wordpress_migrate form with the ['settings'] intact and #group specified does work. This leaves the children with no #group to account for... I've confirmed that children with no #group are not in ['group'] in theme_vertical_tabs, so the issue here is populating ['group']. Perhaps the proper logic is that a fieldset whose parent is a vertical_tabs element and has no explicit #group should default to its parent as a #group. Looking at form_process_fieldset(), which only populates ['groups'] in the presence of #group... But that's it for me for today (and probably tomorrow)...

OK, I believe I've got it - simply had theme_vertical_tabs() check its direct children in addition to the contents of ['group']. Let's see how the testbot likes it.

Before rolling it for D8 - now we're doing that #access check twice within the function, is that a sufficient threshold to break it out to a helper function as part of this patch?

Well, the Form API tests were happy with removing that chunk from form_pre_render_fieldset(), but a few specific forms out there were not. Restoring it breaks the new "Child with #group found" test, it seems like being a child AND setting #group cancel each other out. I've played around with it some more and don't have a complete solution yet, maybe someone with more experience with the Form API internals can take a crack at it.

When going through #14 again, I had trouble actually getting a screen that didn't include a "Revision information" tab.

This is a bit of a concern, but I'd love to have someone else go through & verify this before posting a new issue.

This needs a whole re-write, not an optimistic re-roll. The two functions this addresses are form_pre_render_fieldset() & function theme_vertical_tabs() neither of which is there anymore. The patch is so old it doesn't even include core/

Anyways, I think this is still an outstanding issue but would be good to check first before proceeding.

Oops

Note that everything from #30 forward is about D7.

Sorry @mikeryan multitasking....

Updated issue summary

Straight re-roll of the patch in #39, so no interdiff.

Hmm... it seems that the controls which are supposed to be in vertical tabs are displayed twice, once by themselves, and once in a vertical tabset...

... this is probably causing some of the test fails.

I'm not really sure why FileFieldWidgetTestCase->testPrivateFileComment() and ForumIndexTestCase->testForumIndexStatus() would be failing... those seem unrelated.

The duplicate fieldset appears to be a result of the following, which was removed in #39... I don't understand why though. @mikeryan?

diff --git a/includes/form.inc b/includes/form.inc
index f1691ad..8a17878 100644
--- a/includes/form.inc
+++ b/includes/form.inc
@@ -3800,24 +3800,6 @@ function form_pre_render_fieldset($element) {
     }
   }
 
-  if (isset($element['#group'])) {
-    $group = $element['#group'];
-    // If this element belongs to a group, but the group-holding element does
-    // not exist, we need to render it (at its original location).
-    if (!isset($element['#groups'][$group]['#group_exists'])) {
-      // Intentionally empty to clarify the flow; we simply return $element.
-    }
-    // If we injected this element into the group, then we want to render it.
-    elseif (!empty($element['#group_fieldset'])) {
-      // Intentionally empty to clarify the flow; we simply return $element.
-    }
-    // Otherwise, this element belongs to a group and the group exists, so we do
-    // not render it.
-    elseif (element_children($element['#groups'][$group])) {
-      $element['#printed'] = TRUE;
-    }
-  }
-
   return $element;
 }

I've put that chunk of code back, and the test-fails which seem to be related to this pass on my local environment, so let's see how the full suite of tests fare.

The duplicate fieldset appears to be a result of the following, which was removed in #39... I don't understand why though. @mikeryan?

I have no direct memory of this (it has been almost three years, after all), but #38 appears to be where I'm discussing removing it. What I do recall is a large investment of time to understand the D7 Form API internals enough to get as far as I did, and I'm afraid I don't have time to invest relearning it now, sorry.

The updated IS under UI changes says "Visually, none". But there are visual implications in some cases (which is why I got involved in the first place, see #32).

Update issue summary to specify that anonymous users will have to go to node/add/page in order to see zero vertical tabs.

There don't appear to be any visual differences for the test case in #14 nor the path used in the failing test case. See attached screenshots.

I'll try to figure out a test case for the Wordpress Migrate module, and check if there are any visual differences there.

***

The failing test case is puzzling. I don't see any differences between the HTML output of the patched and unpatched versions, nor does Imagemagick detect any visual differences. That being said, I don't see the third tab (which is supposed to be there) in either the patched or unpatched version. I'll have to do some more investigation into when/why the test was introduced and exactly what it is supposed to be testing.

I'll try to figure out a test case for the Wordpress Migrate module, and check if there are any visual differences there.

Note that as of Wordpress Migrate 7.x-2.2 (released 2012-11-29), the module no longer uses vertical tabs. The controls previously in vertical tabs were moved into a wizard in #1826134: Support new Migrate wizard framework to make it compatible with Migrate 7.x-2.6.

Here's a test case:

1. Create a new Drupal 7.x-dev site with the minimal install profile.
2. Download migrate-7.x-2.5 and wordpress_migrate-7.x-2.1
3. Enable the core taxonomy, path core modules
4. Log in as a user which has the migration information and migrate wordpress blogs permissions.
5. Go to admin/content/wordpress
6. You see 4 vertical tabs, "Map content", "Text formats", "Map taxonomy", and "Path aliases".

... but, like the failing test, I don't see any differences between the HTML output of the patched and unpatched versions, nor does Imagemagick detect any visual differences (see attached screenshots). Even manually going through each tab and comparing them doesn't show me any visual differences.

***

The failing test still does indeed fail on the patched version and pass on the unpatched version, so I'll try to figure out why, and see if that provides any insight.

The failing test still does indeed fail on the patched version and pass on the unpatched version, so I'll try to figure out why, and see if that provides any insight.

... wait a minute... the assertion which fails does not exist in the unpatched version.

However, it makes sense to have the test, because some forms (e.g.: the one in block_admin_configure()) create fieldsets like tab3 in the test...

  $form['visibility'] = array(
    '#type' => 'vertical_tabs',
    '#attached' => array(
      'js' => array(drupal_get_path('module', 'block') . '/block.js'),
    ),
  );

  // Per-path visibility.
  $form['visibility']['path'] = array(
    '#type' => 'fieldset',
    '#title' => t('Pages'),
    '#collapsible' => TRUE,
    '#collapsed' => TRUE,
    '#group' => 'visibility',
    '#weight' => 0,
  );

... while others (e.g.: the one in user_admin_settings()) create fieldsets like tab1 in the test...

  $form['email'] = array(
    '#type' => 'vertical_tabs',
  );
  // ...

  $form['email_admin_created'] = array(
    '#type' => 'fieldset',
    '#title' => t('Welcome (new user created by administrator)'),
    '#collapsible' => TRUE,
    '#collapsed' => (variable_get('user_register', USER_REGISTER_VISITORS_ADMINISTRATIVE_APPROVAL) != USER_REGISTER_ADMINISTRATORS_ONLY),
    '#description' => t('Edit the welcome e-mail messages sent to new member accounts created by an administrator.') . ' ' . $email_token_help,
    '#group' => 'email',
  );

... updating the patch to better explain this, and test it more explicitly.

I have no idea why this happens, but in the case when a fieldset is BOTH a child element of the vertical_tabs form element AND has a #group set (e.g.: the block_admin_configure() case), none of the fieldsets in the vertical tabs area is output unless at least one fieldset in the vertical tab area has a '#weight'.

The assertion in FormsElementsVerticalTabsFunctionalTest::testTabsShowForChildOrGroup() was failing because none of the fieldsets had a '#weight' set.

Since '#weight' is supposed to be optional, I'm going to go out on a limb here and say that's not supposed to happen.

Since '#weight' is supposed to be optional, I'm going to go out on a limb here and say that's not supposed to happen.

Initial investigation into this problem suggests it won't be easily solved.

***

Since all instances in core of vertical tabs with sub-fieldsets that are BOTH child elements of the vertical_tabs form element and have a #group set also have #weights, I suggest we split that problem off into a separate issue (#2563339: Vertical tab fieldsets that are children of the vertical tab element AND have the #group set to their parent won't be rendered unless at least one of them has a #weight), add #weights to the form defined in _form_test_vertical_tabs_3_form(), and get this one committed.

Whoops, needs review.

Is the optional '#weight' issue also going to be be a problem in Drupal 8?

Is the optional '#weight' issue also going to be be a problem in Drupal 8?

I've posted a response in #2563339-6: Vertical tab fieldsets that are children of the vertical tab element AND have the #group set to their parent won't be rendered unless at least one of them has a #weight.

I reviewed current approach in solving the issue and seems it's not very stable.
Tabs could be empty not only because of #access=FALSE. After an investigation, I figured out that problem comes from active_tab hidden value, which is always present.

The ideal solution is — add the active_tab element only if there are visible tabs.
And because it's done on a #process state, it's unknown if visible tabs present. But I didn't find an easy way to achieve this.

I propose to check if rendered #children data is not empty, except system active_tab field.
This actually implies if nothing rendered except the system element — there is no sense to render vertical tabs.

See patch in the attachment.