Problem/Motivation

Applying the states API to a vertical tab removes the tab content, but not the link in the vertical tab menu:

Steps to reproduce:

$form['toggle_visible'] = array(
      '#type' => 'checkbox',
      '#title' => $this->t('Toggle visibility'),
      '#default_value' => FALSE,
    );
    $form['vertical_tabs1'] = array(
      '#type' => 'vertical_tabs',
      '#default_tab' => 'edit-details1',
    );
    $form['details1'] = array(
      '#type' => 'details',
      '#title' => $this->t('details1'),
      '#open' => TRUE,
      '#group' => 'vertical_tabs1',
      '#states' => array(
        'visible' => array(
          'input[name=toggle_visible]' => array('checked' => FALSE),
        ),
      ),
    );

Proposed resolution

Make states apply to the vertical tab and update the vertical tab menu accordingly.

Remaining tasks

  1. Write a patch
  2. Review
  3. Commit

User interface changes

States are applied to the vertical tab and update the vertical tab menu accordingly.

API changes

To be determined.

Data model changes

None.

Original report by Berdir:

See #767212-12: #states can't hide/show fieldsets.

The attached patch fixes it, it's probably a bit hackish, but that was the best I could come up with my limited JS skills :)

What it does is add a derived id (based on the fieldset id) to each vertical tab li element. Then it overrides the state:visibile handler for those fieldsets and instead hides/shows the corresponding button. It also switches back to the nearest fieldset if it is currently active when hidden again.

I guess it might be possible to pass the vertical_tab element somehow directly to that bind callback, which would make that id stuff unecessary.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Berdir’s picture

Issue tags: +FAPI #states

Tagged.

sun’s picture

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.

Berdir’s picture

I'll check that out.

I've only tested it with having a fieldset in the middle and with having one in the beginning, and it worked fine for me.

BenK’s picture

Subscribing

anon’s picture

Subscribing

anon’s picture

FileSize
1.21 KB
PASSED: [[SimpleTest]]: [MySQL] 33,569 pass(es). View

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

anon’s picture

Version: 8.x-dev » 7.x-dev

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

Johnny vd Laar’s picture

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

Johnny vd Laar’s picture

FileSize
1.21 KB
PASSED: [[SimpleTest]]: [MySQL] 37,043 pass(es). View

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

anon’s picture

FileSize
2.38 KB
PASSED: [[SimpleTest]]: [MySQL] 38,709 pass(es). View

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

Berdir’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work

This needs to be fixed in 8.x first, and then backported.

fenda’s picture

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

nod_’s picture

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

cweagans’s picture

Wim Leers’s picture

Component: javascript » forms system
Issue tags: +JavaScript

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

Wim Leers’s picture

dealancer’s picture

Issue summary: View changes

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();".

dealancer’s picture

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

dealancer’s picture

Version: 7.x-dev » 8.1.x-dev

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

josephdpurcell’s picture

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.

josephdpurcell’s picture

Issue tags: +Dublin2016

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

lucastockmann’s picture

Assigned: Unassigned » lucastockmann

Working on this in dublin.

lucastockmann’s picture

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)
  ),
),
lucastockmann’s picture

Assigned: lucastockmann » Unassigned
Status: Needs work » Needs review

The last submitted patch, 18: make-states-work-for-vertical-tabs-1148950-18.patch, failed testing.

SteffenR’s picture

Assigned: Unassigned » SteffenR

I'll have a look on your patch luca.

SteffenR’s picture

Assigned: SteffenR » Unassigned
Status: Needs review » Reviewed & tested by the community

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.

Manuel Garcia’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
2.39 KB
766 bytes

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.

droplet’s picture

Component: forms system » markup
Status: Needs review » Needs work
Issue tags: +Needs JS testing

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

droplet’s picture

Component: markup » javascript
mbrc’s picture

Assigned: Unassigned » mbrc
Issue tags: +drupalironcamp2016
mbrc’s picture

Assigned: mbrc » Unassigned
Status: Needs work » Needs review
FileSize
2.59 KB
2.95 KB

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.
mbrc’s picture

Title: #states doesn't work for vertical tab fieldsets » #states doesn't work for vertical tabs

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

nod_’s picture

Status: Needs review » Needs work

+ 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.

Manuel Garcia’s picture

Assigned: Unassigned » Manuel Garcia
Manuel Garcia’s picture

Assigned: Manuel Garcia » Unassigned
Status: Needs work » Needs review
FileSize
2.26 KB
1.44 KB

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

Still needs work based on #37.

Manuel Garcia’s picture

Status: Needs review » Postponed (maintainer needs more info)
FileSize
1.44 KB

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.

Manuel Garcia’s picture

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

navneet0693’s picture

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.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

idebr’s picture

Title: #states doesn't work for vertical tabs » Applying #states to a vertical tab does not update the vertical tabs menu
Issue summary: View changes
Status: Postponed (maintainer needs more info) » Needs review
FileSize
21.24 KB

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

Manuel Garcia’s picture

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

idebr’s picture

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

Manuel Garcia’s picture

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)

Manuel Garcia’s picture

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

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Manuel Garcia’s picture

robpowell’s picture

#50 worked for me. Disregard, wrong page.