Applying #states to a vertical tab does not update the vertical tabs menu

I fear the "select another vertical tab" logic is not necessarily complete - I had similarly simple code in filter.admin.js in the beginning, but thorough manual testing revealed lots of bugs with that. Of course, filter.admin.js doesn't use #states, but the active vertical tab logic should be comparable.

Subscribing

Subscribing

@Sun, What do you think of this patch?
Its from D7 dev.

Changed the Drupal verison, if this isnt ok, then please change it back.

The patch breaks the functionality to assign menu's to nodes.

This patch solves the problem by looking into the e.target

This patch is the same as in #9 but it also hides the wrapping div when no fieldsets are visible.

Looking to sponsor someone to get this fixed in D8 and back ported to D7. Message me off interested!

need D8 reroll and use e.preventDefault(); instead of return false;

Fixing tags per http://drupal.org/node/1517250

Bump, would be nice to have this fixed in D8 :)

Marked #1777970: Vertical tabs don't support #state visibility as a duplicate of this one.

I've tried patch #10. It works, however when the verical tab is shown it also gets focus, which should not happen. This is because tabShow() is called which does focus: "this.focus();".

Here is re rolled patch which fixes problem described in #17.

This patch appears to work correctly. I can confirm that by adding a vertical tab with config like:

    '#states' => array(
      'visible' => array(
        ':input[name="scheme"]' => array('value' => 'private'),
      ),
    ),

The tab is hidden.

I have a few nit picks:

1. +++ b/misc/vertical-tabs.js
   @@ -26,7 +27,16 @@ Drupal.behaviors.verticalTabs = {
   +      tab_pane_set.wrap('<div class="vertical-tabs clearfix"></div>').before(tab_list)
   

   Perhaps more readable would be:

   this_pane_set
     .wrap(...)
     .before(...)
     .bind(..);
   

   Similar to line ~49.

2. +++ b/misc/vertical-tabs.js
   @@ -38,6 +48,27 @@ Drupal.behaviors.verticalTabs = {
   +                // Get the corresponding vertical_tab button.
   

   This comment is either obscure, or misplaced.

This is a small-ish chunk of work that someone could sprint on today.

Working on this in dublin.

Re-rolled this patch. Also followed comments #13 and #22.

Tested with this snippet:

'#states' => array(
  'visible' => array(
    ':input[name="user_email_verification"]' =>  array('checked' => FALSE)
  ),
),

I'll have a look on your patch luca.

Looks fine for me - i did some testing with a custom field to hide/show the "Advanced" field section on the node form.

Thx luca.

+++ b/core/misc/vertical-tabs.js
@@ -53,7 +54,19 @@
+          .bind('updateTabPaneSet', function() {
+          var tab_set = tab_pane_set.parent();
+          tab_set.show();
+          var $fieldsets = $('> fieldset:visible', this);
+          if ($fieldsets.length == 0) {
+              tab_set.hide();
+            }

Indentation is off here

Fixing #30.

As a side note, core includes a .editorconfig file, which you can use to help prevent indentation issues in the future =) - just install http://editorconfig.org/#download for your IDE of choice and you'll be all set.

We already used jQuery inside the script, let use jQuery.on and adding the namespace to the event.

+++ b/core/misc/vertical-tabs.js
@@ -43,6 +43,7 @@
+        var tab_pane_set = $(this);

$tabPaneSet

Notes:

• Fixed #31.
• Changed "fieldset" to "pane" or "vertical tab", depending on the context.
• Only use tabHide() to hide the tab if it's currently active/visible. If we're hiding an inactive tab, we just need to hide the tab title. This way we prevent the unwanted refocusing of tabs.
• Use stopPropagation() instead of preventDefault() to prevent bubbling.

Removing "fieldsets" from the title, since in D8 we're not using them anymore to create vertical tabs.

+ var $tabPaneSet = $(this);

we alreday have $this defined earlier, do we need this new variable?

Also I'm really not looking forward to having #states code outside states.js. Can we at least separate this callback in a differnet file, verticaltabs.states.js for exemple? I'd feel better.

Simple one, just addressing + var $tabPaneSet = $(this);

Still needs work based on #37.

While working on this, I've found that this is already working fine on 8.3.x at least.

I have built a test module for this purpose, which you can find attached. It sets up a form with a few different use cases, different states and it seems fine even on nested vertical tabs.

I haven not been able to find any steps to reproduce this problem so I am postponing this issue for now, feel free to check out the attached module to double check.

Re #40 just enable the module and go to /vertical_tabs_states_test/form/default

I am not able to reproduce it ! The module upload by @Manuel is working without problem, even examples VerticalTabsDemo, http://cgit.drupalcode.org/examples/tree/fapi_example/src/Form works fine.

I can confirm the states API works for #type => vertical_tabs, but not #type => details, see the screen capture in the updated issue summary.

Thanks @idebr for the report, can you share here the code to see/troubleshoot the bug please?

Hey Manuel,

I updated the test module you supplied in #40 and moved the states to the details. The code I changed is available in the issue summary

Rerolling #39 due to #2818825: Rename all JS files to *.es6.js and compile them .

I now see what you mean @idebr, and you're right, that doesn't work at the moment, so thanks a LOT for following up on this!
So yes we do need to fix this.

One thing to note is that for the optional state, you actually have to use it on the input fields themselves, doesn't work if you use it on details, not sure if this is a bug we should fix as well, or if its ok to be like this. The rest of the states changes do work on details with the patch applied though.

I attach the updated module to test the states on details, and also showing that last bit I mention about optional states (enable and go to /vertical_tabs_states_test/form/default)

Some more findings, toggling enabled on details works without the patch on #47.

So so far the ones not working without the patch that I know of are:

• visibility (elements inside the details element do get hidden, but the tab itself remains visible)
• optional on details

With the patch applied, still not working:

• optional on details

Reroll due to #2880007: Auto-fix ESLint errors and warnings

#50 worked for me. Disregard, wrong page.

I can verify that this fixes the issue. I thought I was configuring the #states array incorrectly or missing some "special case" since it was vertical tabs. Glad to know this is actually a bug. I have to say though, this is a pretty big bug and the priority should probably be escalated and backport patches be made.

Thanks for working on this!

The current fix introduces a few errors in our JS coding style rules. Before:

Errors:
==============================
no-use-before-define: 75
max-len: 70
camelcase: 65
no-restricted-syntax: 54
no-shadow: 42
new-cap: 18
default-case: 16
no-continue: 9
no-new: 9

With patch:

Errors:
==============================
no-use-before-define: 75
max-len: 70
camelcase: 66
no-restricted-syntax: 54
no-shadow: 42
new-cap: 18
default-case: 16
no-new: 9
no-continue: 9
no-var: 3
vars-on-top: 2
eqeqeq: 2
space-before-funct
ion-paren: 1
indent: 1
keyword-spacing: 1

Reference: #2912962: Step 1 JS codestyle: [meta] Fix JS coding standards in core

We probably also need an automated test that will ensure the bug stays fixed. Thanks!

- Fixes coding standards issues.

+++ b/core/misc/vertical-tabs.es6.js
@@ -72,7 +72,19 @@
+          .on('updateTabPaneSet', () => {

Can we have a better name and with namespacing?

+++ b/core/misc/vertical-tabs.es6.js
@@ -88,6 +100,31 @@
+              if (e.target === this && e.trigger) {

in what case, "e.target != this" ?

+++ b/core/misc/vertical-tabs.es6.js
@@ -88,6 +100,31 @@
+                  if (!e.value) {
...
+                  else if (e.value) {

can we add some comments to show what this condition trying to do?

+++ b/core/misc/vertical-tabs.es6.js
@@ -72,7 +72,19 @@
+            tabSet.show();
+            const $panes = $('> details:visible', this);
+            if ($panes.length === 0) {

any better way to skip $.show? can we verify it in another non-DOM manipulation way? Thanks.

** Missing tests

Hi all,

I was checking the issue and send an update with a different approach just updating the vertical-tab title to set the same status and allow the default behavior for the status works and trying to affect less vertical-tabs js.

I hope this works!

Regards.

Rerolled #54 for 8.5 and 8.6 (patch applies to both).

*edit* Commented on wrong issue, sorry...

I missed a small change. Think this should fix it.

It seems the cody style was changed to camelCase in #2915784: 1/3 JS codestyle: camelcase.

vertical_tab -> verticalTab

Thanks for the heads up @askibinski addressing #63 here.

Reroll against 8.7.x after #2981652: [Aug. 9 2018] Format core JavaScript using recently add Prettier was committed.

Still needs an automated JavaScript test.

Initial tests for #states is being worked on here #2702233: [backport] Add JavaScript tests for Form API #states: required, visible, invisible, expanded, checked, unchecked

Setting to needs work to address #55 and for adding test coverage.

To me, patch in #66 doesn't fix the issue. It hides a tab when it is inactive, but for an active (open) tab, it hides the tab, but the content doesn't change. Imho, in case of hiding an active tab, it should be hidden with its content and default tab should be made active (content displayed and the tab itself should be selected).

Reroll for 9.1.x

A reroll of #74 for 9.3.x.

Fix the code style.

#78 applies but wrongly hides vertical tabs for us in the situation described in this issue so I think it needs work.

I think that this became bound to the wrong value with the changes due to deprecating jquery.once.

Claro theme has a copy of this JavaScript and will need to be changed too.

@cilefen
Your patch at #80 causes an undefined variable "verticalTab".

You are right though that Claro has a mostly identical copy of these js, except for some classnames I think. So I created a patch based on #78 for Claro, since most people are probably going to use Claro or Gin these days. Not sure if this issue needs splitting for that, because eventually I believe the Claro override js will be removed when #3081489: Remove duplicate code from veritcal-tabs.js in Claro is implemented. So, an alternative would be to work on that issue, create a core patch for that and then use the patch for misc/js.

I also stripped the "If there are no visible tab panes, hide the whole vertical-tabs area" part since it adds a bit of fragile code and the current approach left me with a lot of leftover styling/classes in Claro (when using vertical tabs with field_group) which maybe could better be handled in a separate ticket.

Also also, I created a similar patch for when horizontal tabs are used with the field_group module: #3264569: Support applying #states to a horizontal tab

So I think for this to be commit-able we need patches for the plain and Claro files.

All duplicate claro code has been removed in #3081489: Remove duplicate code from veritcal-tabs.js in Claro. Rerolled the patch for 9.4.

Another reroll for 9.5.

Reroll for 10.1.x.
Please review.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Reroll for 10.2.x

The patch from #91 works as far as #states are usable for vertical tabs. However, with Drupal v10.4.0 I currently encounter the problem, that the initial state of impacted tabs is not set correctly. In other words, in my form I have a checkbox that should determine if a tab menu entry for a tab should be visible as long as the checkbox is actually checked. If the initial state of the checkbox is unchecked, the corresponding tab menu entry still get displayed nevertheless until I uncheck and re-check the controlling checkbox. The initial display of the tab contents works though (so, the to be conditionally displayed tab contents itself is NOT visible - it's just the menu entry in the tab menu).

It seems to me as if only the visibility state of the tab menu entry is set wrong for the initial state and it's not a fault of the controlling checkbox/form element. Why do I think that? Because in the tab that is to be conditionally displayed (for which the menu entry should NOT be visible, but is indeed accessible and therefore one can navigate to the tab contents) there are form elements contained that should receive a "required" flag when the same checkbox that is controlling the tab visibility is checked - and this works. In the initial state (so, when the tab itself is wrongly displayed) the dependant form elements within that tab are NOT required and only get required once the checkbox is actually ticked.

Unfortunately I was not able to spot the code fragment that's responsible for the state initialization and therefore can't provide an updated patch. :-(

// edit:
I just realized that the patch from #91 ist NOT code fragment causing the problem! On line 92 of the vertical-tabs script there is the command "tabList.append(verticalTab.item);" that unconditionally adds a menu entry to the tab menu. This should only get done conditionally...