Vertical tabs: Javascript Error when the group "additional_settings" is empty

HEAD is broken.

Is this a duplicate of at least part of the work entailed in #357300: Place all top level form elements into a default vertical tab group?

And how does this tie in to the effort to ensure that vertical tabs are accessible — #467296: Accessibility improvements for vertical tabs ?

OK, maybe I'm misunderstanding the way the pieces fit together. It's just interesting to me that the same issue — lack of permissions leading to rendering problems — was discussed and hacked on in #357300: Place all top level form elements into a default vertical tab group. Nobody participating in this issue is also participating there, so it seems that if each of you were to read the other's thread, you might get one (or both) issues solved.

And how Javascript handles the tabs and their operation appears to be essential to not only that issue but also #467296: Accessibility improvements for vertical tabs .

Adding tag

I ran into an offshoot of this bug -- collapsible fieldsets don't work in node edit dialogs for users without privileges that necessitate a vertical-tab (the javascript error mentioned in this report prevents interacting with collapsible fieldsets). This seems rather major to me... but must be most sites either don't use collapsible fieldsets there, or all users have some privileges.

My one concern is that by fixing this at the theme level, we leave the javascript error unattended (if someone creates a vertical-tab div manually, or overrides the theming it will still pop up). I'm attaching a javascript patch (hopefully I'm doing it right) to prevent the error. I think both of these fixes used together is the best solution to the bug.

Resubmitting patch as a unification of the two.

3rd try at uploading this patch. Bear with me, it's my first time.

I like it! I'd like it even more if you did this so that data() isn't called twice... :-)

       if (!focus) {
         focus = $('> .vertical-tabs-pane:first', this);
       }
-      focus.data('verticalTab').focus();
+     // Only try and get focus if there is a tab.    <--- Also remember punctuation! :-)
+     var verticalTab = focus.data('verticalTab');
+      if (verticalTab != undefined) {
+        verticalTab.focus();
+      }
     });
   }
 };

Done and done!

Watch out for the killer whitespace! RTBCed.

Hm. Same but slightly differently applied fix for the JS lives in #679530-5: administer filters permission should not affect text format widget - testing focus.length should be sufficient based on my testing?

Pushing to critical as anonymous users can't use the node add page without this fix.

Whoa, sorry. Needs an update to HEAD.

Updated to HEAD.

+++ misc/vertical-tabs.js	15 Mar 2010 17:49:54 -0000
@@ -42,7 +42,11 @@ Drupal.behaviors.verticalTabs = {
       if (focus.length) {
-        focus.data('verticalTab').focus();
+        // Only try and get focus if the tab truly exists.
+        var verticalTab = focus.data('verticalTab');
+        if (verticalTab != undefined) {
+          verticalTab.focus();
+        }
       }

Are you sure that this is still required? The focus.length condition got introduced in one of my other patches (mentioned above) and should prevent the JS error already

145 critical left. Go review some!

You make a good point. The focus.length check will do it for us. We don't really want verticaltabs.js, or the empty div if there arn't any Vertical Tabs though.

...which makes it less critical.

+++ includes/form.inc	15 Mar 2010 17:49:53 -0000
@@ -2652,10 +2652,17 @@ function form_process_vertical_tabs($ele
+  // Return HTML only if at least one of the tabs has been rendered.
+  if (!empty($element['group']['#children'])) {
...
+  else {
+    return '';
+  }

The "else" is superfluous. However, I'd even recommend to reverse the !empty() condition and turn this into an early-out.

+++ includes/form.inc	15 Mar 2010 17:49:53 -0000
@@ -2652,10 +2652,17 @@ function form_process_vertical_tabs($ele
+    // Add required JavaScript and Stylesheet.
+    drupal_add_library('system', 'vertical-tabs');

Is there a reason for why we add this in a theme function and don't use #attached?

Powered by Dreditor.

Just tested and the empty div is gone.

See before:

See after.. Text format info does look a teeny bit too close to the button on a page creation form...

Looks fine on an article create form because of the tags:

Otherwise does what it says on the tin!

not yet RTBC

Sorry... oh dear I need to be more careful.

I didn't mean to mark this as critical or RTBC. Still needs a little work, I think, as demonstrated above.

This cleans it up a bit by moving over to #post_render to inject the #attached library. Also switches to theme('container').

#31: 551426.patch queued for re-testing.

#31: 551426.patch queued for re-testing.