This is a patch which adds vertical tabs to the node form. Everything that was previously in core as a fieldset on the node form (besides the input format) is now a vertical tab, and every vertical tab has a summary of it's contents.

There are currently 2 items left to do (although there may be more based on comments): document the code and write theme-specific CSS. I can document the code but please don't ask me to design.

CommentFileSizeAuthor
#255 vt-screenshot.png42.23 KBLes Lim
#253 jumping_tabs_half.gif302.79 KBmatt2000
#225 group.mdown_.txt2.55 KBkkaefer
#224 #group.mdown_.txt2.55 KBkkaefer
#214 vertical_tabs_changelog.patch1007 byteskeith.smith
#206 field_fix_widgets_process-323112-206.patch10.64 KByched
#203 vertical-tabs-strong.patch1.45 KBkkaefer
#202 field_fix_widgets_process-323112-200.patch10.57 KByched
#201 field_fix_widgets_process-323112-199.patch10.46 KByched
#195 vertical_tabs.patch36.56 KBquicksketch
#192 vertical-tabs-18.patch32.91 KBkkaefer
#187 vertical-tabs-17.patch31.5 KBkkaefer
#180 323112-verticaltabs.patch34.43 KBRobLoach
#177 vertical-tabs-16.patch30.91 KBkkaefer
#175 vertical-tabs-15.patch30.49 KBkkaefer
#172 vertical-tabs-14.patch30.38 KBkkaefer
#171 vt_html_problem.gif53.36 KBLes Lim
#164 vertical-tabs-13.patch30.32 KBkkaefer
#162 fieldset-summaries.png26.38 KBkkaefer
#162 vertical-tabs-12.patch29.26 KBkkaefer
#151 vertical-tabs-12.patch24.79 KBLes Lim
#145 vertical-tabs-11.patch24.7 KBkkaefer
#138 ie6.png101.46 KBkkaefer
#138 ie7.png147.54 KBkkaefer
#138 ie8-ie8.png161.04 KBkkaefer
#138 ie8-ie7.png160.03 KBkkaefer
#138 ie8-quirks.png142.42 KBkkaefer
#138 Firefox when holding the mouse button down on the tab144.41 KBkkaefer
#138 Firefox when mouse button is released142.47 KBkkaefer
#136 vertical-tabs-10.patch24.47 KBkkaefer
#133 Win_XP_IE_6.PNG63.04 KBandreiashu
#133 Win_Vista_IE_7.PNG45.82 KBandreiashu
#132 vertical_tabs_FF_marquee.png14.71 KBandreiashu
#126 screenshot_323112.png26.1 KBkkaefer
#125 vertical-tabs-9.patch24.4 KBkkaefer
#123 vertical-tabs-8.patch23.73 KBkkaefer
#121 vertical-tabs-7.patch23.09 KBkkaefer
#119 vertical-tabs-4.patch29.27 KBkkaefer
#117 vertical-tabs-4.patch29.28 KBchx
#115 vertical-tabs-6.patch29.46 KBkkaefer
#114 vertical-tabs-5.patch20.51 KBkkaefer
#113 vertical-tabs-4.patch29.28 KBkkaefer
#112 vertical-tabs-3.patch28.88 KBkkaefer
#103 verticaltabs_frh.patch29.52 KBFrando
#100 verticaltabs_frh.patch29.48 KBFrando
#92 drupal_vertical_tabs_1.patch26.39 KBkkaefer
#91 drupal_vertical_tabs.patch28.61 KBquicksketch
#90 vertical-tabs-2.patch25 KBkkaefer
#81 drupal_vertical_tabs.patch28.52 KBquicksketch
#78 vertical-tabs.patch24.49 KBkkaefer
#72 vertical_tabs_323112-72.patch23.91 KBultimateboy
#68 vertical_tabs_-323112-58.patch23.89 KBbennybobw
#66 vertical_tabs_-323112-57.patch13.64 KBbennybobw
#66 vtabs_ie6.png12.56 KBbennybobw
#64 vertical_tabs_-323112-56.patch13.61 KBbennybobw
#63 vertical_tabs_-323112-55.patch13.57 KBbennybobw
#62 vertical_tabs_-323112-54.patch13.55 KBbennybobw
#59 vertical_tabs_IE6-323112-59.png5.6 KBLes Lim
#57 vertical_tabs-323112-53.patch23.67 KBcatch
#50 vertical_tabs-323112-50.patch23.67 KBdmitrig01
#48 vertical_tabs-323112-47.patch22.68 KBcatch
#46 vertical_tabs-323112-45.patch22.22 KBcatch
#41 vertical_tabs-323112-41.patch13.7 KBdmitrig01
#24 vertical_tabs-323112-24.patch22.01 KBmfer
#21 vertical_tabs-323112-21.patch13.93 KBmfer
#10 vertical-tabs-323112-10.patch23.44 KBdmitrig01
#8 vertical-tabs-323112-8.patch23.39 KBdmitrig01
Screenshot31.71 KBdmitrig01
vertical-tabs.patch22.16 KBdmitrig01
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dmitrig01’s picture

Note: this is a partial screenshot that cuts some stuff off. you'll have to install it yourself to play with it

Dave Reid’s picture

Seems odd having Input Format node option all by itself; could that also be in a vertical tab? Also feels like the option box needs some kind of heading or title to it. If the comment options tab is selected, the options "page" looks so empty. :( Degrades nicely without JavaScript, and this would be a nice +1 for usability.

yched’s picture

"Seems odd having Input Format node option all by itself; could that also be in a vertical tab?"

No, because the format selector is not a property of the node, it's a property of the 'body' field.
Other fields (CCK text fields...) will come with their own format selector attached.

Dries’s picture

I want. I think this patch should be in the patch spotlight. :-)

Only did a quick review but found several debug statements like, 'z' and 'console.log('hi');'. I'll do a 'deeper dive' once this patch is in a proper state (i.e. 'code needs review' instead of 'code needs work').

Keep up the great work, Dmitri.

Anonymous’s picture

This is a great way to reduce the amount of information on the screen. Editing a node and seeing the page seem to go on forever with no submit button in sight, is daunting.

I have two concerns with this approach that I think can be easily addressed:

  1. possible overuse: This example establishes a pattern that should be maintained throughout the core and in contributed modules. In the screenshot above, only the node's ancillary information is in the vertical tabs. The primary information remains visible at all times. We should never hide the information that's considered the most important on the interface. This is the pattern that Wordpress follows for blog post entries. This should be addressed in the User Interface Best Practices guide that's in development.
  2. accessibility: I haven't looked at the patch code yet, so I don't know how the output is structured in the markup. It would be great if we could make these as accessible as possible without breaking or hindering the visual interface.
birdmanx35’s picture

Subscribing.

dmitrig01’s picture

@#5
I can't deal with #1, but as for #2: the markup is structured like fieldsets.

dmitrig01’s picture

FileSize
23.39 KB

We can get theme support in later. Here's the patch with documentation - please review.

keith.smith’s picture

I know this is CNW, but in case you reroll:

+    // If it's not a full array (with keys scallback and arguments), find out what it is.

"scallback"? :)

+      // We do not immediately add the JS because otherwise it would be added before the collapse.js
+      // was added, but we need collapse.js to do it's magic on fieldsets first.

We don't normally abbreviate "JavaScript" as "JS". Also, "it's" should be "its" here.

+      // Otherwise add the CSS class denoting there is no descriptoin, so we can style it potentially differently.

"description"

+    // Put the <ul> in, but before the buttons
+    // Calculate the hight of the highest thing.
       // Attachments fieldset

End in period.

+    // If there is a callback, then call it and stick the restul in .description.

"restul"

dmitrig01’s picture

Status: Needs work » Needs review
FileSize
23.44 KB

fixed. please review

keith.smith’s picture

Yep. Except for the it's/its thing.

moshe weitzman’s picture

I like the UI and something like this is core worthy IMO. I have a couple significant issues though.

  • #vertical_tab should be named '#summary callback'. The value is a callback function which returns a summary of the tab.
  • I was surprised to see that the value of vertical_tagb is a javascript function. then i thought you were being sexy and updating the summary as the user checked boxes and entered textfields. but this patch does not do that. It *really* should do live updating. Did you ever try to get that going? If thats not feesible, then the summary should be provided by php.
  • It is a bit crazy to require a javascript function for each and every tab. We should have some helper functions which one can call with 'summary callback' and 'summary arguments' which will just get the checkbox value for you or textfield value or whatever. an alternatve is that we allow a jquery expression as the value.
  • 'promoted to front page' is too verbose for a summary. Should be 'Promoted'. Same for 'Sticky'. Authoring information should omit the date.
  • I think we could add a widget in the vertical tabs presentation which expands all the tabs and appends the form fields. It is just like an 'expand all' for fieldsets.
  • The summaries were empty when i previewed a new node. they reverted back to fieldsets when i edited the node. Maybe this is my own install bugging.
  • As dmitri says, we need to sex up the right hand side of the tab UI. It is painfully spare ATM.
alpritt’s picture

  • I'm a little concerned about accessibility here. Do screen readers ignore javascript? If they don't we've jumbled the text around for the field titles so much that it is going to be incomprehensible. We desperately need to find someone who can make informed comments on such issues.
  • The live updating was working in an earlier patch, so I guess it just broke here. It is needed, I believe.
  • I'm not getting any summary text for either comments or menus in Firefox, but am in Safari.
  • Like #12, I also have the summaries disappearing when previewing.
  • I don't believe we should be thinking about an expand all feature in this patch. It would be nice, but seems like feature creep for this issue.
  • I've started to play around with the design. I don't mind leaving that for a follow up issue if that helps push things along, but I'll continue to work on it regardless.
dmitrig01’s picture

  1. I'm mixed on this one - it's could also be a boolean, and it really determines whether a vertical tab will be rendered or not.
  2. This is slowed, but I guess I could do it. it operates on .change, but I could do .keyup, .keypress, etc.
  3. I like this point, a generalized one. Most are specific, but there are some that are more generalized
  4. Is this translatable? I'm just taking the titles of the checkboxes
  5. I don't think this would look good, so if you could mock it up, that would be great
  6. I haven't seen this but I haven't tried it either, so can someone else verify?
  7. This is a themer's issue - can we get the rest of it in first, and then get this in?
nielsvm’s picture

subscribing

mfer’s picture

Status: Needs review » Needs work

On a quick review I noticed that this doesn't look good in themes other than garland. Try marvin and you'll see what I mean. Also, why was the letter z appended to the comment.js file?

moshe weitzman’s picture

Status: Needs work » Needs review

@mfer - dmitri already asked for help with theming. and i'm sure the 'z' is a typo. this is at a stage where substantive reviews are welcome and needed.

alpritt’s picture

Status: Needs review » Needs work

@14:

2. As mentioned above the live updating is broken in this patch even for onchange; though it worked before.
6. Yes, I can verify. When previewing a node with a validation error, I get the tabs without the summaries. When previewing without a validation error, I get the old collapsible fieldsets. When editing an existing node or creating a new one, the tabs render correctly (apart from the problem I mentioned in comment #13.

Accessibility:

I just spent a few hours reading up on related accessibility issues. As I speculated above, screenreaders do not ignore javascript. So we have a few problems. This is going to be difficult to explain without a demonstration, but let me try to describe how it is experienced with a screen reader:

Action: User navigates to first tab
Spoken: "link: revision information don't create new revision"
Action: User clicks return key to open up link (they don't really know it is a tab)
Spoken: Nothing is said. User won't know anything has happened (this may depend on the screen reader, but is the case in some at least).
Action: User navigates to next tab
Spoken: "link: authoring information by admin on two zero zero eight one zero two six twenty hours two minutes forty seven seconds plus one hundred."
Action: Clicks return again, and again nothing appears to happen. The user continues navigating one section at at time, until they reach the last tab and finally make their way into the opened fieldset and to the edit box for changing the 'authored by' information. To get back to another tab they will have to cycle up each tab one at a time until they get to the chosen one.

This is how the page looks with CSS styles removed, which should help demonstrate the flow:
tab_accessibility

You can get an idea of this flow by tabbing through the page one link at a time. After opening a tab, the next focused item really should be the items in the opened fieldset, not the next vertical tab. Currently this would be annoying for anyone with motor skill difficulties, and verging on incomprehensible for the screen reader users.

So solutions:

a) Don't reorder the DOM elements. They need to be in the same logical order they were before. So the tab link should be followed by the fieldset.
b) We need a:focus to have a visual indicator for those navigating using the tab key (this was broken by setting outline: none.) You need to keep a visual indicator otherwise this is broken for anyone who has to use a keyboard.
c) The summaries should not be links, which will avoid them being read out along with the tab title.

moshe weitzman’s picture

@alpritt - fieldsets have all the same problems today. i would appreciate if you could pour your energy into the existing issue for this - #58941: 'display: none' causes problems for screenreaders. there is a lot of good discussion there. this change neither helps nor hurts screenreaders AFAICT.

alpritt’s picture

@19: That issue is related, but rearranging elements in the DOM makes it worse. The problem with display: none, is that some screen readers ignore it and some don't. And in the case of collapsible fieldsets, we have another problem in that when the link is clicked nothing seems to happen because the screen reader won't update the user of new data that has been added to the page.

However, with collapsible fieldsets, the user will discover the contents of the fieldset after tabbing to the very next element. With this current implementation of tabs, the next item is another tab so they will remain clueless until they randomly come across it after they have cycled through all the tabs. This is also a problem for anyone with motor disabilities, because it means they have to press more buttons to get into and out of their chosen fieldset.

However, if we do this right and keep everything in the correct contextual order (tab, tab contents; tab, tab contents; etc), we would not only not create any new problems that I can see, but we would also make the situation slightly better because now the action of the link is predictable. When activating a tab it will always show the contents of that tab as the very next thing. With collapsible fieldsets, on the other hand, activating the link may just as well close the fieldset as open it up.

So depending on how we implement this we do can either 'help' or 'hurt' screen readers.

mfer’s picture

The attached patch helps clean this up to work in other themes (at least the other core ones).

@alpritt - one possibility to avoid display:none is instead to set the region to height: 0; overflow: hidden;. Would this be more accessible to screen readers?

alpritt’s picture

@21: Maybe. That would make it more consistent, but would just mean everything is always read out which I'm not sure is the desired behaviour. In any case, that particular debate does belong in the other issue.

Your patch does not contain about half the files it needs to work.

mfer’s picture

@alpritt oops. I see what I did. Forgot the new files in the patch. I'll post the changes later today when I'm back at my dev box.

what about structuring this like a definitions list. For example:

<dl>
  <dt>tab</dt>
  <dd>
    tab contents
  </dd>
  <dt>tab</dt>
  <dd>
    tab contents
  </dd>
  <dt>tab</dt>
  <dd>
    tab contents
  </dd>
</dl>

Then we can position everything in order. We may not want to use this exact syntax. The same thing can be accomplished with div but just this general idea.

mfer’s picture

Here's a patch with all the files to help the css along.

alpritt’s picture

This is just a quick thing and has some problems, but I'm thinking keep the design very simple; especially for the base theming.

verticaltab-design

dmitrig01’s picture

The summaries were made into links so the whole tab was clickable

Owen Barton’s picture

Subscribing

Dries’s picture

Issue tags: +Favorite-of-Dries
ximo’s picture

@dmitrig01: You could make the whole tab clickable with jQuery and add a "hand" cursor with CSS. Users with JS disabled would still see the link, so no big loss there.

I also think alpritt's info on accessibility is very important, it would be great to deal with this now that we're amping up fieldsets. Using definition lists is a smart idea, although not extremely semantic. Maybe normal divs work just as well, with one class for tabs and one for content?

SeanBannister’s picture

+1

webchick’s picture

Component: usability » node system
Issue tags: +Usability

This is a favourite-of-webchick, too. :)

webchick’s picture

mfer’s picture

The holdup on this, as far as I am aware, is addressing the accessibility concerns. Does anyone know the 'right' way to address these?

Owen Barton’s picture

I think #21 clearly addressed the accessibility concerns (although the exact CSS approach could vary, as long as it is a well tested visual-only hiding technique). #23 seems like a step backwards because it would not (I assume) use semantic fieldsets.

With the current crop of screen readers having everything read out (in a sensible order, with semantic, skippable HTML) is probably the best we can aim for - once we start making things such that they are not read out we will inevitably hit lots of compatibility problems, as well as the fundamental issue in that there is no way for the page to signal to the screenreader that new content has been made readable on the page.

Gurpartap Singh’s picture

Xano’s picture

Subscribing.

Great idea, Dmitri!

lut4rp’s picture

int’s picture

Subscribing

catch’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

dmitrig01’s picture

Status: Needs work » Needs review
FileSize
13.7 KB

rerolled

Status: Needs review » Needs work

The last submitted patch failed testing.

quicksketch’s picture

Not sure how I missed this. Sub.

alexanderpas’s picture

karschsp’s picture

subscribe

catch’s picture

Status: Needs work » Needs review
FileSize
22.22 KB

Patch didn't add the new js or css files. edit: but it's still not working for me. Leaving at CNR for another look.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
22.68 KB

With menu.js this time.

catch’s picture

Priority: Normal » Critical

The vertical tabs don't actually work for me on this - just get fieldsets as before.

If anyone could a. test it themselves and report back, and b. fix it if they happen to find what the issue is, that'd be great. We want to include this in Usability testing in Baltimore in less than a week.

dmitrig01’s picture

Fixed it

Les Lim’s picture

Just applied the patch in #50 and it isn't working. The vertical-tabs.js file needs to be loaded before its dependents; right now, it's the last script loaded in .

Les Lim’s picture

Here's the same patch as #50, but when drupal_add_js is called, it gives a weight of 0 instead of 3.

Serious +1 for seeing this in D7.

Les Lim’s picture

Once upon a time, there was a patch attached to #52. All right, no problem; it's line 102 of the patch in #50, or line #1959 of form.inc after patching.

catch’s picture

Same patch with weight 0 as described in #53. Please test.

catch’s picture

ejort’s picture

subscribe

catch’s picture

Trying the upload a different way.

dmitrig01’s picture

+(function($) {
+
+Drupal.verticalTabs.nodeFormAttachments = function() {
+  var size = $('#upload-attachments tbody tr').size();
+  if (size) {
+    return Drupal.formatPlural(size, '1 attachment', '@count attachments');
+  }
+  else {
+    return Drupal.t('No attachments');
+  }
+}
+
+})

needs a (jQuery) at the end

Les Lim’s picture

Sorry, the patch in #50 is fine. My reasoning didn't feel right, so I tried it again on a fresh install. I must have applied it wrong before.

A few problems:

1) Previewing is still an issue, as reported above. if I preview/save with a validation error, the summaries disappear. if I preview it and there's no validation error, I get a weird doubling in the tabs; the tab titles repeat themselves like "Publishing options,Publishing options" and there are no summaries.

2) In IE7, the summaries never show up, even on a clean node add form.

3) In IE6, the tabs don't show up at all. Screenshot attached.

yoroy’s picture

Issue tags: +ui-pattern

tagging. Getting the idea we're nearly there with this.

bennybobw’s picture

So when the page first loads, form_process_vertical_tabs is called once. When you click preview (or save with a validation error), it's called twice, so the resulting JSON data looks different:

"verticalTabs":{"edit-revision-information":{"name":"Revision information" 

vs.

"verticalTabs":{"edit-revision-information":{"name":["Revision information","Revision information"]

That's why we're getting some wonkiness on save/preview.

bennybobw’s picture

Here's a patch to try to fix the JSON issue.

bennybobw’s picture

line 71 of vertical-tabs.js v.args should be v.arguments. Or 'arguments' should be changed to 'args' in form.inc I'm not sure what the precedent is here. This patch is the same as the last on, but with v.arguments.

It's still throwing an error in IE though.

bennybobw’s picture

.apply requires an array, not a string as the second argument. Patch attached. This fixes the error in IE.

dmitrig01’s picture

your patch didn't add the new files. see http://drupal.org/patch/create#new

bennybobw’s picture

So it wasn't showing up in IE6 because of the double float margin bug, so I added display: inline to the list. Unfortunately, it's causing a weird layout issue in IE6 and IE7.

bennybobw’s picture

Crap. Sorry bad patch. I was wondering why mine was so much smaller.

bennybobw’s picture

Reroll with new files....
I think I got them all....

dmitrig01’s picture

So how does this solve the problem?

bennybobw’s picture

There are three problems that the last patch (finally with all the files :-)) helped solve

1) Form_process_vertical_tabs now only adds each element to the JSON data once. When the form was previewed or re-displayed after a validation error, it was adding the elements twice and this was messing with the js -- the callback wasn't being added at all (giving an undefined value) and the names were being printed as "Form name, Form name" (see comment 59).

2) .apply requires an array not a string so the default is now an empty array being passed as '#arguments'

3) display: inline -- The tabs show up in IE6 now (before they were off the screen), but for some reason the top line renders as indented.

PixelClever’s picture

subscribing

ultimateboy’s picture

Small tweak to the CSS to make the right side line up with the rest of the form elements on the page. Simply added 5% margin.

+.vertical-tabs {
+  margin-left: 180px;
+  margin-right: 5%;
+}

Status: Needs review » Needs work

The last submitted patch failed testing.

Bevan’s picture

Oh cool! I can't believe I only just found out my Season of Usability work haas finally fruitioned into a core patch!

dmitrig01’s picture

Status: Needs work » Needs review

Let's run this by the bot again.

dmitrig01’s picture

Also - it seems the fail is COMPLETELY unrelated - "user language settings". I think the fail is there before and after

kkaefer’s picture

Status: Needs review » Needs work

Here are some issues I found:

  • There are no outlines on links in tabs which makes it hard to spot which tab is selected when tabbing
  • Margins and paddings should be improved
  • Garland should get a look that matches the theme
  • When previewing, subtitles don’t appear
  • Active tab is not really obvious (too little contrast)
  • Node tabs should get better description, e.g. the comment tab (this is probably another issue)
  • The script assumes that only one vertical tab section is present per page
  • Names of callback functions have to be specified explicitly, but should not be necessary or should be autogenerated
  • Should not add itself at the very top of drupal.js to Drupal. No other JS widget in Drupal does that.
  • Should use theme functions for creating markup
  • Attaching vertical tabs works by adding them to the list in Drupal.settings. A better way would be to add classes to elements that should appear as tabs.
  • Height is set by JavaScript but could also be set automatically by CSS
  • JavaScript assumes that the vertical tabs appear right before the submit buttons
  • Script should not remove the fieldsets but instead hide the legend and border via CSS
kkaefer’s picture

FileSize
24.49 KB

Patch fixes some of the issues. Upload is not yet working correctly. Comments for JS are missing and JS could be refactored a bit to reduce code redundancy.

kkaefer’s picture

functionality works in IE7 but styling is a bit off. This might or might not be fixed in a future version of the patch.

quicksketch’s picture

Assigned: dmitrig01 » quicksketch

I'm working on fixing the problems described in #77.

quicksketch’s picture

FileSize
28.52 KB

This patch makes the following changes:

- Styling is now consistent between Safari, FF, and IE7. IE6 looks a little bit different since it doesn't support min-height, no big deal though.
- I had to switch back to using JS to set the height of the panes, since IE doesn't support display: table.
- Added Garland theming.

yoroy’s picture

Status: Needs work » Needs review

bot, if you'd be so kind…

kkaefer’s picture

The problem with fixing the height is that it may cause problems with pane contents changing size (e.g. if there's a collapsible fieldset within a vertical tab pane). It should be possible to make IE6/7 display it correctly by using conditional comment css. The ul and the container can be floated left with appropriate widths.

quicksketch’s picture

The problem with fixing the height is that it may cause problems with pane contents changing size (e.g. if there's a collapsible fieldset within a vertical tab pane).

Hmm, yes this is true. I don't see how display: table would have fixed this problem either though, wouldn't it have caused the sizing to change as you switched between panes? The new version I posted in #81 uses min-height rather than height, so it won't cause content to be lost or hidden at least.

As for using conditional CSS, core currently doesn't have any conditional CSS (other than in Garland), nor does it have a mechanism for supporting conditional CSS other than hand-typing it in the theme. Whatever we come up with, it'll need to work in all browsers without conditional comments.

kkaefer’s picture

I checked the patch and was wondering whether you removed the formatting (bold for selected tabs and the gray lines) on purpose or whether that was done unintentionally.

kkaefer’s picture

When using display:table, the pane automatically adapts its size to the contents. min-height probably does something similar, but it still feels a bit unclean to set the layout with CSS...

We currently don't have conditional comments; that's true. But does that mean that we may not add it to drupal_add_css? Also, we might be able to come up with a solution using the star hack - or none at all and let it degrade in IE.

Also, why are we using width:95%?

Les Lim’s picture

We could alternatively apply CSS conditionally through jQuery browser sniffing.

quicksketch’s picture

Also, why are we using width:95%?

It was part of the consistency changes in #72, though really we should simply put that 95% in the Garland style.css rather than vertical-tabs.css, to match Garland's styling of other fieldsets.

It's also worth thinking about how we could even possibly accomplish the same effect in IE even if we did use conditional CSS. I spent a good 3 hours coming up with the current styles which are consistent across IE7/FF/Safari, but without manually setting the height or using JS, I don't know of any way to accomplish the same effect.

ejort’s picture

Just a quick note that 'height' has the similar behaviour in IE 6 to 'min-height' in other browsers. So if you're just looking to apply a minimum height you can apply it for IE6 as "_height: FOOpx".

kkaefer’s picture

FileSize
25 KB

This CSS reproduces the display: table effect in IE6,7, Firefox3 and WebKit. Garland styling is still missing.

quicksketch’s picture

FileSize
28.61 KB

Wow, much much better. Thanks kkaefer. Talk about being shown up in the CSS arena. :)

Same patch as #90 with Garland default styling and color module support.

kkaefer’s picture

FileSize
26.39 KB

Fixed a bug where background didn't show up properly in WebKit and another bug which I don't remember right now :(. We should have someone who is familiar with RTL look over this patch.

kkaefer’s picture

Uploading doesn't work but the tests don't seem to catch it. This needs a test for uploading.

kkaefer’s picture

Status: Needs review » Needs work
Les Lim’s picture

An issue with IE6: the tab status only appears to update when the changed element loses focus. That's fine and dandy for text inputs, but when clicking on radio buttons and checkboxes it has the effect of making the interface appear perpetually one step behind.

An unrelated suggestion is to add a status flag for "Not published" when "Published" is unchecked.

dmitrig01’s picture

Testingbot doesn't do JS, that's why it works.

dmitrig01’s picture

I don't have a proper working environment to roll a patch, but to fix upload, replace

$cached_form['attachments']

with

$cached_form['miscellaneous']['attachments']

in upload.module

chx’s picture

Such an issue with IE6 is an issue we do not need to fix. If it works approximately, that's good, but it's time to cut off IE6. It's a serious waste of time and effort to make Drupal work 100% on IE6.

kika’s picture

Sorry for coming likely too late to the issue. While I really do appreciate all the hard work what have went into the patch, I'd still like express some concerns:

1. Not enough abstraction

Let's back one level up for a moment. What is what we redesign here? Existing node settings is based on UI pattern called Closable Panel . What we do in this patch we replace that hardcoded UI pattern with other hardcoded UI pattern Card Stack, specifically one of the many variations of it: "Column of names" / "Vertical Tabs".

This hardcoding is not good, this makes re-usage and UI elements way harder. My proposal is to split the data structure and actual presentation of the pattern. Latest advancements in our drupal_render()able page structures allows us to do it.

So, instead enveloping stuff into historic wrapper...

['stuff']['#type'] = "fieldset" ;
['stuff']['#title'] = t('Stuff');

['stuff']['1st stuff'][ ...
['stuff']['2nd stuff'][ ...

...what is bolted to an existing HTML form element we put our containers of stuff -- "cards", "sheets", "panels", "layers", whatever we call them -- into a semantically more abstract wrapper.


['stuff']['#type'] = "stuff_container" // actually we need semantically sound name ;)
['stuff']['#title'] = t('Stuff');

['stuff']['1st stuff'][ ...
['stuff']['2nd stuff'][ ...

After that we can set a default presentation/rendering of these wrapped sheets using #theme (or some other specially introduced) property. Say we use vertical tabs:


['stuff']['#type'] = "stuff_container" 
['stuff']['#title'] = t('Stuff');
['stuff']['#theme'] = "tabs_vertical"

['stuff']['1st stuff'][ ...
['stuff']['2nd stuff'][ ...

Setting ['#theme'] = "tabs_vertical" renders our set of sheets into something what this very patch tries to implement.

Note that this is just one of the many forms of presentation/renderings of a card stack / closable panels. We could as well use other rendering (never mind the random names):


['#theme'] = "closable_panels" // rendering into current collapsible fieldset-based panels

['#theme'] = "horizontal_tabs" // a horizontal variation of tabs

['#theme'] = "dropdown" // a new text format selector introduced recently in D7 [1]

['#theme'] = "accordion" //  [2]

[1] http://drupal.org/node/304330
[2] http://www.welie.com/patterns/showPattern.php?patternID=accordion

And the best bit: these navigable sheets can be used EVERYWHERE. It also might map with the proposed "rule-based theming" Young Hahn is talking about http://www.developmentseed.org/blog/2009/mar/01/limitations-drupal-theme... if I got it right.

2. Technically complex and unhelpful tab descriptions

I kind of understand the motivations to provide tab descriptions as a "1-liner mini property sheet" but they do not seem to fill their original purpose -- a glimpse overview of node properties without actually opening tabs. There are serious UI constraints: a single-line description just can not carry all the information what's hidden inside tab. I see "the most important bit" have been chosen but based on what metric this is chosen? Why "Authoring information: By admin" carry more value than, say, "Authoring information: 12. March?" (In, say, single-user blog constantly looking at "By admin" carry no value) That contents of surfaced "important bit" seems wildly vary between sites and use cases.

Also, the whole JS-magic going on in the patch to update the descriptions is a bit scary and adds lot of stuff to each module. Do we absolutely need it? Could we introduce it, erm, later?

Frando’s picture

Status: Needs work » Needs review
FileSize
29.48 KB

Well, regarding 1: The thing is, all of the vertical tabs is purely JS. There's not really a need to use a different theme function at all, because the vertical tabs *ARE* rendereded as fieldsets, not as vertical tabs. The only required HTML markup is a wrapper div with the right class set. That's it.

So regarding semantical correctness: I'd say this is fine. We keep printing stuff containers as fieldset, and if JavaScript is enabled, we make them into vertical tabs. All this is perfectly fine alterable via hook_form_alter (i.e., add a different wrapper div / class and do whatever you want in js).

Regarding no. 2: The JS magic to update the desriptions is purely optional. I think what we have is a lot better than no descriptions. It would be a pity not to include it in the patch, IMO. They can still be improved after this goes in.

The attached patch:
* adds a #theme function for the vertical tabs container, to make it simpler for developers to use (no need to deal with #prefix, #suffix and class names, just add #theme = 'vertical_tabs_panes' to the container element).
* removes the dotted outline for vertical tab links, which looked ugly
* fixes upload as per dmitig's suggestion. works fine now!

kika’s picture

Another html-semantic problem is that collapsible fieldsets originally only took care of form elements. As we try to move towards generic navigable container what could contain _anything_, do we still need to always wrap it into html even when containers contain no form elements whatsoever?

kika’s picture

I likely completely miss the formapi and JS workflow here but why why node.pages.inc needs to inject

+ drupal_add_js('misc/vertical-tabs.js', array('weight' => 0));
+ drupal_add_css('misc/vertical-tabs.css');

or know anything about how vertical tabs are actually rendered? Could it be so all you need to do is set #theme = 'tabs_vertical' to a containter and all the vertical tabs (or collapsible fieldsets or accordion or...) is created magically for you?

Frando’s picture

FileSize
29.52 KB

#102: Good point. Moved the drupal_add_css and drupal_add_js calls into the theme function.

kika’s picture

again, perhaps a complete miss of JS callback logic but...

All Drupal.behaviors.*VTab look very similar: they pass the value of certain field in fieldset to a parent tab. Dependent of field contents sometimes raw value is not suitable, then the value is replaced by some string.

What if this callback logic could be defined in FormAPI structure itself, basically marking a certain FAPI field "pass this value to parent" by special property? (Dunno about more complex field value replacement). Then attach some generic JS to a wrapper what identifies these specially marked items inside form and "listen" these properties and update outer wrapper tab desrctiptions.

Sorry if this is bshit.

kkaefer’s picture

Status: Needs review » Needs work

@Frando: Please re-add the ugly border (in Firefox; Safari renders it differently). Otherwise, it's impossible to see what tab you have focussed when navigating by keyboard.

@kika: I said in an earlier comment that these Drupal.behavior.*VTab functions need to be refactored so that have less duplicate code.

jrabeemer’s picture

Bug:

If I have a user named admin, I type "a", the drop down pops out, I click admin, I click off, the tab says, "By a" It only updates based on what is typed.

I don't think the event update is firing.

kkaefer’s picture

@momendo: good catch. This bug may also be browser dependent on whether the browser fires the "onchange" event when the content is programatically set (by the dropdown script).

dmitrig01’s picture

@kkaefer I doubt it does, and i'm not sure this is a browser thing. Maybe autocomplete should do it.

jrabeemer’s picture

I'm having trouble making a diff patch with my CVS. This patch is fricking huge, so here's the one line fix.

Change line 19 in vertical-tabs.js
.change(function() {

to

.bind("change blur", function() {

The event wasn't firing consistently but blur catches it when you click off the field.

Or this might be better. It updates in realtime instead of lazily updating.

.bind("click change blur", function() {

kkaefer’s picture

Status: Needs work » Needs review
FileSize
28.88 KB

Updated the patch to most recent Drupal HEAD.

Refactored the way description callbacks are attached. Instead of

Drupal.behaviors.nodeMenuVTab = {
  attach: function(context) {
    $('#edit-menu').data('verticalTabCallback', function() {
      return $('#edit-menu-link-title', this.fieldset).val() || Drupal.t('Not in menu');
    }).each(function() {
      Drupal.verticalTab &amp;&amp; Drupal.verticalTab.update(this);
    });
  }
};

we now just write

Drupal.verticalTab.addDescription('edit-menu', function (context) {
  return $('#edit-menu-link-title', context).val() || Drupal.t('Not in menu');
});

to attach the description callback to a fieldset. For this to work, vertical-tabs.js has to be included before the calls to Drupal.verticalTab.addDescription(). The vertical-tabs.js now has a weight of JS_DEFAULT - 1.

There’s a small rendering bug in IE7 and below (not occuring in IE8 and IE8 with IE7 mode enabled): When the window is resized, the tab column is misaligned until the user clicks on a tab (it then jumps to the correct place). I think it’s good to go since this is not a major bug which only occurs in rare cases.

Another major known issue is that the descriptions are missing after the form has been submitted. I suspect this is due to the form function not being called on subsequent form renderings (the form data is cached). That means, that calls to drupal_add_js which add the JS files with the description callbacks are not executed when the form is rendered from cache. We need a more generic facility for handling that kind of behavior and I consider it out of scope for this patch.

I tested in Safari 4/WebKit nightly, Firefox 3.0, IE 6/7/8. I am not aware of any more bugs that need to be fixed or code that has to be rewritten in this patch.

kkaefer’s picture

FileSize
29.28 KB

As per chx’ request, I removed #processing for non-#input elements which was needed by a previous version of the patch. Form IDs are however still generated for non-#input elements. This patch also has more JavaScript documentation.

kkaefer’s picture

FileSize
20.51 KB

This is an alternative patch and requires http://drupal.org/node/409464. It fixes the Pareview problem.m

kkaefer’s picture

FileSize
29.46 KB

Patch from #114 with missing files. If RTBC, please use patch #113.

Status: Needs review » Needs work

The last submitted patch failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
29.28 KB

I am reposting Konstantin's great patch so that we can RTBC. Also http://vt.drupal4hu.com/node/add/page user vt/pass vt welcomes testers. Please do not credit me! this is a repost.

catch’s picture

Status: Needs review » Needs work
+  // Add required JavaScript and Stylesheet. It's 
+};
+
+
+Drupal.verticalTab.prototype = {

extra line break?

+  // Updates the tab's description

no period.

$form['miscellaneous']['menu']['link_title'] 

Can someone explain why we use $form['miscellaneous']? Doesn't seem very inviting as a place to put stuff.

On the demo site, the first tab (menu settings) has a garland gradient background when active, all the other tabs have a white background. I know why this is but it looks a bit weird. I'd rather see the gradient applied only to the right hand div - or possibly not at all.

This is all minor stuff, looks a lot cleaner than earlier revisions.

kkaefer’s picture

Assigned: quicksketch » kkaefer
Status: Needs work » Needs review
FileSize
29.27 KB

The extra line break is for visualizing that the prototype section begins and left there intentionally. We can remove it, though.

I chose `$form['miscellaneous']` because I needed a place to put stuff. We can of course rename it to anything we want.

In Garland, applying the gradient to only one side looks weird. Applying it to tabs which are below the first looks broken as well. We can remove the gradient completely or leave it like it's now.

quicksketch’s picture

Regarding "$form['miscellaneous']", I'd like to consider some alternatives. We currently have 3 options that I can see for telling form elements to be a part of vertical tabs:

  1. Each element that needs to be in a vertical tab set has a custom property like "#vertical_tab", which identifies which vertical tab set it should be placed. This was the original approach in this issue, only that it assumed a single set of vertical tabs in the form (afaik). Making the #vertical_tab property specify a vertical tab set would allow for multiple sets per form.
  2. Put all the elements we want in a vertical tab set within a single FAPI parent. This is the current approach which kkaefer added in #78. It should also be able to handle multiple tab sets per form.
  3. Make a vertical tabs element, which specifies which other form elements should be within it. This is the approach used by the Drupal 6 Vertical Tabs module. This has the downside that it can only reasonably support one vertical tab set per form, since things could go awry if two vertical tab sets claimed the same element.

I'm going to discuss with kkaefer in IRC at the moment, and I'll post what sort of conclusions we come to about these three options.

kkaefer’s picture

FileSize
23.09 KB

This patch doesn’t group tabs into another element anymore. Instead, '#vertical_tab => 'additional_settings' is used to group fieldsets into a vertical tab widget. For each vertical tab widget, a form element has to be created:

$form['additional_settings'] = array(
  '#type' => 'vertical_tabs',
);

which represents the location of the vertical tab widget.

A #process functions for fieldsets having that property set also adds '#printed' => TRUE so that the fieldset is not printed where it usually should be printed. The theme function for the vertical tabs widget adds the original elements as children to itself (using references) and resets the #printed attribute so that when rendering, the fieldsets appears as if they were in the vertical tabs widget. This has the advantage of not affecting the form structure at all, so validating continues to work and form elements stay where they are.

Additionally, the #vertical_tab_js and #vertical_tab_css attributes have been added. They have to be an array; their children are either paths for use with the respective drupal_add_* function or an array with parameters to pass to that function. This solves the form caching problem outlined in #112.

chx’s picture

  1. http://vt.drupal4hu.com/node/add/page user vt/pass vt welcomes testers of #121.
  2. Although this patches fires#process on non-input elements and consequently removes $edit from the #process signature, it's not a big deal -- for example, nor core nor CCK used this and equivalent functionality is now available with #value_callback.
  3. This is a fine stopgap solution for the CSS/JS storage problem until a more generic solution is written and added to core. This is linked above and should not hold up this patch.
kkaefer’s picture

FileSize
23.73 KB

Some minor improvements:

  • Now respects the weights of fieldsets when putting them into the vertical tabs widget. That means also that Menu options are now at the op by default.
  • When previewing the node (or for validation errors), the tab that was selected before the form was submitted is automatically selected instead of the first tab.
  • A default tab can now be set for a vertical tab widget by specifying the HTML ID (!) in #default_tab.
quicksketch’s picture

This introduces two new types: #type = 'vertical_tabs' and #type = 'vertical_tab'.

I couldn't find any use of the 'vertical_tab' type, is it necessary?

If we introduce vertical-tabs.css we'll need to make vertical-tabs-rtl.css also.

Thanks kkaefer for these excellent improvements. I feel relieved that this patch is getting the proper attention to how vertical tabs works architecturally with FAPI in addition to the attention its already received regarding front-end usability.

kkaefer’s picture

FileSize
24.4 KB

The type 'vertical_tab' is a leftover and is removed in this patch.

I added RTL support in this patch, but since I don't know a RTL language, someone who is more familiar with RTL support needs to check this.

kkaefer’s picture

FileSize
26.1 KB

This is a screenshot of the RTL interface:

kbahey’s picture

chx setup a test site with this patch, and asked me to take a look. It was RTL but in English still.

Judging from kkaefer's screenshot above with Arabic, and chx's test site, I can't see anything that is obviously wrong.

So let us go ahead with this. It is far better than the clutter we have now in node/add/blah

keith.smith’s picture

Are the untranslated bits in #126 just strings that aren't translated in that language pack?

kkaefer’s picture

Yes, it's because the Arabic translation I used was (understandably) not updated for Drupal 7. The strings that didn't change were translated, however. JavaScript translation works as well (see the tab description texts, which are generated by JavaScript).

int’s picture

Status: Needs review » Reviewed & tested by the community

I think that, we can now commit this, and after that, we can improve others functionalities if are any more left...

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Let's get some folks testing this in different versions of Firefox, IE, Safari, Opera, etc. and post back with your results.

andreiashu’s picture

FileSize
14.71 KB

I'm sure that I'm not the only one that is annoyed by that marquee thing that FF puts on active anchor elements.
We can fix that with an outline: 0 none; css property for active anchors at least for Vertical Tabs.

This is valid only for Firefox afaik.

andreiashu’s picture

FileSize
45.82 KB
63.04 KB

Tested the basic functionality (access 'node/add/page' and clicked on tabs) on:
Win XP
FF - oki
Opera - oki
Safari - oki
IE 6 - not oki (no surprise here = attached screenshot)

Vista
IE 7 - no oki (attached screenshot)
FF - oki
Chrome - oki

Note that on both IE6 and IE7 if I click on each of the tabs then everything re-renders correctly - the bug that you see in the screenshots appears only when you access the page.

Xano’s picture

Just something that crossed my mind: how well does this work with rather wide content, like at admin/build/modules?

andreiashu’s picture

@Xano I don't understand. At admin/build/modules I don't get vertical tabs.

kkaefer’s picture

FileSize
24.47 KB

Patch fixes the shifting column in IE 6, 7 and 8 in LTR mode. I couldn’t find a fix for RTL mode that didn’t involve messing with direction: :(

webchick’s picture

Status: Needs review » Needs work

IE issues need fixing. Thanks a lot for testing, andreiashu !

kkaefer’s picture

Status: Needs work » Needs review
FileSize
142.47 KB
144.41 KB
142.42 KB
160.03 KB
161.04 KB
147.54 KB
101.46 KB

Patch #136 also removes the outline from focussed active tabs (it's still present on other tabs to preserve keyboard navigability).

Attached are some screenshots in various browsers.

andreiashu’s picture

I could also retest on ie7 + FF + chrome on Vista.
Everything seems oki now :)

Great work !

Xano’s picture

@Xano I don't understand. At admin/build/modules I don't get vertical tabs.

I haven't tested the patch, so I don't know which fieldsets are being converted and which ones aren't, but it was just a random example to illustrate that things can go wrong if the tab's contents are too wide.

//Edit:@#141: My bad. I forgot after so long a time.

andreiashu’s picture

@Xano: read the first two lines of this issue's description :)

Dries’s picture

I reviewed this patch and support it (hence the Dries' favorites tag). It can be committed once the browser issues have been taken care of. Good work all.

andreiashu’s picture

@Dries as far as I know, everything works oki now in all major browsers (FF, IE, Opera, WebKit flavors (Safari, Chrome)). There were some problems in IE but kaefer solved them in #136.

Is there any browser/platform you want to test on ?

catch’s picture

Status: Needs review » Needs work

No phpdoc here:

+function form_process_vertical_tabs($element, &$form_state, $entire_form) {
kkaefer’s picture

Status: Needs work » Needs review
FileSize
24.7 KB

Added PHPDoc to said function.

@Dries: The only remaining browser problem I am aware of is that the tab column is misaligned in Internet Explorer 6 in right-to-left writing mode until the user clicks on the widget (it then jumps to the correct spot). I tried approximately two hours to solve this particular problem in Internet Explorer but could not come up with a solution that didn't involve questionable hacks (I found one solution that involved setting the writing mode to left-to-right for the vertical tabs wrapper and then setting it back for the contents). I will not spend more time on solving this problem in Internet Explorer 6. It's still possible to use the tabs in IE6; nothing is hidden, overlapping or not working; it's just a misalignment: the tab column is on the wrong side of the tab panes until the user clicks on a tab or the tab pane.

webchick’s picture

kkaefer: Could you post a screenshot of the problem?

kkaefer’s picture

catch’s picture

Looked at the video. IE6 in RTL, when it's perfectly usable, doesn't seem like a sufficient reason to hold this up. Going to be a very tiny percentage of users, especially by the time we release.

We put sticky tableheaders into Drupal 6, and they'll never work in IE6, ever. So having a degraded experience in IE6 already has a precedent. Only thing I'd ask is what about falling back to collapsible fieldsets in IE6/RTL? IMO that'd be worse than the bug shown in the screencast though.

jrabeemer’s picture

Is there any way to trigger a form event to force IE6 to reformat? That would require some JS and no CSS hacks.

chx’s picture

Status: Needs review » Reviewed & tested by the community

I do not think such a minor problem should hold such a major UX advantage.

Les Lim’s picture

FileSize
24.79 KB

Instead of outputting nothing for the descriptor if "Published" is unchecked, I'd still like to see a "Not published" descriptor under "Publishing options". Here's the relevant part of node.js in the latest patch (#145):

Drupal.verticalTab.addDescription('edit-options', function (context) {
  var vals = [];

  $('input:checked', context).parent().each(function() {
    vals.push($(this).text());
  });

  return vals.join(', ') || Drupal.t('None');
});

The following snippet added just before the return would do it:

  if (!$('#edit-status').is(':checked')) {
    vals.unshift(Drupal.t('Not published'));
  }

Patch rerolled and attached.

int’s picture

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

http://vt.drupal4hu.com/node/add/page is updated. It's switched to RTL.

Bojhan’s picture

From a usability perspective this seems RTBC to me, lesmana's patch makes sense and all the other visual elements have remained intact during this lengthy process. As chx stated, the minor problems that occur now shouldn't hold this issue off much longer, looking forward seeing this patch in D7.

Dries’s picture

Just spent 40 minutes with this patch. I think we're very close to being able to commit it.

1. I'd like to discuss the keyword '#vertical_tab' and '#vertical_tab_js' in the form API. At the form API level, it is probably better to describe a feature rather than an appearance? Maybe someone wants to theme these /options/ differently? It shouldn't matter that it is 'vertical'. I'm not sure it is easy to clean-up and to keep a clear separation throughout the code but I wanted to bring it up.

2. "Handles reset of the vertical tabs storage reset." -- I don't understand this description. It should also answer why this is useful.

3. "Handles addition of fieldsets to a vertical tabs widget." -- It doesn't explain why that function is handy.

Let's address 2 and 3, and discuss 1.

Dries’s picture

Slightly off topic but here is a video of a very cool Drupal interface builder: http://is.gd/qH85. Implementation details at http://is.gd/qH8n.

I think Vertical Tabs is easier to use because of the status messages, but I really like the flexibility demonstrated in that video.

What do you guys think of this, and how does it affect Vertical Tabs? I'm bringing this up because I don't want to paint ourselves in a corner.

jrabeemer’s picture

That's amazing. I thought I was watching a Joomla demo. It reminds me of Wordpress's drag-n-drop interface in 2.7 Coltrane. I'm not advocating it, but we have to keep perspective.

Let's watch Wordpress's chameleon-like interface...

http://wordpress.org/development/2008/12/coltrane/

*Photoshop-style collapsible menu-to-icon admin
*Drag-n-drop panels-like blocks
*In-place editing like drupal.hu.
*GUI Installable plugins

Anonymous’s picture

I am the developer of Interface, the module Dries references in his previous post.

Over on the Trellon blog, I tried to describe how vertical tabs and Interface would co-exist. Interface is going to be released with a behavior called Tabbable, that will allow people to create tab groups in a way that is very similar to vertical tabs. It would also offer horizontal tabs, I guess you could say, and right vertical tabs, and underneath horizontal tabs, and lots of other ways to put tabs into a Drupal interface.

In terms of how these would play together, vertical tabs is really just another thing we have to account for in the drag and drop features of interface. It's similar to a fieldset in a way, it is something that can contain form elements, and we need to be able to a) detect when it is present and b) make sure Interface knows how to interact with them through the form api. They can both play together nicely, we have put a lot of work into making sure there is a structure for doing that through the Interface API.

M

rsvelko’s picture

@chx: access denied on http://vt.drupal4hu.com/node/add/page - by design?

maybe OFFTOPIC: @interface developers: WOW - you rock. When and where will this interface module begin to exist as an official contrib? Since vtabs is going to be in it as a display option.

Anonymous’s picture

@rsvelko - you're right, this is off-topic. Created a new thread here: http://tinyurl.com/c5bo7m

kkaefer’s picture

Status: Needs review » Needs work

Working on a better patch...

kkaefer’s picture

Status: Needs work » Needs review
FileSize
29.26 KB
26.38 KB

This is a major update:

  • The JavaScript code for tab descriptions/summaries has been refactored into a general ‘element summary’. Any element can now have a summary. It can be set with $('selector').setSummary(summary);. summary can either be a function that is called when the summary is retrieved or a string (meaning the element has a static summary).
  • All tab description code has been converted to the element summary API. This means that summaries are now ‘attached’ to fieldsets, not to the tabs. Fieldsets now also have descriptions, as seen in the above screenshot.
  • All fieldsets can now have a #group property, indicating the group they belong to. If that group exists, they are output as part of that group. If it doesn’t exist, nothing happens to them.
  • All fieldsets are now groups, meaning that they can act as containers for rendering purposes only.
  • The #type = 'vertical_tab' element is now merely a placeholder for a fieldset with a different #theme_wrapper and a hidden field for storing the selected tab (so that it can be preselected on subsequent form renderings).
  • #vertical_tab_js and #vertical_tab_css are now named #attached_js and #attached_css and can be added to any form element.

This patch has the benefit that the vertical tabs can easily be removed, just by unsetting $form['additional_settings']. They can also be themed differently, just by changing the #theme_wrapper.

kkaefer’s picture

Status: Needs review » Needs work

Bug: when previewing, the tabs appear twice.

kkaefer’s picture

Status: Needs work » Needs review
FileSize
30.32 KB

Fixes the bug where tabs appear twice when previewing and adds more documentation to JavaScript.

chx’s picture

ultimateboy’s picture

Issue tags: +UBUserTesting2009

During UB usability testing, a participant read the text under "Revision information" (which says "Don't create a new revision") and he said out loud "Ok, I guess I won't create a new revision". Can we change the wording of this phrase to not be a command? Maybe something as simple as "No revision" and "New revision"?

(also tagging with UBUserTesting2009 because we applied this patch to the testing environments)

wretched sinner - saved by grace’s picture

@ultimateboy - I would think that that should be a seperate issue, as it changes the current text. Fixing it here would be Kitten Killing (TM)

kkaefer’s picture

Making copy changes *just* based on one person's reaction doesn't seem like a wise idea. That being said, I /do/ think "No revision"/"New revision" is a better wording. However, I also think we should refrain from changing every little bit of copy in this patch and defer these to other patches once this gets in. Please review this patch as much as possible in different browsers.

tstoeckler’s picture

I think this has been tested thoroughly already, especially due to chx's test page. (I have at least confirmed it working perfectly on Windows/Linux FF and IE.)
From my POV this is RTBC, unless one of the core committers (or anyone else, for that matter) wants the bug in IE to use a collapsible fieldset as a fallback. I'm +1 on leaving it the way it is, though, because, as has been said, it's not something that makes the form less usable or less user-friendly.

int’s picture

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

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

Here's a problem: the Menu Settings descriptor evaluates the Menu Link Title value as HTML. I can even type in SCRIPT tags and run javascript directly on the page. This didn't happen in the "Authoring information" section for some reasion.

Screenshot of dread attached.

kkaefer’s picture

FileSize
30.38 KB

Thanks for catching that. The new page properly escapes input.

kkaefer’s picture

Status: Needs work » Needs review
catch’s picture

The latest updates all look a lot better than last time I reviewed this.

I noticed theme_vertical_tabs(&$element) - no @param for $element

and there are unneeded spaces next to all the @return.

I'll try to do a more meaningful review and some finer nitpicking later. Leaving at CNR in the hope someone else has time to give it a thorough going over.

kkaefer’s picture

FileSize
30.49 KB

Added @element for the theme function. I don't know what spaces you are talking about next to @return, though.

cwgordon7’s picture

kkaefer - I believe that catch means that comment blocks like this:

+ * @param $form_state
+ *   The $form_state array for the form this fieldset belongs to.
+ *
+ * @return
+ *   The processed element

Should be formatted like this:

+ * @param $form_state
+ *   The $form_state array for the form this fieldset belongs to.
+ * @return
+ *   The processed element.
kkaefer’s picture

FileSize
30.91 KB

Removed the space.

int’s picture

I think that, the size of this patch, made me believe that we need to commit this, and next take care of this words problems... This are maybe late some other fixes....

kkaefer’s picture

While rerolling anyway, I applied the wording changes. Please open a separate issue for further wording changes once this patch is committed. What we need now are thorough reviews, from a security, FAPI, JavaScript, UX perspective.

RobLoach’s picture

FileSize
34.43 KB

Fixes some standard whitespace nit-picks.

I was also thinking about attached_js/css and was thinking of turning it into something like:

  // When the array key is a number, the value becomes $data in drupal_add_js/css().
  '#attached_js' => array(drupal_get_path('module', 'book') .'/book.js'),

  // When the array key isn't a number, the value becomes the $options in drupal_add_js/css().
  '#attached_css' => array(
    drupal_get_path('module', 'book') .'/book.css' => array(
      'type' => 'file',
      'weight' => 10,
    )
  ),

As you can see, you can either associate the $data as the array key or the value, depending on whether the key is a number or not. To do something like we have above where we're giving book.css a weight of 10, we'd perform the following with the current solution:


  // When the array key isn't a number, the value becomes the $options in drupal_add_js/css().
  '#attached_css' => array(
    array(
      drupal_get_path('module', 'book') .'/book.css',
      array(
        'type' => 'file',
        'weight' => 10,
      )
    )
  ),

Thoughts?

Bojhan’s picture

kkaefer: I think we gave it enough UX reviews already, I took a look if anything drasticly changed - but it seems only a couple wording issues should be solved in follow ups. As for the non-latin version, that seemed oke as well.

kkaefer’s picture

@RobLoach: Yeah, I think it kind of makes sense to add a shortcut for adding files. On the other hand, it adds yet another spoon of sugar for a not so common action. I'm torn between both ways.

greenSkin’s picture

Very nice. What's missing to get this to work on '/admin/build/modules'? I'm looking to implement Module Filter for Drupal 7 and would love to allow it to work with vertical tabs.

int’s picture

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

Status: Reviewed & tested by the community » Needs review

While it's good to see that people like the patch, we really need the patch to be reviewed, from an architectural perspective. Please don't set this patch back to RTBC until someone who is very familiar with FAPI has reviewed this patch.

Frando’s picture

I tried out an earlier version, and we have already quite a few UX reviews, and pretty much all browser problems seem to be solved.

So here's a code review, especially regarding the FAPI related stuff.

A huge +1 to the general direction the patch took after #162, thanks kkaefer. Generalizing vertical tabs and fieldsets into general "groups" that can have descriptions and that don't have to be based on a fixed array tree to function is amazing. This gives a lot of flexibiltiy to interface building in Drupal and will make it much much easier to try out new interface models while reusing lots of existing functionality, esp. the available, logical #groups and the summaries provides by javascript.

So - IMO the concept is great and adds a whole layer of flexibilty to FAPI.

Some more comments on the actual code:

* collapse.js / form.js:
The changes look all good and are a definite improvement to previous versions of the patch. Both getting the summary (fieldsets and vertical tabs) and updating/creating it (individual fieldsets) are very straightforward now.

* form.inc:
Allowing all elements and not only #input elements to have a #process handler sounds good to me. No real reason to special case input elements here.
Making all elements have an #id, and not only input elements, is OK with me as well. It's not needed for all elements, but for the sake of consistency I think it's good to have it for all elements.

* function form_process_fieldset() looks good to me as well. Using form_state is totally suitable, and cheating drupal_render() is tricky, but perfeclty legible here. Very nice idea to make the element's #group_members property a reference to the appropriate $form_state['groups'] key!

* function form_process_vertical_tabs()
This function could use a code comment on why the process handler adds a fieldset as a child element and why #theme_wrapper and #parents are set the way they are.
Also, the drupal_add_css and drupal_add_js calls could be replaced by setting #attached_css and #attached_js in hook_elements, right?
Otherwise it's fine.

Great patch, nice to see #process and #pre_render used more. IMO this patch is ready to go in. It uses modern FAPI features in a very nice way and adds much more flexiblity.

Please set this to RTBC once the minor concerns in form_process_vertical_tabs are addressed.

kkaefer’s picture

FileSize
31.5 KB

Thanks for the review. I updated the patch to include documentation in form_process_vertical_tabs().

Also incorporates RobLoach’s suggestions for streamlining the #attached_js/css array. Since you can’t specify an array as an array key in PHP, the 'data' key of the array is used for $data. This is the usage:

// When the array key is a number, the value becomes $data in drupal_add_js/css().
'#attached_js' => array(drupal_get_path('module', 'book') .'/book.js'),

// When the array key isn't a number, the value becomes the $options in drupal_add_js/css().
'#attached_css' => array(
  drupal_get_path('module', 'book') .'/book.css' => array(
    'type' => 'file',
    'weight' => 10,
  )
),

// When the array key is a number, and the value is an array, the 'data' key is used as $data.
'#attached_js' => array(
  array(
    'data' => array('mymodule' => array(...)),
    'type' => 'setting'
  ),
),

I

don’t think that moving the CSS/JS in the theme function to use #attached_js/#attached_css should be done in this patch. After deciding whether it’s actually a good idea, we should change all theme functions to use that, not just one.

Frando’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the documentation improvements. I am OK with leaving the drupal_add_css/js calls in for now. The new syntax for #attached_css / js looks good. I tested once more and skimmed the code and did not spot anything I did not like. RTBC as per my review in #186. Great work.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Awesome!! Great work everyone!

From an architectural perspective, this looks really great. I love that it re-uses existing fieldsets and contrib authors converting theirs to use this mechanism is a two-line change. It degrades perfectly and also, HOLY CRAP it works in Stark!! :D

One final nit-picky webchick pass and I'd also love one more review by someone other than kkaefer who is strong in JS/CSS, and then I believe this is good to go. :) YES!

+      foreach ($elements['#attached_' . $kind] as $data => $options) {
+        // If the value is not an array, it's a filename and passed as first
+        // (and only) argument.
+        if (!is_array($options)) {
+          $data = $options;
+          $options = NULL;
+        }
+        // If the key is a number, the key was not used for supplying $data,
+        // so $data is taken from the array.
+        if (is_numeric($data)) {
+          $data = $options['data'];
+          unset($options['data']);
+        }
+        call_user_func('drupal_add_' . $kind, $data, $options);
+      }
+    }

That first logic branch here has great comments ("If the value is not an array, it's a filename..."), but the second branch less-so. The comments there seem to just be re-iterating what the code is already telling me. If $data is keyed numerically, what are we dealing with here? Inline snippets of JS?

+      // Trick drupal_render() into believing this has already been output.
+      // The group widget will rerender this later. It's only set to
+      // #printed if the group already exists. That way, we can be sure
+      // that the fieldset /will/ be rendered later.
+      $element['#printed'] = TRUE;

While in general this comment is pretty good, because this is so backwards from what you'd expect, I think we should refine it a bit more (specifically, I get lost at "That way, we can be sure that the fieldset /will/ be rendered later." Why?). There were some nuances in #121 that I don't see expressed here.

As I'm browsing through with the web developer toolbar in Firefox, it keeps giving me warnings like this in form.js (I get this during the installer, too):

Warning: Unknown pseudo-class or pseudo-element 'select'.
Source File: http://localhost/core/node/1/edit
Line: 0
Warning: The 'charCode' property of a keyup event should not be used. The value is meaningless.
Source File: http://localhost/core/node/1/edit
Line: 0

I'm not really clued in enough to know what those warnings mean, but I don't get them without this patch. (Oh, wait. I do get one of the charCode warnings but not 500 of them, so something is different.)

+  $element[$name . '__active_tab'] = array(

Double-underscores are a bit odd here. It's not really an issue since no module developer is ever going to have to deal with it, but is there precedence for using a special token like this? Do other parts of the API do this as well, and is __ what they use?

+  // To save us from modifying the existing element and changing its #type,
+  // a new form element is created as a child. 

Just a question. Why is it preferable to create a new copy of the element rather than changing its type? That would certainly seem the most natural thing to me to do, and would seem like it requires a lot less memory, processing, etc.

+    // Store a reference to this fieldset for the vertical tabs processing function.

Needs to wrap at 80 chars.

Everywhere you're using #attached_js:

+      '#attached_js' => array(drupal_get_path('module', 'path') .'/path.js'),
+        '#attached_js' => array(drupal_get_path('module', 'upload') .'/upload.js'),
etc.

We now concatenate with a space before and after the dot.

webchick’s picture

I'm putting a call out on Twitter to see if that yields some reviews from JS folks, since I don't see any of the normal crew in IRC atm.

If someone heeds that, hello person from Twitter! :) This patch adds sexy awesomeness to Drupal:

Sexy Awesomeness

This patch is pretty big, but it has already been very thoroughly reviewed by a lot of the core developers. The only problem is that core developers, by and large, can blow your mind with the crazy things they can do with PHP, but we tend to plug our ears and go "lalalalala" about JS stuff. That's where you come in!

Please download a copy of Drupal 7, then apply the patch and then take notes as you examine the JavaScript code inside (it's all in misc/collapse.js and misc/form.js I think, although reviewing the CSS changes would also be nice due to aforementioned "lalalalala" problem).

Is it clear what's going on? Is the code well-documented, using good naming conventions so that it's easy to follow? Are there more optimal ways some of it could be done? Etc. There are tips on reviewing patches in the handbook.

Then, post a reply here with your thoughts, what you checked out, and what you think of the patch in general. Avoid "+1, looks great!" and instead try and focus on thoughtful, detailed replies about why certain code works or doesn't work. And while you're here, feel free to check out other JavaScript-related patches that need attention.

Thank you for your support! :D

kkaefer’s picture

No, don't look at collapse.js, that'll make your eyes bleed. Look at misc/vertical-tabs.js instead.

kkaefer’s picture

Status: Needs work » Needs review
FileSize
32.91 KB

I added more verbose comments to #attached_* and $element['#printed'] = TRUE as well as form.js. The charCode warning is not from this JavaScript file but probably from jQuery.

Warning: Unknown pseudo-class or pseudo-element ‘select’.

I accidentally used :select instead of :input. Using :select attached this handler to all elements, whereas :input should only add it to form elements. Since adding it to more elements would only impact performance but not functionality, I didn’t catch this error. What plugin do you use for validation?

Just a question. Why is it preferable to create a new copy of the element rather than changing its type? That would certainly seem the most natural thing to me to do, and would seem like it requires a lot less memory, processing, etc.

If we changed the existing type, we’d have to do manual processing of all #process functions. By adding a new element inside the current element, we are avoiding duplicating parts of the form process and render functions inside the #type => 'vertical_tabs'#process function.

We now concatenate with a space before and after the dot.

Uh, and I actually wrote that patch…

kkaefer’s picture

I used __ to avoid collisions. For example, if there’s foo[bar_baz][boo] and foo[bar][baz][boo], they’d result in the same name. Admittedly, this is pretty rare and could also affect other parts of Drupal, so I'm not sure whether we should leave that in.

ChrisKennedy’s picture

Status: Needs review » Needs work

Love it :)

Bunch of very minor/nitpicky thoughts:

misc/collapse.js: should the use of parentheses to surround fieldset.getSummary() be translatable? [summary.html(text ? ' (' + text + ')' : '');]

misc/vertical-tabs.css: there is a "position:relative;" and "font-weight:normal" that need a space.

misc/vertical-tabs.js: while epic, the " * Vertical Tabs." comment at the top provides no additional value over the filename and can be removed.

misc/vertical-tabs.js: " $('> fieldset', this).each(function(i) {" -> given that we don't use "i" in that function, I wouldn't specify "i" in the function definition.

misc/vertical-tabs.js: " * Vertical Tab" as the summary comment for Drupal.verticalTab seems a bit too concise.

misc/vertical-tabs.js: the frequent usage of one-line IF statements without brace enclosures seems non-standard for Drupal JS.

misc/vertical-tabs.js: ".append(tab.title = $('<strong></strong>').text(settings.title))" -> why wrap the title in a strong tag then have font-weight:normal applied via CSS? Shouldn't that just be a span, perhaps with a class?

misc/book/book.js: bookFieldsetSummaries has a multi-condition if-else clause without brace closures - kind of ugly to read.

modules/book/book.module: #attached_js line uses the old string-concatenation coding style. [ .'/book.js']

modules/upload/upload.module: here too: .'/upload.js'

modules/node/node.js: Drupal.t("No revision"); randomly uses double quotes rather than single quotes.

It might be nice if admin/build/node-type and admin/user/settings used tabs since those pages have a lot of fieldsets, perhaps as a separate issue.

It seems to me that "vertical" doesn't add much value and really this is a patch for "JavaScript Tabs." I would vote to shorten the global name to jTabs.

quicksketch’s picture

Status: Needs work » Needs review
FileSize
36.56 KB

Here's a reroll that addresses all the nitpicks in #194 (a great review, btw).

  • misc/collapse.js: should the use of parentheses to surround fieldset.getSummary() be translatable? [summary.html(text ? ' (' + text + ')' : '');]
    This isn't necessary because the text is already translated when each module calls fieldset.setSummary()
  • misc/vertical-tabs.css: there is a "position:relative;" and "font-weight:normal" that need a space.
    Fixed.
  • misc/vertical-tabs.js: while epic, the " * Vertical Tabs." comment at the top provides no additional value over the filename and can be removed.
    Removed. I agree this brings it inline with other JS files.
  • misc/vertical-tabs.js: " $('> fieldset', this).each(function(i) {" -> given that we don't use "i" in that function, I wouldn't specify "i" in the function definition.
    Removed. You're right it's not necessary.
  • misc/vertical-tabs.js: " * Vertical Tab" as the summary comment for Drupal.verticalTab seems a bit too concise.
    Now reads: " * The vertical tab object represents a single tab within a tab group."
  • misc/vertical-tabs.js: the frequent usage of one-line IF statements without brace enclosures seems non-standard for Drupal JS.
    misc/book/book.js: bookFieldsetSummaries has a multi-condition if-else clause without brace closures - kind of ugly to read.
    I agree, Drupal currently always includes braces. I found 6 instances of braces needing to be added that I corrected. Most statements already included them.
  • misc/vertical-tabs.js: ".append(tab.title = $('').text(settings.title))" -> why wrap the title in a strong tag then have font-weight:normal applied via CSS? Shouldn't that just be a span, perhaps with a class?
    Switching this to a span with class "title" was trivial and works just as well. One less CSS rule too.
  • modules/book/book.module: #attached_js line uses the old string-concatenation coding style. [ .'/book.js']
    modules/upload/upload.module: here too: .'/upload.js'
    Seems these were already fixed.
  • modules/node/node.js: Drupal.t("No revision"); randomly uses double quotes rather than single quotes.
    Fixed.
  • It might be nice if admin/build/node-type and admin/user/settings used tabs since those pages have a lot of fieldsets, perhaps as a separate issue.
    Definitely a separate issue.
  • It seems to me that "vertical" doesn't add much value and really this is a patch for "JavaScript Tabs." I would vote to shorten the global name to jTabs.
    The CSS is all very specific to the vertical layout and I think this helps group everything together (the vertical-tabs.css file relates to vertical-tabs.js, etc.) In the situation where a new convention is desired, a module would probably create a new FAPI type and just change the #type to something else besides 'vertical_tabs'. kkaefer already removed all other references to vertical tabs from FAPI, so any UI convention can be used with this summary feature. Therefor I think Vertical Tabs is a fine name.
  • The only thing I had to add was that our functions should be defined function() {, not function () {, which I fixed in the summary implementations for each module.
quicksketch’s picture

misc/collapse.js: should the use of parentheses to surround fieldset.getSummary() be translatable? [summary.html(text ? ' (' + text + ')' : '');]

I realize I misinterpreted this question. This has to do with the translation of parenthesis, not the text. I don't really have an opinion on this matter, perhaps it should be deferred to a followup issue? Seems like the same question as translation of colons within form element labels (see the three-year old #67211: Show colon after form title only when there is no other punctuation).

quicksketch’s picture

Status: Needs review » Reviewed & tested by the community

Finally, my promised JavaScript review.

This is the first JavaScript file we're adding to core (other than packages like Farbtastic and jquery.forms.js) that defines new jQuery methods. The line $.fn.setSummary = ... is making a new method available so that we can use $('fieldset').setSummary() and $('fieldset').getSummary(). I think this is fantastic and we should start doing more of this in our core JavaScript. It reduces the amount of Drupal-specific conventions and brings us inline with the jQuery community.

Also new in this patch is the use of Namespaced Events. That is, we're frequently declaring anonymous functions like .bind('formUpdated.summary', function() {}). This is also a good improvement, since if jQuery can handle the management of our anonymous functions in a central manner, it makes unbinding JavaScript events much easier. We could for example remove the update functionality by using $('.class').unbind('.summary');.

The new "formUpdated" event is a cool improvement that many modules could find very useful, such as AJAX Comment or Form Builder. Basically we'll have an event that will fire when any element changes within a form, providing a very helpful event that we can use for AJAX requests, or do as Vertical Tabs does and use this to update elements on the page just through normal JavaScript.

All in all, this patch is crazy awesome. It introduces some new conventions that we haven't used before, but I think it brings us up-to-date with what the larger jQuery community is doing.

In addition, I know this patch has already gotten the FAPI-ok, but I'd like to add my additional support the FAPI changes kkaefer has made here. The addition of the #group property will make life so much easier when dealing with visually restructuring the form.

I gave this patch one more browsing with FF3, Safari, IE6/7 in both Garland and Stark and confirmed that everything is still appearing perfectly.

This patch gets my 100% approval.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs documentation

With Frando and quicksketch's +1, and ChrisKennedy's great review, I looked over this patch once more annnnnnnnnnnnnnnd... committed to HEAD! WOOHOO!!

Lovely work, everyone. :) Marking NW because we need some documentation on how module developers can use this lovely new feature.

xmacinfo’s picture

Title: Vertical Tabs » Vertical Tabs (readme.txt and documentation)

This is simply awesome. I love the summaries displayed in each tab.

First this first. Reading quicksketch summary of new features, I guess we need to add a few lines in the README.txt file. Can we do this before any other documentation is made?

keith.smith’s picture

Title: Vertical Tabs (readme.txt and documentation) » Vertical Tabs (CHANGELOG.txt and documentation)
yched’s picture

Title: Vertical Tabs (CHANGELOG.txt and documentation) » Vertical Tabs
Status: Needs work » Needs review
FileSize
10.46 KB

Absoluetly great patch, but the change of the signature of #process callbacks breaks Field API 'widgets'
The tests that would have detected this are still pending reviews along with #375907: Make text field use D7 '#text_format' selector, but I'd expect a patch that changes a func signature to search and fix existing occurrences ?

Attached patch fixes widgets for all core field types, and include the 'text field' tests that were previously included in the other patch. It also includes two trivial fixes for text fields that are currently pending commit in other threads, but are needed for the tests to pass.

yched’s picture

Same patch, fixes comment errors.

kkaefer’s picture

FileSize
1.45 KB

Thanks for the great reviews and all your help to land this patch.

However, I strongly disagree with changing the title from strong to span. strong has syntactic relevance and means that this text is more important than other text. span on the other hand doesn’t add any semantic meaning. It is completely irrelevant how the browser styles this by default.

The attached patch changes the span back to a strong. This patch is independent from Yves’ patch above and shouldn’t interfere with it.

yched’s picture

Minor bug : description for the 'Authored by' field reads 'Leave blank for Anonymous', but it is when emptied, the summary reads 'By on 2009-03-16 21:18:25 +0100'

webchick’s picture

Status: Needs review » Needs work

Ok, looked through yched's patch @#202. Confirmed that without the rest of the hunks, the test yields 71 passes, 21 fails, and 154 exceptions which is great, so we don't end up accidentally breaking this again.

+    $entity_type = 'test_entity';
+    $this->field_name = drupal_strtolower($this->randomName(). '_field_name');
+    $this->field = array('field_name' => $this->field_name, 'type' => $field_type);
...
+    field_create_instance($this->instance);

This could really do with a one-liner comment for easier scanning.

I asked yched if it was worth pulling this into a separate function since this code is used once in each test, and he said likely not. At #368639: Remove drupalCreateField/Instance in favour of standard API functions it was stated that all this function ends up being is a wrapper for field_create_field() and you might as well just call it directly.


+ $this->field_name = drupal_strtolower($this->randomName(). '_field_name');
<?code>

needs to be a space proceeding the period.

Everything else seems well-documented in the .test, the other files are just simple copy/paste signature changes and a typo fix.

yched’s picture

Status: Needs work » Needs review
FileSize
10.64 KB

Rerolled for webchick's remarks.

webchick’s picture

Status: Needs review » Needs work

Ok, awesome. Committed #206. :)

Now onto #203...

webchick’s picture

Status: Needs work » Needs review

Grrr.

quicksketch’s picture

I'm fine with #203 (the code is exactly what I pulled out in #195). I couldn't think of a case for using strong, but since it makes little difference from the theming side and it does make a difference for semantics I think it's good to include.

webchick’s picture

kkaefer's #203 makes sense. Titles are semantically relevant, so it makes sense to separate them out from their surroundings and strong is a useful means with which to do that.

I asked quicksketch his opinion and he said, and I quote, "i don't have real <strong> feelings either way" Ba-dum-PSSH! ;)

So committed #203 as well, with the addition of a small comment above the font-weight: normal; rule, since it is indeed very odd-looking:

.vertical-tabs-list li strong {
  /* Strong tags are used around tab titles to indicate importance; however,
     only the currently active tab should actually look bold. */
  font-weight:normal;
}

Now I think this is actually fixed for real, and is now back to NW for the docs. :)

Thanks for the great work on the follow-ups, folks!

yched’s picture

Should #204 be handled in a different thread ?

webchick’s picture

Yeah, sorry. I missed #204 initially. I created #431300: Authoring information vertical tab mishandles Anonymous. for it.

webchick’s picture

Status: Needs review » Needs work
keith.smith’s picture

Status: Needs work » Needs review
FileSize
1007 bytes

First draft of CHANGELOG entry. I really thought this deserved more than just the single line, as it would be good to highlight this as a UI option for contrib. Hopefully, additional lines can be added at the bottom as (or if) other pages are converted to VT.

webchick’s picture

Status: Needs review » Needs work

Committed. Thanks, Keith! :)

Back to NW for docs again.

birdmanx35’s picture

Hmm, as far as Keith's patch, I like "features automatic summaries and increased usability" better than "increases usability". Could I get more picky?

keith.smith’s picture

Yeah. I had that exact phrase in #216 first, but then went with "increases" as I wasn't sure "increased usability" should be a feature. I don't feel strongly about it either way (I mean, I did change it to the other way, but birdman's suggestion was the initial way I wrote it).

RobLoach’s picture

RobLoach’s picture

Status: Needs work » Needs review

Update docs for the #attached_js/css:

<h3 id="attached_js">Attached JavaScript and CSS for forms</h3>
<a href="http://drupal.org/node/323112">(issue)</a> Individual form elements now have the ability to define what JavaScript and cascading stylesheets are associated with them. This is stated in the <em>#attached_js</em> and <em>#attached_css</em> property.

Drupal 6.x:
<?php
function example_admin_settings() {
  drupal_add_css(drupal_get_path('module', 'example') .'/example.admin.css');
  drupal_add_js('alert("You are visiting the example form.");', 'inline');
  $form['example'] = array(
    '#type' => 'fieldset',
    '#title' => t('Example');
  );
  return $form;
}
?>

Drupal 7.x:
<?php
function example_admin_settings() {
  $form['#attached_css'] = array(drupal_get_path('module', 'example') . '/example.admin.css'),
  $form['#attached_js'] = array(
    'alert("You are visiting the example form.");' => 'inline',
  );
  $form['example'] = array(
    '#type' => 'fieldset',
    '#title' => t('Example');
  );
  return $form;
}
?>

Note that we'll also have to update the Form API Reference.

Status: Needs review » Needs work

The last submitted patch failed testing.

webchick’s picture

Current follow-up "Houston, we have a problem" issues I was able to find:

#431300: Authoring information vertical tab mishandles Anonymous.
#437546: Vertical Tabs: Reduce scrolling up and down
#435646: Vertical Tab summaries are not distinguishable (in Stark)

Would love to get some eyeballs on these so that we can dust this issue off as "fixed."

Current follow-up "let's use VT here" issues I was able to find, which could also use some eyeballs:

#439798: Vertical Tabs for the Node Type Form
#396478: Searchable modules page
#434942: Vertical-tab-ify admin/user/settings

@RobLoach: Those docs look pretty good to me. I'd just add them to the page and we can improve upon it later if they're not clear enough. Right now no one has any idea how to use this since it's not mentioned in the upgrade docs at all. :)

kkaefer’s picture

@RobLoach: maybe you could add some more complex examples, e.g. from http://drupal.org/node/323112#comment-1463664. We should also explain why we added #attached_* (cached forms for which the form function is not called again).

kkaefer’s picture

FileSize
2.55 KB

First suggestion for #group:

#group property for fieldsets

(issue)
Fieldsets now have a #group property which is a string. Setting that string assigns the fieldset to a group with that name. Grouping fieldset does not change their location in the Form API array but rather only alters how the fieldset is rendered.

Fieldsets are also groups themselves, so a fieldset can contain other fieldsets (for rendering purposes only). The name of that group is the name of the fieldset’s array key.

$form['miscellaneous'] = array(
  '#type' => 'fieldset',
  '#title' => t('Miscellaneous'),
);

$form['settings'] = array(
  '#type' => 'fieldset',
  '#title' => t('Settings'),
  '#group' => 'miscellaneous'
);

In this example, the fieldset named settings is added to the group named miscellaneous. The group members become members of the group element for rendering purposes.

To create other form element types which act as groups, the following pattern is suggested:

function example_elements() {
  $types['pane_slider'] = array(
    '#theme_wrapper' => 'pane_slider',
    '#process' => array('form_process_pane_slider'),
  );
  return $types;
}

function form_process_pane_slider($element, &$form_state) {
  // To save us from modifying the existing element and changing its #type,
  // a new form element is created as a child. The default #process hooks
  // are called automatically by the form renderer and we don't have to do
  // that manually.
  $element['group'] = array(
    '#type' => 'fieldset',
    '#theme_wrapper' => '',
    '#parents' => $element['#parents'],
  );
  return $element;
}

function example_theme() {
  return array('pane_slider' => array('arguments' => array('element' => NULL)));
}

function theme_pane_slider($element) {
  return '<div class="pane-slider">' . $element['#children'] . '</div>';
}

In this example, a new form element type named pane_slider is added in the example_elements() hook. Using #theme_wrapper makes sure that all children are rendered (if the #theme function is not overridden) normally. The #process function adds a fieldset which is the actual group. However, that fieldset has an empty theme wrapper (meaning that it’s not actually wrapped in fieldset HTML code). Also, the #parents of that fieldset are altered so that it believes that it is its own parent (and therefore obtains the group elements associated with the group). The theme wrapper function theme_pane_slider() wraps its sole child (the fieldset) which in turn contains the other fieldsets.

kkaefer’s picture

FileSize
2.55 KB
Bevan’s picture

kkaefer. Please see http://drupal.org/node/437546#comment-1508184 with regard to documentation usage of vertical tabs.

RobLoach’s picture

Here's the updated #attached_js/css with more examples as suggested by Konstantin.....

<h3 id="attached_js">Attached JavaScript and CSS for forms</h3>
<a href="http://drupal.org/node/323112">(issue)</a> Individual form elements now have the ability to define what JavaScript and cascading stylesheets are associated with them. This is stated in the <a href="http://api.drupal.org/api/file/developer/topics/forms_api_reference.html/7#attached_js">#attached_js</a> and <a href="http://api.drupal.org/api/file/developer/topics/forms_api_reference.html/7#attached_css">#attached_css</a> property.

Drupal 6.x:
<?php
function example_admin_settings() {
  // Add example.admin.css
  drupal_add_css(drupal_get_path('module', 'example') .'/example.admin.css');
  // Add some inline JavaScript
  drupal_add_js('alert("You are visiting the example form.");', 'inline');
  // Add a JavaScript setting.
  drupal_add_js(array('mymodule' => 'example'), 'setting');
  $form['example'] = array(
    '#type' => 'fieldset',
    '#title' => t('Example');
  );
  return $form;
}
?>

Drupal 7.x:
<?php
function example_admin_settings() {
  $form['#attached_css'] = array(
    // Add example.admin/css.
    drupal_get_path('module', 'example') . '/example.admin.css'
  ),
  $form['#attached_js'] = array(
    // Add some inline JavaScript.
    'alert("You are visiting the example form.");' => 'inline',
    // Add a JavaScript setting. Note that when the key is a number, the 'data' property will be used.
    array(
      'data' => array('mymodule' => array(...)),
      'type' => 'setting'
    ),
  );
  $form['example'] = array(
    '#type' => 'fieldset',
    '#title' => t('Example');
  );
  return $form;
}
?>
kkaefer’s picture

@RobLoach: looks good. Thanks!

Xano’s picture

@RobLoach: the conversion guide needs info on WHY developers should use these properties. It vaguely describes what they do, but not why they could be useful.

RobLoach’s picture

I added Attached JavaScript and CSS for forms to the list yesterday. @Xano, what exactly are you looking for? "This is useful when there is JavaScript or CSS that is unique to the given form field"? Or something? If you get any ideas, feel free to add it.

Xano’s picture

When is JS or CSS unique to the given form field? What difference does it make if JS or CSS are added through the drupal_add_X() functions or through these properties? I guess it's for usage with hook_form_alter(), but there's no final answer to this "why" in the guide.

kkaefer’s picture

The reason we added #attached_css and #attached_js is outlined in #112`: If forms are cached, the form function is not run when the form is rerendered. However, the JavaScript files used to be added in the form function. So on cached multistep forms (like the node preview), these JavaScript files were unavailable.

mgifford’s picture

Issue tags: +Accessibility

Just tagging accessibility issues

neochief’s picture

Just for your information, there's another bug fixed in 6.x #478078: "Don't create new revision" in revisions tab.

TheRec’s picture

The example for inline attached_js in #220 is wrong :

  $form['#attached_js'] = array(
    'alert("You are visiting the example form.");' => 'inline',
  );

should be

  $form['#attached_js'] = array(
    'alert("You are visiting the example form.");' => array('type' => 'inline'),
  );

** EDIT ** I edited the "Converting 6.x modules to 7.x" page accordingly.

emmajane’s picture

This issue has been tagged as needing documentation, but it looks like a lot of docs work has already been done? Is there anything that is written that still needs review? Please let me know what you think needs to be done so that we can mark the docs part as "finished." Thanks!

Bevan’s picture

Issue tags: -Needs documentation

Emma Jane; I think that may have been in reference to #226 of this issue. See http://drupal.org/node/437546#comment-1508184 for more details.

emmajane’s picture

@bevan do you mean the request for HIG/ UX guidelines? It would be great if we could get a specific target for the documentation team to work on...so that we knew when the documentation was finished for this issue. :)

Bevan’s picture

Issue tags: +Needs documentation

Emma Jane;

On looking more closely, webchick added the "Needs Documentation" tag in comment #198, I think with regards to the API functions that this implements – probably hook_element(), FAPI usage, and theme overrides. (I haven't looked at the code though so I'm not sure.)

With regard to #226 and HCI/HIG/UX/UI guidelines, I suggest that we include a screenshot showing good usage of the UI pattern, plus about 5 screenshots showing bad uses of vertical tabs, with the associated text (probably with more detail). I think this should accompany the API usage examples and documentation, in order to be most accessible to V.Tabs implementors and consistent with most Drupal developer's workflows. (I don't think an HCI/HIG/UI/UX guidelines doc repository would be very useful for the community – though that's another discussion.)

Bad uses of vertical tabs:

  • Too many vertical tabs, more than 9: Too many for users to keep in short term memory. Takes up too much vertical space (with descriptions). Break out the tabs into multiple pages or find a more suitable UI pattern
  • Too few vertical tabs, 1, 2: Redundant. Don't use vertical tabs.
  • Nested vertical tabs. Disorientating. Multiple pages/forms or find another UI pattern.
  • A pane that is too long. Drawers go out of viewport in order to see content, thus defeating purpose of vertical tabs. Split it into multiple panes, move the whole pane onto it's own page.
  • Multiple vertical tabs on one page. Vertical tabs can look quite similar and a user may get disorientated. There may be cases where this is appropriate, but can be confusing, and alternatives should be considered: Multiple pages, other UI patterns.
Bojhan’s picture

Assigned: kkaefer » Bojhan

I will work on this documentation.

Everett Zufelt’s picture

Was accessibility for screen-reader and keyboard only users ever dealt with in this patch? Is there a site where I can test vtabs in action?

Bojhan’s picture

Issue tags: -Needs documentation

Yes it was, and your local Drupal 7 install.

I am going to attack the docs, so removing the tag to take over the Docs team radar, since it will take me quite some time.

yoroy’s picture

Everett, they are part of the form for creating content. If you have no problems changing an author, publishing date, menu item or creating a revision for a noce, then the vertical tabs are working fine :-)

Everett Zufelt’s picture

@yoroy
Apologies for not getting to test this sooner. But, IMO there are accessibility problems with vertical tabs.

1. As a screen-reader user there is no way for me to know which tab is selected, or what clicking on the link to show / hide each tab will do.

2. I expect that if I click on a link that will show content, that the content will be somewhere near the link, it took me some time to figure out where the display content was.

Recommendations

1. There needs to be clear non-visual indication that this is indeed a tabstrip.
2. There needs to be non-visual indication of which tab is active.
3. There needs to be an easy way to find the displayed content once a tab is selected. This problem may become an easier task if 1 and 2 above are solved.

See #467296: Accessibility improvements for vertical tabs

Somewhat Related Issues:
#521852: Local tasks lack semantic markup to indicate an active task
#541568: Link to expand / collapse fieldsets has poorly accessible link text

mgifford’s picture

Also see this issue for keyboard only users:
#567390: Use vertical tabs with keyboard only

Cliff’s picture

Subscribing. Lots to read this weekend. [Sigh. Big sigh.]

[Now that I'm not even halfway through trying to read it, even bigger sigh!]

Just wondering — and it seems that there are maybe a dozen issues confronting various aspects of this same problem — if WCAG 2.0 Technique SCR 28 shows the successful way to code this kind of stuff for accessibility.

And maybe when that gets solved, the rest of this will fall into place.

redben’s picture

Regarding the usability issue, this is my two cent, (although it is critisism about the general approche to usability)
A lot of people making web products/websites appraoch usablity as a "one-output-usable-everywhere". I mean they want to make the same html/css work for "normal user" and "special user" or normal browser and say screen reader. I think that usablity is not like crossbrowser design. Normal browser should output the same thing. A screen reader is something else. Just like we make different stylesheets for print and screen, we should have a different html/css for screen readers. I think that this problem should be tackled in themes. Imagine having a special theme solely for screen readers !
Car makers for example do not make a model that targets every kind of customers. Instead they make a version for people with disbalities.

After reviewing my comment i think it is more abut screen readers that usability but...

Just my opinion though !

alexanderpas’s picture

@redben
your analogy and view is completely wrong and broken!

for example: car makers do not make cars for people with only one arm.
for people with one arm, you'll need to add a knob on the steering wheel, which is an adaption on the standard car, just like the screen reader is adaption on the standard browser!

weither you're driving with a disability or not, the roads you're driving on are still the same, just like the webpages you'revisiting.

just remove your mouse, and go browse the internet.

Cliff’s picture

@redben, as @alexanderpas implies, the issue is not just about accessibility for people who use screen readers but accessibility for all — regardless of impairments to vision, hearing, mobility, or cognitive abilities.

In this case, we are mainly concerned with lowering the barriers vertical tabs pose to people who cannot see them and people who cannot easily point to them with a mouse or similar device.

And although the concept of detecting the user agent and switching themes accordingly is intriguing, it doesn't address the barriers for which no user agents are available. For example, how can you unobtrusively detect whether a person can actually use the mouse that is connected to the computer they are using?

It would also add new burdens for site maintenance. Screen-reading apps vary. Are you going to optimize your theme to one in particular, or have a different theme for each? And if a new release of a screen reader adds support for features the previous release couldn't handle, should the corresponding theme be revised so people who have the screen reader's most current version don't miss out on those features? And then will you keep the old version of that theme up so people who still have earlier versions of that screen reader won't encounter a barrier?

So the seemingly simple solution is actually far more complex.

redben’s picture

@Cliff You are right, my bad :P
@alexanderpas : the analogy is not "completely wrong". As you say "for people with one arm, you'll need to add a knob on the steering wheel" not on ALL the produced cars but only for those that need it. The road are the same, the info on the web site is the same. That is why i talked about a "seperate theme", and adaptation of the main site UI.
But then again as Cliff pointed out, user agents is not enought data to adapt the UI...
So the seemingly simple solution is actually far more complex.

So good luck to you people on this issue. Tackling usability issues is a Brave Work !

moshe weitzman’s picture

Issue tags: +Needs documentation

I think this tag was removed by accident. I can't find any docs.

dddave’s picture

No accident about removing the docs tag. See #242.

I leave it in case Bohjan was held up.

matt2000’s picture

FileSize
302.79 KB

Disclaimer: I haven't read the proceeding 252 comments, just reporting my experience; so sorry if this is mentioned elsewhere.

The vertical tabs are quite awkward, in my opinion, when the first tab is not the largest. It can force the window to 'jump' as the document height is modified. Primary example is admin/structure/types/add.

Here is a low res gif animation to demonstrate the experience. Annoying, isn't it?

I think the solution is the for the tabs div to the height of the largest individual tab.

Bevan’s picture

All of the original prototypes I did (Summaries 2, Summaries, 1, RHS) had this feature. It looks like it got lost somewhere along the way. Whether that was intentional or not, I don't know.

Les Lim’s picture

Component: node system » documentation
Priority: Critical » Normal
FileSize
42.23 KB

I've never been particularly bothered by the jump. Having a footer in the admin theme might minimize this effect somewhat.

The problem with fixing the tab height is that it will leave a lot of empty space on tabs with fewer elements, which is going to look buggy. Here's a screenshot.

Bevan’s picture

Component: documentation » node system
Priority: Normal » Critical

Moshe (#251)and dddave (#252); Bojhan worked on http://p1.drupalusability.org/pattern/vertical-tabs just after #242, which I think is what he was referring to there. There is also theme_vertical_tabs() and form_process_vertical_tabs(). From a developer's point of view I think more detail in the handbook and/or api.drupal.org would be great. At the very least a summary of http://p1.drupalusability.org/pattern/vertical-tabs or even a link to it (or it's permanent home).

catch’s picture

Priority: Critical » Normal
sun’s picture

Status: Needs work » Fixed

The implementation of vertical tabs breaks perfectly valid forms.

I am revamping the entire implementation in #657668: Vertical tabs break Form API currently, so if you want to prevent that some undocumented but expected behavior is removed, then you better quickly review the patch over there before it gets committed.

Also, I'm marking this issue fixed, because it doesn't have any traction. Any remaining bug fixes/documentation issues should be moved into separate issues.

Status: Fixed » Closed (fixed)

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

yched’s picture