Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dries’s picture

If we do this, I think we should rename 'Page specific visibility' to 'Page visibility', etc. The titles shouldn't wrap, IMO.

davyvdb’s picture

Here's a patch with the shorter titles.

yoroy’s picture

Priority: Critical » Normal

Critical status is only for stuff that's broken, not the case here.

It's probably ok to apply the vertical tabs here, but I'm not really sure. Do we have use cases where you want to use combinations of page & role specific settings for example? If so, then VT is not very handy, becuase it would be harder to compare two or more settings. If the 80% use case for a block is that only one of these settings is used to control visibility, then it's ok.

Dries’s picture

Priority: Normal » Critical

My experience is that the 80% case is to only use one, but I have certainly used more combinations.

davyvdb’s picture

I used Vertical Tabs here because all these fieldsets have to do with the same subject "visibility". Vertical Tabs makes this look like a group where you can see what your options are. Vertical Tabs is not used throughout core as a mutual exclusive options pattern.

yoroy’s picture

Priority: Critical » Normal

Vertical Tabs is not used throughout core as a mutual exclusive options pattern.

Yes it is. It's one of the most important aspects to consider when thinking about applying this pattern.
Like how it's used on the node edit form. There's no need to see a revision option if you just want to change the author.
Same for the user e-mails in user settings, though you might want to re-use some text, you don't need to see one to be comfortable in editing another. Like Dries says, most of the times its ok here, but I'm not sure if we really win anything here.

davyvdb’s picture

OK I see. But to be honest. The fieldsets (4) are a PITA right now if you want to review all your settings quickly. If you open a fieldset, you have to scroll down and up to go the other ones. It's easy to miss one too because they move. With Vertical Tabs, your clickable titles stay in the same place. So you can review all your settings very fast.

Dries’s picture

Priority: Normal » Critical

@yoroy, good point on the pattern. That might be worth documenting in the vertical tabs API to avoid 'abuse'? Also, so how do you suggest we proceed on this one?

Bojhan’s picture

Priority: Critical » Normal

I dont think this is a win, indeed Vertical tabs is an faster interaction but in a lot of cases you don't need to expand all of the fieldsets. You know what you are looking for in this case, while Vertical tabs is usually used more as a way to quickly go trough additional functionality/meta data - in this case its the most critical functionality of this whole page.

I say we don't continue down this path.

seutje’s picture

I like this, since I mostly only use the page specific settings and I don't like scrolling in general

moshe weitzman’s picture

@Bojhan - please address the benefit described in #7 where you can see at a glance the summary of each fieldset and then dive into just the ones that need changing. That's the main benefit of this pattern IMO - the summary. I think it is useful here.

Update: isn't collapsed fieldsets the same as vert tabs but lacking the summary? In what way is a bunch of collapsed fieldsets superior?

Bojhan’s picture

Oke, so right now I am somewhat guarding the vertical tabs because from where it originated it is designed for the node/add page - we can indeed apply it on other places, but with great caution. There are a lot of reasons but let me explain the most prominent ones.

Useage
We should only use Vertical tabs for meta data or otherwise not necessarily to configure information. Although you can skip these items, they are apart from the title more often than not the only forms on that page agreed? Which turns these vertical tabs into the main source of navigation for one of the most prominent interactions. The intend of Vertical tabs, is not to be the main interaction in a form - because users should be able to skip a vertical tab. If we apply it differently, some you can skip - some you can, but not really skip, and some you can't - people will be required to tab trough all the VT's.

I don't see it as clear that, vertical tabs is just the same as fieldset but with a summary. Yes, it does certainly look that way, but the way they are applied is far differently. We could argue that fieldsets are not good by nature on this page, because they don't expose any summaries - but then again, this page is about configuring something you know what to look for. Vertical tabs tend to be used as something to to fiddle trough.

Design

So onto the current design of this patch, we have vertical tabs that are more then one line. For scanability, we need to assure that each vertical tab is always just one line. This also applies to the descriptions, and I am unsure how we could improve that without losing information value.

Sorry, if it seems like this is an obvious improvement - and I am disagreeing. But I am here to stand guard of behavior that we saw in user testing that we wish to preserve. We spend a great deal of time, making sure that this pattren is used consistently and that should for Drupal 7 mean that people use them the same way as we observed in user testing.

davyvdb’s picture

Has this pattern (grouped settings) been tested in user tests?

Bojhan’s picture

Yes, not in this particular place.

yoroy’s picture

Yes, vertical tabs were tested in Baltimore earlier this year: http://www.drupalusability.org/search/node/vertical
They were tested as part of the node add form and were a succes there because for the most part people *ignored* them. 'Invisible design'; don't bother people with less important stuff, basically.

Moshe: one advantage of expandable fieldsets is that you can expand multiple simultanously, should you want to compare dependable stuff. This is impossible with VT. the summary can only partly do that and probably not enough in this case of blocks settings.

catch’s picture

While I might use more than one visibility setting (although not very often), I never need to compare the values here side by side, and the fieldsets are usually long enough that do so means scrolling up and down anyway - it's big textareas, long lists of checkboxes for roles etc. - so the proximity seems like a good thing here.

RobLoach’s picture

This addition makes sense and really cleans up the page a bit. As catch said, you never have to compare these settings side by side, and even if you do, the summary on the side provides hints towards its contents.

The attached patch adds hints for both "Role specific visibility settings" and "Content type specific visibility settings" when there isn't a selected option. You can see it in action under the Seven theme in the provided screenshot. It also moves to #attached_js for adding block.js.

Regarding the usability, the majority of the participants seemed to like the vertical tabs, but got confused when it came to the order in which they appeared. So, should we re-evaluate the order in which they appear? Thanks.

TD540’s picture

I definitely approve of this. And it would simply add more consistency across the Drupal admin.

But, how about grouping these into one VT group titled "Visibility Settings" (simply by adding a title above the page-/role-/contenttype-/user tabs)

and then, if other modules want to add certain settings tabs, it would be neat if they could also be grouped, either by function, or by module, or whatever that made sense.

just a thought.

davyvdb’s picture

Titles on Vertical Tabs are not supported yet (afaik). Extending can be easily provided using hook_form_alter. Thx for the thoughts!

yched’s picture

If we go this way (which I think is neat), I'd say we can remove the 'Block Specific Settings' fieldset.

RobLoach’s picture

Davy Van Den Bremt at #19

Titles on Vertical Tabs are not supported yet (afaik). Extending can be easily provided using hook_form_alter.

Do you know if there's an issue open for titles on the vertical tabs?

davyvdb’s picture

FileSize
50.81 KB
4.65 KB

Fieldset is not collapsible now. Fieldset title is now Identification since nothing else is in there and to match this with content types edit : admin/structure/node-type/article

stBorchert’s picture

@Davy: is there a reason why you didn''t move "Region settings" into the vertical tabs?

+++ modules/block/block.admin.inc	27 Aug 2009 07:49:54 -0000
@@ -254,11 +254,19 @@ function block_admin_configure(&$form_st
+  $form['block_vis_settings'] = array(

No need to shorten this. Use "block_visibility_settings".

+++ modules/block/block.admin.inc	27 Aug 2009 07:49:54 -0000
@@ -254,11 +254,19 @@ function block_admin_configure(&$form_st
   $form['page_vis_settings'] = array(

same as above (and in all other fieldsets)

This review is powered by Dreditor.

davyvdb’s picture

I wanted to group visibility settings in the tabs.

Bojhan’s picture

I love that you guys are working on this, but I am almost tempted to mark this won't fix. As explained in all the arguments above? Ignoring them and continuing on the implementation won't make them less valid.

davyvdb’s picture

Bojhan. Sorry that we're a bit stubborn on this. But the more I read your reasons I'm getting more and more convinced that admin/structure/node-type/article and admin/config/people/accounts > e-mails shouldn't have vertical tabs either.

Mostyl "they were tested as part of the node add form and were a succes there because for the most part people *ignored* them. 'Invisible design'; don't bother people with less important stuff, basically."

mgifford’s picture

I think it looks better with the visual tabs.

The patch applied cleanly.

I am worried though about creating more vertical tabs before dealing with the accessibility challenges they provide http://drupal.org/node/467296

Bojhan’s picture

@Davy Van Den Bremt At admin/config/people/accounts, you can actually ignore them. I know its a very fine line, I believe that is actually the only good application of this pattren in core beside node/add

davyvdb’s picture

Fair enough. I have no more arguments. Who will be having the final call on this? In case of a negative one, I think this should then be made really transparent to contribute module developers. Because it's very tempting to use this pattern.

janusman’s picture

I kind of agree that creating content (node/add) is a place that is very very frequently used, hence it needs to be streamlined (meaning: compact or hide stuff via VTs).

It will probably be frustrating for (beginner?) admins to "not see" important things like block visibility settings... but I agree, it's a fine line. "Reduce the amount of scrolling" is not really the problem to fix, it's making all the information grokkable.

And, I'm also worried that, just because the code is there, we try to apply them everywhere, so +1 for documenting these sort of controls (heck, I still have discussions about using checkboxes vs. radios vs. selects with seasoned Drupal ppl) =)

IMO I like VT usage here, in that it's grouping different types of visibility settings; once you understand one tab controls visibility, (I assume) you will think the others do something "similar" (which they do). So, if it matters, this has my vote...

davyvdb’s picture

I've been thinking about how I use the block configure screen again. I must agree I fall back to php visibility for user role visibility too, because I always overlook the role specific fieldset. Using VT to group all similar settings make you indeed look further for other options.

I need this pattern a lot actually. And there is no construct to do this except for VT in Drupal for now. Maybe we should use VT to group similar settings too for now and think of a better construct for this in Drupal 8. I don't think there are much other candidates for this pattern (group simmilar settings) in core now.

Status: Needs review » Needs work

The last submitted patch failed testing.

yoroy’s picture

Status: Needs work » Needs review
Cliff’s picture

Like mgifford, I wish that some of the energy devoted here could be transferred to the issue of accessibility of vertical tabs.

Here's one reason these two conversations should be linked: Bojhan said, "Users should be able to skip a vertical tab." If that's the case, then we clearly need the text in these tabs to be headings — because that's the only way that a person who must rely on a screen reader can get a list of just those items and skip to any one or more that they need to work on. (By the way, at #19, when Davy was talking about titles, did he actually mean headings?)

By the way, if anyone involved in this issue knows JQuery, your help is sorely needed at http://drupal.org/node/467296.

FWIW, I'm not sure I grok the party line on when vertical tabs should and shouldn't be used, but it does seem to me that a couple of users here have made a pretty good case for them in this instance.

sun’s picture

Category: feature » task

The region settings are the only settings here that are required. Everything else is totally optional, and it's rather unusual that one wants to combine multiple visibility settings.

Hence, vertical tabs are a valid instrument. (According to Bojhan's own explanation.)

However, feedback on latest screenshot in #22: (http://drupal.org/files/issues/Picture%201_0_135.png)

1) There should be no fieldset for the primary block "identification" itself. Just drop it.

2) The region settings fieldset should be uncollapsed and _not_ collapsible.

3) The vertical tab pane titles need to be shortened, as already outlined in the beginning of this issue: Page visibility, Role visibility, Content type visibility, User visibility. In addition to that, "Role" should come after "Content type".

Dave Reid’s picture

I completely support this. Revised patch that moves the 'Regions' fieldset into the 'Visibility settings' vertical tabs because it affects visibility as well. Moves the normal block settings fields to the base form element instead of it's own fieldset as per sun. Also includes summaries for all fieldsets and fixes default values for the radio buttons.

However, the tabledrag code was failing when included on the individual block settings page, so I added a check if Drupal.tableDrag exists. Now, on the block listing pages, the vertical tabs code conflicts. So there's a problem here.

Also includes before/after screenshots.

sun’s picture

FileSize
75.59 KB

Awesome. Let's do the following:

block-visibility.png

Dave Reid’s picture

We can't really do 'Not restricted' for the page visibility since it's 'All pages except for those specified'. That would however be an option with #514256: "Show on all pages" - block specific settings.

Made the other changes and removed the silly t('!theme', array('!theme' => $theme->info['name'])) since we don't translate theme names.

Still need to solve the JS conflict on the block listing page.

sun’s picture

+++ modules/block/block.admin.inc	29 Nov 2009 05:33:05 -0000
@@ -203,14 +204,35 @@ function block_admin_configure($form, &$
+  // Module-specific block configurations.
+  if ($settings = module_invoke($block->module, 'block_configure', $block->delta)) {
+    foreach ($settings as $k => $v) {
+      $form[$k] = $v;
+    }
+  }
@@ -236,31 +258,18 @@ function block_admin_configure($form, &$
-  // Module-specific block configurations.
-  if ($settings = module_invoke($block->module, 'block_configure', $block->delta)) {
-    foreach ($settings as $k => $v) {
-      $form['block_settings'][$k] = $v;
-    }
-  }

There _must_ be an additional key, so please revert the removal of the 'block_settings' element key. We can shorten to 'settings' though.

+++ modules/block/block.admin.inc	29 Nov 2009 05:33:05 -0000
@@ -203,14 +204,35 @@ function block_admin_configure($form, &$
+      'library' => array('system', 'vertical-tabs'),

Somehow, this is missing an array().

+++ modules/block/block.admin.inc	29 Nov 2009 05:33:05 -0000
@@ -236,31 +258,18 @@ function block_admin_configure($form, &$
-  $form['page_vis_settings'] = array(
+  $form['page_visibility'] = array(

Ideally, we put all of these into a dedicated form element key, i.e.:

$form['visibility']['page']

That said, the internal key should be 'path' here, IMHO.

+++ modules/block/block.admin.inc	29 Nov 2009 05:33:05 -0000
@@ -312,13 +322,14 @@ function block_admin_configure($form, &$
+  $form['content_type_visibility'] = array(
...
+  $form['content_type_visibility']['types'] = array(

Likewise, the internal key should be 'node', IMHO.

+++ modules/block/block.js	29 Nov 2009 05:33:05 -0000
@@ -2,6 +2,55 @@
+        vals.push($(this).parent().find('label').text().replace(/\s+$/, '') + ': ' + Drupal.checkPlain($(this).find('option:selected').text()));

1) $.trim()

2) Shouldn't the regions already be check_plain'ed?

+++ modules/block/block.js	29 Nov 2009 05:33:05 -0000
@@ -2,6 +2,55 @@
+      $("input[type^='checkbox']:checked", context).parent().each(function() {
...
+      $("input[type^='checkbox']:checked", context).parent().each(function() {

Why are these ^anchored?

This review is powered by Dreditor.

Dave Reid’s picture

Revised patch that fixes all the previous concerns. Still has JS conflict.

sun’s picture

The added condition for Drupal.behaviors.blockDrag is correct. So no worries. Ideally, those two behaviors would not live in the same file, but well.

I did some wording, condition, and coding style tweaks, and I really think this is AWESOME and ready to fly.

David_Rothstein’s picture

This is shaping up to be a great patch. But:

I don't understand why "region" should be included in the vertical tabs with the others. It is a much more commonly used setting, and one of the fundamental things people do when they go to create or edit a block. This seems to go against the purpose of vertical tabs that was described by Bojhan and others above. @sun's suggestion in #35 of making it an uncollapsible fieldset makes much more sense.

Wording issues:

  1. Replacing "pages" with "paths" in the UI is not a good idea. Maybe deep within the UI where we are describing to people what to type into the text field, but definitely not in the summary tab... "Pages" is much clearer.
  2. I don't think the overly-concise wording in the summary tab helps here; we have room for an extra word or two and (like in some of the earlier patches) should use it to make sure the various settings are clear, since these can be rather confusing as it is. I would suggest:
    • "Restricted to paths" => "Restricted to certain pages"
    • "Not configurable" => "Not configurable per user"
    • It would be nice to replace "Not restricted" with something like "Displayed for all ___" but I guess the usability nightmare that is the overlap between per-page and per-content-type restrictions makes that dangerous... I think, though, that "Not restricted by page", "Not restricted by role", etc might at least be better here.
  3. Consider whether "limited" might be a better word choice than "restricted" in all of the above (not sure about that one, though).

The suggestion in #35 of putting Roles after Content Types also seems worth doing.

sun’s picture

I implemented the suggestions by David. And, yeah, also my earlier suggestion to keep the region settings out of the vertical tabs.

However, when using custom summaries for each tab, i.e. "Not restricted by role." instead of just "Not restricted.", then it is visually much harder to see a difference between the summaries. I think the purpose of the fieldset summaries should be to quickly see in which tab a non-default configuration exists.

Tinkering about this some more, how about not outputting a summary in case there was nothing configured?

Either way would work for me. Though I could see how "Not restricted." could be helpful for novice users.

sun’s picture

Assigned: Unassigned » sun

Anyone?

mcrittenden’s picture

Status: Needs review » Needs work

Subscribe. +1 on the concept and screenshots.

Small nitpick: for the "Page" setting, let's change "specified below" to just "specified" and "if the following php code...." to "if the specified php code". "Below" and "Following" only really apply when they're written directly above the options, not to the side.;

sun’s picture

Status: Needs work » Needs review

oh, the screenshots are outdated. The new summaries for "Pages" is:

+++ modules/block/block.js	30 Nov 2009 13:10:42 -0000
@@ -2,6 +2,53 @@
+      if (!$('textarea[name="pages"]', context).val()) {
+        return Drupal.t('Not restricted by page.');
+      }
+      else {
+        return Drupal.t('Restricted to certain pages.');
+      }

...and this status is solely determined by whether there is a value in the textarea. Tricky, eh? ;)

Actually, without verifying, but this widget/fieldset most likely also needs an #elemenent_validate handler to ensure that a path was entered in the textarea when the "Show block on specific paths" option was selected. But that's a different/separate issue.

I recommend testing this patch live, because screenshots aren't very good in showcasing different interaction states.

This review is powered by Dreditor.

David_Rothstein’s picture

Status: Needs review » Needs work
FileSize
59.27 KB

Reviewed the code again and everything looks great. I was about to mark this RTBC but then trying it out, I found that the vertical tab summaries for roles and content types no longer work correctly (they don't update at all, regardless of which boxes you check). Probably not that hard of a fix, but I don't have time to look into it at the moment....

Once that's fixed, this should be RTBC in my opinion.

Also I'm attaching a screenshot of the page settings for those who are interested - although indeed, to really experience this patch it's necessary to try it out.

David_Rothstein’s picture

Also, very very very minor, but I believe elsewhere we don't use periods at the end of the vertical tab summaries (e.g., they should all be like "Not restricted by page" rather than "Not restricted by page.")

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
14.32 KB

yeah, form element labels for radios and checkboxes have changed in the markup. Fixed that and removed the trailing periods.

RTBC according to David.

yoroy’s picture

Can we have a screenshot please?

sun’s picture

What's wrong with the screenshot David posted?

But as mentioned before, better apply the patch and look at it live.

janusman’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
5.11 KB

Something broken; picking values in the Content Types and Roles tabs shows nothing (or just periods) on the tab:

2009-12-04_132053.png

sun’s picture

Status: Needs work » Needs review

errrr... did you test the latest patch on latest Drupal HEAD and did you clear your browser's cache? This sounds and looks like one of the three conditions is not met.

sun’s picture

Status: Needs review » Reviewed & tested by the community

I seriously think that this raised issue is bogus, because I tested the latest patch manually. So back to RTBC.

RobLoach’s picture

Confirmed. I tested it today and didn't run into any problems. RTBC * 2. Janus, make sure you clear both your server cache and your browser cache.

yoroy’s picture

Status: Reviewed & tested by the community » Needs review

Sorry, overlooked the screenshot that was already posted above. Applied the patch and I'm reviewing it, already found some issues with the interface text changes. Posting full review shortly.

yoroy’s picture

Status: Needs review » Needs work

d7

The settings behind the 'Pages' tab start mentioning paths instead of pages. This makes me think. Don't make me think. Yet. Suggest to rename the label back to 'show block on specific pages'. Maybe the description below the path text area can help make the connection between pages and paths more clear. "Specify pages by using their paths. …"

The summaries for the 'no restrictions' state are repeating the labels ( '…by page, …by role' etc). This seems redundant.

d7

- Show for content types? Not sure 'for' is the best word here. Show along content types? Show with content types?
- The description still uses 'post' where we now want to use 'content' or 'piece of content'. Not introduced by this patch but we should fix it. First sentence could be reworded less formally as "Show this block only on pages that display content of the given type(s)."
- Since these settings are a mix of settings that either exclude or include visibility through making a selection, it makes sense to have the summary say 'Show on: (type), (type)' here.

d7

- Another 'broken promise'. The tab mentions 'Users' but this is not confirmed by the label. You are making me think again! :)
Maybe 'Visibility configurable by the user'? Not pretty but a lot more specific. Open to suggestions this one.
- The summary 'Not configurable *per* user' still sounds like a top-down setting that isn't in the hands of the user herself but some admin specific per-user setting. 'Not configurable *by the* user would make this more explicit.
- Remove the comma in the 'Show this block by default…' line.

sun’s picture

Status: Needs work » Needs review
FileSize
14.2 KB

Eat this.

Dries’s picture

Would be great to get sign off from @yoroy on the latest patch. Good idea? Bad idea? Meets your bar, yoroy?

sun’s picture

@yoroy: I am still thinking about the entirely-reversed-logic idea you raised. We should discuss that in a separate issue, because this one is really about using vertical tabs only. :)

yoroy’s picture

FileSize
14.21 KB

'Users' tab is ok now. Customizable is an ugly word, but it is what it is.
Welcome to d7 | d7

'Pages' tab had a spelling error and could do with one more tweak.
Welcome to d7 | d7

Tried to fix these in attached patch. Otherwise looking good. RTBC if this shows up green

One more observation: Bojhan and me both were not a fan of this proposal. It's been good to see you guys making your own design decisions, specifically in keeping the 'regions' setting out of this. With only the visibility settings grouped together we think this works now. Good work.

@sun: that's why I only mentioned it in IRC :)

sun’s picture

Status: Needs review » Reviewed & tested by the community

Yay! Thanks, yoroy! :)

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

David_Rothstein’s picture

Status: Fixed » Needs review
FileSize
2.05 KB
37.97 KB
38.14 KB

This really came out great - a joy to play around with.

Trying it out again, I have a nitpick with the page visibility options - makes a good followup (although some of the problems weren't caused by this issue):

  1. Since the committed patch removed the word "below", you're now forced to think (even for a split second) about where you look to find the list of pages. I don't like to think :) Solution: Just remove the form label on the text area below it, so the radio buttons refer naturally to the text area underneath (but leave the label for screen readers, of course, and while we're at it, make it a bit more descriptive for them).
  2. There were three different types of sentence phrasing on the three options, so I standardized them - and got rid of the "Show ... on" phrasing since we don't need to repeat the same phrasing that is in the title.
  3. Speaking of repetition, why repeat variants of the word "specified" four different times in the same section when a simpler and clearer word like "listed" can be used instead? :)
  4. The text for the PHP option was overall just weird, so I cleaned that up a bit too.

Before:
page-visibility-before.png

After:
page-visibility-after.png

Hopefully a fair amount cleaner now?

sun’s picture

I'd rename the first from "Every page" to "All pages", which I apparently did earlier, but yoroy reverted. The idea was to have the option's effect in the first word and also use "pages" (plural) for each:

- All pages
- Only ... pages

Furthermore, while discussing this entire issue/patch with yoroy in IRC yesterday, I had a monster WTF moment:

Why is a PHP-code-driven visibility setting intermixed with paths at all?

David_Rothstein’s picture

I'd rename the first from "Every page" to "All pages", which I apparently did earlier

Done.

Why is a PHP-code-driven visibility setting intermixed with paths at all?

Interesting question. I'm guessing because if it were a separate setting, then it's not clear which of the two would take precedence... on the other hand, we have that exact same issue already with paths vs content types, and the world hasn't ended yet.

Probably that should be for a separate patch, but it does sound like having "Advanced (PHP)" or what not as as a separate tab would make a fair amount of sense!

sun’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

Just a note on PHP, they're all stored the same and evaluated based on the radio option, it's not pretty in there - but separate issue entirely.

sun’s picture

+++ modules/block/block.admin.inc	7 Dec 2009 01:44:02 -0000
@@ -236,113 +241,122 @@ function block_admin_configure($form, &$
+  $form['visibility'] = array(
+    '#type' => 'vertical_tabs',
...
+    '#group' => 'visibility_settings',
...
+    '#group' => 'visibility_settings',
...
+    '#group' => 'visibility_settings',
...
+    '#group' => 'visibility_settings',

Just a note on the already committed patch: This should NOT have worked at all and is caused by an entirely broken implementation of vertical tabs.

I revamped the whole implementation and also fixed this wrong #group assignment in #657668: Vertical tabs break Form API

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed #67 to HEAD. This was just string clean-up and unrelated to sun's comment in #70.

I presume this is fixed now, since the "vertical tabs are entirely broken" issue (?!?) is being dealt with separately.

Status: Fixed » Closed (fixed)
Issue tags: -JavaScript, -Usability, -block, -vertical tabs

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